From 949d5e39d437430af1da8cbfa88587c5581e4bbe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Chalet?= Date: Sat, 30 May 2020 14:30:40 +0200 Subject: [PATCH] Fix an issue with the status code pages middleware integration and tweak the ASP.NET Core hosts to use AuthenticateResult.NoResult() --- .../OpenIddictServerAspNetCoreHandler.cs | 43 ++++++----- .../OpenIddictServerOwinHandler.cs | 24 ++++++- .../OpenIddictServerHandlers.cs | 2 +- .../OpenIddictServerProvider.cs | 15 +++- .../OpenIddictValidationAspNetCoreHandler.cs | 43 ++++++----- .../OpenIddictValidationOwinHandler.cs | 24 ++++++- .../OpenIddictValidationEvents.cs | 5 ++ .../OpenIddictValidationHandlers.cs | 72 ++++++++++++------- .../OpenIddictValidationProvider.cs | 15 +++- 9 files changed, 178 insertions(+), 65 deletions(-) diff --git a/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreHandler.cs b/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreHandler.cs index b5b2eeb0..e4d5172a 100644 --- a/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreHandler.cs +++ b/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreHandler.cs @@ -43,7 +43,7 @@ namespace OpenIddict.Server.AspNetCore : base(options, logger, encoder, clock) => _provider = provider; - protected override async Task InitializeHandlerAsync() + public async Task HandleRequestAsync() { // Note: the transaction may be already attached when replaying an ASP.NET Core request // (e.g when using the built-in status code pages middleware with the re-execute mode). @@ -62,18 +62,6 @@ namespace OpenIddict.Server.AspNetCore var context = new ProcessRequestContext(transaction); await _provider.DispatchAsync(context); - // Store the context in the transaction so that it can be retrieved from HandleRequestAsync(). - transaction.SetProperty(typeof(ProcessRequestContext).FullName, context); - } - - public async Task HandleRequestAsync() - { - var transaction = Context.Features.Get()?.Transaction ?? - throw new InvalidOperationException("An unknown error occurred while retrieving the OpenIddict server context."); - - var context = transaction.GetProperty(typeof(ProcessRequestContext).FullName) ?? - throw new InvalidOperationException("An unknown error occurred while retrieving the OpenIddict server context."); - if (context.IsRequestHandled) { return true; @@ -144,6 +132,14 @@ namespace OpenIddict.Server.AspNetCore else if (context.IsRejected) { + // Note: the missing_token error is special-cased to indicate to ASP.NET Core + // that no authentication result could be produced due to the lack of token. + // This also helps reducing the logging noise when no token is specified. + if (string.Equals(context.Error, Errors.MissingToken, StringComparison.Ordinal)) + { + return AuthenticateResult.NoResult(); + } + var properties = new AuthenticationProperties(new Dictionary { [OpenIddictServerAspNetCoreConstants.Properties.Error] = context.Error, @@ -154,9 +150,24 @@ namespace OpenIddict.Server.AspNetCore return AuthenticateResult.Fail("An error occurred while authenticating the current request.", properties); } - return AuthenticateResult.Success(new AuthenticationTicket( - context.Principal, - OpenIddictServerAspNetCoreDefaults.AuthenticationScheme)); + else + { + // Store the token to allow any ASP.NET Core component (e.g a controller) + // to retrieve it (e.g to make an API request to another application). + var properties = new AuthenticationProperties(); + properties.StoreTokens(new[] + { + new AuthenticationToken + { + Name = context.TokenType, + Value = context.Token + } + }); + + return AuthenticateResult.Success(new AuthenticationTicket( + context.Principal, properties, + OpenIddictServerAspNetCoreDefaults.AuthenticationScheme)); + } } protected override async Task HandleChallengeAsync([CanBeNull] AuthenticationProperties properties) diff --git a/src/OpenIddict.Server.Owin/OpenIddictServerOwinHandler.cs b/src/OpenIddict.Server.Owin/OpenIddictServerOwinHandler.cs index d00c5159..8b840d70 100644 --- a/src/OpenIddict.Server.Owin/OpenIddictServerOwinHandler.cs +++ b/src/OpenIddict.Server.Owin/OpenIddictServerOwinHandler.cs @@ -58,6 +58,10 @@ namespace OpenIddict.Server.Owin public override async Task InvokeAsync() { + // Note: due to internal differences between ASP.NET Core and Katana, the request MUST start being processed + // in InitializeCoreAsync() to ensure the request context is available from AuthenticateCoreAsync() when + // active authentication is used, as AuthenticateCoreAsync() is always called before InvokeAsync() in this case. + var transaction = Context.Get(typeof(OpenIddictServerTransaction).FullName) ?? throw new InvalidOperationException("An unknown error occurred while retrieving the OpenIddict server context."); @@ -137,6 +141,14 @@ namespace OpenIddict.Server.Owin else if (context.IsRejected) { + // Note: the missing_token error is special-cased to indicate to Katana + // that no authentication result could be produced due to the lack of token. + // This also helps reducing the logging noise when no token is specified. + if (string.Equals(context.Error, Errors.MissingToken, StringComparison.Ordinal)) + { + return null; + } + var properties = new AuthenticationProperties(new Dictionary { [OpenIddictServerOwinConstants.Properties.Error] = context.Error, @@ -147,7 +159,17 @@ namespace OpenIddict.Server.Owin return new AuthenticationTicket(null, properties); } - return new AuthenticationTicket((ClaimsIdentity) context.Principal.Identity, new AuthenticationProperties()); + else + { + // Store the token to allow any OWIN/Katana component (e.g a controller) + // to retrieve it (e.g to make an API request to another application). + var properties = new AuthenticationProperties(new Dictionary + { + [context.TokenType] = context.Token + }); + + return new AuthenticationTicket((ClaimsIdentity) context.Principal.Identity, properties); + } } protected override async Task TeardownCoreAsync() diff --git a/src/OpenIddict.Server/OpenIddictServerHandlers.cs b/src/OpenIddict.Server/OpenIddictServerHandlers.cs index db79d9c9..f3bb8c68 100644 --- a/src/OpenIddict.Server/OpenIddictServerHandlers.cs +++ b/src/OpenIddict.Server/OpenIddictServerHandlers.cs @@ -210,7 +210,7 @@ namespace OpenIddict.Server OpenIddictServerEndpointType.Authorization => (context.Request.IdTokenHint, TokenTypeHints.IdToken), OpenIddictServerEndpointType.Logout => (context.Request.IdTokenHint, TokenTypeHints.IdToken), - // Generic tokens received by the introspection and revocation can be of any type. + // Tokens received by the introspection and revocation endpoints can be of any type. // Additional token type filtering is made by the endpoint themselves, if needed. OpenIddictServerEndpointType.Introspection => (context.Request.Token, null), OpenIddictServerEndpointType.Revocation => (context.Request.Token, null), diff --git a/src/OpenIddict.Server/OpenIddictServerProvider.cs b/src/OpenIddict.Server/OpenIddictServerProvider.cs index 7911b25d..1af83826 100644 --- a/src/OpenIddict.Server/OpenIddictServerProvider.cs +++ b/src/OpenIddict.Server/OpenIddictServerProvider.cs @@ -56,15 +56,24 @@ namespace OpenIddict.Server switch (context) { case BaseRequestContext notification when notification.IsRequestHandled: - _logger.LogDebug("The request was handled in user code."); + if (_logger.IsEnabled(LogLevel.Debug)) + { + _logger.LogDebug("The event context was marked as handled by {Name}.", handler.GetType().FullName); + } return; case BaseRequestContext notification when notification.IsRequestSkipped: - _logger.LogDebug("The default request handling was skipped from user code."); + if (_logger.IsEnabled(LogLevel.Debug)) + { + _logger.LogDebug("The event context was marked as skipped by {Name}.", handler.GetType().FullName); + } return; case BaseValidatingContext notification when notification.IsRejected: - _logger.LogDebug("The request was rejected in user code."); + if (_logger.IsEnabled(LogLevel.Debug)) + { + _logger.LogDebug("The event context was marked as rejected by {Name}.", handler.GetType().FullName); + } return; default: continue; diff --git a/src/OpenIddict.Validation.AspNetCore/OpenIddictValidationAspNetCoreHandler.cs b/src/OpenIddict.Validation.AspNetCore/OpenIddictValidationAspNetCoreHandler.cs index 03138a37..7fd7fa30 100644 --- a/src/OpenIddict.Validation.AspNetCore/OpenIddictValidationAspNetCoreHandler.cs +++ b/src/OpenIddict.Validation.AspNetCore/OpenIddictValidationAspNetCoreHandler.cs @@ -40,7 +40,7 @@ namespace OpenIddict.Validation.AspNetCore : base(options, logger, encoder, clock) => _provider = provider; - protected override async Task InitializeHandlerAsync() + public async Task HandleRequestAsync() { // Note: the transaction may be already attached when replaying an ASP.NET Core request // (e.g when using the built-in status code pages middleware with the re-execute mode). @@ -59,18 +59,6 @@ namespace OpenIddict.Validation.AspNetCore var context = new ProcessRequestContext(transaction); await _provider.DispatchAsync(context); - // Store the context in the transaction so that it can be retrieved from HandleRequestAsync(). - transaction.SetProperty(typeof(ProcessRequestContext).FullName, context); - } - - public async Task HandleRequestAsync() - { - var transaction = Context.Features.Get()?.Transaction ?? - throw new InvalidOperationException("An unknown error occurred while retrieving the OpenIddict validation context."); - - var context = transaction.GetProperty(typeof(ProcessRequestContext).FullName) ?? - throw new InvalidOperationException("An unknown error occurred while retrieving the OpenIddict validation context."); - if (context.IsRequestHandled) { return true; @@ -141,6 +129,14 @@ namespace OpenIddict.Validation.AspNetCore else if (context.IsRejected) { + // Note: the missing_token error is special-cased to indicate to ASP.NET Core + // that no authentication result could be produced due to the lack of token. + // This also helps reducing the logging noise when no token is specified. + if (string.Equals(context.Error, Errors.MissingToken, StringComparison.Ordinal)) + { + return AuthenticateResult.NoResult(); + } + var properties = new AuthenticationProperties(new Dictionary { [OpenIddictValidationAspNetCoreConstants.Properties.Error] = context.Error, @@ -151,9 +147,24 @@ namespace OpenIddict.Validation.AspNetCore return AuthenticateResult.Fail("An error occurred while authenticating the current request.", properties); } - return AuthenticateResult.Success(new AuthenticationTicket( - context.Principal, - OpenIddictValidationAspNetCoreDefaults.AuthenticationScheme)); + else + { + // Store the token to allow any ASP.NET Core component (e.g a controller) + // to retrieve it (e.g to make an API request to another application). + var properties = new AuthenticationProperties(); + properties.StoreTokens(new[] + { + new AuthenticationToken + { + Name = context.TokenType, + Value = context.Token + } + }); + + return AuthenticateResult.Success(new AuthenticationTicket( + context.Principal, properties, + OpenIddictValidationAspNetCoreDefaults.AuthenticationScheme)); + } } protected override async Task HandleChallengeAsync([CanBeNull] AuthenticationProperties properties) diff --git a/src/OpenIddict.Validation.Owin/OpenIddictValidationOwinHandler.cs b/src/OpenIddict.Validation.Owin/OpenIddictValidationOwinHandler.cs index d7584ec8..3841c0df 100644 --- a/src/OpenIddict.Validation.Owin/OpenIddictValidationOwinHandler.cs +++ b/src/OpenIddict.Validation.Owin/OpenIddictValidationOwinHandler.cs @@ -58,6 +58,10 @@ namespace OpenIddict.Validation.Owin public override async Task InvokeAsync() { + // Note: due to internal differences between ASP.NET Core and Katana, the request MUST start being processed + // in InitializeCoreAsync() to ensure the request context is available from AuthenticateCoreAsync() when + // active authentication is used, as AuthenticateCoreAsync() is always called before InvokeAsync() in this case. + var transaction = Context.Get(typeof(OpenIddictValidationTransaction).FullName) ?? throw new InvalidOperationException("An unknown error occurred while retrieving the OpenIddict validation context."); @@ -134,6 +138,14 @@ namespace OpenIddict.Validation.Owin else if (context.IsRejected) { + // Note: the missing_token error is special-cased to indicate to Katana + // that no authentication result could be produced due to the lack of token. + // This also helps reducing the logging noise when no token is specified. + if (string.Equals(context.Error, Errors.MissingToken, StringComparison.Ordinal)) + { + return null; + } + var properties = new AuthenticationProperties(new Dictionary { [OpenIddictValidationOwinConstants.Properties.Error] = context.Error, @@ -144,7 +156,17 @@ namespace OpenIddict.Validation.Owin return new AuthenticationTicket(null, properties); } - return new AuthenticationTicket((ClaimsIdentity) context.Principal.Identity, new AuthenticationProperties()); + else + { + // Store the token to allow any OWIN/Katana component (e.g a controller) + // to retrieve it (e.g to make an API request to another application). + var properties = new AuthenticationProperties(new Dictionary + { + [context.TokenType] = context.Token + }); + + return new AuthenticationTicket((ClaimsIdentity) context.Principal.Identity, properties); + } } protected override async Task TeardownCoreAsync() diff --git a/src/OpenIddict.Validation/OpenIddictValidationEvents.cs b/src/OpenIddict.Validation/OpenIddictValidationEvents.cs index e5b21c44..6557a007 100644 --- a/src/OpenIddict.Validation/OpenIddictValidationEvents.cs +++ b/src/OpenIddict.Validation/OpenIddictValidationEvents.cs @@ -267,6 +267,11 @@ namespace OpenIddict.Validation /// Gets or sets the token to validate. /// public string Token { get; set; } + + /// + /// Gets or sets the expected type of the token. + /// + public string TokenType { get; set; } } /// diff --git a/src/OpenIddict.Validation/OpenIddictValidationHandlers.cs b/src/OpenIddict.Validation/OpenIddictValidationHandlers.cs index 1e99a71d..cead04db 100644 --- a/src/OpenIddict.Validation/OpenIddictValidationHandlers.cs +++ b/src/OpenIddict.Validation/OpenIddictValidationHandlers.cs @@ -79,8 +79,6 @@ namespace OpenIddict.Validation if (string.IsNullOrEmpty(context.Request.AccessToken)) { - context.Logger.LogError("The request was rejected because the access token was missing."); - context.Reject( error: Errors.MissingToken, description: "The access token is missing."); @@ -89,6 +87,7 @@ namespace OpenIddict.Validation } context.Token = context.Request.AccessToken; + context.TokenType = TokenTypeHints.AccessToken; return default; } @@ -143,7 +142,8 @@ namespace OpenIddict.Validation return; } - if (!await _tokenManager.HasTypeAsync(token, TokenTypeHints.AccessToken)) + // If the type associated with the token entry doesn't match the expected type, return an error. + if (!string.IsNullOrEmpty(context.TokenType) && !await _tokenManager.HasTypeAsync(token, context.TokenType)) { context.Reject( error: Errors.InvalidToken, @@ -220,6 +220,21 @@ namespace OpenIddict.Validation parameters.ValidIssuer = configuration.Issuer ?? context.Issuer?.AbsoluteUri; parameters.IssuerSigningKeys = configuration.SigningKeys; + // If a specific token type is expected, override the default valid types to reject + // security tokens whose actual token type doesn't match the expected token type. + if (!string.IsNullOrEmpty(context.TokenType)) + { + parameters.ValidTypes = new[] + { + context.TokenType switch + { + TokenTypeHints.AccessToken => JsonWebTokenTypes.AccessToken, + + _ => throw new InvalidOperationException("The token type is not supported.") + } + }; + } + // Populate the token decryption keys from the encryption credentials set in the options. parameters.TokenDecryptionKeys = from credentials in context.Options.EncryptionCredentials @@ -241,10 +256,16 @@ namespace OpenIddict.Validation return; } - // Note: tokens that are considered valid at this point are guaranteed to be access tokens, - // as a "typ" header validation is performed by the JWT handler, based on the valid values - // set in the token validation parameters (by default, only "at+jwt" is considered valid). - context.Principal = new ClaimsPrincipal(result.ClaimsIdentity).SetTokenType(TokenTypeHints.AccessToken); + // Attach the principal extracted from the token to the parent event context. + context.Principal = new ClaimsPrincipal(result.ClaimsIdentity); + + // Store the token type (resolved from "typ" or "token_usage") as a special private claim. + context.Principal.SetTokenType(result.TokenType switch + { + JsonWebTokenTypes.AccessToken => TokenTypeHints.AccessToken, + + _ => throw new InvalidOperationException("The token type is not supported.") + }); context.Logger.LogTrace("The self-contained JWT token '{Token}' was successfully validated and the following " + "claims could be extracted: {Claims}.", context.Token, context.Principal.Claims); @@ -491,22 +512,25 @@ namespace OpenIddict.Validation // (using the "typ" header) or by ASP.NET Core Data Protection (using per-token-type purposes strings). // To ensure tokens deserialized using a custom routine are of the expected type, a manual check is used, // which requires that a special claim containing the token type be present in the security principal. - var type = context.Principal.GetTokenType(); - if (string.IsNullOrEmpty(type)) + if (!string.IsNullOrEmpty(context.TokenType)) { - throw new InvalidOperationException(new StringBuilder() - .AppendLine("The deserialized principal doesn't contain the mandatory 'oi_tkn_typ' claim.") - .Append("When implementing custom token deserialization, a 'oi_tkn_typ' claim containing ") - .Append("the type of the token being processed must be added to the security principal.") - .ToString()); - } + var type = context.Principal.GetTokenType(); + if (string.IsNullOrEmpty(type)) + { + throw new InvalidOperationException(new StringBuilder() + .AppendLine("The deserialized principal doesn't contain the mandatory 'oi_tkn_typ' claim.") + .Append("When implementing custom token deserialization, a 'oi_tkn_typ' claim containing ") + .Append("the type of the token being processed must be added to the security principal.") + .ToString()); + } - if (!string.Equals(type, TokenTypeHints.AccessToken, StringComparison.OrdinalIgnoreCase)) - { - throw new InvalidOperationException(new StringBuilder() - .AppendFormat("The type of token associated with the deserialized principal ({0})", type) - .AppendFormat("doesn't match the expected token type ({0}).", TokenTypeHints.AccessToken) - .ToString()); + if (!string.Equals(type, context.TokenType, StringComparison.OrdinalIgnoreCase)) + { + throw new InvalidOperationException(new StringBuilder() + .AppendFormat("The type of token associated with the deserialized principal ({0}) ", type) + .AppendFormat("doesn't match the expected token type ({0}).", context.TokenType) + .ToString()); + } } return default; @@ -544,7 +568,7 @@ namespace OpenIddict.Validation var date = context.Principal.GetExpirationDate(); if (date.HasValue && date.Value < DateTimeOffset.UtcNow) { - context.Logger.LogError("The request was rejected because the access token was expired."); + context.Logger.LogError("The authentication demand was rejected because the token was expired."); context.Reject( error: Errors.InvalidToken, @@ -597,7 +621,7 @@ namespace OpenIddict.Validation var audiences = context.Principal.GetAudiences(); if (audiences.IsDefaultOrEmpty) { - context.Logger.LogError("The request was rejected because the access token had no audience attached."); + context.Logger.LogError("The authentication demand was rejected because the token had no audience attached."); context.Reject( error: Errors.InvalidToken, @@ -609,7 +633,7 @@ namespace OpenIddict.Validation // If the access token doesn't include any registered audience, return an error. if (!audiences.Intersect(context.Options.Audiences, StringComparer.Ordinal).Any()) { - context.Logger.LogError("The request was rejected because the access token had no valid audience."); + context.Logger.LogError("The authentication demand was rejected because the token had no valid audience."); context.Reject( error: Errors.InvalidToken, diff --git a/src/OpenIddict.Validation/OpenIddictValidationProvider.cs b/src/OpenIddict.Validation/OpenIddictValidationProvider.cs index 67773208..846ba964 100644 --- a/src/OpenIddict.Validation/OpenIddictValidationProvider.cs +++ b/src/OpenIddict.Validation/OpenIddictValidationProvider.cs @@ -56,15 +56,24 @@ namespace OpenIddict.Validation switch (context) { case BaseRequestContext notification when notification.IsRequestHandled: - _logger.LogDebug("The request was handled in user code."); + if (_logger.IsEnabled(LogLevel.Debug)) + { + _logger.LogDebug("The event context was marked as handled by {Name}.", handler.GetType().FullName); + } return; case BaseRequestContext notification when notification.IsRequestSkipped: - _logger.LogDebug("The default request handling was skipped from user code."); + if (_logger.IsEnabled(LogLevel.Debug)) + { + _logger.LogDebug("The event context was marked as skipped by {Name}.", handler.GetType().FullName); + } return; case BaseValidatingContext notification when notification.IsRejected: - _logger.LogDebug("The request was rejected in user code."); + if (_logger.IsEnabled(LogLevel.Debug)) + { + _logger.LogDebug("The event context was marked as rejected by {Name}.", handler.GetType().FullName); + } return; default: continue;