Skip to content

Make TokenExtensions.UpdateTokenValue() tokenValue parameter nullable #62482

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

Merged
merged 8 commits into from
Jul 1, 2025

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 tdykstra and a team 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