Update AuthenticationManager.AuthenticateAsync to return an AuthenticationTicket

[kevinchalet]

In June, the AuthenticateAsync method was updated to return a ClaimsPrincipal instead of an AuthenticationResult (a type similar to AuthenticationTicket). As a consequence, retrieving the authentication properties is much harder, and can only be done using the rather unusual overload taking an AuthenticateContext parameter:

#323 (comment)

@HaoK's main argument was that retrieving authentication properties was not really frequent. Sadly, I don't think it's true, as retrieving the expiration date from the ticket is quite popular: http://stackoverflow.com/a/34535003/542757

If you really think having a method returning a ClaimsPrincipal is important, why not adding an extension method?

public static async Task<ClaimsPrincipal> GetPrincipalAsync(this AuthenticationManager manager, string scheme) {
    if (manager == null) {
        throw new ArgumentNullException(nameof(manager));
    }

    return (await manager.AuthenticateAsync(scheme))?.Principal;
}

[brockallen]

+1

[HaoK]

I'm not opposed to bringing back AuthenticateResult I guess, perhaps as an AuthenticateUser overload instead of a Get...

public abstract class AuthenticationManager {
  public AuthenticationResult AuthenticateAsync(string authenticationScheme);
  // extension method
  public ClaimsPrincipal AuthenticateUserAsync(string authenticationScheme);

[muratg]

@Eilon

[kevinchalet]

Is it too late for RC2? I think it would be worth fixing that before RTM.

[Eilon]

I don't really know what this stuff is 😄

If @HaoK and/or @Tratcher think this is useful, sure, we can look into adding it.

[Eilon]

And @blowdart for that matter.

[blowdart]

I'm fine with it

[Tratcher]

@HaoK can you take this? And give us an estimate on churn? I'm concerned that this will be too much churn for RC2.

[HaoK]

I can take it, its not too bad if we do something like:

Today's Authenticate => AuthenticateUser
New Autenticate returns AuthenticateResult again

Its a breaking change but not that widespread outside of identity and maybe Authorize filter in MVC

[HaoK]

Or if we want to minimize churn we can flip the names...

Authenticate stays the same returning Principal.
AuthenticateProperties returns both

[kevinchalet]

As mentioned by @HaoK the impact should be limited (https://github.com/search?q=user%3Aaspnet+AuthenticateAsync&ref=reposearch&type=Code&utf8=%E2%9C%93), but I can help with the task 👏

I also think we should remove the AuthenticateAsync(AuthenticateContext context) overload, as it's the only one that takes a context parameter, which is inconsistent and weird in a public API.

[Eilon]

We'll bring back the old AuthenticateResult as a new AuthenticateInfo, and then add a new method, something like public AuthenticateInfo AuthenticateUserAndGetInfoAsync(string authenticationScheme);

[HaoK]

Thoughts on AuthenticateInfo GetAuthenticateInfoAsync(string authenticationScheme) for the new overload?

[Eilon]

I dig it.

[Tratcher]

Task<AuthenticateDetails> GetAuthenticateDetailsAsync(string authenticationScheme)?

[kevinchalet]

I think @HaoK's initial proposition was a good compromise, and was clear enough, but as long as the overload taking an AuthenticateContext parameter is removed from the public contract, I'm fine 😄

[HaoK]

I'm ambivalent between AuthenticateDetails/Info. Maybe AuthenticateResult is best, since that satisfies being different from IdentityModel's AuthenticationResult and is what we want to convey (this is the result of an Authenticate)

[HaoK]

So here's what I'm thinking for the new shape:

   // On the abstract base AuthenticationManager:
   public abstract Task<AuthenticateResult> GetAuthenticateResultAsync(string authenticationScheme);
   public abstract Task<ClaimsPrincipal> AuthenticateAsync(string authenticationScheme);
  // Remove: public abstract void AuthenticateAsync(string authenticationScheme);

  public class AuthenticateResult {
    public ClaimsPrincipal Principal { get; set; }
    public AuthenticationProperties Properties { get; set; }
  }

[kevinchalet]

Doesn't aspnet/Security already have an AuthenticateResult?

[HaoK]

Ugh that's right, name was too good to still be available :) That's the problem with having the same method I guess... Whelp back to AuthenticateInfo/Details then...

[HaoK]

3a7f6a7