Skip to content

Add a feature for accessing the AuthenticateResult #33408

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 7 commits into from
Jun 26, 2021

Conversation

BrennanConroy
Copy link
Member

@BrennanConroy BrennanConroy commented Jun 9, 2021

  • Add the IAuthenticateResult feature
  • Add a class to keep User and AuthenticateResult consistent
  • Add ExpiresUtc value to ticket properties (easiest property to reason about with multiple schemes)

TODO: Add tests

Fixes #20847

Authentication.Abstractions package

namespace Microsoft.AspNetCore.Authentication
{
    public interface IAuthenticateResultFeature
    {
        AuthenticateResult? AuthenticateResult { get; set; }
    }
}

@BrennanConroy BrennanConroy added the area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer label Jun 9, 2021
@BrennanConroy BrennanConroy requested review from Tratcher and HaoK June 9, 2021 17:48
@Tratcher
Copy link
Member

You've got a workable setup, time for some tests.

@Kahbazi
Copy link
Member

Kahbazi commented Jun 10, 2021

Should this feature also set here :

var defaultAuthenticate = await Schemes.GetDefaultAuthenticateSchemeAsync();
if (defaultAuthenticate != null)
{
var result = await context.AuthenticateAsync(defaultAuthenticate.Name);
if (result?.Principal != null)
{
context.User = result.Principal;
}
}

@BrennanConroy BrennanConroy marked this pull request as ready for review June 15, 2021 02:28
@Tratcher
Copy link
Member

Should this feature also set here :

var defaultAuthenticate = await Schemes.GetDefaultAuthenticateSchemeAsync();
if (defaultAuthenticate != null)
{
var result = await context.AuthenticateAsync(defaultAuthenticate.Name);
if (result?.Principal != null)
{
context.User = result.Principal;
}
}

@BrennanConroy we talked about this didn't we? Why didn't we do it? It would be nice to have.

@BrennanConroy
Copy link
Member Author

@BrennanConroy we talked about this didn't we? Why didn't we do it? It would be nice to have.

We might have but I don't remember what was said. Main issue is we'll need to write some ugly code in Authorization middleware to check if the AuthenticationFeatures class has been set on the features by the Authentication middleware.

@Tratcher
Copy link
Member

Authorization can ignore and replace whatever the Authentication did. Authentication only applies to the default auth scheme.

@BrennanConroy BrennanConroy added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Jun 21, 2021
@ghost
Copy link

ghost commented Jun 21, 2021

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@BrennanConroy BrennanConroy added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Jun 21, 2021
@@ -71,6 +72,12 @@ public async Task Invoke(HttpContext context)
{
context.User = result.Principal;
}
if (result?.Succeeded ?? false)
{
var authFeatures = new AuthenticationFeatures(result);
Copy link
Member

Choose a reason for hiding this comment

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

Allocations!

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't look at the rest of the Auth middleware stuff then 😢

@BrennanConroy BrennanConroy requested review from Tratcher and HaoK June 25, 2021 16:32
@BrennanConroy BrennanConroy merged commit dbf84ea into main Jun 26, 2021
@BrennanConroy BrennanConroy deleted the brecon/authresult branch June 26, 2021 02:31
@ghost ghost added this to the 6.0-preview7 milestone Jun 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set the AuthenticationResult via a new feature on the HttpContext
5 participants