Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[release/2.3] Merge from internal #60878

Merged
merged 2 commits into from
Mar 11, 2025
Merged

[release/2.3] Merge from internal #60878

merged 2 commits into from
Mar 11, 2025

Conversation

wtgodbe
Copy link
Member

@wtgodbe wtgodbe commented Mar 11, 2025

Should be merge-committed, not squashed

Stephen Halter and others added 2 commits February 25, 2025 21:17
…h wrong 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.
@wtgodbe wtgodbe requested review from halter73 and Copilot March 11, 2025 19:36
@wtgodbe wtgodbe requested a review from a team as a code owner March 11, 2025 19:36
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Mar 11, 2025
Copy link
Contributor

Hey @dotnet/aspnet-build, looks like this PR is something you want to take a look at.

Copy link
Contributor

@Copilot 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 merges changes from the internal release/2.3 branch with updates focused on sign-in behavior and test coverage for the sign-in workflow.

  • Updated the resignation flows to rely on an explicit parameter list without suppressing warnings.
  • Added two new tests to verify proper error logging and no-op behavior in RefreshSignInAsync when sign-in conditions are not met.
  • Revised RefreshSignInAsync in the core to handle unauthenticated and mismatched user cases with early returns after error logging.

Reviewed Changes

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

File Description
src/Identity/test/Identity.Test/SignInManagerTest.cs Updated test methods for resigning sign in, including improved ClaimsIdentity usage and additional tests for error cases.
src/Identity/Core/src/SignInManager.cs Modified RefreshSignInAsync to log errors and exit early if sign-in preconditions are not satisfied.
Comments suppressed due to low confidence (2)

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

  • [nitpick] Consider replacing the hard-coded 'authscheme' string with a constant or configuration value to improve consistency and reduce duplication.
var id = new ClaimsIdentity("authscheme");

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

  • [nitpick] The literal 'different' is used to simulate a mismatched user ID; consider using a more descriptive constant or variable name for clarity.
manager.Setup(m => m.GetUserId(claimsPrincipal)).Returns("different");

@wtgodbe wtgodbe merged commit c98fdef into release/2.3 Mar 11, 2025
2 of 4 checks passed
@wtgodbe wtgodbe deleted the wtgodbe/MergeInt23 branch March 11, 2025 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants