[BITAU-137] Create AuthenticatorSyncKit SDK #913

brant-livefront · 2024-09-10T16:49:33Z

🎟️ Tracking

📔 Objective

This PR adds the basic structure for sharing an AuthenticatorSyncShared Swift package from the main PM app repo. This allows us to share all of the code between the PM and Authenticator apps for building the sync feature, but doesn't require us to make a whole new git repo for managing it (it is simply shared from this repo).

This PR is just about getting the basic structure set up correctly and getting feedback on this approach. However, there is the first piece of shared code SharedKeychainRepository which adds the ability to read/write keys in the shared keychain via the App Group.

The next step after this PR is merged would then be a PR from the Authenticator repo to pull in this via Swift Package.

⏰ Reminders before review

Contributor guidelines followed
All formatters and local linters executed and passed
Written new unit and / or integration tests where applicable
Protected functional changes with optionality (feature flags)
Used internationalization (i18n) for all UI strings
CI builds passed
Communicated to DevOps any deployment requirements
Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

👍 (:+1:) or similar for great changes
📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
❓ (:question:) for questions
🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
🎨 (:art:) for suggestions / improvements
❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
⛏ (:pick:) for minor or nitpick changes

github-actions · 2024-09-10T17:03:29Z

Checkmarx One – Scan Summary & Details – 531c2169-217d-4fab-bb23-8faeee013698

No New Or Fixed Issues Found

codecov · 2024-09-10T17:51:33Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.64%. Comparing base (121a26d) to head (80f221e).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #913      +/-   ##
==========================================
+ Coverage   88.62%   88.64%   +0.01%     
==========================================
  Files         620      621       +1     
  Lines       39200    39261      +61     
==========================================
+ Hits        34742    34803      +61     
  Misses       4458     4458

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

KatherineInCode · 2024-09-10T17:46:09Z

Configs/AuthenticatorSyncShared.xcconfig

+#include "./Common.xcconfig"
+#include? "./Local.xcconfig"
+
+PRODUCT_BUNDLE_IDENTIFIER = $(ORGANIZATION_IDENTIFIER).AuthenticatorSyncShared


🎨 All of our other bundle IDs are kebab-cased as near as I can tell; it probably makes sense for us to continue that

Fixed in next commit. 👍

KatherineInCode · 2024-09-10T17:48:36Z

project.yml

@@ -55,12 +55,14 @@ schemes:
        - BitwardenAutoFillExtension
        - BitwardenShareExtension
        - BitwardenShared
+        - AuthenticatorSyncShared


🎨 Can this be alphabetized?

I assume you mean the whole file? (i.e. it should be the first target as well). I'll have it updated to that in the next commit

We're not the most consistent about alphabetization with the targets (notably, we put BitwardenTests after Bitwarden but before BitwardenActionExtension), but yeah, in general if we have a list of things that doesn't have an enforced order, by convention we like to alphabetize them.

Updated in the latest commit. Let me know if we need to adjust anything else 👍

KatherineInCode · 2024-09-10T17:52:31Z

Package.swift

+    ],
+    products: [
+        .library(
+            name: "AuthenticatorSyncShared",


🤔 I'm not super in love with this name, though I can't think of anything better. Just, if in the future we want to put more things in a shared library between the Password Manager and Authenticator apps, keeping it in the same package makes sense, but the name might not at that point. Probably moot, realistically, but something to think about

🤔 I see your point. We could possibly name it like the App Group and go with BitwardenAuthenticatorShared? That would simply imply where it's used (in both apps) rather than relating it just to sync.

Alternatively, we could keep this one scoped to just Sync (maybe even name it simply SyncShared) and set a pattern for future packages to be targeted to just one feature? i.e. The main package right now is BitwardenShared, but we are only sharing one product (AuthenticatorSyncShared). If you wanted to share Feature X in the future, you could make a FeatureXShared product/target and share via this same mechanism, rather than putting it also in the same product to share. That would add a bit more on the Authenticator side, however, as you'd need to explicitly pull in each product that you're interested in using.

Hm. I see merits to both approaches. If we did it by-feature, I'd suggest actually maybe AuthenticationSyncShared, so that it's more clearly "this is syncing authentication things".

But also having a broader one that makes it easier in the future for us to just move things from BitwardenShared to it in order to share between the two projects... (though personally, I would have called that BitwardenShared so I don't know 😂 )

@fedemkr Do you have a strong opinion on whether we want to name more broadly or do a per-feature naming, or something else entirely?

Naming is hard, can't think of a really good name for this 🤔 .

I think it depends on the scope (and possible future scope) of this. I would guess that what we're possibly going to share between the two apps would be auth data and syncing the vault.
Additionally, I wouldn't put general "helpers" or controls shared between any potential apps as I would add these kind of stuff to an independent package we can reuse amongst all current/future apps without nothing specific of a particular one, e.g. we could also potentially use this future package on the Watch app (I guess for now it doesn't hurt much adding this here until we have the independent package if we really need to)

Having said this I'm fine with the particular naming for now to denote what the package is about and helps not to include general stuff in there (unless really needed). Furthermore, we can revisit the naming while we include stuff if needed.
I thought about the next ones but not strongly in favor of any of these nor thinking they're better than the original one, although don't really like much the suffix Shared as we're using that for internal targets:

AuthenticatorSyncCore

AuthenticatorSyncEngine

AuthenticatorSyncManager

AuthenticatorSyncKit

AuthenticatorSyncFoundation

I'm only a little cautious about naming here because changing the name of a package like this, when it's shared, could be mildly disruptive between the two projects.

I do actually like Kit as a suffix; that's something I've used before for this sort of thing, now that I think about it, and it's kind of how Apple likes it. AuthenticationSyncKit? AuthenticatorSyncKit? Something like that seems like it would be good.

Yeah, yesterday reviewed Apple's naming and Kit and Foundation are usually used as suffixes 😄 . Totally agree let's go with AuthenticatorSyncKit 😄 (I wouldn't go for Authentication given that if I recall correctly this would also allow the app to send ciphers from one place to the other)

Sounds good to me!

Sounds good to me as well - I'll migrate to AuthenticatorSyncKit 👍

@KatherineInCode @fedemkr OK, I think I've got all the names changed over. Latest commit should have everything running with the new name. But do be on the lookout for anything I might have missed.

fedemkr · 2024-09-10T18:28:37Z

AuthenticatorSyncShared/SharedKeychainRepository.swift

+
+    // MARK: Methods
+
+    /// Retrieve the value for the specifi item from the Keychain Service.


⛏️ Typo

Suggested change

/// Retrieve the value for the specifi item from the Keychain Service.

/// Retrieve the value for the specific item from the Keychain Service.

👍 will fix in next commit

fedemkr · 2024-09-10T18:46:00Z

AuthenticatorSyncShared/SharedKeychainRepository.swift

+        if let resultDictionary = foundItem as? [String: Any],
+           let data = resultDictionary[kSecValueData as String] as? Data {
+            return data
+        } else {
+            throw AuthenticatorKeychainServiceError.keyNotFound(item)
+        }


⛏️ Invert using guard to avoid extra branches:

Suggested change

if let resultDictionary = foundItem as? [String: Any],

let data = resultDictionary[kSecValueData as String] as? Data {

return data

} else {

throw AuthenticatorKeychainServiceError.keyNotFound(item)

}

guard let resultDictionary = foundItem as? [String: Any],

let data = resultDictionary[kSecValueData as String] as? Data else {

throw AuthenticatorKeychainServiceError.keyNotFound(item)

}

return data

👍 will fix in next commit

fedemkr · 2024-09-10T19:23:59Z

AuthenticatorSyncShared/Tests/SharedKeychainRepositoryTests.swift

+
+@testable import AuthenticatorSyncShared
+
+final class SharedKeychainRepositoryTests: XCTestCase {


🤔 Could we add a base class for XCTestCase and get some of BitwardenTestCase methods into it? Like assertAsyncThrows which are really handy to use. Additionally, I'd like to have a base class for tests in case we need to do changes affecting the entire test suite.

Sounds good. I was trying to make sure we didn't duplicate too much, but totally understand wanting to have one base class to work from 👍 assertAsyncThrows will definitely be handy to have. I'll add in the next commit.

Added this in the latest commit. I took out the UI-related stuff (partially because we don't have UI for this in the package, but also because that had some BitwardenShared dependencies as well).

fedemkr · 2024-09-10T22:02:55Z

AuthenticatorSyncShared/Tests/SharedKeychainRepositoryTests.swift

+        do {
+            _ = try await subject.getAuthenticatorKey()
+            XCTFail("Expected error `keyNotFound` was never thrown!")
+        } catch let caughtError as AuthenticatorKeychainServiceError {
+            XCTAssertEqual(caughtError, error)
+        } catch let caughtError {
+            XCTFail(
+                "The error caught (\(caughtError)) does not match the type of error provided (\(error))."
+            )
+        }


🎨 assertAsyncThrows can be used instead of this to be more clean leveraging the new AuthenticatorSyncSharedTestCase.

This should be updated in the latest.

fedemkr

Looks good, just a few tests missing.

fedemkr · 2024-09-11T18:29:58Z

AuthenticatorSyncKit/SharedKeychainRepository.swift

+
+        guard let resultDictionary = foundItem as? [String: Any],
+              let data = resultDictionary[kSecValueData as String] as? Data else {
+            throw AuthenticatorKeychainServiceError.keyNotFound(item)


⛏️ Could tests be added for when foundItem is not [String: Any] and another one for when resultDictionary[kSecValueData as String] is not of Data type?

Good catch 👍 Added those in the latest.

fedemkr

Awesome thanks! Sorry but I've just realized of something else I didn't see in the past reviews regarding test naming.

fedemkr · 2024-09-12T11:41:04Z

AuthenticatorSyncKit/Tests/SharedKeychainRepositoryTests.swift

+
+    /// Verify that `deleteAuthenticatorKey()` issues a delete with the correct search attributes specified.
+    ///
+    func testDeleteAuthenticatorKey() async throws {


🎨 Use the same naming as we do in the main app:
test_{subjectFuncName}_{additionalContext}
So in this case it would be:
test_deleteAuthenticatorKey_success()
This also applies to the other tests in this file.

👍 should be fixed in latest

…at we're not ready for release.

brant-livefront · 2024-09-12T13:40:36Z

@fedemkr @victor-livefront Sorry, I realized right before merging that we should have a bundle version of 0.1.0 on the Swift package, rather than 1.0. We've got more things to put in there before it's 1.0 and that matches what the Android side is doing as well.

This dismissed your approvals, so I'll need a re-approval before I merge.

brant-livefront added 2 commits September 10, 2024 11:18

[BITAU-137] Create AuthenticatorSyncShared SDK

ee7f3b7

Fix typo in doc comment

8b82eab

brant-livefront requested review from fedemkr, victor-livefront and KatherineInCode September 10, 2024 16:49

Merge branch 'main' into brant/create-authenticator-sync-shared-SDK

25ab43d

KatherineInCode reviewed Sep 10, 2024

View reviewed changes

fedemkr reviewed Sep 10, 2024

View reviewed changes

brant-livefront added 2 commits September 10, 2024 16:19

Merge branch 'main' into brant/create-authenticator-sync-shared-SDK

fd89c3e

Updates to respond to PR feedback

793bd46

fedemkr reviewed Sep 10, 2024

View reviewed changes

brant-livefront added 3 commits September 11, 2024 12:22

Migrated to use assertAsyncThrows now that we have made it available

d772d9a

Merge branch 'main' into brant/create-authenticator-sync-shared-SDK

d1ae7a7

10000 Rename AuthenticatorSyncShared to AuthenticatorSyncKit

07cf635

fedemkr changed the title ~~[BITAU-137] Create AuthenticatorSyncShared SDK~~ [BITAU-137] Create AuthenticatorSyncKit SDK Sep 11, 2024

fedemkr reviewed Sep 11, 2024

View reviewed changes

brant-livefront added 2 commits September 11, 2024 17:24

Added additional tests. Cleaned up doc comments and naming

6464998

Merge branch 'main' into brant/create-authenticator-sync-shared-SDK

503c888

fedemkr reviewed Sep 12, 2024

View reviewed changes

Updated Test method names

439f079

fedemkr previously approved these changes Sep 12, 2024

View reviewed changes

victor-livefront previously approved these changes Sep 12, 2024

View reviewed changes

Updated marketing version to 0.1.0 to match Android and to reflect th…

80f221e

…at we're not ready for release.

brant-livefront dismissed stale reviews from victor-livefront and fedemkr via 80f221e September 12, 2024 13:37

victor-livefront approved these changes Sep 12, 2024

View reviewed changes

brant-livefront merged commit 6af83fc into main Sep 12, 2024
8 checks passed

brant-livefront deleted the brant/create-authenticator-sync-shared-SDK branch September 12, 2024 13:52


		// MARK: Methods

		/// Retrieve the value for the specifi item from the Keychain Service.

	/// Retrieve the value for the specifi item from the Keychain Service.
	/// Retrieve the value for the specific item from the Keychain Service.


		@testable import AuthenticatorSyncShared

		final class SharedKeychainRepositoryTests: XCTestCase {

[BITAU-137] Create AuthenticatorSyncKit SDK #913

[BITAU-137] Create AuthenticatorSyncKit SDK #913

Uh oh!

Conversation

🎟️ Tracking

📔 Objective

⏰ Reminders before review

🦮 Reviewer guidelines

Uh oh!

Uh oh!

No New Or Fixed Issues Found

Uh oh!

Uh oh!

Codecov Report

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!

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!