Repository navigation
Rotate security stamp on passkey removal - #69394
Conversation
Co-authored-by: Youssef1313 <31348972+Youssef1313@users.noreply.github.com>
Co-authored-by: Youssef1313 <31348972+Youssef1313@users.noreply.github.com>
Co-authored-by: Youssef1313 <31348972+Youssef1313@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The newly added unit test incorrectly verifies a store call (GetSecurityStampAsync) that does not occur, so it will fail even when the implementation is correct.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates ASP.NET Core Identity passkey removal to rotate a user’s security stamp before persisting the user record, and adds a regression test to validate the behavior when the backing store supports security stamps.
Changes:
- Rotate the security stamp during
RemovePasskeyCoreAsyncbefore callingUpdateUserAsync. - Add a unit test intended to verify
RemovePasskeyAsyncupdates the security stamp whenIUserSecurityStampStoreis supported.
File summaries
| File | Description |
|---|---|
| src/Identity/Extensions.Core/src/UserManager.cs | Rotates the security stamp as part of passkey removal before persisting the user update. |
| src/Identity/test/Identity.Test/UserManagerTest.cs | Adds a regression test for security stamp rotation on passkey removal. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| } | ||
| } | ||
|
|
||
| private async Task<IdentityResult> RemovePasskeyCoreAsync(TUser user, byte[] credentialId) |
There was a problem hiding this comment.
This method can be inlined, and the UpdateUserAsync along with the metrics update could be merged into a single call to UpdateUserAndRecordMetricAsync for consistency with the rest of the class, but I'm only making the minimal change here.
We can follow with code cleanup later as needed.
There was a problem hiding this comment.
I’m concerned that rotating the security stamp here may be too broad. It invalidates all active Identity sessions for the user, even when they are only cleaning up a stale or unused passkey.
The documentation and Identity UI templates could explain that, if the passkey was lost or potentially compromised, the user should also choose “Log out everywhere” to invalidate existing sessions. This would distinguish routine credential management from account-compromise recovery while still giving users a clear secure path. Is global logout on every passkey removal an intentional requirement?
|
Researched a little bit, we could keep the current session active by refreshing sign-in, which removing an external login actually does. @copilot Update the passkey removal callsites to match the same behavior as external login removal with regards to refreshing sign-in. |
Co-authored-by: Youssef1313 <31348972+Youssef1313@users.noreply.github.com>
Co-authored-by: Youssef1313 <31348972+Youssef1313@users.noreply.github.com>
Updated the passkey deletion flow in the Blazor Web template to call |
|
Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime. |
|
@copilot Please continue this PR's passkey-removal stamp-consistency fix and its existing call-site feedback. First confirm current main still differs from the documented credential-change stamp contract and that this PR remains applicable. If it is already fixed or a human design decision is needed, report that rather than inventing unrelated changes. Otherwise bring this PR up to date with main as needed, resolve conflicts, and address valid outstanding review feedback and PR-caused CI failures within the existing functional scope. Use faithful regression coverage for the actual removal and sign-in-refresh paths, demonstrating failure without the fix and success with it. Preserve existing valid behavior; do not weaken tests or suppress warnings just to make CI green. Report unrelated or infrastructure failures with evidence and no code change. Preserve the PR's current non-draft status. Do not merge, close, relabel, or work on other issues or PRs. Do not assess or discuss security implications. Use GPT models only, with no subagents; report if you cannot honor this request. Please reply with what changed, exact validation results, and any remaining blockers. |
halter73
left a comment
There was a problem hiding this comment.
We could consider expanding the passkey-deletion flow in TestBasicInteractionAsync to verify the RefreshSignInAsync bit keeps a valid session even after the security stamp update. We'd probably want to update it to use a fake clock and zero validation interval like this Identity functional test, but I don't think it's necessary.
Current main ( Acceptance criteria: faithful removal/persistence and sign-in-refresh regressions fail for the expected reason without each fix and pass with it; stores without stamp support and failed-removal behavior remain valid; relevant existing tests pass unchanged. CI evidence: the latest product CI and template CI checks passed (Azure builds 1608513 and 1608515). The newer failed Copilot run stopped before implementation with No edits, merge, commits, or new test runs were performed. I stopped because the available publishing tool updates the PR description, and automated review offers no GPT-only model selector. I cannot guarantee your unchanged-body and GPT-only/no-subagent constraints with those tools. The PR remains open and non-draft with its title and body unchanged. Red/green behavioral validation remains outstanding. |
This hardens passkey removal to rotate the security stamp.
Security stamp is documented in https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.identity.identityuser-1.securitystamp?view=aspnetcore-10.0#microsoft-aspnetcore-identity-identityuser-1-securitystamp as:
We already do it for removing an external login, but we don't do that for passkey removal. A passkey is technically a "credentials change" similar to removing an external login.