Refactor FXIOS-10788 [Logins] Apply necessary adjustments for A-S EncryptorDecryptor update #24108

issammani · 2025-01-13T17:24:03Z

📜 Tickets

💡 Description

This PR:

Updates code to use Login struct instead of EncryptedLogin.
Removes nested struct calls ( like .secFields, .fields ) since data structures are now flat.
Removes LoginEntryFlattened since LoginEntry is flat in the new implementation.
Removes deprecated methods and imports.
Adds getKey to implement KeyManager interface.

⚠️ NOTE 1
I ran tests locally and the only failure is not related:

Also tested locally adding/deleting/updating and everything seems to work correctly.

⚠️ NOTE 2
We can only merge this once this PR merges and a new A-S swift component is created. We will need to update the A-S version in this PR as well for BR to pass.

⚠️ NOTE 3
This PR is big enough. I will address other nice-to-haves in follow-ups:

~~FXIOS-11049: Use is_empty for HasSyncedLogins which doesn't require decryption~~ Tackled in this PR as well.
FXIOS-11050: Use has_logins_by_base_domain for domain search related operations.
FXIOS-5603: Remove code using Deferred<Maybe<...>>() since it's hard to read and instead use completions.

📝 Checklist

You have to check all boxes before merging

Filled in the above information (tickets numbers and description of your work)
Updated the PR name to follow our PR naming guidelines
Wrote unit tests and/or ensured the tests suite is passing
When working on UI, I checked and implemented accessibility (minimum Dynamic Text and VoiceOver)
If needed, I updated documentation / comments for complex code and public methods
If needed, added a backport comment (example @Mergifyio backport release/v120)

issammani · 2025-01-13T17:25:46Z

Adding do-not-merge label since we can only merge this once the A-S version is bumped.

nbhasin2 · 2025-01-13T23:07:24Z

firefox-ios/Storage/Rust/RustLogins.swift

+        let key = rustKeys.keychain.string(forKey: rustKeys.loginPerFieldKeychainKey)
+        let encryptedCanaryPhrase = rustKeys.keychain.string(forKey: rustKeys.canaryPhraseKey)
+        return (key, encryptedCanaryPhrase)


no need for background call here?

I had some code before that called getKeychainData in the background and removed it 😓 Looking at it now, I think it makes sense to keep it in the background still, since I am not sure how compute intensive it is. Good catch !

nbhasin2 · 2025-01-13T23:08:07Z

firefox-ios/Storage/Rust/RustLogins.swift

+            let rustKeys = RustLoginEncryptionKeys()
+            let (key, encryptedCanaryPhrase) = getKeychainData(rustKeys: rustKeys)


Suggested change

let rustKeys = RustLoginEncryptionKeys()

let (key, encryptedCanaryPhrase) = getKeychainData(rustKeys: rustKeys)

let rustKeys = RustLoginEncryptionKeys()

let (key, encryptedCanaryPhrase) = getKeychainData(rustKeys: rustKeys)

strange indent

nbhasin2 · 2025-01-13T23:08:50Z

firefox-ios/Storage/Rust/RustLogins.swift

+        // This method will be called internally by rust when it needs to encrypt/decrypt logins.
+        // NOTE: Since this is called internally by rust and each CRUD operation will acquire a mutex lock on the db
+        // before doing anything we must make sure that getStoredKey is called before doing any CRUD operation in swift.
+        public func getKey() throws -> Data {


Note: should put an example somewhere or show how this works for reference

nbhasin2

Overall looks

mergify · 2025-01-20T09:24:06Z

This pull request has conflicts when rebasing. Could you fix it @issammani? 🙏

mergify · 2025-01-21T14:43:23Z

This pull request has conflicts when rebasing. Could you fix it @issammani? 🙏

DonalMe · 2025-01-21T15:13:23Z

@issammani #24265 is the Application Services bump that contains mozilla/application-services#6469
If you're ready with this PR?

mobiletest-ci-bot · 2025-01-22T20:09:38Z

	Messages
📖	Project coverage: 34.14%
📖	Edited 25 files
📖	Created 1 files

Client.app: Coverage: 32.37

File	Coverage
BrowserViewController.swift	3.52%	⚠️
AddCredentialViewController.swift	2.62%	⚠️
Authenticator.swift	0.0%	⚠️
MockLoginProvider.swift	100.0%	✅
LoginListViewModel.swift	64.44%	✅
CredentialAutofillCoordinator.swift	57.34%	✅
LoginCellView.swift	0.0%	⚠️
PasswordManagerSelectionHelper.swift	100.0%	✅
LoginsHelper.swift	0.0%	⚠️
LoginRecordExtension.swift	0.0%	⚠️
LoginDataSource.swift	32.05%	⚠️
PasswordDetailViewController.swift	2.51%	⚠️
LoginStorage.swift	51.61%	✅
PasswordManagerViewModel.swift	33.77%	⚠️

CredentialProvider.appex: Coverage: 21.41

File	Coverage
LoginRecordExtension.swift	0.0%	⚠️

NotificationService.appex: Coverage: 25.99

File	Coverage
LoginRecordExtension.swift	0.0%	⚠️

ShareTo.appex: Coverage: 31.42

File	Coverage
LoginRecordExtension.swift	0.0%	⚠️

libStorage.a: Coverage: 56.12

File	Coverage
RustLogins.swift	32.5%	⚠️
MockMZKeychainWrapper.swift	100.0%	✅

Generated by 🚫 Danger Swift against 5c056ce

issammani · 2025-01-23T03:34:04Z

firefox-ios/firefox-ios-tests/Tests/XCUITests/LoginsTests.swift

-            } else {
-                XCTAssertEqual(app.tables["Login List"].cells.count, defaultNumRowsLoginsList + 2)
-            }
+            XCTAssertEqual(app.tables["Login List"].cells.count, defaultNumRowsLoginsList + 2)


@isabelrios The conditional check was needed before because in Bitrise keychain is not accessible and the code failed silently on writes. So everytime the test attempted to add a login it generated a new encryption key and deleted the old logins, resulting in having only one login stored at a time on BR. Now that I am mocking keychain the key should be retrieved so we have access to both logins ( or any n number of logins ) on CI.
( confirmed by adding some debug logs on another BR run ⏬ )

great, thank you!

issammani requested a review from nbhasin2 January 13, 2025 17:24

issammani requested a review from a team as a code owner January 13, 2025 17:24

issammani added the Do Not Merge ⛔️ This issue is a work in progress and is not ready to land label Jan 13, 2025

issammani changed the title ~~Refactor FXIOS-10788 [Logins] Apply necessary adjustments for A-S EncryptorDecryptor updates~~ Refactor FXIOS-10788 [Logins] Apply necessary adjustments for A-S EncryptorDecryptor update Jan 13, 2025

nbhasin2 reviewed Jan 13, 2025

View reviewed changes

nbhasin2 approved these changes Jan 14, 2025

View reviewed changes

jo mentioned this pull request Jan 15, 2025

EncryptorDecryptor Trait for Logins Component mozilla/application-services#6469

Merged

5 tasks

issammani force-pushed the feat/update-enc-dec branch from 9238a45 to 8ee6f31 Compare January 21, 2025 11:27

issammani had a problem deploying to github-pages January 21, 2025 11:27 — with GitHub Actions Failure

issammani force-pushed the feat/update-enc-dec branch from 3a6e775 to 8ee6f31 Compare January 21, 2025 14:42

issammani added 5 commits January 21, 2025 15:47

chore: update EncryptedLogin to Login and fix compile errors

abdfd2d

chore: fix tests

2f17d7b

fix: lint

37c07bd

fix: address feedback

a19cb09

chore: manually update spm

33af679

issammani force-pushed the feat/update-enc-dec branch from 8ee6f31 to 33af679 Compare January 21, 2025 14:50

fix: use isEmpty instead of listLogins

8158414

issammani mentioned this pull request Jan 21, 2025

Refactor [v136] Auto update SPM with latest rust-component 136.0.20250121100536 #24265

Closed

issammani force-pushed the feat/update-enc-dec branch 2 times, most recently from eb022e8 to e7f06db Compare January 21, 2025 16:35

fix: revert spm file

239fb83

issammani force-pushed the feat/update-enc-dec branch 3 times, most recently from 30eac48 to 8536b0c Compare January 21, 2025 17:45

issammani force-pushed the feat/update-enc-dec branch 4 times, most recently from 0ce0fca to 94f9815 Compare January 21, 2025 19:39

issammani mentioned this pull request Jan 22, 2025

Refactor [v136] Auto update SPM with latest rust-component 136.0.20250122050310 #24283

Merged

issammani force-pushed the feat/update-enc-dec branch 5 times, most recently from dcd7e44 to 1451fac Compare January 22, 2025 17:27

feat: use .data from KeychainWrapper

dbe8916

issammani force-pushed the feat/update-enc-dec branch from 1451fac to 5c9cadc Compare January 22, 2025 19:57

fix: add mock for MZKeychainWrapper

a610d90

issammani force-pushed the feat/update-enc-dec branch from 5c9cadc to a610d90 Compare January 22, 2025 22:38

issammani requested a review from a team as a code owner January 22, 2025 23:46

issammani requested a review from isabelrios January 22, 2025 23:46

fix: failing UI test

ebe047b

issammani force-pushed the feat/update-enc-dec branch from ef1f91d to ebe047b Compare January 23, 2025 00:22

fix: lint

403ffe9

issammani force-pushed the feat/update-enc-dec branch from 700c7a3 to 403ffe9 Compare January 23, 2025 02:01

issammani added 3 commits January 23, 2025 03:26

fix: bump table cell count for BR

76e46fd

fix: remove conditional checking of logins cells

8587d2e

chore: trigger ci

5c056ce

issammani commented Jan 23, 2025

View reviewed changes

isabelrios approved these changes Jan 23, 2025

View reviewed changes

issammani merged commit 3de5980 into main Jan 23, 2025
13 checks passed

issammani deleted the feat/update-enc-dec branch January 23, 2025 13:16

data-sync-user mentioned this pull request Jan 23, 2025

[Logins] Use is_empty from A-S for hasSyncedLogins #24106

Closed

issammani removed the Do Not Merge ⛔️ This issue is a work in progress and is not ready to land label Jan 23, 2025

		let rustKeys = RustLoginEncryptionKeys()
		let (key, encryptedCanaryPhrase) = getKeychainData(rustKeys: rustKeys)

Refactor FXIOS-10788 [Logins] Apply necessary adjustments for A-S EncryptorDecryptor update #24108

Refactor FXIOS-10788 [Logins] Apply necessary adjustments for A-S EncryptorDecryptor update #24108

Uh oh!

Conversation

issammani commented Jan 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📜 Tickets

💡 Description

📝 Checklist

Uh oh!

issammani commented Jan 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nbhasin2 left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Jan 20, 2025

Uh oh!

mergify bot commented Jan 21, 2025

Uh oh!

DonalMe commented Jan 21, 2025

Uh oh!

mobiletest-ci-bot commented Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Client.app: Coverage: 32.37

CredentialProvider.appex: Coverage: 21.41

NotificationService.appex: Coverage: 25.99

ShareTo.appex: Coverage: 31.42

libStorage.a: Coverage: 56.12

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

issammani commented Jan 13, 2025 •

edited

Loading

mobiletest-ci-bot commented Jan 22, 2025 •

edited

Loading