-
Notifications
You must be signed in to change notification settings - Fork 224
Support sharing keychain items using App Groups #230
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
c20b9c4
2aab94f
58d070e
472c42c
7edd31b
31d3a02
53632ce
40bf6b9
6368754
cd02f28
67b7571
903a071
fa682b1
90ec0d5
050d046
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| // | ||
| // SharedAccessGroupIdentifier.swift | ||
| // SharedGroupIdentifier.swift | ||
| // Valet | ||
| // | ||
| // Created by Dan Federman on 2/25/20. | ||
|
|
@@ -21,7 +21,7 @@ | |
| import Foundation | ||
|
|
||
|
|
||
| public struct SharedAccessGroupIdentifier: CustomStringConvertible { | ||
| public struct SharedGroupIdentifier: CustomStringConvertible { | ||
|
|
||
| // MARK: Initialization | ||
|
|
||
|
|
@@ -35,23 +35,47 @@ public struct SharedAccessGroupIdentifier: CustomStringConvertible { | |
| return nil | ||
| } | ||
|
|
||
| self.appIDPrefix = appIDPrefix | ||
| self.prefix = appIDPrefix | ||
| self.groupIdentifier = groupIdentifier | ||
| } | ||
|
|
||
| /// 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 corresponds 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?) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We made it optional in the other method above. I think it was a convenience, similar to how
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's fine, happy to match the existing API. |
||
| #if os(macOS) | ||
| guard !groupPrefix.isEmpty, let groupIdentifier = groupIdentifier, !groupIdentifier.isEmpty else { | ||
| return nil | ||
| } | ||
| #else | ||
| guard groupPrefix == Self.appGroupPrefix, let groupIdentifier = groupIdentifier, !groupIdentifier.isEmpty else { | ||
| return nil | ||
| } | ||
| #endif | ||
|
|
||
| self.prefix = groupPrefix | ||
| self.groupIdentifier = groupIdentifier | ||
| } | ||
|
|
||
| // MARK: CustomStringConvertible | ||
|
|
||
| public var description: String { | ||
| return appIDPrefix + "." + groupIdentifier | ||
| prefix + "." + groupIdentifier | ||
| } | ||
|
|
||
| // MARK: Internal Properties | ||
|
|
||
| internal let appIDPrefix: String | ||
| internal let prefix: String | ||
| internal let groupIdentifier: String | ||
|
|
||
| internal var asIdentifier: Identifier { | ||
| // It is safe to force unwrap because we've already validated that our description is non-empty. | ||
| Identifier(nonEmpty: description)! | ||
| } | ||
|
|
||
| // MARK: Private Static Properties | ||
|
|
||
| private static let appGroupPrefix = "group" | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we enforce the group prefix on non-macOS by having separate inits?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd need to put the entire initializer in the
#if(#ifin 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, looking at this more: I think having the
groupPrefixmakes it clear what thegroupIdentifiershould be. If I make a single-parameter initializer for non-macOS platforms, then I think I'd need to acceptgroupIdentifier's that havegroup.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think making the
groupIdentifieronly 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
groupPrefixmakes 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that! But the tough thing is we already have this initializer for shared access groups:
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.