Updated Credentials Manager #180
Conversation
Renamed Touch Internals -> BioAuthentication Deprecated enableTouchAuth method -> enableBiometricAuth for clarity Expand README Docs Update Circle CI Xcode Version SwiftLint Corrections
cocojoe
force-pushed
the
updated-credentials-manager-face-id
branch
from
9196bdc
to
c94d77e
Compare
README.md
Outdated
| #### Retrieve stored credentials | ||
|
|
||
| Credentials will automatically be renewed (if expired) using the refresh token. The scope `offline_access` is required | ||
| to ensure the refreshtToken is returned. |
There was a problem hiding this comment.
remove the new line after "required". Also typo. I would use "refresh token" instead.
README.md
Outdated
| @@ -207,6 +212,15 @@ credentialsManager.credentials { error, credentials in | |||
| } | |||
| ``` | |||
|
|
|||
| #### Biometric authentication | |||
|
|
|||
| You can enable an additional level of user authentication before retrieving credentials using the biometric authentication | |||
There was a problem hiding this comment.
remove the new line after "biometric" and "authentication" (2nd)
Auth0/CredentialsManager.swift
Outdated
| /// Enable Device Biometric Authentication for additional securtity during credentials retrieval. | ||
| /// | ||
| /// - Parameters: | ||
| /// - title: main message to display in TouchID prompt |
There was a problem hiding this comment.
parameters talk about "touch" id. Should be changed to "bio" something
There was a problem hiding this comment.
Well it is only used during a touch prompt, it's not valid for face id. could be more verbose perhaps 'display when using touch id'
There was a problem hiding this comment.
I don't know how this works as haven't tested myself. So when a face is expected no message is displayed, nothing that can be customized?
There was a problem hiding this comment.
No, it just comes up with this standard image / text. https://support.apple.com/library/content/dam/edam/applecare/images/en_US/ios11-iphone-x-wallet-apple-pay-animation.gif
Auth0/CredentialsManager.swift
Outdated
| /// | ||
| /// - Parameters: | ||
| /// - title: main message to display in TouchID prompt | ||
| /// - cancelTitle: cancel message to display in TouchID prompt (iOS 10+) | ||
| /// - fallbackTitle: fallback message to display in TouchID prompt after a failed match | ||
| @available(*, deprecated, message: "see enableBioAuth(withTitle title:, cancelTitle:, fallbackTitle:)") |
There was a problem hiding this comment.
so just deprecated because of the name change? No difference in the parameters??
There was a problem hiding this comment.
Yes, for the sake of clarity it should be the more generic bio as touchid might imply to some developers that it is ONLY for touch
There was a problem hiding this comment.
The new name is better but deprecating the method just because of the name doesn't sound right to me. Make this call 👍
There was a problem hiding this comment.
Additional comment: Message says "see enableBioAuth" but the method is called "enableBiometricAuth". Also if these methods are going to have the same logic, make one call the other instead of copy-pasting the logic.
There was a problem hiding this comment.
Made that change in the most recent commit.
Renamed Touch Internals -> BioAuthentication
Deprecated enableTouchAuth method -> enableBiometrics for clarity
Expand README Docs
Update Circle CI Xcode Version