Skip to content

Conversation

@wtgodbe
Copy link
Member

@wtgodbe wtgodbe commented Mar 11, 2025

Since this is a patch, instead of throwing an exception in cases where we wouldn't before like we do in the PR to main, we instead log an error and fail to refresh the cookie if RefreshSignInAsync is called with a TUser that does not have the same user ID as the currently authenticated user.

While this does not make it as obvious to developers that something has gone wrong, error logs are still pretty visible, and stale cookies are something web developers have to account for regardless. The big upside to not throwing in the patch is that we do not have to react to it in the email change confirmation flow to account for the possibility of RefreshSignInAsync throwing.

…g user

Since this is a patch, instead of throwing an exception in cases where we wouldn't before like we do in the PR to `main`, we instead log an error and fail to refresh the cookie if RefreshSignInAsync is called with a TUser that does not have the same user ID as the currently authenticated user.

While this does not make it *as* obvious to developers that something has gone wrong, error logs are still pretty visible, and stale cookies are something web developers have to account for regardless. The big upside to not throwing in the patch is that we do not have to react to it in the email change confirmation flow to account for the possibility of RefreshSignInAsync throwing.

----
#### AI description  (iteration 1)
#### PR Classification
Bug fix

#### PR Summary
This pull request disables the `RefreshSignInAsync` method if it is called with the wrong user or if the user is not authenticated.
- `src/Identity/Core/src/SignInManager.cs`: Added checks to log errors and return early if the user is not authenticated or if the authenticated user ID does not match the provided user ID.
- `src/Identity/test/Identity.Test/SignInManagerTest.cs`: Added tests to verify that `RefreshSignInAsync` logs errors and does not proceed if the user is not authenticated or if authenticated with a different user.
@wtgodbe wtgodbe requested review from Copilot and halter73 March 11, 2025 19:51
@ghost ghost added the area-identity Includes: Identity and providers label Mar 11, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR prevents RefreshSignInAsync from throwing exceptions by logging errors and skipping the refresh when the supplied user does not match the authenticated user. It updates error handling in RefreshSignInAsync and adds new tests to verify the behavior in cases of unauthenticated requests and mismatched user IDs.

  • Updated the RefreshSignInAsync method in SignInManager.cs to log appropriate error messages and return early.
  • Adjusted existing tests and added new tests in SignInManagerTest.cs to verify that RefreshSignInAsync does not refresh the sign-in when conditions are not met.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/Identity/test/Identity.Test/SignInManagerTest.cs Updated and added tests to check that RefreshSignInAsync logs errors and does not invoke sign-in when the user is unauthenticated or mismatched.
src/Identity/Core/src/SignInManager.cs Refactored RefreshSignInAsync to log error messages and abort refreshing sign-in when the user is not valid.
Comments suppressed due to low confidence (2)

src/Identity/test/Identity.Test/SignInManagerTest.cs:637

  • [nitpick] The test method name 'ResignInNoOpsAndLogsErrorIfNotAuthenticated' may be ambiguous as it tests the RefreshSignInAsync behavior; consider renaming it to 'RefreshSignInNoOpsAndLogsErrorIfNotAuthenticated' for clarity.
[Fact] public async Task ResignInNoOpsAndLogsErrorIfNotAuthenticated()

src/Identity/test/Identity.Test/SignInManagerTest.cs:661

  • [nitpick] The test method name 'ResignInNoOpsAndLogsErrorIfAuthenticatedWithDifferentUser' may be misleading since it verifies RefreshSignInAsync behavior; consider renaming it to 'RefreshSignInNoOpsAndLogsErrorIfAuthenticatedWithDifferentUser' for improved clarity.
[Fact] public async Task ResignInNoOpsAndLogsErrorIfAuthenticatedWithDifferentUser()

@wtgodbe wtgodbe enabled auto-merge (squash) March 12, 2025 00:12
@wtgodbe wtgodbe disabled auto-merge March 12, 2025 00:12
@wtgodbe wtgodbe enabled auto-merge (squash) March 12, 2025 00:12
@wtgodbe wtgodbe merged commit d42bdf6 into main Mar 12, 2025
27 checks passed
@wtgodbe wtgodbe deleted the wtgodbe/PortFix31125 branch March 12, 2025 01:00
@dotnet-policy-service dotnet-policy-service bot added this to the 10.0-preview3 milestone Mar 12, 2025
@hempe
Copy link

hempe commented May 2, 2025

  1. Could you please update the docs for the method as well.
    The current docs do not mention anywhere that the user must already be logged in!
    In the old code, if the user wasn't sign-in, this worked like a normal call to "SignInAsync".

This "patch" broke some edge-cases in our productive environment when azure updated dotnet for our webapps.

Suggested docs:

    /// <summary>
    /// Refresh the sign with the specified <paramref name="user"/>, whilst preserving the existing
    /// AuthenticationProperties of the current signed-in user like rememberMe, as an asynchronous operation.
    /// </summary>
    /// <remarks> User must already be signed in, and user id must match existing user. </remarks>
    /// <param name="user">The user to refresh the sign-in for.</param>
    /// <returns>The task object representing the asynchronous operation.</returns>
  1. Useless null checks: on line 180, 181 and 196 you check for auth?.
    But on line 165 you access auth.Succeeded so if auth could be null this would throw already on line 165

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

Labels

area-identity Includes: Identity and providers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants