Skip to content

Add nullable annotations to Authentication.Core & Authentication.Cookies #24307

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 1 commit into from
Jul 31, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Diagnostics.CodeAnalysis;
using System.Security.Claims;

namespace Microsoft.AspNetCore.Authentication
Expand All @@ -19,6 +20,7 @@ protected AuthenticateResult() { }
/// <summary>
/// If a ticket was produced, authenticate was successful.
/// </summary>
[MemberNotNullWhen(true, nameof(Ticket))]
public bool Succeeded => Ticket != null;

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public static Task<AuthenticateResult> AuthenticateAsync(this HttpContext contex
/// <param name="context">The <see cref="HttpContext"/> context.</param>
/// <param name="scheme">The name of the authentication scheme.</param>
/// <returns>The result.</returns>
public static Task ChallengeAsync(this HttpContext context, string scheme) =>
public static Task ChallengeAsync(this HttpContext context, string? scheme) =>
context.ChallengeAsync(scheme, properties: null);

/// <summary>
Expand Down Expand Up @@ -72,7 +72,7 @@ public static Task ChallengeAsync(this HttpContext context, string? scheme, Auth
/// <param name="context">The <see cref="HttpContext"/> context.</param>
/// <param name="scheme">The name of the authentication scheme.</param>
/// <returns>The task.</returns>
public static Task ForbidAsync(this HttpContext context, string scheme) =>
public static Task ForbidAsync(this HttpContext context, string? scheme) =>
context.ForbidAsync(scheme, properties: null);

/// <summary>
Expand Down Expand Up @@ -109,7 +109,7 @@ public static Task ForbidAsync(this HttpContext context, string? scheme, Authent
/// <param name="scheme">The name of the authentication scheme.</param>
/// <param name="principal">The user.</param>
/// <returns>The task.</returns>
public static Task SignInAsync(this HttpContext context, string scheme, ClaimsPrincipal principal) =>
public static Task SignInAsync(this HttpContext context, string? scheme, ClaimsPrincipal principal) =>
context.SignInAsync(scheme, principal, properties: null);

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ public class AuthenticationTicket
/// <param name="principal">the <see cref="ClaimsPrincipal"/> that represents the authenticated user.</param>
/// <param name="properties">additional properties that can be consumed by the user or runtime.</param>
/// <param name="authenticationScheme">the authentication middleware that was responsible for this ticket.</param>
public AuthenticationTicket(ClaimsPrincipal principal, AuthenticationProperties? properties, string? authenticationScheme)
public AuthenticationTicket(ClaimsPrincipal principal, AuthenticationProperties? properties, string authenticationScheme)
Copy link
Member

Choose a reason for hiding this comment

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

Should authenticationScheme throw ArgumentNullException if null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We generally haven't added explicit null-checks to our code to express the non-nullness. IIRC, there was one code path that would result in an arg-null \ null-ref if the scheme was null, but I wasn't a 100% certain if this was the right default

{
if (principal == null)
{
Expand All @@ -41,17 +41,17 @@ public AuthenticationTicket(ClaimsPrincipal principal, string authenticationSche
/// <summary>
/// Gets the authentication type.
/// </summary>
public string? AuthenticationScheme { get; private set; }
public string AuthenticationScheme { get; }

/// <summary>
/// Gets the claims-principal with authenticated user identities.
/// </summary>
public ClaimsPrincipal Principal { get; private set; }
public ClaimsPrincipal Principal { get; }

/// <summary>
/// Additional state values for the authentication session.
/// </summary>
public AuthenticationProperties Properties { get; private set; }
public AuthenticationProperties Properties { get; }

/// <summary>
/// Returns a copy of the ticket.
Expand Down
2 changes: 1 addition & 1 deletion src/Http/Http.Abstractions/src/PathString.cs
Original file line number Diff line number Diff line change
Expand Up @@ -424,7 +424,7 @@ public override int GetHashCode()
/// <param name="left">The left parameter</param>
/// <param name="right">The right parameter</param>
/// <returns>The ToString combination of both values</returns>
public static string operator +(PathString left, string right)
public static string operator +(PathString left, string? right)
{
// This overload exists to prevent the implicit string<->PathString converter from
// trying to call the PathString+PathString operator for things that are not path strings.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Diagnostics;
using System.Linq;
using System.Security.Claims;
using System.Text.Encodings.Web;
Expand All @@ -27,9 +28,9 @@ public class CookieAuthenticationHandler : SignInAuthenticationHandler<CookieAut

private DateTimeOffset? _refreshIssuedUtc;
private DateTimeOffset? _refreshExpiresUtc;
private string _sessionKey;
private Task<AuthenticateResult> _readCookieTask;
private AuthenticationTicket _refreshTicket;
private string? _sessionKey;
private Task<AuthenticateResult>? _readCookieTask;
private AuthenticationTicket? _refreshTicket;

public CookieAuthenticationHandler(IOptionsMonitor<CookieAuthenticationOptions> options, ILoggerFactory logger, UrlEncoder encoder, ISystemClock clock)
: base(options, logger, encoder, clock)
Expand All @@ -41,7 +42,7 @@ public CookieAuthenticationHandler(IOptionsMonitor<CookieAuthenticationOptions>
/// </summary>
protected new CookieAuthenticationEvents Events
{
get { return (CookieAuthenticationEvents)base.Events; }
get { return (CookieAuthenticationEvents)base.Events!; }
set { base.Events = value; }
}

Expand Down Expand Up @@ -86,7 +87,7 @@ private void CheckForRefresh(AuthenticationTicket ticket)
}
}

private void RequestRefresh(AuthenticationTicket ticket, ClaimsPrincipal replacedPrincipal = null)
private void RequestRefresh(AuthenticationTicket ticket, ClaimsPrincipal? replacedPrincipal = null)
{
var issuedUtc = ticket.Properties.IssuedUtc;
var expiresUtc = ticket.Properties.ExpiresUtc;
Expand All @@ -102,7 +103,7 @@ private void RequestRefresh(AuthenticationTicket ticket, ClaimsPrincipal replace
}
}

private AuthenticationTicket CloneTicket(AuthenticationTicket ticket, ClaimsPrincipal replacedPrincipal)
private AuthenticationTicket CloneTicket(AuthenticationTicket ticket, ClaimsPrincipal? replacedPrincipal)
{
var principal = replacedPrincipal ?? ticket.Principal;
var newPrincipal = new ClaimsPrincipal();
Expand All @@ -122,7 +123,7 @@ private AuthenticationTicket CloneTicket(AuthenticationTicket ticket, ClaimsPrin

private async Task<AuthenticateResult> ReadCookieTicket()
{
var cookie = Options.CookieManager.GetRequestCookie(Context, Options.Cookie.Name);
var cookie = Options.CookieManager.GetRequestCookie(Context, Options.Cookie.Name!);
Copy link
Member

Choose a reason for hiding this comment

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

What happens if Options.Cookie.Name is null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the default case, this is guaranteed to be non-null. The option configures it to a value and you cannot assign a null value to it.

if (string.IsNullOrEmpty(cookie))
{
return AuthenticateResult.NoResult();
Expand Down Expand Up @@ -157,7 +158,7 @@ private async Task<AuthenticateResult> ReadCookieTicket()
{
if (Options.SessionStore != null)
{
await Options.SessionStore.RemoveAsync(_sessionKey);
await Options.SessionStore.RemoveAsync(_sessionKey!);
}
return AuthenticateResult.Fail("Ticket expired");
}
Expand All @@ -176,6 +177,7 @@ protected override async Task<AuthenticateResult> HandleAuthenticateAsync()
return result;
}

Debug.Assert(result.Ticket != null);
var context = new CookieValidatePrincipalContext(Context, Scheme, Options, result.Ticket);
await Events.ValidatePrincipal(context);

Expand Down Expand Up @@ -244,15 +246,15 @@ protected virtual async Task FinishResponseAsync()

Options.CookieManager.AppendResponseCookie(
Context,
Options.Cookie.Name,
Options.Cookie.Name!,
cookieValue,
cookieOptions);

await ApplyHeaders(shouldRedirectToReturnUrl: false, properties: properties);
}
}

protected async override Task HandleSignInAsync(ClaimsPrincipal user, AuthenticationProperties properties)
protected async override Task HandleSignInAsync(ClaimsPrincipal user, AuthenticationProperties? properties)
{
if (user == null)
{
Expand Down Expand Up @@ -299,7 +301,7 @@ protected async override Task HandleSignInAsync(ClaimsPrincipal user, Authentica
signInContext.CookieOptions.Expires = expiresUtc.ToUniversalTime();
}

var ticket = new AuthenticationTicket(signInContext.Principal, signInContext.Properties, signInContext.Scheme.Name);
var ticket = new AuthenticationTicket(signInContext.Principal!, signInContext.Properties, signInContext.Scheme.Name);

if (Options.SessionStore != null)
{
Expand All @@ -324,14 +326,14 @@ protected async override Task HandleSignInAsync(ClaimsPrincipal user, Authentica

Options.CookieManager.AppendResponseCookie(
Context,
Options.Cookie.Name,
Options.Cookie.Name!,
cookieValue,
signInContext.CookieOptions);

var signedInContext = new CookieSignedInContext(
Context,
Scheme,
signInContext.Principal,
signInContext.Principal!,
signInContext.Properties,
Options);

Expand All @@ -344,7 +346,7 @@ protected async override Task HandleSignInAsync(ClaimsPrincipal user, Authentica
Logger.AuthenticationSchemeSignedIn(Scheme.Name);
}

protected async override Task HandleSignOutAsync(AuthenticationProperties properties)
protected async override Task HandleSignOutAsync(AuthenticationProperties? properties)
{
properties = properties ?? new AuthenticationProperties();

Expand All @@ -369,7 +371,7 @@ protected async override Task HandleSignOutAsync(AuthenticationProperties proper

Options.CookieManager.DeleteCookie(
Context,
Options.Cookie.Name,
Options.Cookie.Name!,
context.CookieOptions);

// Only redirect on the logout path
Expand Down Expand Up @@ -449,7 +451,7 @@ protected override async Task HandleChallengeAsync(AuthenticationProperties prop
await Events.RedirectToLogin(redirectContext);
}

private string GetTlsTokenBinding()
private string? GetTlsTokenBinding()
{
var binding = Context.Features.Get<ITlsTokenBindingFeature>()?.GetProvidedTokenBindingId();
return binding == null ? null : Convert.ToBase64String(binding);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ public CookieBuilder Cookie
/// <summary>
/// If set this will be used by the CookieAuthenticationHandler for data protection.
/// </summary>
public IDataProtectionProvider DataProtectionProvider { get; set; }
public IDataProtectionProvider? DataProtectionProvider { get; set; }

/// <summary>
/// The SlidingExpiration is set to true to instruct the handler to re-issue a new cookie with a new
Expand Down Expand Up @@ -111,28 +111,28 @@ public CookieBuilder Cookie
/// </summary>
public new CookieAuthenticationEvents Events
{
get => (CookieAuthenticationEvents)base.Events;
get => (CookieAuthenticationEvents)base.Events!;
set => base.Events = value;
}

/// <summary>
/// The TicketDataFormat is used to protect and unprotect the identity and other properties which are stored in the
/// cookie value. If not provided one will be created using <see cref="DataProtectionProvider"/>.
/// </summary>
public ISecureDataFormat<AuthenticationTicket> TicketDataFormat { get; set; }
public ISecureDataFormat<AuthenticationTicket> TicketDataFormat { get; set; } = default!;

/// <summary>
/// The component used to get cookies from the request or set them on the response.
///
/// ChunkingCookieManager will be used by default.
/// </summary>
public ICookieManager CookieManager { get; set; }
public ICookieManager CookieManager { get; set; } = default!;

/// <summary>
/// An optional container in which to store the identity across requests. When used, only a session identifier is sent
/// to the client. This can be used to mitigate potential problems with very large identities.
/// </summary>
public ITicketStore SessionStore { get; set; }
public ITicketStore? SessionStore { get; set; }

/// <summary>
/// <para>
Expand Down
6 changes: 3 additions & 3 deletions src/Security/Authentication/Cookies/src/CookieExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,13 @@ public static AuthenticationBuilder AddCookie(this AuthenticationBuilder builder
public static AuthenticationBuilder AddCookie(this AuthenticationBuilder builder, string authenticationScheme)
=> builder.AddCookie(authenticationScheme, configureOptions: null);

public static AuthenticationBuilder AddCookie(this AuthenticationBuilder builder, Action<CookieAuthenticationOptions> configureOptions)
public static AuthenticationBuilder AddCookie(this AuthenticationBuilder builder, Action<CookieAuthenticationOptions>? configureOptions)
=> builder.AddCookie(CookieAuthenticationDefaults.AuthenticationScheme, configureOptions);

public static AuthenticationBuilder AddCookie(this AuthenticationBuilder builder, string authenticationScheme, Action<CookieAuthenticationOptions> configureOptions)
public static AuthenticationBuilder AddCookie(this AuthenticationBuilder builder, string authenticationScheme, Action<CookieAuthenticationOptions>? configureOptions)
=> builder.AddCookie(authenticationScheme, displayName: null, configureOptions: configureOptions);

public static AuthenticationBuilder AddCookie(this AuthenticationBuilder builder, string authenticationScheme, string displayName, Action<CookieAuthenticationOptions> configureOptions)
public static AuthenticationBuilder AddCookie(this AuthenticationBuilder builder, string authenticationScheme, string? displayName, Action<CookieAuthenticationOptions>? configureOptions)
{
builder.Services.TryAddEnumerable(ServiceDescriptor.Singleton<IPostConfigureOptions<CookieAuthenticationOptions>, PostConfigureCookieAuthenticationOptions>());
builder.Services.AddOptions<CookieAuthenticationOptions>(authenticationScheme).Validate(o => o.Cookie.Expiration == null, "Cookie.Expiration is ignored, use ExpireTimeSpan instead.");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ public CookieSignedInContext(
HttpContext context,
AuthenticationScheme scheme,
ClaimsPrincipal principal,
AuthenticationProperties properties,
AuthenticationProperties? properties,
CookieAuthenticationOptions options)
: base(context, scheme, options, properties)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ public CookieSigningInContext(
AuthenticationScheme scheme,
CookieAuthenticationOptions options,
ClaimsPrincipal principal,
AuthenticationProperties properties,
AuthenticationProperties? properties,
CookieOptions cookieOptions)
: base(context, scheme, options, properties)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ public CookieSigningOutContext(
HttpContext context,
AuthenticationScheme scheme,
CookieAuthenticationOptions options,
AuthenticationProperties properties,
AuthenticationProperties? properties,
CookieOptions cookieOptions)
: base(context, scheme, options, properties)
=> CookieOptions = cookieOptions;
Expand Down
4 changes: 2 additions & 2 deletions src/Security/Authentication/Cookies/src/ICookieManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ public interface ICookieManager
/// <param name="context"></param>
/// <param name="key"></param>
/// <returns></returns>
string GetRequestCookie(HttpContext context, string key);
string? GetRequestCookie(HttpContext context, string key);

/// <summary>
/// Append the given cookie to the response.
Expand All @@ -26,7 +26,7 @@ public interface ICookieManager
/// <param name="key"></param>
/// <param name="value"></param>
/// <param name="options"></param>
void AppendResponseCookie(HttpContext context, string key, string value, CookieOptions options);
void AppendResponseCookie(HttpContext context, string key, string? value, CookieOptions options);

/// <summary>
/// Append a delete cookie to the response.
Expand Down
4 changes: 2 additions & 2 deletions src/Security/Authentication/Cookies/src/LoggingExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ namespace Microsoft.Extensions.Logging
{
internal static class LoggingExtensions
{
private static Action<ILogger, string, Exception> _authenticationSchemeSignedIn;
private static Action<ILogger, string, Exception> _authenticationSchemeSignedOut;
private static Action<ILogger, string, Exception?> _authenticationSchemeSignedIn;
private static Action<ILogger, string, Exception?> _authenticationSchemeSignedOut;

static LoggingExtensions()
{
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<Project Sdk="Microsoft.NET.Sdk">
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<Description>ASP.NET Core middleware that enables an application to use cookie based authentication.</Description>
Expand All @@ -9,6 +9,7 @@
<GenerateDocumentationFile>true</GenerateDocumentationFile>
<PackageTags>aspnetcore;authentication;security</PackageTags>
<IsPackable>false</IsPackable>
<Nullable>enable</Nullable>
</PropertyGroup>

<ItemGroup>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ public PostConfigureCookieAuthenticationOptions(IDataProtectionProvider dataProt
/// <param name="options">The options instance to configure.</param>
public void PostConfigure(string name, CookieAuthenticationOptions options)
{
options.DataProtectionProvider = options.DataProtectionProvider ?? _dp;
options.DataProtectionProvider ??= _dp;

if (string.IsNullOrEmpty(options.Cookie.Name))
{
Expand Down
Loading