Skip to content
This repository has been archived by the owner on Dec 13, 2018. It is now read-only.

Add AccessDeniedPath support to the OIDC/OAuth2/Twitter providers #1887

Merged
merged 9 commits into from Nov 15, 2018
11 changes: 11 additions & 0 deletions samples/OpenIdConnectSample/Startup.cs
Expand Up @@ -63,6 +63,7 @@ public void ConfigureServices(IServiceCollection services)
o.ResponseType = OpenIdConnectResponseType.CodeIdToken;
o.SaveTokens = true;
o.GetClaimsFromUserInfoEndpoint = true;
o.AccessDeniedPath = "/access-denied-from-remote";

o.ClaimActions.MapAllExcept("aud", "iss", "iat", "nbf", "exp", "aio", "c_hash", "uti", "nonce");

Expand Down Expand Up @@ -126,6 +127,16 @@ await context.SignOutAsync(OpenIdConnectDefaults.AuthenticationScheme, new Authe
return;
}

if (context.Request.Path.Equals("/access-denied-from-remote"))
{
await WriteHtmlAsync(response, async res =>
{
await res.WriteAsync($"<h1>Access Denied error received from the remote authorization server</h1>");
await res.WriteAsync("<a class=\"btn btn-default\" href=\"/\">Home</a>");
});
return;
}

if (context.Request.Path.Equals("/Account/AccessDenied"))
{
await context.SignOutAsync(CookieAuthenticationDefaults.AuthenticationScheme);
Expand Down
Expand Up @@ -422,7 +422,7 @@ protected override async Task HandleForbiddenAsync(AuthenticationProperties prop
var returnUrl = properties.RedirectUri;
if (string.IsNullOrEmpty(returnUrl))
{
returnUrl = OriginalPathBase + Request.Path + Request.QueryString;
returnUrl = OriginalPathBase + OriginalPath + Request.QueryString;
}
var accessDeniedUri = Options.AccessDeniedPath + QueryString.Create(Options.ReturnUrlParameter, returnUrl);
var redirectContext = new RedirectContext<CookieAuthenticationOptions>(Context, Scheme, Options, properties, BuildRedirectUri(accessDeniedUri));
Expand All @@ -434,7 +434,7 @@ protected override async Task HandleChallengeAsync(AuthenticationProperties prop
var redirectUri = properties.RedirectUri;
if (string.IsNullOrEmpty(redirectUri))
{
redirectUri = OriginalPathBase + Request.Path + Request.QueryString;
redirectUri = OriginalPathBase + OriginalPath + Request.QueryString;
}

var loginUri = Options.LoginPath + QueryString.Create(Options.ReturnUrlParameter, redirectUri);
Expand Down
12 changes: 11 additions & 1 deletion src/Microsoft.AspNetCore.Authentication.OAuth/OAuthHandler.cs
Expand Up @@ -63,6 +63,16 @@ protected override async Task<HandleRequestResult> HandleRemoteAuthenticateAsync
var error = query["error"];
if (!StringValues.IsNullOrEmpty(error))
{
// Note: access_denied errors are special protocol errors indicating the user didn't
kevinchalet marked this conversation as resolved.
Show resolved Hide resolved
// approve the authorization demand requested by the remote authorization server.
// Since it's a frequent scenario (that is not caused by incorrect configuration),
// denied errors are handled differently using HandleAccessDeniedErrorAsync().
// Visit https://tools.ietf.org/html/rfc6749#section-4.1.2.1 for more information.
if (StringValues.Equals(error, "access_denied"))
{
return await HandleAccessDeniedErrorAsync(properties);
}

var failureMessage = new StringBuilder();
failureMessage.Append(error);
var errorDescription = query["error_description"];
Expand Down Expand Up @@ -194,7 +204,7 @@ protected override async Task HandleChallengeAsync(AuthenticationProperties prop
{
if (string.IsNullOrEmpty(properties.RedirectUri))
{
properties.RedirectUri = CurrentUri;
properties.RedirectUri = OriginalPathBase + OriginalPath + Request.QueryString;
}

// OAuth2 10.12 CSRF
Expand Down
6 changes: 2 additions & 4 deletions src/Microsoft.AspNetCore.Authentication.OAuth/OAuthOptions.cs
Expand Up @@ -3,11 +3,9 @@

using System;
using System.Collections.Generic;
using Microsoft.AspNetCore.Authentication;
using Microsoft.AspNetCore.Authentication.OAuth;
using Microsoft.AspNetCore.Authentication.OAuth.Claims;
using Microsoft.AspNetCore.Http.Authentication;
using System.Globalization;
using Microsoft.AspNetCore.Authentication.OAuth.Claims;
using Microsoft.AspNetCore.Http;

namespace Microsoft.AspNetCore.Authentication.OAuth
{
Expand Down
Expand Up @@ -186,7 +186,7 @@ public async virtual Task SignOutAsync(AuthenticationProperties properties)
properties.RedirectUri = BuildRedirectUriIfRelative(Options.SignedOutRedirectUri);
if (string.IsNullOrWhiteSpace(properties.RedirectUri))
{
properties.RedirectUri = CurrentUri;
properties.RedirectUri = OriginalPathBase + OriginalPath + Request.QueryString;
}
}
Logger.PostSignOutRedirect(properties.RedirectUri);
Expand Down Expand Up @@ -312,7 +312,7 @@ protected override async Task HandleChallengeAsync(AuthenticationProperties prop
// 2. CurrentUri if RedirectUri is not set)
if (string.IsNullOrEmpty(properties.RedirectUri))
{
properties.RedirectUri = CurrentUri;
properties.RedirectUri = OriginalPathBase + OriginalPath + Request.QueryString;
}
Logger.PostAuthenticationLocalRedirect(properties.RedirectUri);

Expand Down Expand Up @@ -520,6 +520,16 @@ protected override async Task<HandleRequestResult> HandleRemoteAuthenticateAsync
// if any of the error fields are set, throw error null
if (!string.IsNullOrEmpty(authorizationResponse.Error))
{
// Note: access_denied errors are special protocol errors indicating the user didn't
kevinchalet marked this conversation as resolved.
Show resolved Hide resolved
// approve the authorization demand requested by the remote authorization server.
// Since it's a frequent scenario (that is not caused by incorrect configuration),
// denied errors are handled differently using HandleAccessDeniedErrorAsync().
// Visit https://tools.ietf.org/html/rfc6749#section-4.1.2.1 for more information.
if (string.Equals(authorizationResponse.Error, "access_denied", StringComparison.Ordinal))
{
return await HandleAccessDeniedErrorAsync(properties);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Tratcher pretty much like RemoteFailure, the fact AccessDenied is protocol-agnostic prevents us from flowing things like the OIDC authorization response, which may limit extensibility. Not sure it's really a problem, but let me know if you'd prefer having multiple (package-specific) AccessDenied events instead of a global one.

Copy link
Member

Choose a reason for hiding this comment

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

There's no other parameters in the message we'd be missing out on, right?

The most awkward bit seems to be having to downcast the Options if you wanted anything specific. Or could you flow the generic TOptions? I remember trying that and having lots of issues before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no other parameters in the message we'd be missing out on, right?

We could update HandleAccessDeniedErrorAsync() to take a message string containing the error/error_description/error_uri if you think it could be useful. Other than these parameters, nope, nothing else.

Copy link
Member

Choose a reason for hiding this comment

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

I can see those being useful if properly populated. It seems like you'd want to flow a generic property bag to the event rather than hardcoded parameters. I guess you could pass them through AuthProperties or some HttpContext field if you really needed to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The most awkward bit seems to be having to downcast the Options if you wanted anything specific. Or could you flow the generic TOptions? I remember trying that and having lots of issues before.

Yep. RemoteAuthenticationEvents is not generic so we can't easily flow TOptions. We could have a generic AccessDeniedContext<TOptions> class derived from AccessDeniedContext but it wouldn't help much as it would still be exposed as AccessDeniedContext by the events class.

It seems like you'd want to flow a generic property bag to the event rather than hardcoded parameters. I guess you could pass them through AuthProperties or some HttpContext field if you really needed to.

We can certainly do that, but I'm a bit reluctant as it would be fairly inconsistent (we don't do that anywhere else).

}

return HandleRequestResult.Fail(CreateOpenIdConnectProtocolException(authorizationResponse, response: null), properties);
}

Expand Down
Expand Up @@ -55,12 +55,14 @@ protected override async Task<HandleRequestResult> HandleRemoteAuthenticateAsync

var properties = requestToken.Properties;

// REVIEW: see which of these are really errors

var denied = query["denied"];
if (!StringValues.IsNullOrEmpty(denied))
{
return HandleRequestResult.Fail("The user denied permissions.", properties);
// Note: denied errors are special protocol errors indicating the user didn't
// approve the authorization demand requested by the remote authorization server.
// Since it's a frequent scenario (that is not caused by incorrect configuration),
// denied errors are handled differently using a special "access denied" exception.
return await HandleAccessDeniedErrorAsync(properties);
}

var returnedToken = query["oauth_token"];
Expand Down Expand Up @@ -130,7 +132,7 @@ protected override async Task HandleChallengeAsync(AuthenticationProperties prop
{
if (string.IsNullOrEmpty(properties.RedirectUri))
{
properties.RedirectUri = CurrentUri;
properties.RedirectUri = OriginalPathBase + OriginalPath + Request.QueryString;
}

// If CallbackConfirmed is false, this will throw
Expand Down
Expand Up @@ -2,8 +2,8 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Security.Claims;
using System.Globalization;
using System.Security.Claims;
using Microsoft.AspNetCore.Authentication.OAuth.Claims;
using Microsoft.AspNetCore.Http;

Expand Down
Expand Up @@ -83,7 +83,7 @@ protected override async Task HandleChallengeAsync(AuthenticationProperties prop
// Save the original challenge URI so we can redirect back to it when we're done.
if (string.IsNullOrEmpty(properties.RedirectUri))
{
properties.RedirectUri = CurrentUri;
properties.RedirectUri = OriginalPathBase + OriginalPath + Request.QueryString;
}

var wsFederationMessage = new WsFederationMessage()
Expand Down
@@ -0,0 +1,26 @@
// 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 Microsoft.AspNetCore.Http;

namespace Microsoft.AspNetCore.Authentication
{
/// <summary>
/// Provides access denied failure context information to handler providers.
/// </summary>
public class AccessDeniedContext : HandleRequestContext<RemoteAuthenticationOptions>
{
public AccessDeniedContext(
HttpContext context,
AuthenticationScheme scheme,
RemoteAuthenticationOptions options)
: base(context, scheme, options)
{
}

/// <summary>
/// Additional state values for the authentication session.
/// </summary>
public AuthenticationProperties Properties { get; set; }
}
}
Expand Up @@ -8,12 +8,18 @@ namespace Microsoft.AspNetCore.Authentication
{
public class RemoteAuthenticationEvents
{
public Func<AccessDeniedContext, Task> OnAccessDenied { get; set; } = context => Task.CompletedTask;
public Func<RemoteFailureContext, Task> OnRemoteFailure { get; set; } = context => Task.CompletedTask;

public Func<TicketReceivedContext, Task> OnTicketReceived { get; set; } = context => Task.CompletedTask;

/// <summary>
/// Invoked when there is a remote failure
/// Invoked when an access denied error was returned by the remote server.
/// </summary>
public virtual Task AccessDenied(AccessDeniedContext context) => OnAccessDenied(context);

/// <summary>
/// Invoked when there is a remote failure.
/// </summary>
public virtual Task RemoteFailure(RemoteFailureContext context) => OnRemoteFailure(context);

Expand Down
52 changes: 41 additions & 11 deletions src/Microsoft.AspNetCore.Authentication/LoggingExtensions.cs
Expand Up @@ -7,17 +7,20 @@ namespace Microsoft.Extensions.Logging
{
internal static class LoggingExtensions
{
private static Action<ILogger, string, Exception> _authSchemeAuthenticated;
private static Action<ILogger, string, Exception> _authSchemeNotAuthenticated;
private static Action<ILogger, string, string, Exception> _authSchemeNotAuthenticatedWithFailure;
private static Action<ILogger, string, Exception> _authSchemeChallenged;
private static Action<ILogger, string, Exception> _authSchemeForbidden;
private static Action<ILogger, string, Exception> _remoteAuthenticationError;
private static Action<ILogger, Exception> _signInHandled;
private static Action<ILogger, Exception> _signInSkipped;
private static Action<ILogger, string, Exception> _correlationPropertyNotFound;
private static Action<ILogger, string, Exception> _correlationCookieNotFound;
private static Action<ILogger, string, string, Exception> _unexpectedCorrelationCookieValue;
private static readonly Action<ILogger, string, Exception> _authSchemeAuthenticated;
private static readonly Action<ILogger, string, Exception> _authSchemeNotAuthenticated;
private static readonly Action<ILogger, string, string, Exception> _authSchemeNotAuthenticatedWithFailure;
private static readonly Action<ILogger, string, Exception> _authSchemeChallenged;
private static readonly Action<ILogger, string, Exception> _authSchemeForbidden;
private static readonly Action<ILogger, string, Exception> _remoteAuthenticationError;
private static readonly Action<ILogger, Exception> _signInHandled;
private static readonly Action<ILogger, Exception> _signInSkipped;
private static readonly Action<ILogger, string, Exception> _correlationPropertyNotFound;
private static readonly Action<ILogger, string, Exception> _correlationCookieNotFound;
private static readonly Action<ILogger, string, string, Exception> _unexpectedCorrelationCookieValue;
private static readonly Action<ILogger, Exception> _accessDeniedError;
private static readonly Action<ILogger, Exception> _accessDeniedContextHandled;
private static readonly Action<ILogger, Exception> _accessDeniedContextSkipped;

static LoggingExtensions()
{
Expand Down Expand Up @@ -65,6 +68,18 @@ static LoggingExtensions()
eventId: 16,
logLevel: LogLevel.Warning,
formatString: "The correlation cookie value '{CorrelationCookieName}' did not match the expected value '{CorrelationCookieValue}'.");
_accessDeniedError = LoggerMessage.Define(
eventId: 17,
logLevel: LogLevel.Information,
formatString: "Access was denied by the resource owner or by the remote server.");
_accessDeniedContextHandled = LoggerMessage.Define(
eventId: 18,
logLevel: LogLevel.Debug,
formatString: "The AccessDenied event returned Handled.");
_accessDeniedContextSkipped = LoggerMessage.Define(
eventId: 19,
logLevel: LogLevel.Debug,
formatString: "The AccessDenied event returned Skipped.");
}

public static void AuthenticationSchemeAuthenticated(this ILogger logger, string authenticationScheme)
Expand Down Expand Up @@ -121,5 +136,20 @@ public static void UnexpectedCorrelationCookieValue(this ILogger logger, string
{
_unexpectedCorrelationCookieValue(logger, cookieName, cookieValue, null);
}

public static void AccessDeniedError(this ILogger logger)
{
_accessDeniedError(logger, null);
}

public static void AccessDeniedContextHandled(this ILogger logger)
{
_accessDeniedContextHandled(logger, null);
}

public static void AccessDeniedContextSkipped(this ILogger logger)
{
_accessDeniedContextSkipped(logger, null);
}
}
}
Expand Up @@ -5,6 +5,7 @@
using System.Security.Cryptography;
using System.Text.Encodings.Web;
using System.Threading.Tasks;
using Microsoft.AspNetCore.WebUtilities;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;

Expand Down Expand Up @@ -241,5 +242,45 @@ protected virtual bool ValidateCorrelationId(AuthenticationProperties properties

return true;
}

protected virtual async Task<HandleRequestResult> HandleAccessDeniedErrorAsync(AuthenticationProperties properties)
{
Logger.AccessDeniedError();
var context = new AccessDeniedContext(Context, Scheme, Options)
{
Properties = properties
};
await Events.AccessDenied(context);

if (context.Result != null)
{
if (context.Result.Handled)
{
Logger.AccessDeniedContextHandled();
}
else if (context.Result.Skipped)
{
Logger.AccessDeniedContextSkipped();
}

return context.Result;
}

// If an access denied endpoint was specified, redirect the user agent.
// Otherwise, invoke the RemoteFailure event for further processing.
if (Options.AccessDeniedPath.HasValue)
{
string uri = Options.AccessDeniedPath;
if (!string.IsNullOrEmpty(Options.ReturnUrlParameter) && !string.IsNullOrEmpty(properties?.RedirectUri))
{
uri = QueryHelpers.AddQueryString(uri, Options.ReturnUrlParameter, properties.RedirectUri);
}
Response.Redirect(uri);

return HandleRequestResult.Handle();
}

return HandleRequestResult.Fail("Access was denied by the resource owner or by the remote server.", properties);
}
}
}
Expand Up @@ -89,6 +89,22 @@ public override void Validate()
/// </summary>
public PathString CallbackPath { get; set; }

/// <summary>
/// Gets or sets the optional path the user agent is redirected to if the user
/// doesn't approve the authorization demand requested by the remote server.
/// This property is not set by default. In this case, an exception is thrown
/// if an access_denied response is returned by the remote authorization server.
/// </summary>
public PathString AccessDeniedPath { get; set; }

/// <summary>
/// Gets or sets the name of the parameter used to convey the original location
/// of the user before the remote challenge was triggered up to the access denied page.
/// This property is only used when the <see cref="AccessDeniedPath"/> is explicitly specified.
/// </summary>
// Note: this deliberately matches the default parameter name used by the cookie handler.
public string ReturnUrlParameter { get; set; } = "ReturnUrl";

/// <summary>
/// Gets or sets the authentication scheme corresponding to the middleware
/// responsible of persisting user's identity after a successful authentication.
Expand Down
4 changes: 2 additions & 2 deletions test/Microsoft.AspNetCore.Authentication.Test/GoogleTests.cs
Expand Up @@ -378,7 +378,7 @@ public async Task ReplyPathWithErrorFails(bool redirect)
} : new OAuthEvents();
});
var sendTask = server.SendAsync("https://example.com/signin-google?error=OMG&error_description=SoBad&error_uri=foobar&state=protected_state",
".AspNetCore.Correlation.Google.corrilationId=N");
".AspNetCore.Correlation.Google.correlationId=N");
if (redirect)
{
var transaction = await sendTask;
Expand Down Expand Up @@ -1205,7 +1205,7 @@ public AuthenticationProperties Unprotect(string protectedText)
Assert.Equal("protected_state", protectedText);
var properties = new AuthenticationProperties(new Dictionary<string, string>()
{
{ ".xsrf", "corrilationId" },
{ ".xsrf", "correlationId" },
{ "testkey", "testvalue" }
});
properties.RedirectUri = "http://testhost/redirect";
Expand Down