[Valet 4.0] Convert APIs to throw rather than return booleans or enums by dfed · Pull Request #198 · square/Valet

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:04:56Z

Sources/Valet/MigrationError.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:05:09Z

Sources/Valet/MigrationError.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:05:21Z

Sources/Valet/MigrationError.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:07:19Z

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:17:08Z

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:20:13Z

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:26:26Z

Sources/Valet/Internal/SecItem.swift


-    // 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:27:43Z

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:39:00Z

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:55:15Z

README.md

 ```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:04:49Z

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:11:57Z

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:03Z

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-20T23:48:43Z

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-18T22:15:51Z

Sources/Valet/Internal/Keychain.swift

            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-18T22:20:29Z

Sources/Valet/Internal/Keychain.swift

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


Much nicer 👍

fdiaz · 2020-01-18T22:21:32Z

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:27:47Z

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:32:50Z

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:35:36Z

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:36:53Z

Sources/Valet/SecureEnclaveValet.swift

        return valet(with: identifier, accessControl: accessControl)
    }
-
+    


nit: Unnecessary empty space

Resolved in d1b2d80

Tests/ValetTests/MigrationErrorTests.swift

fdiaz · 2020-01-21T02:42:52Z

Sources/Valet/MigrationError.swift


 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 Compare December 26, 2019 00:45

dfed mentioned this pull request Dec 26, 2019

[Public Beta] Valet 4 wishlist #179

Closed

10 tasks

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 December 26, 2019 02:01

dfed commented Dec 26, 2019

View reviewed changes

dfed requested review from EricMuller22, NickEntin and gfontenot December 26, 2019 02:57

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 December 30, 2019 03:33

dfed added 17 commits December 29, 2019 19:39

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 4 commits January 19, 2020 10:09

Update Mac tests to use try on containsObject

1a6fade

Add description to KeychainError

f718dc0

Fix macOS tests after throwing on removeAllObjects in setUp

265e3c8

Increase test coverage of error files

b1d205b

NickEntin reviewed Jan 20, 2020

View reviewed changes

dfed added 7 commits January 20, 2020 15:24

vanillaValet -> valet, anotherFlavor -> iCloudValet

cadeb20

Rename test

f78e319

Revert comment change

7bf227d

Update whitespace

e60f5d3

Update comment formatting

2b20666

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

eca2b7d

This reverts commit cadeb20.

Use permutation valet rather than vanillaValet multiple times

93b4cef

NickEntin reviewed Jan 21, 2020

View reviewed changes

Sources/Valet/SecureEnclave.swift Show resolved Hide resolved

NickEntin approved these changes Jan 21, 2020

View reviewed changes

dfed added 3 commits January 20, 2020 17:29

Indentation and test separation

697e53f

Add comment re why we're checking for errSecInteractionNotAllowed

883ace0

Better comment formatting

efeced6

dfed force-pushed the dfed--throws-api branch from 40e8082 to efeced6 Compare January 21, 2020 01:32

fdiaz approved these changes Jan 21, 2020

View reviewed changes

dfed and others added 7 commits January 20, 2020 18:56

> 0 -> not isEmpty

249188f

Use Throws rather than Note

e7023c9

whitespace

d1b2d80

Add final to test class

029927a

Make test class final

40344ef

Expand default

4e9eb8b

Rename internal containsObject methods to performCopy

fd5feba

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

dfed deleted the dfed--throws-api branch January 21, 2020 16:11


		// MARK: Internal Enum

		internal enum DataResult<SuccessType> {

	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 {

		return valet(with: identifier, accessControl: accessControl)
		}

Conversation

dfed commented Dec 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dfed commented Dec 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bachand commented Dec 29, 2019

Uh oh!

dfed commented Jan 20, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fdiaz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dfed commented Dec 25, 2019 •

edited

Loading

dfed commented Dec 26, 2019 •

edited

Loading