Add biometrics #42
Add biometrics #42
Conversation
# Conflicts: # Cely Demo/AppDelegate.swift # Cely Demo/User.swift # Sources/CelyConstants.swift # Sources/LocksmithError.swift # Sources/Storage/CelyKeychain.swift # Sources/Storage/CelySecureStorage.swift # Tests/CelyTests.swift
…`Result` with public facing methods
Adding keychain items such as "limitOne", "returnData", "class", "server" was causing errors.
.gitignore
Outdated
| docs/ | ||
|
|
||
| Cely.xcworkspace/xcshareddata/IDEWorkspaceChecks.plist |
There was a problem hiding this comment.
Files in xcshareddata should not be ignored per https://developer.apple.com/library/archive/releasenotes/DeveloperTools/RN-Xcode/Chapters/Introduction.html
There was a problem hiding this comment.
Thanks for looking out!
Sources/Core/CelyConstants.swift
Outdated
| @@ -77,3 +58,8 @@ public extension UIWindow { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| public enum AccessibilityOptions { | |||
There was a problem hiding this comment.
This name is confusing, because these options don't seem to have anything to do with Accessibility in the sense of providing access to people with disabilities. Would something along the lines of AccessControlOptions be more in line?
There was a problem hiding this comment.
Great suggestion!
I believe this is a much better name.
| func set(query: KeychainObject) throws { | ||
| // try adding first | ||
| let newQuery = query.toSetMap(withValue: true) | ||
| let status: OSStatus = SecItemAdd(newQuery as CFDictionary, nil) |
| } | ||
| } | ||
|
|
||
| func set(_ value: Any?, forKey key: String, securely _: Bool = true, persisted _: Bool = true) throws { |
There was a problem hiding this comment.
Why are we even providing securely and persisted options if neither of those values are used?
There was a problem hiding this comment.
This is because CelySecureStorage implements CelyStorageProtocol.
Purpose:
Add biometrics functionality.
New Cely Credentials API
Please refer to documentation docs for usage. I know I pulled the trigger a little early around the documentation site, buuuttttt I wanted to focus on the experience around documentation and have that drive the code for this release. Please give your honest opinion and API design and potential shortcomings.
Cely Documentation
This is going to be something new and entirely different so don't feel responsible for this part of the code. It uses
mkdocsto generate the site. But if you are able to check out the docs and give your thoughts that would be amazing! I understand all this was made in a cave and should be viewed as a First Draft.What did I do?
CelyCredentialsAPICelyDemo/BiometricsLoginViewController.swiftthat in the Cely Demo Target that uses newCelyCredentialsAPICelyDemo/Appdelegate.swiftto point toBiometricsLoginViewController.swiftfor testingCelyDemo/SettingsViewController.swiftthat logs credentials to the consoleStorageResultwith SwiftResultCelyStorageProtocol.removeAllData()withclearStorage()CelyKeychainKeychainObject(Keychain Items)CelySecureStorageKeychainObjectand storing them withCelyKeychainCelyStorageProtocol(.clearStorage()gets called fromCelyStoragewhen user is logging out)KeychainObjectKeychainItem. (I'm not entirely in love with how this came out aroundtoGetMap/toLookupMapsince they aren't really descriptive on what they're used for.)Notes
When I created this PR, it was to finally get the code in front of someone else. In terms of what this PR sought out to do (add the biometrics functionality), it does just that. but as far as how we get there, that's where I have thoughts. Things such as should we allow the developer to use
kSecClassGenericPasswordinstead ofkSecClassInternetPasswordor usekSecAttrAccessibleWhenUnlockedinstead ofkSecAttrAccessibleWhenUnlockedThisDeviceOnly. Definitely read Cely's documentation over Understanding Keychain to form an opinion on what Cely should/shouldn't do.In all, I would say this PR is 97% done. Really the only thing I don't like is around
KeychainObject, but that class is only used internally and not exposed to the user/developer. so I'm not sure if we want to give that class the green light for now and just improve it at a later time.Lastly, this PR did a lot. I really wish it didn't, but I wanted to do one big push for v3 to get the ball rolling.
How to Test: