auto include pubkey based on rules discussed in #3445#3487
Conversation
tomholub
left a comment
There was a problem hiding this comment.
Excellent! Thank you for splitting it out. Some comments below.
| export class ComposeMyPubkeyModule extends ViewModule<ComposeView> { | ||
|
|
||
| private toggledManually = false; | ||
| private wkdLongids: { [acctEmail: string]: string[] } = {}; |
There was a problem hiding this comment.
It is preferable to compare fingerprints over longids whenever possible - fingerprints are slightly more secure, and there is no advantage using longids.
The only place that makes sense to use longids is when looking up public keys of a signed message, because the signature only contains a longid. Similarly encrypted message only mentions longid of the key it's encrypted for. In all other situations, let's use fingerprints.
There was a problem hiding this comment.
Done, but I had to parse the KeyInfo.private field as we don't currently have the primary fingerprint there, only longid and a set of all fingerprints. Or do we have to do a "must include all fingerprints" check? Also, do we need to attach our key only if "Sign" option is ticked?
extension/chrome/elements/compose-modules/compose-my-pubkey-module.ts
Outdated
Show resolved
Hide resolved
extension/chrome/elements/compose-modules/compose-my-pubkey-module.ts
Outdated
Show resolved
Hide resolved
| for (const recipient of foreignRecipients) { | ||
| // new message, and my key is not uploaded where the recipient would look for it | ||
| if (! await this.view.recipientsModule.doesRecipientHaveMyPubkey(contact.email)) { | ||
| if (! await this.view.recipientsModule.doesRecipientHaveMyPubkey(recipient)) { |
There was a problem hiding this comment.
doesRecipientHaveMyPubkey seems like it would be better to place it in this file and not in recipients module, but hard to say for sure without opening the IDE, and not overly important
| } | ||
| // if recipient uses same domain, we assume they use flowcrypt | ||
| const pgpClient = this.myOwnDomain === recipientDomain ? 'flowcrypt' : 'pgp-other'; | ||
| const pgpClient = this.myOwnDomain === Str.getDomainFromEmailAddress(email) ? 'flowcrypt' : 'pgp-other'; |
There was a problem hiding this comment.
I suppose we'll be dropping pgpClient from everywhere. If you want to chunk up the upcoming PR, this could also be done in a separate PR.
There was a problem hiding this comment.
Let's drop it in the PR #3445 that actually "deletes" the client field from the database. This branch still attempts to store it.
| } | ||
|
|
||
| public static getDomainFromEmailAddress = (emailAddr: string) => { | ||
| // todo: parseEmail()? |
There was a problem hiding this comment.
I think parseEmail would be sensible here, and throw if it returns "undefined"
tomholub
left a comment
There was a problem hiding this comment.
Looks good, should I merge it?
This test |
From memory, enterprise extension has a more restricted manifest file which means we have to list the mock domains in manifest file. Or in OS hosts files, or both. Is it possible to use an existing mock domain that some other tests use? Just a different user on the same domain? If yes, please try that to limit amount of external configuration needed for the tests. |
…rise configuration
…nterprise configuration
|
Enterprise is still failing to access Wkd at |
|
I fixed that tests on master now. |
This PR is part of #3332 refactoring
It implements "auto include pubkey" feature based on
this comment #3445 (comment)
close #3491