Skip to content

Conversation

@shethaadit
Copy link
Contributor

Summary

This PR updates the TokenExtensions.UpdateTokenValue() method signature to make the tokenValue parameter nullable (string? instead of string), addressing the oversight mentioned in #62257.

Description

The tokenValue parameter in TokenExtensions.UpdateTokenValue() is expected to be nullable as it:

  • Is not null-guarded in the implementation
  • Updates a dictionary entry where values are deliberately nullable
  • Is commonly used with the result of GetTokenValue() which returns string?

This change eliminates CS8604 compiler warnings when passing nullable values to the method.

Changes

  • Updated UpdateTokenValue() method signature to accept string? for tokenValue parameter
  • Added test cases to verify nullable behavior:
    • StoreTokens_StoresTokensWithNullValues - Verifies storing tokens with null values
    • UpdateTokenValue_WorksWithNullGetTokenValueResult - Tests updating with null from GetTokenValue()
    • GetTokens_ExcludesTokensWithNullValues - Confirms null-valued tokens are excluded from GetTokens()

Customer Impact

This is a compile-time only change that:

  • Eliminates nullable reference type warnings (CS8604) in common usage patterns
  • Provides better API clarity about expected parameter nullability
  • Has no runtime impact or breaking changes (binary compatible)

Example of fixed scenario:

// No longer produces CS8604 warning
properties.UpdateTokenValue("refresh_token", result.Properties.GetTokenValue("refresh_token"));

Fixes #62257

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jun 26, 2025
@github-actions github-actions bot added the needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically label Jun 26, 2025
@martincostello martincostello added area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer and removed needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically labels Jun 26, 2025
Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

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

I was tempted to say we should make a breaking change and start throwing for null token values rather than continue putting null values in the properties dictionary, but I don't think it's worth it. The behavior isn't that much different than updating a token value with the empty string.

Can you update https://github.com/dotnet/aspnetcore/blob/main/src/Http/Authentication.Abstractions/src/PublicAPI.Unshipped.txt to include the following? That should fix the RS0016 errors.

#nullable enable
*REMOVED*static Microsoft.AspNetCore.Authentication.AuthenticationTokenExtensions.UpdateTokenValue(this Microsoft.AspNetCore.Authentication.AuthenticationProperties! properties, string! tokenName, string! tokenValue) -> bool
static Microsoft.AspNetCore.Authentication.AuthenticationTokenExtensions.UpdateTokenValue(this Microsoft.AspNetCore.Authentication.AuthenticationProperties! properties, string! tokenName, string? tokenValue) -> bool

Thanks!

@shethaadit shethaadit requested a review from halter73 June 26, 2025 18:29
@shethaadit shethaadit requested review from a team and tdykstra as code owners June 26, 2025 23:14
@halter73 halter73 enabled auto-merge (squash) June 30, 2025 21:08
@halter73 halter73 merged commit a94e3e8 into dotnet:main Jul 1, 2025
28 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the 10.0-preview7 milestone Jul 1, 2025
@kevinchalet
Copy link
Contributor

I was tempted to say we should make a breaking change and start throwing for null token values rather than continue putting null values in the properties dictionary, but I don't think it's worth it.

If you're not against making a potentially-observable change, a better approach would be to remove the token entry when the value is null, which is what the AuthenticationProperties Set*() methods do for null values. E.g:

/// <summary>
/// Set or remove a <see cref="bool"/> value in the <see cref="Items"/> collection.
/// </summary>
/// <param name="key">Property key.</param>
/// <param name="value">Value to set or <see langword="null" /> to remove the property.</param>
protected void SetBool(string key, bool? value)
{
if (value.HasValue)
{
Items[key] = value.GetValueOrDefault().ToString();
}
else
{
Items.Remove(key);
}
}

/// <summary>
/// Set or remove a string value from the <see cref="Items"/> collection.
/// </summary>
/// <param name="key">Property key.</param>
/// <param name="value">Value to set or <see langword="null" /> to remove the property.</param>
public void SetString(string key, string? value)
{
if (value != null)
{
Items[key] = value;
}
else
{
Items.Remove(key);
}
}

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

Labels

area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consider making TokenExtensions.UpdateTokenValue()'s tokenValue parameter nullable

4 participants