Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NEW] Settings to enable E2E encryption for Private and Direct Rooms by default #16928

Merged
merged 6 commits into from
Mar 20, 2020

Conversation

rodrigok
Copy link
Member

No description provided.

@rodrigok rodrigok added this to the 3.1.0 milestone Mar 18, 2020
@rodrigok rodrigok requested a review from sampaiodiego March 18, 2020 21:06
@lgtm-com
Copy link

lgtm-com bot commented Mar 18, 2020

This pull request introduces 2 alerts when merging 88ad339 into 4a14f44 - view on LGTM.com

new alerts:

  • 2 for Useless assignment to local variable

Copy link
Member

@sampaiodiego sampaiodiego left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you think GENERAL should be created using createRoom as well?

Rooms.createWithIdTypeAndName('GENERAL', 'c', 'general', {

import { settings } from '../../settings';

callbacks.add('beforeCreateRoom', ({ type, extraData }) => {
if (['d', 'p'].includes(type)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test is kinda happening two times.. I think leaving only the if below makes more sense.

app/lib/server/functions/createDirectRoom.js Outdated Show resolved Hide resolved
app/e2e/server/beforeCreateRoom.js Outdated Show resolved Hide resolved
app/cas/server/cas_server.js Outdated Show resolved Hide resolved
rodrigok and others added 5 commits March 19, 2020 21:13
Co-Authored-By: Diego Sampaio <chinello@gmail.com>
Co-Authored-By: Diego Sampaio <chinello@gmail.com>
Co-Authored-By: Diego Sampaio <chinello@gmail.com>
…by-default

# Conflicts:
#	app/meteor-accounts-saml/server/saml_server.js
@lgtm-com
Copy link

lgtm-com bot commented Mar 20, 2020

This pull request introduces 2 alerts when merging edacf1a into 292a7c3 - view on LGTM.com

new alerts:

  • 2 for Useless assignment to local variable

@sampaiodiego sampaiodiego merged commit 12fe0c3 into develop Mar 20, 2020
@sampaiodiego sampaiodiego deleted the option-enable-e2e-by-default branch March 20, 2020 05:08
@sampaiodiego sampaiodiego mentioned this pull request Apr 9, 2020
@reetp
Copy link

reetp commented Apr 11, 2020

Why this was merged when:

a) It is still considered Beta
""E2E_Enable_alert": "This feature is currently in beta!"

b) It is still not supported on any of the mobile apps?
RocketChat/Rocket.Chat.ReactNative#141

I just don't get the logic of enforcing this by default when the code is not considered stable, and the apps have no support.

Also noted that these probably still apply (for those who do not read documentation)

  • Bots may not be able to see encrypted messages until they implement support for it.
  • Uploads will not be encrypted in this version.

@rodrigok
Copy link
Member Author

@reetp this is not enabled by default, but a setting to be able to enable it by default.

There are some clients that want this feature since it doesn't hurt anyone we added the option.

Yes, it's still in beta, it's not supported by our mobile apps yet, but if you need this and use only the web clients you may use this new setting.

@reetp
Copy link

reetp commented Apr 12, 2020

@reetp this is not enabled by default, but a setting to be able to enable it by default.

KK - sorry but the heading was not very clear.

There are some clients that want this feature since it doesn't hurt anyone we added the option.

Indeed

Yes, it's still in beta, it's not supported by our mobile apps yet, but if you need this and use only the web clients you may use this new setting.

Yup.

Thanks for the clarification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants