issue #3895 Show all fingerprints for recipient

[IvanPizhenko]

This PR introduces showing all recepient's public key fingerprints

close#3985

---

Tests(delete all except exactly one):

• Difficult to test (explain why) - not sure at the moment how to do it.

---

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

[IvanPizhenko]

@tomholub@rrrooommmaaa I think I basically did it, but some CI test got broken, and from what I see in the CI logs I absolutely don't get any ideas what needs to be fixed. Please have a look on this.

[tomholub]

@IvanPizhenko I think you're missing local ci secrets? To run tests locally. See https://github.com/FlowCrypt/flowcrypt-browser/wiki/Writing-and-running-tests meanwhile I'll send them over email.

[tomholub]

You're right that this is basically it. The broken tests could be that some behavior has changed (maybe for the better?) and the tests will need to be updated. Or a bug caused by this change, or a combination of both - that has to be investigated.

[IvanPizhenko]

@IvanPizhenko I think you're missing local ci secrets? To run tests locally. See https://github.com/FlowCrypt/flowcrypt-browser/wiki/Writing-and-running-tests meanwhile I'll send them over email.

Thanks Tom, yes, I really wasn't even thinking that secrets are needed to run tests locally, but now I see.

[IvanPizhenko]

@tomholub I made it passing tests

[IvanPizhenko]

Now should I add some UI for this?

[IvanPizhenko]

@tomholub@rrrooommmaaa Ready for review

[tomholub]

Now should I add some UI for this?

I suppose you could modify an existing test. You won't be able to test the hover but you could extract the title attribute in a test that has a green recipient (most of them) and in a test that has an expired one. I think that's enough.

[rrrooommmaaa]

Hi @IvanPizhenko
Contact isn't stored in the database

• test/source/platform/store/contact-store.ts -- this is a mock database used only in tests, so it is implemented to the extent required for tests to run -- it doesn't fully mimick the real store's behaviour.
• extension/js/common/platform/store/contact-store.ts -- this is the real one.
  As you can see, only Pubkey, Email and Revocation entities are stored there. Contact type is only left for some consumer classes that we haven't refactored yet -- it emulates the older storage by picking the best candidate pubkey for emal and including it in the Contact. If consumer class needs more data then the best pubkey, it shouldn't use Contact type nor ContactStore.get. We will remove these type and method when no more consumers are left (updating the mock contact-store.ts in due course).

[rrrooommmaaa]

Which of them "doesn't require the results" and why? I assumed the second one,

Yes, the second one only needs to warn in a situation when 1) lookup resulted in network failure or 2) saving to the store caused an exception. These are the situations when lookupPubkeyFromKeyserversAndUpsertDb returns false.
We can refactor it to (re-)throw an exception and rewrite the second call with .catch(...) as Tom suggested.
If lookup yielded no keys -- that's not an error, that's the NO_PGP situation.

The first call extracts the keys from the storage if no error happens.

 // re-query the storage
return (await ContactStore.getOneWithAllPubkeys(undefined, email))?.sortedPubkeys ?? [];

If no keys, it will return an empty set (this is not 'fail').
Previously, re-quering was done in lookupPubkeyFromKeyserversAndUpsertDb after all updates to return the sorted keys, but this is not required for the second case, so I made this method lighter, removed re-querying.
And one more thing -- it may look like lookupPubkeyFromKeyserversAndUpsertDb can format PubkeyInfo[] out of Key[] it obtained, right? That's not good, because in the storage we have the table Revocations that stores fingerprints of revoked keys that may not exist in the database (Pubkeys table) -- that is pre-emptive external revocation. So (in a rare case) the lookup method receives a valid key, saves it to the storage, and after re-querying the storage, this key maybe returned as revoked. This is why PubkeyInfo has revoked property regardless of the fact that Key itself also has it.

[IvanPizhenko]

This is why PubkeyInfo has revoked property regardless of the fact that Key itself also has it.

Maybe this should be documented via some comments in the code.

[IvanPizhenko]

@rrrooommmaaa I'm thinking about how to refactor lookupPubkeyFromKeyserversAndUpsertDb() and I have 2 questions:

1. Why it has this check:

constlookupResult=awaitthis.view.pubLookup.lookupEmail(email);if(lookupResult&&email){

i.e. is it ever possible that lookupEmail (if it didn't threw) would return undefined?

2. Why also check email here? It seems to be non-nullable parameter. Why at all check it after calling lookupEmail() and not before?

[rrrooommmaaa]

I guess these methods should go to test/source/tests/page-recipe/compose-page-recipe.ts

[IvanPizhenko]

Moved them.

[rrrooommmaaa]

is it ever possible that lookupEmail (if it didn't threw) would return undefined?

It's not possible. Maybe it was done to trick the linter or something. Feel free to refactor.

Why also check email here? It seems to be non-nullable parameter. Why at all check it after calling lookupEmail() and not before?

email can be ''. The order should be refactored, I guess.

[rrrooommmaaa]

Maybe this should be documented via some comments in the code.

Sure. Where would the appropriate place be, in your opinion?

[IvanPizhenko]

Maybe this should be documented via some comments in the code.

Sure. Where would the appropriate place be, in your opinion?

I've added, in the PubKeyInfo type declaration.

[IvanPizhenko]

@rrrooommmaaa So I've tried to implement all the notes above. Are there any other improvements to be done here?

[rrrooommmaaa]

@rrrooommmaaa So I've tried to implement all the notes above. Are there any other improvements to be done here?

The list looks badly formatted to me.

[rrrooommmaaa]

Should look like

Valid public key fingerprints:
FF5E C310 4388 CA0F 4014 EBC9 2B6A C469 03E7 16E4 (openpgp)
6FE1 16D2 759F 0FFA C562 3E7E 10D6 E379 41EA A0BB (x509)
Expired public key fingerprints:
whatever (openpgp)
Revoked public key fingerprints:
D666 2C5F B9BD E9DA 01F3 994A AA1E F832 D8CC A4F2 (openpgp)

[rrrooommmaaa]

I suggest to display x509 instead of s/mime as we do on other pages, e.g. Contacts.

[rrrooommmaaa]

you can import as many keys as needed to the same "contact" from https://github.com/FlowCrypt/flowcrypt-browser/blob/master/test/source/tests/tooling/consts.ts
(e.g. 6FE1 16D2 759F 0FFA C562 3E7E 10D6 E379 41EA A0BB (x509) is smimeCert).
by overwriting the extracted email before pressing "import"

[IvanPizhenko]

@rrrooommmaaa Fixed it