FXCM-2273: Refactor login/encryption code into a new support crate - #7542
FXCM-2273: Refactor login/encryption code into a new support crate#7542oskirby wants to merge 25 commits into
Conversation
27fea06 to
042d197
Compare
aa20b6c to
26697cb
Compare
jo
left a comment
jo
left a comment
There was a problem hiding this comment.
Thank you very much for this work!
I think that - contrary to our expectations - this represents a breaking change for Android and Swift users, since error handling has changed and the moved import. Therefore, accompanying pull requests for mobile applications would be necessary 😬 see https://github.com/mozilla/application-services/blob/main/docs/howtos/breaking-changes.md
And could you add a CHANGELOG entry?
| [External = "encryption"] | ||
| typedef interface StaticKeyManager; |
| bytes decrypt(bytes ciphertext); | ||
| }; | ||
| [External = "encryption"] | ||
| typedef trait EncryptorDecryptor; |
There was a problem hiding this comment.
I am not sure but this might be
typedef trait_with_foreign EncryptorDecryptor;
| bytes get_key(); | ||
| }; | ||
| [External = "encryption"] | ||
| typedef trait KeyManager; |
There was a problem hiding this comment.
and this
typedef trait_with_foreign KeyManager;
The pull request has been modified, dismissing previous reviews.
a6af9f0 to
8706813
Compare
8706813 to
88f8453
Compare
88f8453 to
51cb1fa
Compare
mhammond
left a comment
mhammond
left a comment
There was a problem hiding this comment.
just a quick driveby, but this looks great!
| Ok(encryption::create_canary(text, key)?) | ||
| } | ||
|
|
||
| pub fn check_canary(canary: &str, text: &str, key: &str) -> ApiResult<bool> { |
| @@ -0,0 +1,30 @@ | |||
| [package] | |||
| name = "encryption" | |||
There was a problem hiding this comment.
I don't like this name, it's too generic and kinda competes with rc_crypto for things generically called "encryption". I think we need some qualification here. Not sure what it might be - something likelogins-encryption, but clearly you are intending to make it more generic than that. I've no great ideas but maybe we can bike-shed?
There was a problem hiding this comment.
I agree, and I am open to suggestions as well.
There was a problem hiding this comment.
I think something along the lines of 'credential-cryptography` would better represent the intended purpose of this crate, but that is a bit of a lengthy name.
There was a problem hiding this comment.
still a fan of enc_dec and renaming EncryptorDecryptor to EncDec
There was a problem hiding this comment.
I'm good with either of those options. enc_dec has the advantage of matching exactly how it is used. Happy to leave this decision to y'all!
3 participants
This attempts to refactor the credential encryption code and move it into it's own support crate so that it can be re-used by other modules in the a-s ecosystem, and is a prerequisite for supporting encrypted autofill (eg: to store things like payment details).
There were some changes made to the uniffi API to enable this work, but logically there should be no meaningful changes to the API, and it should be compatible with no changes to the consumers of this code. It's a bit unclear to me how serious these changes are, and whether they constitute an API change. The changes include:
logins.udland into a new crateencryption.udl.logins.udlas external types.LoginsApiErrorto anEncryptionApiError.NSSKeyManagertype now takes akey_nameparameter, though as far as I can tell, theNSSKeyManagerwas never acually exposed by uniffi.Related JIRA Issues:
Pull Request checklist
[ci full]to the PR title.