Cancellation part 2: Don't emit failed states on cancellation - #223
Cancellation part 2: Don't emit failed states on cancellation#223msirringhaus wants to merge 2 commits into
Conversation
iinuwa
left a comment
iinuwa
left a comment
There was a problem hiding this comment.
Generally looks good. I'll check on that lint
| /// ceremony first, or because the user or client explicitly cancelled it. | ||
| /// This is an expected, non-error termination and should not be treated as an | ||
| /// authenticator failure. | ||
| RequestCancelled, |
There was a problem hiding this comment.
I was going to comment that this should only refer to system cancellation if this is being sent across the wire from credentialsd to credentialsd-ui.
But then I releasized that although is still in common, I think this whole enum belongs in credentialsd/credential_service...
Nothing to do in this PR, but it'd be good to clean this up later.
There was a problem hiding this comment.
Actually, I think it's fine to add this to this PR already. Let me quickly move it over.
| Failed(Error), | ||
|
|
||
| // This isn't actually sent from the server. | ||
| UserCancelled, |
| // Usually, comparing the discriminant is enough, but PinNotSet/NeedsPin | ||
| // can be repeated multiple times with different or the same error reasons | ||
| // (PIN wrong, PIN too short, PIN too long, etc.) | ||
| let state_changed = match (&state, &prev_nfc_state) { | ||
| (NfcStateInternal::PinNotSet { .. }, NfcStateInternal::PinNotSet { .. }) => true, | ||
| (NfcStateInternal::NeedsPin { .. }, NfcStateInternal::NeedsPin { .. }) => true, | ||
| (new_state, old_state) => { | ||
| std::mem::discriminant(new_state) != std::mem::discriminant(old_state) | ||
| } |
There was a problem hiding this comment.
Follow up for later: I realized that this is still wrong: NeedsUserVerification can be emitted multiple times in a row with different arguments. I think that we should probably remove my clever discriminant thing and just be explicit.
2 participants
Don't emit a failed state, if the transport gets cancelled.
Additionally: Sync'ed some changes in the USB code over to NFC