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

feat: E2EE warnings on search and audit panel #32551

Merged
merged 15 commits into from
Jun 21, 2024
Merged

Conversation

yash-rajpal
Copy link
Member

@yash-rajpal yash-rajpal commented Jun 4, 2024

Proposed changes (including videos or screenshots)

While end-to-end encryption in Rocket.Chat provides enhanced privacy and security for communications, it inherently restricts certain functionalities, such as auditing messages, searching message content, and interacting with bots within encrypted rooms. Users may not be aware of these limitations, which can lead to confusion and a mismatch between user expectations and the platform's capabilities. Incorporating clear disclaimers into the user interface will help set accurate expectations and improve user satisfaction by transparently communicating the trade-offs involved with encryption.

image image

Issue(s)

Steps to test or reproduce

Further comments

E2EE2-10

@yash-rajpal yash-rajpal requested a review from a team as a code owner June 4, 2024 19:31
Copy link
Contributor

dionisio-bot bot commented Jun 4, 2024

Looks like this PR is ready to merge! 🎉
If you have any trouble, please check the PR guidelines

Copy link

changeset-bot bot commented Jun 4, 2024

🦋 Changeset detected

Latest commit: 7f4c3c5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 34 packages
Name Type
@rocket.chat/i18n Minor
@rocket.chat/meteor Minor
@rocket.chat/mock-providers Patch
@rocket.chat/ui-contexts Major
@rocket.chat/web-ui-registration Major
@rocket.chat/fuselage-ui-kit Major
@rocket.chat/ui-client Major
@rocket.chat/gazzodown Major
@rocket.chat/livechat Patch
@rocket.chat/ui-avatar Major
@rocket.chat/ui-video-conf Major
@rocket.chat/uikit-playground Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/core-typings Minor
@rocket.chat/rest-typings Minor
@rocket.chat/apps Patch
@rocket.chat/core-services Patch
@rocket.chat/cron Patch
@rocket.chat/model-typings Patch
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/presence-service Patch
@rocket.chat/queue-worker Patch
@rocket.chat/stream-hub-service Patch
@rocket.chat/api-client Patch
@rocket.chat/license Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/ddp-client Patch
@rocket.chat/models Patch
@rocket.chat/instance-status Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

codecov bot commented Jun 4, 2024

Codecov Report

Attention: Patch coverage is 57.14286% with 3 lines in your changes missing coverage. Please review.

Project coverage is 56.54%. Comparing base (e47ae76) to head (7f4c3c5).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #32551      +/-   ##
===========================================
- Coverage    56.62%   56.54%   -0.08%     
===========================================
  Files         2486     2482       -4     
  Lines        54921    54876      -45     
  Branches     11364    11358       -6     
===========================================
- Hits         31097    31032      -65     
- Misses       21151    21163      +12     
- Partials      2673     2681       +8     
Flag Coverage Δ
e2e 56.08% <57.14%> (-0.12%) ⬇️
unit 72.12% <ø> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link
Member

@dougfabris dougfabris left a comment

Choose a reason for hiding this comment

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

I'm thinking we're adding unnecessary complexity in the user experience. Basically we're adding the support for finding the encrypted rooms and when they select, we warn them they can't proceed with what they want (audit feature). If the issue is that users are being frustrated because they don't know they can't use these features for e2e encryption we're failing on the explanation of our features. We should look for an alternative to warn them such as displaying these callouts based on e2e being enabled on the workspace, saying they won't be able to find the encrypted channels for example. Kinda similar what you're doing on search messages.

And this change definitely isn't a chore

@yash-rajpal
Copy link
Member Author

Thanks for the review doug, and I'm always having hard time figuring out if something is a chore or not 🤔

But the idea is, encrypted rooms are still auditable, and encrypted rooms might contain some un-encrypted messages which can be audited. The warning is just to let the users know about the limitations.

If we stop showing encrypted rooms on audit panel then the admin won't be able to see the un-encrypted messages which could still be audited.

@yash-rajpal yash-rajpal changed the title chore: E2EE warnings on search and audit panel feat: E2EE warnings on search and audit panel Jun 5, 2024
Copy link
Contributor

@hugocostadev hugocostadev left a comment

Choose a reason for hiding this comment

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

fix errors and implement tests

packages/i18n/src/locales/en.i18n.json Outdated Show resolved Hide resolved
@hugocostadev
Copy link
Contributor

Tests requirement dismissed due to the nature of this feature, does not worth to do e2e tests to check warning

hugocostadev
hugocostadev previously approved these changes Jun 12, 2024
@milton-rucks milton-rucks added this to the 6.10 milestone Jun 13, 2024
.changeset/friendly-months-attack.md Outdated Show resolved Hide resolved
apps/meteor/client/lib/getRoomTypeTranslation.ts Outdated Show resolved Hide resolved
@yash-rajpal yash-rajpal requested a review from a team as a code owner June 18, 2024 21:58
@milton-rucks milton-rucks added the stat: QA assured Means it has been tested and approved by a company insider label Jun 19, 2024
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Jun 19, 2024
@yash-rajpal yash-rajpal removed the request for review from dougfabris June 19, 2024 19:57
@yash-rajpal yash-rajpal requested review from dougfabris and removed request for dougfabris June 20, 2024 13:09
@yash-rajpal yash-rajpal added stat: ready to merge PR tested and approved waiting for merge and removed stat: ready to merge PR tested and approved waiting for merge labels Jun 20, 2024
@ggazzo ggazzo merged commit 768cad6 into develop Jun 21, 2024
48 checks passed
@ggazzo ggazzo deleted the e2e-design-warnings branch June 21, 2024 05:17
This was referenced Jun 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stat: QA assured Means it has been tested and approved by a company insider stat: ready to merge PR tested and approved waiting for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants