Add support for multiple Valets within the same access group #297

efirestone · 2023-05-12T03:37:32Z

Previously, the shared access group identifier was used for both the access group itself, as well as the uniqueness identifier for the given Valet. This adds an optional additional identifier parameter that can be specified when creating a shared access group Valet. The identifier adds an additional element of uniqueness, so two Valets with the same shared access group can exist, and their data will not overlap, so long as they have different identifiers.

The default for this value is nil, which keeps the existing behavior for full backward compatibility.

dfed

Overall this seems reasonable to me. We're taking on a bit more complexity, but it's not too shabby. I'd love to better understand the use case for this, and why we're only adding this functionality to shared access Valets: is the use case specific to shared access Valets or is your current need currently scoped to shared access Valets?

Also: thoughts on updating the README to document this new API?

Gemfile

Sources/Valet/Valet.swift

dfed · 2023-05-12T03:41:31Z

Tests/ValetIntegrationTests/ValetIntegrationTests.swift

@@ -317,6 +318,19 @@ class ValetIntegrationTests: XCTestCase
        }
    }

+    func test_stringForKey_withDifferingIdentifierInSameAccessGroup_throwsItemNotFound() throws


Should we add the opposite as well: same identifier, different shared access group identifier?

dfed · 2023-05-12T03:42:41Z

Tests/ValetIntegrationTests/ValetIntegrationTests.swift

+            XCTAssertEqual(error as? KeychainError, .itemNotFound)
+        }
+    }
+


It might be nice to add a test that migrating between two different valets with different identifiers works as expected. (I know the test matrix is big + annoying but the more tests we have the more confident we are that this stuff actually works)

The migration matrix doesn't seem to care about the types of the Valets that are involved in the migration, and more about the variations and keys and values. For example, there's nothing testing the migration of iCloud Valets to group access Valets to local Valets, etc. And I think that makes sense given that the migration uses the standard get/set/remove methods in all cases, and those are tested separately for each type.

Happy to add something here, but not entirely sure which tests of value are missing.

Eh that's fair. I was going off memory. If we don't have similar tests then we're good to go here. I would, however, like to see test coverage improve (based on the code of comment), but I'm currently in mobile so I haven't checked details. Let me know if I can help offer possible tests to up coverage.

The Codecov report says The diff coverage is 100.00%., so I think all the new content is covered. It went down overall, but I'm not sure how that's calculated.

codecov-commenter · 2023-05-12T03:55:03Z

Codecov Report

Merging #297 (1a9a224) into master (4ca4f30) will decrease coverage by 4.30%.
The diff coverage is 100.00%.

❗ Current head 1a9a224 differs from pull request most recent head 7bfe6ae. Consider uploading reports for the commit 7bfe6ae to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #297      +/-   ##
==========================================
- Coverage   86.81%   82.52%   -4.30%     
==========================================
  Files          16       16              
  Lines         986      990       +4     
==========================================
- Hits          856      817      -39     
- Misses        130      173      +43

Impacted Files	Coverage Δ
Sources/Valet/Internal/Service.swift	`97.29% <100.00%> (+0.11%)`	⬆️
Sources/Valet/SecureEnclave.swift	`35.41% <100.00%> (-16.67%)`	⬇️
Sources/Valet/SecureEnclaveValet.swift	`63.52% <100.00%> (-12.67%)`	⬇️
Sources/Valet/SinglePromptSecureEnclaveValet.swift	`52.52% <100.00%> (-23.24%)`	⬇️
Sources/Valet/Valet.swift	`94.05% <100.00%> (-0.50%)`	⬇️

efirestone · 2023-05-12T04:29:35Z

Is the use case specific to shared access Valets or is your current need currently scoped to shared access Valets?

Our use case is only for shared group access Valets. The same thing could easily be extended to iCloud and other shared Valets as well, but I figured I'd start here to scope it down. Happy to add the other cases in this PR or a follow up too though.

Also: thoughts on updating the README to document this new API?

Yup, good call. On it.

These changes happened automatically as a result of using a newer Xcode version

efirestone · 2023-05-12T05:39:57Z

Is the use case specific to shared access Valets or is your current need currently scoped to shared access Valets?

Our use case is only for shared group access Valets. The same thing could easily be extended to iCloud and other shared Valets as well, but I figured I'd start here to scope it down. Happy to add the other cases in this PR or a follow up too though.

Also: thoughts on updating the README to document this new API?

Yup, good call. On it.

Updated the README and added the ability to add uniqueness identifiers to the other shared types (iCloud, SecureEnclave, SinglePromptSecureEnclave).

dfed

LGTM with a few changes to the README.

README.md

dfed · 2023-05-12T16:56:42Z

README.md

+It is possible to create multiple Valets within the same shared access group be specifying the same group identifier but different unique identifiers, like so:
+
+```
+let valet1 = Valet.sharedGroupValet(with: groupID, identifier: Identifier(nonEmpty: "valet1")!, accessibility: accessibility)
+let valet2 = Valet.sharedGroupValet(with: groupID, identifier: Identifier(nonEmpty: "valet2")!, accessibility: accessibility)
+```
+
+These two Valets cannot access each others data, but they are both accessible from all applications or extensions which have access to the given `groupID`. The `identifier` (but not the `groupIdentifier`) is optional on shared Valets. All Valets created within the same group using the default `identifier` can access the same data.
+


let's move this blurb up to the Valet section and then replace this with:

Suggested change

It is possible to create multiple Valets within the same shared access group be specifying the same group identifier but different unique identifiers, like so:

```

let valet1 = Valet.sharedGroupValet(with: groupID, identifier: Identifier(nonEmpty: "valet1")!, accessibility: accessibility)

let valet2 = Valet.sharedGroupValet(with: groupID, identifier: Identifier(nonEmpty: "valet2")!, accessibility: accessibility)

```

These two Valets cannot access each others data, but they are both accessible from all applications or extensions which have access to the given `groupID`. The `identifier` (but not the `groupIdentifier`) is optional on shared Valets. All Valets created within the same group using the default `identifier` can access the same data.

As with Valets, shared iCloud Valets can be created with an additional identifier, allowing multiple independently sandboxed keychains to exist within the same shared group.

@efirestone we still want this blurb but we want it further up in the README 😉

@dfed can you merge this? I don't have permissions 🙏

@efirestone can we address this comment first? Happy to merge once addressed.

I'm not sure I'm following what you're suggesting (and now the review recommendation can't be committed). Are you saying put this blurb in the Valet section:

As with Valets, shared iCloud Valets can be created with an additional identifier, allowing multiple independently sandboxed keychains to exist within the same shared group.

or to leave that blurb in the "Sharing Secrets" section, and then put the original text up in the Valet section?

Ah! Okay I see where I failed to read here. It's the additional in additional identifier that threw me.

The additional is referring to iCloud Valets, whereas Valets have a primary identifier.

I think we should just remove the "As with Valets" preamble and this'll make more sense to me.

efirestone · 2023-05-15T18:23:11Z

@dfed can you merge this? I don't have permissions 🙏

NickEntin · 2023-05-19T20:40:38Z

Sources/Valet/Valet.swift

    ///   - accessibility: The desired accessibility for the Valet.
+    ///   - identifier: An optional additional uniqueness identifier. Using this identifier allows for the creation of separate, sandboxed Valets within the same shared access group.


nit: Reorder these to match the parameter order.

NickEntin · 2023-05-19T20:46:05Z

Tests/ValetIntegrationTests/ValetIntegrationTests.swift

@@ -317,6 +318,19 @@ class ValetIntegrationTests: XCTestCase
        }
    }

+    func test_stringForKey_withDifferingIdentifierInSameAccessGroup_throwsItemNotFound() throws


Should we add the opposite as well: same identifier, different shared access group identifier?

Previously, the shared access group identifier was used for both the access group itself, as well as the uniqueness identifier for the given Valet. This adds an optional additional `identifier` parameter that can be specified when creating a shared access group Valet. The identifier adds an additional element of uniqueness, so two Valets with the same shared access group can exist, and their data will not overlap, so long as they have different identifiers. The default for this value is `nil`, which keeps the existing behavior for full backward compatibility.

efirestone · 2023-06-02T03:22:42Z

"As with Valets" prefix removed in the README, so should be good to go.

The iOS 13 + iOS 14 builds aren't finishing anymore, I'm guessing due to a GitHub change. It looks like master is having the same problem now. I'm investigating some in that PR, but so far I'm kind of stuck.

dfed · 2023-06-02T03:29:00Z

"As with Valets" prefix removed in the README, so should be good to go.

The iOS 13 + iOS 14 builds aren't finishing anymore, I'm guessing due to a GitHub change. It looks like master is having the same problem now. I'm investigating some in that PR, but so far I'm kind of stuck.

Thanks for addressing!

Re Xcode 12.4: can you file an issue on https://github.com/actions/runner-images? I've had success there in the past when there was a runner configuration issue.

dfed · 2023-06-08T20:13:24Z

Just filed an issue for what we're seeing with CI: actions/runner-images#7687

dfed · 2023-06-09T20:12:17Z

Just merged latest into this branch. Should turn green, at which point merge at will @efirestone. Let me know if you want help publishing the feature version update.

dfed · 2023-06-12T15:50:38Z

Just released this as 4.2.0

efirestone force-pushed the firestone/valet-group-with-id branch from 6dda0ec to d1b4537 Compare May 12, 2023 03:43

dfed reviewed May 12, 2023

View reviewed changes

efirestone added 2 commits May 11, 2023 23:31

Fix warning about missing global source in Gemfile

afaf66d

Commit Xcode-generated changes

b84269e

These changes happened automatically as a result of using a newer Xcode version

efirestone force-pushed the firestone/valet-group-with-id branch 2 times, most recently from b03bd08 to bb3f87a Compare May 12, 2023 05:36

dfed approved these changes May 12, 2023

View reviewed changes

efirestone force-pushed the firestone/valet-group-with-id branch 2 times, most recently from 608dfff to 1f058ad Compare May 12, 2023 19:37

NickEntin approved these changes May 19, 2023

View reviewed changes

efirestone force-pushed the firestone/valet-group-with-id branch from 1f058ad to 90de005 Compare May 31, 2023 17:31

dfed approved these changes Jun 1, 2023

View reviewed changes

efirestone added 2 commits June 1, 2023 22:20

Bump version to 4.2.0

7bfe6ae

efirestone force-pushed the firestone/valet-group-with-id branch from 90de005 to 7bfe6ae Compare June 2, 2023 03:20

dfed mentioned this pull request Jun 5, 2023

Modernize CI #300

Merged

Merge branch 'master' into firestone/valet-group-with-id

d326057

dfed merged commit 89f12b9 into square:master Jun 9, 2023

dfed mentioned this pull request Oct 14, 2024

Adopt Swift 6 and Xcode 16 #322

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for multiple Valets within the same access group #297

Add support for multiple Valets within the same access group #297

efirestone commented May 12, 2023

dfed left a comment

dfed May 12, 2023

NickEntin May 19, 2023

efirestone May 31, 2023

dfed May 12, 2023

efirestone May 12, 2023

dfed May 12, 2023

efirestone May 12, 2023

codecov-commenter commented May 12, 2023 •

edited

Loading

efirestone commented May 12, 2023 •

edited

Loading

efirestone commented May 12, 2023

dfed left a comment

dfed May 12, 2023

dfed May 12, 2023

dfed May 15, 2023

efirestone May 31, 2023

dfed Jun 1, 2023

efirestone commented May 15, 2023

NickEntin May 19, 2023

NickEntin May 19, 2023

efirestone commented Jun 2, 2023

dfed commented Jun 2, 2023

dfed commented Jun 8, 2023

dfed commented Jun 9, 2023

dfed commented Jun 12, 2023

		/// - accessibility: The desired accessibility for the Valet.
		/// - identifier: An optional additional uniqueness identifier. Using this identifier allows for the creation of separate, sandboxed Valets within the same shared access group.

Add support for multiple Valets within the same access group #297

Add support for multiple Valets within the same access group #297

Conversation

efirestone commented May 12, 2023

dfed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented May 12, 2023 • edited Loading

Codecov Report

efirestone commented May 12, 2023 • edited Loading

efirestone commented May 12, 2023

dfed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efirestone commented May 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efirestone commented Jun 2, 2023

dfed commented Jun 2, 2023

dfed commented Jun 8, 2023

dfed commented Jun 9, 2023

dfed commented Jun 12, 2023

codecov-commenter commented May 12, 2023 •

edited

Loading

efirestone commented May 12, 2023 •

edited

Loading