Support sharing keychain items using App Groups #230
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dfed
force-pushed
the
dfed--app-group
branch
2 times, most recently
from
9ea5837 to
5018835
Compare
…e SharedAccessGroup -> SharedGroup
Codecov Report
@@ Coverage Diff @@
## develop--4.0 #230 +/- ##
================================================
+ Coverage 85.30% 85.68% +0.38%
================================================
Files 15 15
Lines 966 992 +26
================================================
+ Hits 824 850 +26
Misses 142 142
|
NickEntin
reviewed
May 29, 2020
Tests/ValetIntegrationTests/SinglePromptSecureEnclaveIntegrationTests.swift
Show resolved
Hide resolved
NickEntin
reviewed
May 29, 2020
NickEntin
reviewed
May 29, 2020
NickEntin
reviewed
May 29, 2020
Valet.xcodeproj/project.pbxproj
Outdated
| "CODE_SIGN_IDENTITY[sdk=iphoneos*]" = "iPhone Developer"; | ||
| CODE_SIGN_STYLE = Automatic; | ||
| DEBUG_INFORMATION_FORMAT = dwarf; | ||
| DEVELOPMENT_TEAM = ""; | ||
| DEVELOPMENT_TEAM = 9XUJ7M53NG; |
There was a problem hiding this comment.
Where did this development team ID come from?
There was a problem hiding this comment.
This is the valet-open-source@squareup.com identifier. This likely shouldn't have been committed.
dfed
commented
May 29, 2020
NickEntin
reviewed
May 29, 2020
| /// A representation of a shared app group identifier. | ||
| /// - Parameters: | ||
| /// - groupPrefix: On iOS, iPadOS, watchOS, and tvOS, this prefix must equal "group". On macOS, this prefix is the application's App ID prefix, which can be found by inspecting the application's provisioning profile, or viewing the application's App ID Configuration on developer.apple.com. This string must not be empty. | ||
| /// - groupIdentifier: An identifier that cooresponds to a value in com.apple.security.application-groups in the application's Entitlements file. This string must not be empty. |
There was a problem hiding this comment.
Suggested change
| /// - groupIdentifier: An identifier that cooresponds to a value in com.apple.security.application-groups in the application's Entitlements file. This string must not be empty. | |
| /// - groupIdentifier: An identifier that corresponds to a value in com.apple.security.application-groups in the application's Entitlements file. This string must not be empty. |
| /// - groupPrefix: On iOS, iPadOS, watchOS, and tvOS, this prefix must equal "group". On macOS, this prefix is the application's App ID prefix, which can be found by inspecting the application's provisioning profile, or viewing the application's App ID Configuration on developer.apple.com. This string must not be empty. | ||
| /// - groupIdentifier: An identifier that cooresponds to a value in com.apple.security.application-groups in the application's Entitlements file. This string must not be empty. | ||
| /// - SeeAlso: https://developer.apple.com/documentation/security/keychain_services/keychain_items/sharing_access_to_keychain_items_among_a_collection_of_apps | ||
| public init?(groupPrefix: String, nonEmptyGroup groupIdentifier: String?) { |
There was a problem hiding this comment.
Should we enforce the group prefix on non-macOS by having separate inits?
#if os(macOS)
public init?(groupPrefix: String, nonEmptyGroup groupIdentifier: String?) {
#else
public init?(nonEmptyGroup groupIdentifier: String?) {
let groupPrefix = Self.appGroupPrefix
#endifThere was a problem hiding this comment.
We'd need to put the entire initializer in the #if (#if in Swift is way less permissive than Objective-C), but that'd be doable. I can separate that out.
There was a problem hiding this comment.
Actually, looking at this more: I think having the groupPrefix makes it clear what the groupIdentifier should be. If I make a single-parameter initializer for non-macOS platforms, then I think I'd need to accept groupIdentifier's that have group. as a prefix, as well as ones that do not. I Kina like this API being explicitly not magical. Thoughts?
There was a problem hiding this comment.
I think making the groupIdentifier only take the non-prefix string is fine as long as we're clear in the headerdoc what the parameter expects (i.e. "this is the identifier following the 'group.' in the entitlement").
I agree with your point that having the groupPrefix makes it more obvious, but I think that's outweighed by having a parameter that only accepts a single string. Don't feel strongly though.
There was a problem hiding this comment.
If we switch to separate initializers, we could also clean up the macOS case by removing the concept of a "group prefix" and specifying that it's the app id prefix.
init?(nonEmptyGroup groupIdentifier: String?)
init?(appIDPrefix: String, nonEmptyGroup groupIdentifier: String?)There was a problem hiding this comment.
I like that! But the tough thing is we already have this initializer for shared access groups:
init?(appIDPrefix: String, nonEmptyGroup groupIdentifier: String?)
There's a good bit of overlap between how these features work. I'm leaning hard towards keeping the APIs explicit for now. We can always add new convenience initializers down the line.
| /// - groupPrefix: On iOS, iPadOS, watchOS, and tvOS, this prefix must equal "group". On macOS, this prefix is the application's App ID prefix, which can be found by inspecting the application's provisioning profile, or viewing the application's App ID Configuration on developer.apple.com. This string must not be empty. | ||
| /// - groupIdentifier: An identifier that cooresponds to a value in com.apple.security.application-groups in the application's Entitlements file. This string must not be empty. | ||
| /// - SeeAlso: https://developer.apple.com/documentation/security/keychain_services/keychain_items/sharing_access_to_keychain_items_among_a_collection_of_apps | ||
| public init?(groupPrefix: String, nonEmptyGroup groupIdentifier: String?) { |
There was a problem hiding this comment.
Does groupIdentifier need to be optional? Given the non-empty requirement, it feels like we should also make it non-optional.
There was a problem hiding this comment.
We made it optional in the other method above. I think it was a convenience, similar to how Identifier works. Open to changing this everywhere. LMK how you're thinking about this.
There was a problem hiding this comment.
That's fine, happy to match the existing API.
Tests/ValetIntegrationTests/SinglePromptSecureEnclaveIntegrationTests.swift
Show resolved
Hide resolved
Co-authored-by: Nick Entin <entin@squareup.com>
NickEntin
approved these changes
May 30, 2020
dfed
added a commit
that referenced
this pull request
Jun 13, 2020
* Do not build branch build on every push on PR branches * Update cocoapods * Start validating podspec on Xcode 11 * Drop Xcode 9 and 10 support * Swift version to 5.0 * Bump minor version of osx_image on .travis.yml in order to access simulators for older OSes * Bump destination for iOS 11 to get CI working * Update tests to reflext iOS 13 simulator's inability to store items that require a passcode to be set * Enable running CI on iOS 10, tvOS 10, and watchOS 3 * Allow kSecAttrService to be a customer-friendly string on Mac * Add a section to the README on choosing the best identifier on a Mac * Update README documentation on choosing a user-friendly identifier * Add ObjC compat layer for new initializers * Remove Always accessibility specifier * Ensure test environment is signed before testing shared access keychain * Add migration helper methods * Use throws rather than return types to indicate error. * Get rid of ErrorHandler * couldNotReadKeychain -> .couldNotAccessKeychain * Make Objective-C bridging methods for accessing values with prompt redundant * Swift 5 updates * Bring API in line with Apple's naming guidelines * Adopt Swift 5 syntax, and enable support for SinglePromptSecureEnclaveValet on tvOS * Fix migrateObjectsFromAlwaysAccessible methods * Fix warning introduced by merge * Bump version to 4.0.0 * Get new migration methods working with Catalina * Update README * Set up code coverage * Run more tests on a single machine * Standardize method naming * Run test coverage on every target * Modernize doc comments * Remove returns * Use SeeAlso * Modernize doc comments * Add Warning * Add objc example * findOrCreate(explicitlySet must use a key that combines service, configuration, accessibility, and sharedAccessGroup to prevent returning the wrong Valet * Utilize testEnvironmentIsSigned before using shared keychains * Use explicitlySetSharedAccessGroupIdentifier when dealing with shared access groups * removeAllObjects() to avoid collisions in tests * Fail test on setup failure * Update whitespace * containsObject(forKey should throw in Swift * Introduce Throws doc comment * Catch closer to the source * Add simple Objective-C compatibility layer tests * Update copyright * Fixup whitespace * try? less in tests * If deleting items throws, then we should surface the failure. * Remove runtime assert, since we will throw the error anyways * Update Mac tests to use try on containsObject * Add description to KeychainError * Fix macOS tests after throwing on removeAllObjects in setUp * Increase test coverage of error files * Use permutation valet rather than vanillaValet multiple times * Indentation and test separation * Add comment re why we're checking for errSecInteractionNotAllowed * Better comment formatting * Use Throws rather than Note * Add final to test classes * Rename internal containsObject methods to performCopy * [Valet 4.0] Add explicit tests for CloudAccessibility (#210) * [Valet 4.0] Get SinglePromptSecureEnclaveIntegrationTests running on tvOS (#209) * Get SinglePromptSecureEnclaveIntegrationTests running on tvOS * Make SinglePromptSecureEnclaveValet available on tvOS 11, not tvOS 10 * [Valet 4.0] Add explicit tests for Configuration (#211) * Use CaseIterable instead of allValues where possible (#212) * Get ValetTouchIDTest building again * Require that App ID Prefix be explicitly passed into Shared Access Group Valets (#218) * Require that App ID Prefix be explicitly passed into Shared Access Group Valets * Add App ID prefix to tests * Create and adopt SharedAccessGroupIdentifier * Update documentation * [Valet 4.0] Update migration guide (#221) * Update migration guide for Valet 4.0 * NickEntin feedback Co-Authored-By: Nick Entin <nckentn@gmail.com> * Remove version from Package.swift (#223) * Add headerdoc comment for removing an object from the keychain * Update headerdoc comments for parameters of type SharedAccessGroupIdentifier * Update headerdoc comment for migration method * Update headerdoc comments for objc compatibility methods * Rename MigrationError cases with `InQueryResult` to `ToMigrate` (#227) * Rename InQueryResult -> ToMigrate * Update comments * Fix typo in README (#229) * Create 'Changing an Accessibility Value After Persisting Data' section in README (#232) * Use correct Valet name in README example * Create Changing an Accessibility Value After Persisting Data section * Get watchOS tests running locally (#233) * Support sharing keychain items using App Groups (#230) * Add App Group group.valet.test * Update syntax for Swift 5 * Enable SharedAccessGroup code to semantically handle AppGroups. Rename SharedAccessGroup -> SharedGroup * README updates * Add sharedAppGroupIdentifier tests to Valet * Add sharedAppGroupIdentifier test to SecureEnclave * Add sharedAppGroupIdentifier test to SinglePromptSecureEnclave * Add objective-c compatibility layer
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR resolves #222. This PR is best reviewed commit-by-commit.