[Valet 4.0] Convert APIs to throw rather than return booleans or enums #198

dfed · 2019-12-25T23:00:41Z

This PR tries to make Valet more semantic Swift by relying on throws rather than Bool or enum return types. It's a bit of a massive change, but IMO this API feels a whole lot nicer to use.

dfed · 2019-12-26T02:56:25Z

Sources/Valet/MigrationResult.swift

    /// Migration failed because the keychain query was not valid.
    case invalidQuery
-    /// Migration failed because no items to migrate were found.
-    case noItemsToMigrateFound


Instead we'll throw KeychainError.itemNotFound.

dfed · 2019-12-26T02:56:25Z

Sources/Valet/MigrationResult.swift

-    /// Migration failed because no items to migrate were found.
-    case noItemsToMigrateFound
-    /// Migration failed because the keychain could not be read.
-    case couldNotReadKeychain


Instead we'll throw KeychainError.couldNotAccessKeychain.

dfed · 2019-12-26T02:56:25Z

Sources/Valet/MigrationResult.swift

@@ -38,8 +31,6 @@ public enum MigrationResult: Int, Equatable {
    case duplicateKeyInQueryResult
    /// Migration failed because a key in the keychain duplicates a key already managed by Valet.
    case keyInQueryResultAlreadyExistsInValet
-    /// Migration failed because writing to the keychain failed.
-    case couldNotWriteToKeychain


This error was never used.

dfed · 2019-12-26T02:56:25Z

README.md

 NSString *const myLuggageCombination = [myValet stringForKey:username];
 ```

-In addition to allowing the storage of strings, Valet allows the storage of `Data` objects via `set(object: Data, forKey key: Key)` and `-objectForKey:`. Valets created with a different class type, via a different initializer, or with a different accessibility attribute will not be able to read or modify values in `myValet`.


welp. This line needed some help. Key was part of #80, and was removed in e54f35b. Also we'd left in the Objective-C description of the method, rather than the Swift one.

dfed · 2019-12-26T02:56:25Z

Sources/Valet/ErrorHandler.swift

-import Foundation
-
-
-public final class ErrorHandler {


We no longer needed a hot-swappable ErrorHandler. We now throw when an error occurs. We also have assertionFailure in a few internal methods where the error would very much be a programmer error within Valet.

dfed · 2019-12-26T02:56:25Z

Sources/Valet/Internal/Keychain.swift

-
-        case let .error(status):
-            switch status {
-            case errSecInteractionNotAllowed, errSecMissingEntitlement:


This error handling got consolidated in SecItem.deleteItems

dfed · 2019-12-26T02:56:25Z

Sources/Valet/Internal/SecItem.swift

 }


 internal final class SecItem {

-    // MARK: Internal Enum
-    
-    internal enum DataResult<SuccessType> {


Excited that we can get rid of these types.

(Note we could still get rid of these even if we didn’t move to a throws API, since Swift 5 has a built-in Result type)

dfed · 2019-12-26T02:56:25Z

Sources/Valet/Internal/SecItem.swift

-            return .error(errSecParam)
+    internal static func copy<DesiredType>(matching query: [String : AnyHashable]) throws -> DesiredType {
+        if query.isEmpty {
+            assertionFailure("Must provide a query with at least one item")


We assert, and then continue here. We'll end up throwing a couldNotAccessKeychain later in this method. But if we hit this assertion, we have problems within Valet.

dfed · 2019-12-26T02:56:25Z

Sources/Valet/SinglePromptSecureEnclaveValet.swift

-    /// - returns: The data currently stored in the keychain for the provided key. Returns `nil` if no object exists in the keychain for the specified key, or if the keychain is inaccessible.
-    @available(swift, obsoleted: 1.0)
-    @objc(objectForKey:userPrompt:userCancelled:)
-    public func 🚫swift_object(forKey key: String, withPrompt userPrompt: String, userCancelled: UnsafeMutablePointer<ObjCBool>?) -> Data? {


these methods are no longer necessary, because the error parameter will have the information to determine if a user cancelled.

dfed · 2019-12-26T02:56:25Z

README.md

@@ -101,17 +101,17 @@ The Accessibility enum is used to determine when your secrets can be accessed. I

 ```swift
 let username = "Skroob"
-myValet.set(string: "12345", forKey: username)
+try? myValet.setString("12345", forKey: username)


Thoughts on whether we should show the error being ignored? Both here and in the Objective-C code below. Previously we were discarding the return type, so the current sample-code maintains the prior behavior.

Seems reasonable to keep the example simple. The presence of try? implies that there is an error case that could (should) be handled in real usage.

dfed · 2019-12-26T02:57:26Z

Tests/ValetIntegrationTests/ValetIntegrationTests.swift

-            setQueue.async { XCTAssertTrue(self.valet.set(string: self.passcode, forKey: self.key)) }
-            removeQueue.async { XCTAssertTrue(self.valet.removeObject(forKey: self.key)) }
+            setQueue.async {
+                do {


we need to use a do/catch block because we're within an async () -> Void block that can't throw.

dfed · 2019-12-26T02:58:59Z

cc @bachand @fdiaz @jqsilver in case y'all are interested. Trying to modernize the API a bit before Airbnb adopts.

bachand · 2019-12-29T04:04:21Z

Thanks @dfed

…s yet. Documentation missing.

…dundant

dfed · 2020-01-20T19:40:43Z

I believe Nick's feedback has been fully addressed. This PR should be ready for review again.

Tests/ValetIntegrationTests/ValetIntegrationTests.swift

Sources/Valet/Internal/Keychain.swift

NickEntin · 2020-01-20T23:20:36Z

Sources/Valet/Internal/Keychain.swift

    }

    // MARK: Contains
    
-    internal static func containsObject(forKey key: String, options: [String : AnyHashable]) -> SecItem.Result {
+    internal static func containsObject(forKey key: String, options: [String : AnyHashable]) -> OSStatus {


Should we be exposing OSStatus in the interface here? Seems like Keychain abstracts away the underlying error codes everywhere else except here.

This is an internal method. The call-sites need to inspect the status code in order to make informed decisions. Specifically, the SecureEnclaveValet and SinglePromptSecureEnclaveValet need to test if an authentication failure error occured, whereas the regular Valet does not.

Previously, we included the necessary information in SecItem.Result. I could theoretically create a new intermediary type (we've deleted SecItem.Result in this PR), but since we're not exposing anything publicly that seemed like overkill? I agree that this is a bit of a sharp edge in an otherwise very simple system.

Yeah, that's fair. Given it's all internal, it seems alright.

NickEntin · 2020-01-20T23:20:36Z

Sources/Valet/Internal/Keychain.swift

+            if Keychain.containsObject(forKey: key, options: destinationAttributes) == errSecItemNotFound {
+                keysToMigrate.insert(key)
+            } else {
+                throw MigrationError.keyInQueryResultAlreadyExistsInValet


Should this throw a different error depending on what containsObject(...) returns?

I don't think so? Note that we're currently preserving existing behavior here (we previously returned a keyInQueryResultAlreadyExistsInValet, but now we're throwing). If we want to update this logic, let's do that in a separate PR 🙂

Sources/Valet/Internal/SecItem.swift

NickEntin · 2020-01-20T23:20:36Z

Sources/Valet/Internal/SecItem.swift

+                throw KeychainError.missingEntitlement
+
+            default:
+                // We succeeded as long as we can confirm that the item is not in the keychain.


Would all of the other errors mean that this succeeded? Seems like errSecSuccess is the only one that means it actually got deleted.

This preserves existing behavior that was previously in Keychain.swift. Let's consider improving this code in a separate pass to reduce churn in this PR.

Ahh okay, I thought this was a change in behavior given that it previously returned .error(status). If this was existing, then LGTM.

Let's add a comment here. This does indeed look weird when we know KeychainError has more errors that it could handle. Also, should we make this explicit by handling all cases instead of using default?

I like making it explicit. I think that makes it more clear, and removes the need for a comment. Good thinking!

Addressed in 4e9eb8b

This reverts commit cadeb20.

NickEntin · 2020-01-21T00:14:24Z

Sources/Valet/Internal/SecItem.swift

+                throw KeychainError.missingEntitlement
+
+            default:
+                // We succeeded as long as we can confirm that the item is not in the keychain.


Ahh okay, I thought this was a change in behavior given that it previously returned .error(status). If this was existing, then LGTM.

Tests/ValetIntegrationTests/ValetIntegrationTests.swift

Sources/Valet/KeychainError.swift

Sources/Valet/SecureEnclave.swift

fdiaz

Mostly nits, this looks great!

fdiaz · 2020-01-21T02:45:26Z

Sources/Valet/Internal/Keychain.swift

@@ -48,216 +48,173 @@ internal final class Keychain {
            var secItemQuery = attributes
            secItemQuery[kSecAttrAccount as String] = canaryKey
            secItemQuery[kSecValueData as String] = Data(canaryValue.utf8)
-            _ = SecItem.add(attributes: secItemQuery)
+            try? SecItem.add(attributes: secItemQuery)


Unrelated to this PR but it feels like a method called canAccess wouldn't have side-effects.

the side effect is invisible to the customer. I have a test to prove that the sentinel I'm writing doesn't appear in allKeys.

You're right that this is weird, but I think it's okay because the customer never has to know it happened.

fdiaz · 2020-01-21T02:45:26Z

Sources/Valet/Internal/Keychain.swift

-        }
-
-        return setObject(data, forKey: key, options: options)
+        try setObject(data, forKey: key, options: options)


Much nicer 👍

fdiaz · 2020-01-21T02:45:26Z

Sources/Valet/Internal/Keychain.swift

    }

    // MARK: Contains
    
-    internal static func containsObject(forKey key: String, options: [String : AnyHashable]) -> SecItem.Result {
+    internal static func containsObject(forKey key: String, options: [String : AnyHashable]) -> OSStatus {


nit: osStatus(forKey?

Or alternative, delete the OSStatus return type, making this throw and replace the return type for a Bool by changing:

return SecItem.containsObject(matching: secItemQuery)

to

let status = SecItem.containsObject(matching: secItemQuery) guard status == errSecSuccess { throw KeychainError(status: status) } return status == errSecSuccess

and making this method throw an KeychainError?

Nevermind, I saw below your reply to @NickEntin and also I saw how you're using this and I think it makes sense to keep this not throwing. Maybe we do want to change the method name though since containsObject sounds like something that will return a Bool?

Fair that the name doesn't match the return type. Thoughts on:

Suggested change

internal static func containsObject(forKey key: String, options: [String : AnyHashable]) -> OSStatus {

internal static func performCopy(matchingKey key: String, options: [String : AnyHashable]) -> OSStatus {

I'm honestly struggling with this one. What this method does under the hood is perform a SecItemCopyMatching, but doesn't retrieve the data. It just needs the OSStatus return type from the copy action.

Or maybe

Suggested change

internal static func containsObject(forKey key: String, options: [String : AnyHashable]) -> OSStatus {

internal static func copyItemStatus(forKey key: String, options: [String : AnyHashable]) -> OSStatus {

My vote is for performCopy(matching:). The fact is returns a status is implied by the return type.

Addressed in fd5feba

Works for me! Thanks for addressing.

fdiaz · 2020-01-21T02:45:26Z

Sources/Valet/Internal/SecItem.swift

        }
    }

-    internal static func containsObject(matching query: [String : AnyHashable]) -> Result {
+    internal static func containsObject(matching query: [String : AnyHashable]) -> OSStatus {
        guard query.count > 0 else {


nit: Let's use isEmpty

addressed in 249188f

fdiaz · 2020-01-21T02:45:26Z

Sources/Valet/Internal/SecItem.swift

+                throw KeychainError.missingEntitlement
+
+            default:
+                // We succeeded as long as we can confirm that the item is not in the keychain.


Let's add a comment here. This does indeed look weird when we know KeychainError has more errors that it could handle. Also, should we make this explicit by handling all cases instead of using default?

fdiaz · 2020-01-21T02:45:26Z

Sources/Valet/SecureEnclaveValet.swift

-            }
-            return SecureEnclave.containsObject(forKey: key, options: keychainQuery)
+    /// - Returns: `true` if a value has been set for the given key, `false` otherwise.
+    /// - Note: Will never prompt the user for Face ID, Touch ID, or password. Method will throw a `KeychainError`.


Let's use Throws instead of Note for the KeychainError comment.

Ah! Missed one! Nice catch 🙂.

Resolved in e7023c9

fdiaz · 2020-01-21T02:45:26Z

Sources/Valet/SecureEnclaveValet.swift

@@ -283,7 +257,7 @@ extension SecureEnclaveValet {
        }
        return valet(with: identifier, accessControl: accessControl)
    }
-
+    


nit: Unnecessary empty space

Resolved in d1b2d80

Tests/ValetTests/MigrationErrorTests.swift

fdiaz · 2020-01-21T02:45:26Z

Sources/Valet/MigrationError.swift

@@ -20,16 +20,11 @@

 import Foundation

+


nit: Empty extra space?

Welcome to Square's style guide. Two empty newlines between imports and code 😉

Tests/ValetTests/KeychainErrorTests.swift

dfed force-pushed the dfed--throws-api branch from fb98f75 to f44c6e0 Dec 26, 2019

dfed mentioned this pull request Dec 26, 2019

[Public Beta] Valet 4 wishlist #179

Closed

9 of 10 tasks complete

dfed changed the title ~~[WIP][Valet 4.0] Convert APIs to throw rather than return booleans or enums~~ [Valet 4.0] Convert APIs to throw rather than return booleans or enums Dec 26, 2019

dfed marked this pull request as ready for review Dec 26, 2019

dfed reviewed Dec 26, 2019

View changes

dfed requested review from EricMuller22, NickEntin and gfontenot Dec 26, 2019

dfed mentioned this pull request Dec 29, 2019

[Valet 4.0] Remove .always accessibility specifier #197

Merged

dfed changed the base branch from dfed--remove-always-accessibility-specifier to develop--4.0 Dec 30, 2019

dfed added 17 commits Dec 25, 2019

Use throws rather than return types to indicate error. No test change…

fbe5b8d

…s yet. Documentation missing.

Get rid of ErrorHandler

72bb5e5

couldNotReadKeychain -> .couldNotAccessKeychain

5127024

Commit the error files

eba3e95

Make Objective-C bridging methods for accessing values with prompt re…

c39612c

…dundant

Update tests

5165c3d

Fix warnings

f6d5c6a

Get tests running

4f57bcf

Simpler flow

80e0a71

Swift 5 updates

aece8fb

Update documentation

61fcab5

Update ValetTouchIDTest

5ebe4ae

Update README again

10ebf88

More swift 5

d05479b

More documentation

a735f5b

Remove incorrect documentation

b4c0f27

More documentation

fe21e04

dfed added 3 commits Jan 20, 2020

Add description to KeychainError

f718dc0

Fix macOS tests after throwing on removeAllObjects in setUp

Loading status checks…

265e3c8

Increase test coverage of error files

Loading status checks…

b1d205b

NickEntin reviewed Jan 20, 2020

View changes

dfed added 7 commits Jan 20, 2020

vanillaValet -> valet, anotherFlavor -> iCloudValet

cadeb20

Rename test

f78e319

Revert comment change

7bf227d

Update whitespace

e60f5d3

Update comment formatting

Loading status checks…

2b20666

Revert "vanillaValet -> valet, anotherFlavor -> iCloudValet"

eca2b7d

This reverts commit cadeb20.

Use permutation valet rather than vanillaValet multiple times

Loading status checks…

93b4cef

NickEntin reviewed Jan 21, 2020

View changes

Sources/Valet/SecureEnclave.swift Show resolved Hide resolved

NickEntin approved these changes Jan 21, 2020

View changes

dfed added 3 commits Jan 21, 2020

Indentation and test separation

697e53f

Add comment re why we're checking for errSecInteractionNotAllowed

883ace0

Better comment formatting

Loading status checks…

efeced6

dfed force-pushed the dfed--throws-api branch from 40e8082 to efeced6 Jan 21, 2020

fdiaz approved these changes Jan 21, 2020

View changes

dfed added 7 commits Jan 21, 2020

> 0 -> not isEmpty

Loading status checks…

249188f

Use Throws rather than Note

Loading status checks…

e7023c9

whitespace

Loading status checks…

d1b2d80

Add final to test class

Verified

This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.

GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits

Loading status checks…

029927a

Make test class final

Verified

This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.

GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits

Loading status checks…

40344ef

Expand default

Loading status checks…

4e9eb8b

Rename internal containsObject methods to performCopy

Loading status checks…

fd5feba

dfed merged commit 46924d8 into develop--4.0 Jan 21, 2020
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

license/cla Contributor License Agreement is signed.
Details

dfed deleted the dfed--throws-api branch Jan 21, 2020

Dec	JAN	Feb
	07
2020	2021	2022

	internal static func containsObject(forKey key: String, options: [String : AnyHashable]) -> OSStatus {
	internal static func performCopy(matchingKey key: String, options: [String : AnyHashable]) -> OSStatus {

	internal static func containsObject(forKey key: String, options: [String : AnyHashable]) -> OSStatus {
	internal static func copyItemStatus(forKey key: String, options: [String : AnyHashable]) -> OSStatus {

square / Valet

[Valet 4.0] Convert APIs to throw rather than return booleans or enums #198

[Valet 4.0] Convert APIs to throw rather than return booleans or enums #198

Conversation

dfed commented Dec 25, 2019 • edited

This comment has been minimized.

dfed Dec 26, 2019 Author Collaborator

This comment has been minimized.

dfed Dec 26, 2019 Author Collaborator

This comment has been minimized.

dfed Dec 26, 2019 Author Collaborator

This comment has been minimized.

dfed Dec 26, 2019 Author Collaborator

This comment has been minimized.

dfed Dec 26, 2019 Author Collaborator

This comment has been minimized.

dfed Dec 26, 2019 Author Collaborator

This comment has been minimized.

dfed Dec 26, 2019 Author Collaborator

This comment has been minimized.

dfed Dec 29, 2019 Author Collaborator

This comment has been minimized.

dfed Dec 26, 2019 Author Collaborator

This comment has been minimized.

dfed Dec 26, 2019 Author Collaborator

This comment has been minimized.

dfed Dec 26, 2019 Author Collaborator

This comment has been minimized.

NickEntin Jan 19, 2020 Collaborator

This comment has been minimized.

dfed Dec 26, 2019 Author Collaborator

dfed commented Dec 26, 2019 • edited

bachand commented Dec 29, 2019

dfed commented Jan 20, 2020

This comment has been minimized.

NickEntin Jan 20, 2020 Collaborator

This comment has been minimized.

dfed Jan 20, 2020 Author Collaborator

This comment has been minimized.

dfed Jan 20, 2020 Author Collaborator

This comment has been minimized.

NickEntin Jan 20, 2020 Collaborator

This comment has been minimized.

NickEntin Jan 20, 2020 Collaborator

This comment has been minimized.

dfed Jan 20, 2020 Author Collaborator

This comment has been minimized.

NickEntin Jan 20, 2020 Collaborator

This comment has been minimized.

dfed Jan 20, 2020 Author Collaborator

This comment has been minimized.

NickEntin Jan 21, 2020 Collaborator

This comment has been minimized.

fdiaz Jan 21, 2020 Collaborator

This comment has been minimized.

dfed Jan 21, 2020 Author Collaborator

This comment has been minimized.

NickEntin Jan 21, 2020 Collaborator

fdiaz left a comment

This comment has been minimized.

fdiaz Jan 21, 2020 Collaborator

This comment has been minimized.

dfed Jan 21, 2020 Author Collaborator

This comment has been minimized.

fdiaz Jan 21, 2020 Collaborator

This comment has been minimized.

fdiaz Jan 21, 2020 Collaborator

This comment has been minimized.

fdiaz Jan 21, 2020 Collaborator

This comment has been minimized.

fdiaz Jan 21, 2020 Collaborator

This comment has been minimized.

dfed Jan 21, 2020 Author Collaborator

This comment has been minimized.

dfed Jan 21, 2020 Author Collaborator

This comment has been minimized.

NickEntin Jan 21, 2020 Collaborator

This comment has been minimized.

dfed Jan 21, 2020 Author Collaborator

This comment has been minimized.

dfed commented Dec 25, 2019 •

edited

dfed Dec 26, 2019
Author Collaborator

dfed Dec 26, 2019
Author Collaborator

dfed Dec 26, 2019
Author Collaborator

dfed Dec 26, 2019
Author Collaborator

dfed Dec 26, 2019
Author Collaborator

dfed Dec 26, 2019
Author Collaborator

dfed Dec 26, 2019
Author Collaborator

dfed Dec 29, 2019
Author Collaborator

dfed Dec 26, 2019
Author Collaborator

dfed Dec 26, 2019
Author Collaborator

dfed Dec 26, 2019
Author Collaborator

NickEntin Jan 19, 2020
Collaborator

dfed Dec 26, 2019
Author Collaborator

dfed commented Dec 26, 2019 •

edited

NickEntin Jan 20, 2020
Collaborator

dfed Jan 20, 2020
Author Collaborator

dfed Jan 20, 2020
Author Collaborator

NickEntin Jan 20, 2020
Collaborator

NickEntin Jan 20, 2020
Collaborator

dfed Jan 20, 2020
Author Collaborator

NickEntin Jan 20, 2020
Collaborator

dfed Jan 20, 2020
Author Collaborator

NickEntin Jan 21, 2020
Collaborator

fdiaz Jan 21, 2020
Collaborator

dfed Jan 21, 2020
Author Collaborator

NickEntin Jan 21, 2020
Collaborator

fdiaz Jan 21, 2020
Collaborator

dfed Jan 21, 2020
Author Collaborator

fdiaz Jan 21, 2020
Collaborator

fdiaz Jan 21, 2020
Collaborator

fdiaz Jan 21, 2020
Collaborator

fdiaz Jan 21, 2020
Collaborator

dfed Jan 21, 2020
Author Collaborator

dfed Jan 21, 2020
Author Collaborator

NickEntin Jan 21, 2020
Collaborator

dfed Jan 21, 2020
Author Collaborator

fdiaz Jan 21, 2020
Collaborator

fdiaz Jan 21, 2020
Collaborator

dfed Jan 21, 2020
Author Collaborator

fdiaz Jan 21, 2020
Collaborator

fdiaz Jan 21, 2020
Collaborator

dfed Jan 21, 2020
Author Collaborator

fdiaz Jan 21, 2020
Collaborator

dfed Jan 21, 2020
Author Collaborator

fdiaz Jan 21, 2020
Collaborator

dfed Jan 21, 2020
Author Collaborator