Skip to content

Navigation Menu

Sign in
Sign up

Cancellation part 2: Don't emit failed states on cancellation - #223

Open
msirringhaus wants to merge 2 commits into
linux-credentials:main from
msirringhaus:more_cancellation
Open

Cancellation part 2: Don't emit failed states on cancellation #223
msirringhaus wants to merge 2 commits into
linux-credentials:main from
msirringhaus:more_cancellation

Conversation

@msirringhaus

@msirringhaus msirringhaus commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Don't emit a failed state, if the transport gets cancelled.

Additionally: Sync'ed some changes in the USB code over to NFC

@iinuwa iinuwa left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good. I'll check on that lint

Comment thread credentialsd-common/src/model.rs Outdated
/// 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,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@msirringhaus msirringhaus Sep 7, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks :)

Comment on lines +231 to +239
// 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)
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Cf. main...cancellation-stuff#diff-94b7794c18400cd8b6fd8532bbe079602658017f62fc608bdac2c3ebe0cbf6a5R501-R531

@iinuwa iinuwa left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, thank you!

This PR is good to merge, can you go ahead and so if you're at a good stopping point?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iinuwa iinuwa iinuwa approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /