Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Hide algorithm from the message about turning on encryption #3813

Closed
wants to merge 2 commits into from

Conversation

aaronraimist
Copy link
Contributor

Fixes element-hq/element-web#8829

Before:
Screen Shot 2020-01-07 at 11 34 40 AM

After:
Screen Shot 2020-01-07 at 11 34 54 AM

Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

code lgtm, just double checking with product (and CI)

@nadonomy
Copy link
Contributor

nadonomy commented Jan 7, 2020

Also LGTM! :)

@ara4n
Copy link
Member

ara4n commented Jan 7, 2020

might be worth keeping the info around somewhere though (perhaps hidden behind a <details/>)?
what happens if someone sends an m.room.encryption with an unrecognised algorithm? we don't want to claim the room is encrypted if the client then can't actually encrypt for that algorithm.

@uhoreg
Copy link
Member

uhoreg commented Jan 7, 2020

I think the information should still be available in the "View Source" for the event, for those who care about it?

@aaronraimist
Copy link
Contributor Author

Would need matrix-org/matrix-spec-proposals#2184

@nadonomy
Copy link
Contributor

nadonomy commented Jan 7, 2020

I think the information should still be available in the "View Source" for the event, for those who care about it?

This seems reasonable. Is it tractable for us to also display the encryption algorithm being used in Room Settings?

@turt2live
Copy link
Member

(shouldn't need spec support - we can render whatever we want)

@aaronraimist
Copy link
Contributor Author

aaronraimist commented Jan 7, 2020

what happens if someone sends an m.room.encryption with an unrecognised algorithm? we don't want to claim the room is encrypted if the client then can't actually encrypt for that algorithm.

This would be something for another issue. This problem already exists.
Screen Shot 2020-01-07 at 11 57 11 AM

The room appears to be encrypted (shows icons and stuff) but doesn't let me send anything.

@uhoreg
Copy link
Member

uhoreg commented Jan 7, 2020

I think that for the average end-user, just removing the algorithm is no worse than what we have security-wise, since they don't know what algorithm is right. We should probably have another issue for displaying an error message for when the algorithm is something we don't understand. The room decoration should probably also be something that reflects that we don't understand the encryption algorithm (maybe the same decoration as an unencrypted room?).

So my vote would be to merge this as-is, and create new issues for dealing with unknown algorithms.

@turt2live
Copy link
Member

This is blocked on design/product oversight to settle the <details> or no <details> argument.

@jryans jryans requested a review from nadonomy January 16, 2020 11:16
@jryans
Copy link
Collaborator

jryans commented Jan 16, 2020

This is blocked on design/product oversight to settle the <details> or no <details> argument.

I've requested @nadonomy's review in hopes of getting a decision on this.

@jryans jryans removed the request for review from nadonomy January 16, 2020 17:58
@jryans
Copy link
Collaborator

jryans commented Jan 16, 2020

While chatting more with @ara4n, he would like to have some kind of handling of unknown algorithms at the same time as the propose hiding here. I think he came up with a decent approach:

  • When the algorithm is the one we expect (m.megolm.v1.aes-sha2), hide the algorithm as in this existing PR: %(senderName)s turned on end-to-end encryption.
  • For anything else, show the algorithm as unrecognised: %(senderName)s turned on end-to-end encryption (unrecognised algorithm %(algorithm)s).

@aaronraimist, could you update the PR to that, if it seems reasonable to you as well?

@jryans jryans removed the blocked label Jan 16, 2020
@jryans
Copy link
Collaborator

jryans commented Jan 24, 2020

@aaronraimist, thanks again for working on this! We've decided we want to have this resolved for FOSDEM, so I've opened #3936 with the approach mentioned above. Sorry for the delay in reaching as decision here.

@jryans jryans closed this Jan 24, 2020
@aaronraimist aaronraimist deleted the hide-enc-algo branch March 15, 2020 19:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can we hide the algo used when turning on encryption
6 participants