Skip to content
This repository was archived by the owner on Nov 20, 2018. It is now read-only.

Switch to GetAuthenticationInfoAsync #616

Closed
wants to merge 2 commits into from
Closed
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
@@ -0,0 +1,24 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Security.Claims;

namespace Microsoft.AspNetCore.Http.Authentication
{
/// <summary>
/// Used to store the results of an Authenticate call.
/// </summary>
public class AuthenticateInfo
{
/// <summary>
/// The <see cref="ClaimsPrincipal"/>.
/// </summary>
public ClaimsPrincipal Principal { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious: is there any particular reason to use publicly settable properties instead of get-only properties + public constructor?

Copy link
Member Author

Choose a reason for hiding this comment

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

No particular reason, if you guys feel strongly this should be readonly ctor properties, easy enough change.


/// <summary>
/// The <see cref="AuthenticationProperties"/>.
/// </summary>
public AuthenticationProperties Properties { get; set; }
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,18 +20,14 @@ public abstract class AuthenticationManager

public abstract IEnumerable<AuthenticationDescription> GetAuthenticationSchemes();

public abstract Task<AuthenticateInfo> GetAuthenticateInfoAsync(string authenticationScheme);
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, since this is the main "API", this one should probably be named AuthenticateAsync (as indicated by AuthenticateInfo's XML documentation, BTW). The other one is just a wrapper, so it may make more sense to switch the two names.

Copy link
Member

Choose a reason for hiding this comment

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

Switching the names would be a major break that we want to avoid. Not nearly as many people will use this version.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, TBH, how many people are using passive authentication these days? 5%? 10%?
These APIs are rather complicated for beginners and most will prefer using active authentication (which is the default for both cookies and bearer) or use Identity. In this case, retrieving the principal via HttpContext.User or ControllerBase.User will be much easier.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah so of that 5/10%, I'd say only another 5/10% of those passive calls will want the AuthenticationProperties, most will only want the Principal...

Copy link
Contributor

Choose a reason for hiding this comment

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

That's why I think a simpler name like GetUserAsync or GetPrincipalAsync would be better for the "get just the principal" use case (people often mix up AuthenticateAsync and SignInAsync).

Anyway, as long as the odd AuthenticateAsync overload is removed, I'm fine 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

(oh, and GetUserAsync would be - almost - consistent with HttpContext.User and ControllerBase.User 😄)


// Will remove once callees have been updated
public abstract Task AuthenticateAsync(AuthenticateContext context);

public virtual async Task<ClaimsPrincipal> AuthenticateAsync(string authenticationScheme)
{
if (authenticationScheme == null)
{
throw new ArgumentNullException(nameof(authenticationScheme));
}

var context = new AuthenticateContext(authenticationScheme);
await AuthenticateAsync(context);
return context.Principal;
return (await GetAuthenticateInfoAsync(authenticationScheme))?.Principal;
}

public virtual Task ChallengeAsync()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ public override IEnumerable<AuthenticationDescription> GetAuthenticationSchemes(
return describeContext.Results.Select(description => new AuthenticationDescription(description));
}

// Remove once callers have been switched to GetAuthenticateInfoAsync
public override async Task AuthenticateAsync(AuthenticateContext context)
{
if (context == null)
Expand All @@ -69,6 +70,32 @@ public override async Task AuthenticateAsync(AuthenticateContext context)
}
}

public override async Task<AuthenticateInfo> GetAuthenticateInfoAsync(string authenticationScheme)
{
if (authenticationScheme == null)
{
throw new ArgumentNullException(nameof(authenticationScheme));
}

var handler = HttpAuthenticationFeature.Handler;
var context = new AuthenticateContext(authenticationScheme);
if (handler != null)
{
await handler.AuthenticateAsync(context);
}

if (!context.Accepted)
{
throw new InvalidOperationException($"No authentication handler is configured to authenticate for the scheme: {context.AuthenticationScheme}");
}

return new AuthenticateInfo
{
Principal = context.Principal,
Properties = new AuthenticationProperties(context.Properties)
};
}

public override async Task ChallengeAsync(string authenticationScheme, AuthenticationProperties properties, ChallengeBehavior behavior)
{
if (string.IsNullOrEmpty(authenticationScheme))
Expand Down