Add support for multiple Valets within explicitly set shared groups#357
Add support for multiple Valets within explicitly set shared groups#357antons wants to merge 3 commits intosquare:mainfrom
Conversation
dfed
left a comment
dfed
left a comment
There was a problem hiding this comment.
I'm not entirely sure I understand what problem is being solved here. Our contributing guidelines suggest creating an issue first so we can align before we're at the code-review stage.
Since we're already at the PR stage, the PR description itself needs to be much more fleshed out. What is the motivation? What change is being proposed? Is this change backwards compatible? What should someone adopting this proposed API take into consideration?
Given the potential for foot guns that this proposed API has, I think we likely need to improve the in-code documentation as well. But we can do that once we're aligned on what we're solving.
| /// Creates a shared-access-group Valet with an explicitly set kSecAttrService. This API is intended for use with macOS applications where service identifiers can be user-facing. | ||
| /// - Parameters: | ||
| /// - identifier: The identifier for the Valet's shared access group. Must correspond with the value for keychain-access-groups in your Entitlements file. Must be unique relative to other Valet identifiers. | ||
| /// - accessibility: The desired accessibility for the Valet. | ||
| /// - Returns: A Valet that reads/writes keychain elements that can be shared across applications written by the same development team. | ||
| /// - Warning: Using an explicitly set kSecAttrService bypasses this project’s guarantee that one Valet type will not have access to one another type’s key:value pairs. To maintain this guarantee, ensure that each Valet’s identifier is globally unique. | ||
| /// - SeeAlso: https://github.com/square/Valet/issues/140 | ||
| public class func sharedGroupValet(withExplicitlySet identifier: SharedGroupIdentifier, accessibility: Accessibility) -> Valet { | ||
| findOrCreate(explicitlySet: identifier, configuration: .valet(accessibility)) | ||
| public class func sharedGroupValet(withExplicitlySet groupIdentifier: SharedGroupIdentifier, identifier: Identifier? = nil, accessibility: Accessibility) -> Valet { | ||
| findOrCreate(explicitlySet: groupIdentifier, identifier: identifier, configuration: .valet(accessibility)) | ||
| } |
There was a problem hiding this comment.
need to update documentation comment given the new parameters
| @@ -98,8 +98,8 @@ | |||
| /// - Returns: A Valet (synchronized with iCloud) that reads/writes keychain elements that can be shared across applications written by the same development team. | |||
| /// - Warning: Using an explicitly set kSecAttrService bypasses this project’s guarantee that one Valet type will not have access to one another type’s key:value pairs. To maintain this guarantee, ensure that each Valet’s identifier is globally unique. | |||
| /// - SeeAlso: https://github.com/square/Valet/issues/140 | |||
| public class func iCloudSharedGroupValet(withExplicitlySet identifier: SharedGroupIdentifier, accessibility: CloudAccessibility) -> Valet { | |||
| findOrCreate(explicitlySet: identifier, configuration: .iCloud(accessibility)) | |||
| public class func iCloudSharedGroupValet(withExplicitlySet groupIdentifier: SharedGroupIdentifier, identifier: Identifier? = nil, accessibility: CloudAccessibility) -> Valet { | |||
| static func sharedGroup(with configuration: Configuration, explicitlySetIdentifier identifier: Identifier, accessibilityDescription: String) -> String { | ||
| "VAL_\(configuration.description)_initWithSharedAccessGroupIdentifier:accessibility:_\(identifier)_\(accessibilityDescription)" | ||
| } |
There was a problem hiding this comment.
previously it was possible to have an explicitly set identifier that was not a shared group identifier. it looks like we've removed that ability? is this a breaking change?
There was a problem hiding this comment.
This was not a public API. As far as I can see, this was effectively dead code. It was only referenced by the migration path from v3 Valets, and shared group Valets with explicitly set identifiers did not exist in v3.
Sources/Valet/Valet.swift
Outdated
| .sharedGroupValet(withExplicitlySet: identifier, accessibility: accessibility) | ||
| .sharedGroupValet(withExplicitlySet: identifier, identifier: nil, accessibility: accessibility) |
|
@dfed I’m sorry about the terse description. At this point it’s been more than 6 months since I implemented this, at the same time as #348 (which took many months to merge due to CLA issues), so I had to dive into this again: #297 skipped the (at that time macOS-only) explicitly set identifier API. Shared group Valets continued to use This PR adds the ability to provide an explicitly set This API shares all the caveats of the already-existing explicitly set identifier API. |
2 participants
Following up on #348, this extends #297 to explicitly set shared group Valets.