Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 7, 2025

Update RefreshSignInAsync documentation and remove unnecessary null-conditional operators

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Clarify RefreshSignInAsync requirements and fix inconsistent null checks

Description

The RefreshSignInAsync documentation did not state that users must already be signed in. The method also used inconsistent null-conditional operators—checking auth.Succeeded directly but then using auth?.Properties despite AuthenticateResult.Succeeded guaranteeing non-null properties.

Changes:

  • Added <remarks> to documentation clarifying user must be signed in and user ID must match
  • Removed ?. operators on auth.Principal and auth.Properties after auth.Succeeded validation (lines 211, 212, 228)
  • Retained ?. operators in failure branches where properties may be null
// Before: inconsistent null handling
var authenticationMethod = auth.Principal?.FindFirst(ClaimTypes.AuthenticationMethod);
return (true, auth.Properties?.IsPersistent ?? false);

// After: leverages [MemberNotNullWhen(true)] guarantees
var authenticationMethod = auth.Principal.FindFirst(ClaimTypes.AuthenticationMethod);
return (true, auth.Properties.IsPersistent);

Fixes #{issue number}

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • cdn.jsdelivr.net
    • Triggering command: /home/REDACTED/work/aspnetcore/aspnetcore/.dotnet/dotnet exec --depsfile /home/REDACTED/work/aspnetcore/aspnetcore/artifacts/bin/Microsoft.AspNetCore.Identity.Test/Debug/net10.0/Microsoft.AspNetCore.Identity.Test.deps.json --runtimeconfig /home/REDACTED/work/aspnetcore/aspnetcore/artifacts/bin/Microsoft.AspNetCore.Identity.Test/Debug/net10.0/Microsoft.AspNetCore.Identity.Test.runtimeconfig.json /home/REDACTED/.nuget/packages/xunit.REDACTED.console/2.9.2/tools/netcoreapp2.0/xunit.console.dll /home/REDACTED/work/aspnetcore/aspnetcore/artifacts/bin/Microsoft.AspNetCore.Identity.Test/Debug/net10.0/Microsoft.AspNetCore.Identity.Test.dll -noautoreporters -xml /home/REDACTED/work/aspnetcore/aspnetcore/artifacts/TestResults/Debug/Microsoft.AspNetCore.Identity.Test_net10.0_x64.xml -html /home/REDACTED/work/aspnetcore/aspnetcore/artifacts/TestResults/Debug/Microsoft.AspNetCore.Identity.Test_net10.0_x64.html -notrait Quarantined=true -nocolor (dns block)
  • cdnjs.cloudflare.com
    • Triggering command: /home/REDACTED/work/aspnetcore/aspnetcore/.dotnet/dotnet exec --depsfile /home/REDACTED/work/aspnetcore/aspnetcore/artifacts/bin/Microsoft.AspNetCore.Identity.Test/Debug/net10.0/Microsoft.AspNetCore.Identity.Test.deps.json --runtimeconfig /home/REDACTED/work/aspnetcore/aspnetcore/artifacts/bin/Microsoft.AspNetCore.Identity.Test/Debug/net10.0/Microsoft.AspNetCore.Identity.Test.runtimeconfig.json /home/REDACTED/.nuget/packages/xunit.REDACTED.console/2.9.2/tools/netcoreapp2.0/xunit.console.dll /home/REDACTED/work/aspnetcore/aspnetcore/artifacts/bin/Microsoft.AspNetCore.Identity.Test/Debug/net10.0/Microsoft.AspNetCore.Identity.Test.dll -noautoreporters -xml /home/REDACTED/work/aspnetcore/aspnetcore/artifacts/TestResults/Debug/Microsoft.AspNetCore.Identity.Test_net10.0_x64.xml -html /home/REDACTED/work/aspnetcore/aspnetcore/artifacts/TestResults/Debug/Microsoft.AspNetCore.Identity.Test_net10.0_x64.html -notrait Quarantined=true -nocolor (dns block)
  • stackpath.bootstrapcdn.com
    • Triggering command: /home/REDACTED/work/aspnetcore/aspnetcore/.dotnet/dotnet exec --depsfile /home/REDACTED/work/aspnetcore/aspnetcore/artifacts/bin/Microsoft.AspNetCore.Identity.Test/Debug/net10.0/Microsoft.AspNetCore.Identity.Test.deps.json --runtimeconfig /home/REDACTED/work/aspnetcore/aspnetcore/artifacts/bin/Microsoft.AspNetCore.Identity.Test/Debug/net10.0/Microsoft.AspNetCore.Identity.Test.runtimeconfig.json /home/REDACTED/.nuget/packages/xunit.REDACTED.console/2.9.2/tools/netcoreapp2.0/xunit.console.dll /home/REDACTED/work/aspnetcore/aspnetcore/artifacts/bin/Microsoft.AspNetCore.Identity.Test/Debug/net10.0/Microsoft.AspNetCore.Identity.Test.dll -noautoreporters -xml /home/REDACTED/work/aspnetcore/aspnetcore/artifacts/TestResults/Debug/Microsoft.AspNetCore.Identity.Test_net10.0_x64.xml -html /home/REDACTED/work/aspnetcore/aspnetcore/artifacts/TestResults/Debug/Microsoft.AspNetCore.Identity.Test_net10.0_x64.html -notrait Quarantined=true -nocolor (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Original prompt

For SignInManager.RefreshSignInAsync,

  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. Can we just add a null check during the first if, and if it's null treat the user as unauthenticated and log an error, and then remove the useless null checks later in the method.

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

@dotnet-policy-service
Copy link
Contributor

Greetings human! You've submitted a PR that modifies code that is shared with https://github.com/dotnet/runtime . Please make sure you synchronize this code with the changes in that repo!

Copilot AI and others added 3 commits November 7, 2025 18:50
- Updated XML documentation to clearly state user must be signed in
- Added explicit null check for auth result at the beginning
- Removed unnecessary null-conditional operators after auth.Succeeded check
- All existing tests pass

Co-authored-by: halter73 <54385+halter73@users.noreply.github.com>
AuthenticateAsync guarantees to return non-null result, so the explicit
null check is not needed. The code now only removes unnecessary null-
conditional operators after confirming auth.Succeeded is true.

Co-authored-by: halter73 <54385+halter73@users.noreply.github.com>
Update method reference to include nullable parameter annotation (string?)

Co-authored-by: halter73 <54385+halter73@users.noreply.github.com>
Copilot AI changed the title [WIP] Update SignInManager.RefreshSignInAsync documentation Update RefreshSignInAsync documentation and remove unnecessary null-conditional operators Nov 7, 2025
Copilot AI requested a review from halter73 November 7, 2025 19:00
Copilot finished work on behalf of halter73 November 7, 2025 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants