Return TokenValidatedContext.Fail instead of throwing UnauthorizedAccessException in case of missing Roles / Scopes

[MattKotsenas]

Today, if a token is valid, but both:

1. Has neither Scopes nor Roles claims
2. Not authorized via ACL

Then the JwtBearerHandler will throw an UnauthorizedAccessException that looks like this:

System.UnauthorizedAccessException: IDW10201: Neither scope or roles claim was found in the bearer token.
  at Microsoft.Identity.Web.MicrosoftIdentityWebApiAuthenticationBuilderExtensions.<>c__DisplayClass3_1.<<AddMicrosoftIdentityWebApiImplementation>b__1>d.MoveNext()
  at Microsoft.AspNetCore.Authentication.JwtBearer.JwtBearerHandler.HandleAuthenticateAsync()
  at Microsoft.AspNetCore.Authentication.JwtBearer.JwtBearerHandler.HandleAuthenticateAsync()
  at Microsoft.AspNetCore.Authentication.AuthenticationHandler`1.AuthenticateAsync()
  at Microsoft.AspNetCore.Authentication.AuthenticationService.AuthenticateAsync(HttpContext context, String scheme)
  at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)

which by default results in an unhandled exception in the auth middleware pipeline, which in turn results in a 500 error for the user.

While one could make the argument that a 500 is appropriate in this scenario, as the most likely cause is a misconfiguration on the AAD side, similar types of misconfiguration, such as a missing tid or tenantId claim results in a 401 Unauthorized error.

In order to make this change testable, I did the refactor in three steps, each in its own commit, to make the process easier to follow.

First, I moved the Roles / Scopes validation logic from the registration codepath into a new internal helper method, so that it could be called directly from unit tests. I used the JwtBearerMiddlewareDiagnosticsTests as a template.

Second, to simplify the new helper, I moved the AllowWebApiToBeAuthorizedByACL check out of the token validation codepath, and instead returned it to the registration codepath. This is both a minor performance win for ACL cases (because it skips the event callback when it isn't needed) and slightly reduces complexity of the helper (by making the conditional simpler), at the expense of making the ACL case difficult to test again. If there's desire to test the ACL case, we can make follow up changes.

Lastly, with the logic in a helper and tests written to validate the behavior, I replace the exception with a call to TokenValidatedContext.Fail(message).

Fixes #1716

[MattKotsenas]

This could be a one-line change to switch the throw statement to context.Fail(), but I wanted to provide test coverage of the change, which necessitated a small refactor. If you have thoughts or feelings about better ways to refactor to both make this change and provide test coverage, please let me know!

[MattKotsenas]

Additionally, I searched back through the history to see if there was an intentional change to make this an exception, and I wasn't able to see one back to the "initial commit" 6a393a9.

From browsing, it appears that the exception was to be symmetric with APIs like HttpContext.ValidateAppRole() and HttpContext.VerifyUserHasAnyAcceptedScope(), however those APIs are "user mode" APIs for manual validation inside a daemon app or a controller body, where an exception flow is an appropriate way to ensure processing is interrupted. In this case the exception will always (as far as I see?) be from inside the auth middleware pipeline, where exceptions are discouraged and instead we should return failure to the pipeline.

If I missing any motivation for why the API is this way, please let me know. Thanks!

[jennyf19]

[jennyf19]

Thanks so much for the contribution @MattKotsenas Really appreciate it.