-
Notifications
You must be signed in to change notification settings - Fork 285
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
Passkeys: Errors, Alerts, UI Feedback #1610
Passkeys: Errors, Alerts, UI Feedback #1610
Conversation
You can test the changes in simplenote-ios from this Pull Request by:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@charliescheer sending you a few Threading-Y notes!!
Also: Registration fails 10 out of 10 for me. I've nuked the Weauthn
rows I had, am I missing something?
Thank youuuuu
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Test scenarios are green
✅ Code looks good!
Added a few notes on an approach that would allow us encapsulate all of the complexity (so that it doesn't leak into the VC).
Other than that,
authController.presentationContextProvider = presentationContext | ||
authController.performRequests() | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what I had in mind when we were discussing API approach!
Rough edges, untested, not finished., and has room for probably splitting a bit.
The idea is that all of the business logic doesn't leak into the ViewController, from the outside, you'd expect a simple and straightforward API, with clearly defined spots where it begins and ends.
class PasskeyAuthControllerDelegate: NSObject, ASAuthorizationControllerDelegate {
var onCompletion: ((Result<ASAuthorization, Error>) -> Void)?
public func authorizationController(controller: ASAuthorizationController, didCompleteWithError error: any Error) {
onCompletion?(.failure(error))
}
public func authorizationController(controller: ASAuthorizationController, didCompleteWithAuthorization authorization: ASAuthorization) {
onCompletion?(.success(authorization))
}
}
class PasskeyAuthenticator: NSObject {
let passkeyRemote: PasskeyRemote
let internalAuthControllerDelegate: PasskeyAuthControllerDelegate
init(passkeyRemote: PasskeyRemote = PasskeyRemote(), authControllerDelegate: PasskeyAuthControllerDelegate = .init()) {
self.passkeyRemote = passkeyRemote
self.internalAuthControllerDelegate = authControllerDelegate
}
func attemptPasskeyAuth(challenge: PasskeyAuthChallenge?, in presentationContext: PresentationContext, delegate: ASAuthorizationControllerDelegate) async throws {
guard let challenge else {
throw PasskeyError.couldNotFetchAuthChallenge
}
let challengeData = try Data.decodeUrlSafeBase64(challenge.challenge)
let provider = ASAuthorizationPlatformPublicKeyCredentialProvider(relyingPartyIdentifier: challenge.relayingParty)
let request = provider.createCredentialAssertionRequest(challenge: challengeData)
let controller = ASAuthorizationController(authorizationRequests: [request])
controller.delegate = internalAuthControllerDelegate
controller.presentationContextProvider = presentationContext
let authorization = try await withCheckedThrowingContinuation { (continuation: CheckedContinuation<ASAuthorization, any Error>) in
internalAuthControllerDelegate.onCompletion = { result in
continuation.resume(with: result)
}
controller.performRequests()
}
/// Potentially split
/// Pending actual Simperium Auth
}
} | ||
|
||
controller.simperiumService.authenticate(withUsername: verifyResponse.username, token: verifyResponse.accessToken) | ||
unlockInterface() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally the ViewController doesn't need to deal with json encoding / decoding. More on that below!!
Co-authored-by: Jorge Leandro Perez <jorge@lantean.co>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sending you few comments Charlie!!
There's a bit of responsibility, yet, that can be transferred from the VC into the Passkey* classes. Plus there's a bit of an overlap in between PasskeyAuthControllerDelegate
and PasskeyAuthenticator
, both of them own a PasskeyRemote
instance.
LMK if anything!!
} | ||
} | ||
|
||
controller.performRequests() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this API okay with BG invocation? (we're not necessarily in the main Task)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be I believe. The finishing and ui changes happen on the main thread, the auth can happen in the background I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work Charlie!! Found a retain cycle, other than that, when ready!!
Thank youuu!!
Fix
The first implementation of passkeys works to create passkeys and allow for users to authenticate with them, but it lacks some UI refinement. There is no indication of progress happening, the UI remains unlocked, and there is no indication if the process is succeeding or failing.... Not a great user experience.
So to improve that I have added spinners, ui locking, errors, and alerts so that users have a better idea of what is going on when they use passkeys.
Test
Setup:
Registration:
Login:
Review
(Required) Add instructions for reviewers. For example:
Release
(Required) Add a concise statement to
RELEASE-NOTES.txt
if the changes should be included in release notes. Include details about updating the notes in this section. For example:If the changes should not be included in release notes, add a statement to this section. For example: