[Valet 4.0] Require that App ID Prefix be explicitly passed into Shared Access Group Valets #218
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
NickEntin
reviewed
Feb 25, 2020
README.md
Outdated
| @@ -128,14 +128,14 @@ In addition to allowing the storage of strings, Valet allows the storage of `Dat | |||
| ### Sharing Secrets Among Multiple Applications | |||
|
|
|||
| ```swift | |||
| let mySharedValet = Valet.sharedAccessGroupValet(with: Identifier(nonEmpty: "Druidia")!, accessibility: .whenUnlocked) | |||
| let mySharedValet = Valet.sharedAccessGroupValet(with: Identifier(nonEmpty: "AppID12345.Druidia")!, accessibility: .whenUnlocked) | |||
There was a problem hiding this comment.
I think it might be cleaner to create a distinct SharedAccessGroupIdentifier that takes two separate strings (the shared access group id and the Valet id), rather than using Identifier here. That way it is more obvious what this identifier is, and consumers don't have to know that it needs to be formatted as two strings concatenated with a period.
There was a problem hiding this comment.
I like this a lot. Made code changes in 5078ffc. Changes to documentation incoming.
dfed
dismissed
fdiaz’s stale review
Dismissing Francisco's review since major changes have occurred since he reviewed
Codecov Report
@@ Coverage Diff @@
## develop--4.0 #218 +/- ##
================================================
- Coverage 85.77% 85.44% -0.34%
================================================
Files 14 15 +1
Lines 1097 1065 -32
================================================
- Hits 941 910 -31
+ Misses 156 155 -1
|
dfed
commented
Feb 25, 2020
Sources/Valet/Valet.swift
Outdated
| @@ -377,10 +386,10 @@ public final class Valet: NSObject { | |||
| case .standard: | |||
| serviceAttribute = Service.standard(with: configuration, identifier: identifier, accessibilityDescription: accessibilityDescription) | |||
| #if os(macOS) | |||
| case .sharedAccessGroupOverride: | |||
| serviceAttribute = Service.sharedAccessGroup(with: configuration, explicitlySetIdentifier: identifier, accessibilityDescription: accessibilityDescription) | |||
There was a problem hiding this comment.
This was a bug before (only on the develop--4.0 branch).
It occurs to me that I need to enable explicitly set identifiers that use shared access groups more thought. Right now you can only name your explicitly set shared access group to use the same identifier as your keychain entitlement, which isn't terribly human-readable. I can tackle this in a follow-up PR, as this is an existing shortcoming.
fdiaz
approved these changes
Apr 12, 2020
dfed
changed the title
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 <[email protected]> * 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 removes the magic that automatically determined the shared access group.
There are two big drivers behind this PR:
sharedAccessGroupPrefixreturned the old App ID, where on iOS 13 thesharedAccessGroupPrefixreturned our new App ID. This non-determinism was only found on App Store signed builds – debug builds worked. Given the possibility of incorrect behavior by this code, we believe it is best to be explicit.Note that I need to add this change to the Valet 4 migration guide prior to merging Valet 4 to
master. I'm tracking that in #179 (comment)