Wkd.lookupEmail returns several keys

[rrrooommmaaa]

This PR processes multiple keys from Wkd and some other services

close #3018

---

Tests (delete all except exactly one):

• Tests added or updated TODO:

---

To be filled by reviewers

I have reviewed that this PR... (tick whichever items you personally focused on during this review):

• addresses the issue it closes (if any)
• code is readable and understandable
• is accompanied with tests, or tests are not needed
• is free of vulnerabilities
• is documented clearly and usefully, or doesn't need documentation

[tomholub]

Looking good! Adding some comments.

[tomholub]

Yup - please make acctEmailAttesterPubId an array. Then somewhere here will be acctEmailAttesterPubId !== prv.id or something similar, to see if the public key is consistent. Please change that to check if acctEmailAttesterPubId.includes(prv.id).

[rrrooommmaaa]

So this is the flag I was talking about. It's easily calculated when creating the Key structure.
So we don't really need to keep it in the database, do we?
The main question is: how should we prevent an update operation which replaces a revoked key with its unrevoked (outdated) version?

[tomholub]

So this is the flag I was talking about. It's easily calculated when creating the Key structure.
So we don't really need to keep it in the database, do we?

Ah. My intuition was to update the schema to add this flag to storage. Then filter based on that when pulling keys out of storage.

What you say is that you pull all keys for that user, then parse them and choose the appropriate key to use based on the parsed information. Your approach is simpler, and therefore better.

The main question is: how should we prevent an update operation which replaces a revoked key with its unrevoked (outdated) version?

I tend to do this in two ways - once in high level code and once more - a last resort hard stop - in low level code:

1. a sensible logic preventing unwanted behavior in high-level code, in particular:

• when pulling from pubkey lookup (compose)
• when refreshing public keys from remote sources (compose)
• when verifying messages (does that also update keys? I know it TOFU fetches a key but not sure if it would attempt to update existing key?)
• when importing pubkey sent by another user (pgp_pubkey.htm)
• when importing pubkeys in settings (contacts.htm)

All of the above places would have an if to handle this situation and not update key that's already revoked. Often this will result in a ui modal or toast message to the user if it was user-initiated action. In situations when this was an automatic action (like compose window lookups after user enters recipient), the UI interaction could be skipped and the update would be skipped silently (still in high level code).

2. throw an error in low level function, in case we missed any edge cases. In this case, save and update function would check if existing stored key is revoked. If it is, it will throw new Error("Wrongly attempted to replace a local revoked pubkey with non-revoked version")

If the error ever throws, it will get reported to the backend, and I'll get to know about it. Then we'll have the stack trace to know which case we missed to handle in high level code.

---

I don't know if I'm over doing it. We could just silently skip updating in low level code and don't tell the user. But that could lead to some behavior that may appear buggy to the user.

[rrrooommmaaa]

Ah. My intuition was to update the schema to add this flag to storage. Then filter based on that when pulling keys out of storage.

Actually, to make a unified solution for both OpenPGP and S/MIME revocation (we do want to support CRL later on, don't we?), the easiest would be to add a flag, or keep a table of revoked keys' fingerprints.

[tomholub]

I'll let you choose an approach that you deem appropriate. I'd be ok to prevent updating revoked keys in a followup PR - the situation now is already better then before (it used to not be even able to import revoked keys, I think).

[lgtm-com]

This pull request introduces 1 alert when merging 570d1c5 into 8114532 - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

[lgtm-com]

This pull request introduces 1 alert when merging 444ab4a into 4868497 - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class