Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reuse LocalAuthentication context #74

Merged
merged 2 commits into from
Feb 4, 2020
Merged

Reuse LocalAuthentication context #74

merged 2 commits into from
Feb 4, 2020

Conversation

eaceto
Copy link
Contributor

@eaceto eaceto commented Dec 5, 2019

Changes

Please describe both what is changing and why this is important. Include:

  • Added an instance of LAContext to the Keychain class in order to reuse it.
  • Expose the LAContext as a read property in order to reuse it outside of the Keychain (In Auth0's CredentialManager, for example)

References

Please include relevant links supporting this change such as a:

Testing

Please describe how this can be tested by reviewers. Be specific about anything not tested and reasons why. If helpful, please include manual testing steps as well.

  1. Create an instance of Keychain and configure it to use LocalAuthentication an access when the device is unlocked
keychain = A0SimpleKeychain()
keychain.useAccessControl = true
keychain.defaultAccessiblity = .whenUnlockedThisDeviceOnly
keychain.setTouchIDAuthenticationAllowableReuseDuration(5.0)

let r1 = keychain.setString("TEST 1A", forKey: "KEY 1", promptMessage: "P1")
let r2 = keychain.setString("TEST 2A", forKey: "KEY 2", promptMessage: "P2")       

let v1 = keychain.string(forKey: "KEY 1", promptMessage: "P1B")
let v2 = keychain.string(forKey: "KEY 2", promptMessage: "P2B")
  1. Enable FaceID via declaration in Info.plist
<key>NSFaceIDUsageDescription</key>
<string>Access Keychain</string>
  1. When running the code in (1) the Passcode / Touch ID / Face ID local authentication will be asked only once (will be reused). With the current code it's asked every time a keychain operation is performed.

[ ] This change adds unit test coverage (or why not)

It does not include Unit Test. The Test requires UI Interaction in order to see the behavior

[X] This change has been tested on the latest version of the platform/language or why not

Tested on iOS 13.2

Checklist

[X] I have read the Auth0 general contribution guidelines

[X] I have read the Auth0 Code of Conduct

[X] All existing and new tests complete without errors

@Widcket
Copy link
Contributor

Widcket commented Jan 17, 2020

Hi @eaceto, apologies for the delay. LAContext is only available on iOS and macOS. This library supports tvOS and watchOS as well, that's why the CI build is failing. Can you please put the feature behind platform checks in order to maintain compatibility?

@eaceto
Copy link
Contributor Author

eaceto commented Jan 17, 2020

Hello Rita, how are you? Thanks for replying, and sorry for not fixing that earlier. I was on holidays.

Let me add this checks now in order to fix the CI errors. Thanks!

@eaceto
Copy link
Contributor Author

eaceto commented Jan 17, 2020

Hi @Widcket ! All tests passed, now. If you want to perform a detailed review on the Pull Request, please let me know.

SimpleKeychain/A0SimpleKeychain.m Outdated Show resolved Hide resolved
SimpleKeychain/A0SimpleKeychain.m Outdated Show resolved Hide resolved
SimpleKeychain/A0SimpleKeychain.m Outdated Show resolved Hide resolved
SimpleKeychain/A0SimpleKeychain.m Outdated Show resolved Hide resolved
@eaceto
Copy link
Contributor Author

eaceto commented Jan 21, 2020

@Widcket all changes where made, and branch was rebased to current master (due to a PR about SPM)

SimpleKeychain/A0SimpleKeychain.h Outdated Show resolved Hide resolved
SimpleKeychain/A0SimpleKeychain.h Outdated Show resolved Hide resolved
SimpleKeychain/A0SimpleKeychain.h Outdated Show resolved Hide resolved
SimpleKeychain/A0SimpleKeychain.m Outdated Show resolved Hide resolved
SimpleKeychain/A0SimpleKeychain.m Outdated Show resolved Hide resolved
SimpleKeychain/A0SimpleKeychain.m Outdated Show resolved Hide resolved
@Widcket Widcket requested a review from cocojoe January 28, 2020 20:29
@Widcket Widcket merged commit 3d14e9d into auth0:master Feb 4, 2020
@Widcket Widcket added this to the vNext milestone Feb 5, 2020
@Widcket Widcket mentioned this pull request Feb 5, 2020
@eaceto eaceto deleted the feature/use-local-authentication-context branch March 19, 2020 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants