Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Both UI versions call GetLoginsAsync without checking SupportsUserLogin, causing stamp-capable stores without external-login support to throw instead of showing the no-login state.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds external-login reauthentication before setting passwords in Identity UI V4/V5.
Changes:
- Adds a protected, expiring reauthentication marker.
- Gates password creation and updates both UI versions and public page-model APIs.
- Adds functional coverage and cookie-aware test infrastructure.
File summaries
| File | Description |
|---|---|
src/Identity/UI/src/ReauthenticationMarker.cs |
Reauthentication marker implementation |
src/Identity/UI/src/PublicAPI.Unshipped.txt |
New public API entries |
src/Identity/UI/src/Microsoft.AspNetCore.Identity.UI.csproj |
Data Protection reference |
src/Identity/UI/src/Areas/Identity/Pages/V4/Account/Manage/SetPassword.cshtml.cs |
V4 password and reauthentication handlers |
src/Identity/UI/src/Areas/Identity/Pages/V4/Account/Manage/SetPassword.cshtml |
V4 confirmation UI |
src/Identity/UI/src/Areas/Identity/Pages/V5/Account/Manage/SetPassword.cshtml.cs |
V5 password and reauthentication handlers |
src/Identity/UI/src/Areas/Identity/Pages/V5/Account/Manage/SetPassword.cshtml |
V5 confirmation UI |
src/Identity/test/Identity.FunctionalTests/UserStories.cs |
Updated password-setting workflow |
src/Identity/test/Identity.FunctionalTests/Pages/Account/Manage/SetPassword.cs |
Reauthentication test helper |
src/Identity/test/Identity.FunctionalTests/Microsoft.AspNetCore.Identity.FunctionalTests.csproj |
Test assembly reference |
src/Identity/test/Identity.FunctionalTests/ManagementTests.cs |
Reauthentication scenarios |
src/Identity/test/Identity.FunctionalTests/Infrastructure/WebApplicationFactoryExtensions.cs |
Shared cookie-container client |
src/Identity/test/Identity.FunctionalTests/IdentityUserTests/IdentityUserManagementTests.cs |
Unsupported-store coverage |
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| /// This API supports the ASP.NET Core Identity default UI infrastructure and is not intended to be used | ||
| /// directly from your code. This API may change or be removed in future releases. | ||
| /// </summary> | ||
| public virtual Task<IActionResult> OnPostReauthenticateAsync(string provider) => throw new NotImplementedException(); |
There was a problem hiding this comment.
I actually think these are discovered via PopulateHandlerMethods which internally uses BindingFlags.Public :/
There was a problem hiding this comment.
I tried moving the handlers to the internal implementation, but Razor Pages did not discover them and the functional tests failed. I kept the handlers public on the abstract model and made only the properties internal.
|
Can we settle what reauthentication should require before finalizing the API? I’d lean toward following the normal sign-in requirements for the same account, including 2FA when applicable and honoring “Remember this browser.” We should make the same decision for the Blazor flow from #68522. That should help determine whether these callbacks belong on SetPassword or in a shared reauthentication flow. |
|
Happy to settle that first. One thing I found: the external re-challenge doesn't ask the provider to re-verify anything, so if the browser still has a live session there it can come straight back without the user touching a credential. On 2FA and remember-browser, those two seem to cancel out here. Remember-browser is a cookie, so someone in the victim's browser has it too and skips the 2FA. Could easily be missing something though. |
API proposal in #69376
Adding a password to an account that does not have one now asks you to confirm with one of the account's
linked external logins first. Today the page trusts the sign-in cookie alone, so anyone holding a stolen
cookie can add a password and carry on using it after the cookie is revoked. This is the Identity UI half
of the gate the Blazor template got in #68522, reauthentication before adding a passkey or setting a
password.
New page model members
IsReauthenticated,CurrentLoginsand the two handlers are public because the V4 and V5 views have toread the state and post to them. The confirmation itself is an internal
ReauthenticationMarker: adata-protected cookie holding the user id and current security stamp, valid for five minutes, so setting
the password or signing out everywhere invalidates it.
I clear the marker before issuing the provider challenge, so a failed or abandoned confirmation cannot
leave an older valid one in place.
Behaviour changes
A user store without security stamp support can no longer set a password here, since there is nothing to
bind the confirmation to. The page says that plainly instead of reporting it as a refused confirmation.
An account whose linked provider is no longer configured also has nothing to confirm with.
Not included
Identity UI has no passkey pages, so an external login is the only confirmation method. The password
itself is not one, because the page only appears when the account has no password.
Testing
Functional tests cover a missing, malformed, expired, other-user and stale-stamp marker, an unavailable
or unlinked provider, redisplay after a failed post, and the unsupported-store path with a mocked store.
They pass locally.