From 45d90453c8fbbc2be153188cab6d47c51790e5c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Chalet?= Date: Tue, 16 Jun 2020 02:43:21 +0200 Subject: [PATCH] Fix the ProcessHostRedirectionResponse handler to be invoked after ProcessRedirectionResponse --- .../OpenIddictServerAspNetCoreBuilder.cs | 5 +- ...OpenIddictServerAspNetCoreConfiguration.cs | 24 +++- .../OpenIddictServerAspNetCoreExtensions.cs | 4 +- ...ServerAspNetCoreHandlers.Authentication.cs | 2 +- ...enIddictServerAspNetCoreHandlers.Device.cs | 4 +- ...nIddictServerAspNetCoreHandlers.Session.cs | 12 +- .../OpenIddictServerAspNetCoreHandlers.cs | 104 +++++++++--------- .../OpenIddictServerAspNetCoreOptions.cs | 5 +- .../OpenIddictServerOwinHandlers.Device.cs | 2 +- .../OpenIddictServerOwinHandlers.Session.cs | 10 +- .../OpenIddictServerOwinHandlers.cs | 104 +++++++++--------- ...penIddictServerIntegrationTests.Session.cs | 55 +++++++++ 12 files changed, 208 insertions(+), 123 deletions(-) diff --git a/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreBuilder.cs b/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreBuilder.cs index 40634442..6a46075c 100644 --- a/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreBuilder.cs +++ b/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreBuilder.cs @@ -72,8 +72,11 @@ namespace Microsoft.Extensions.DependencyInjection /// Enables error pass-through support, so that the rest of the request processing pipeline is /// automatically invoked when returning an error from the interactive authorization and logout endpoints. /// When this option is enabled, special logic must be added to these actions to handle errors, that can be - /// retrieved using + /// retrieved using . /// + /// + /// Important: the error pass-through mode cannot be used when the status code pages integration is enabled. + /// /// The . [EditorBrowsable(EditorBrowsableState.Advanced)] public OpenIddictServerAspNetCoreBuilder EnableErrorPassthrough() diff --git a/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreConfiguration.cs b/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreConfiguration.cs index e7684125..3866265c 100644 --- a/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreConfiguration.cs +++ b/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreConfiguration.cs @@ -18,7 +18,8 @@ namespace OpenIddict.Server.AspNetCore /// public class OpenIddictServerAspNetCoreConfiguration : IConfigureOptions, IConfigureOptions, - IPostConfigureOptions + IPostConfigureOptions, + IPostConfigureOptions { /// /// Registers the OpenIddict server handler in the global authentication options. @@ -97,5 +98,26 @@ namespace OpenIddict.Server.AspNetCore .ToString()); } } + + /// + /// Populates the default OpenIddict server ASP.NET Core options and + /// ensures that the configuration is in a consistent and valid state. + /// + /// The name of the options instance to configure, if applicable. + /// The options instance to initialize. + public void PostConfigure([CanBeNull] string name, [NotNull] OpenIddictServerAspNetCoreOptions options) + { + if (options == null) + { + throw new ArgumentNullException(nameof(options)); + } + + if (options.EnableErrorPassthrough && options.EnableStatusCodePagesIntegration) + { + throw new InvalidOperationException(new StringBuilder() + .Append("The error pass-through mode cannot be used when the status code pages integration is enabled.") + .ToString()); + } + } } } diff --git a/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreExtensions.cs b/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreExtensions.cs index 9dac76c2..d7dbe6d5 100644 --- a/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreExtensions.cs +++ b/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreExtensions.cs @@ -63,7 +63,9 @@ namespace Microsoft.Extensions.DependencyInjection ServiceDescriptor.Singleton, OpenIddictServerAspNetCoreConfiguration>(), ServiceDescriptor.Singleton, OpenIddictServerAspNetCoreConfiguration>(), - ServiceDescriptor.Singleton, OpenIddictServerAspNetCoreConfiguration>() + ServiceDescriptor.Singleton, OpenIddictServerAspNetCoreConfiguration>(), + + ServiceDescriptor.Singleton, OpenIddictServerAspNetCoreConfiguration>() }); return new OpenIddictServerAspNetCoreBuilder(builder.Services); diff --git a/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreHandlers.Authentication.cs b/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreHandlers.Authentication.cs index 7ae24f32..1dcde30b 100644 --- a/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreHandlers.Authentication.cs +++ b/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreHandlers.Authentication.cs @@ -57,8 +57,8 @@ namespace OpenIddict.Server.AspNetCore ProcessFormPostResponse.Descriptor, ProcessQueryResponse.Descriptor, ProcessFragmentResponse.Descriptor, - ProcessStatusCodePagesErrorResponse.Descriptor, ProcessPassthroughErrorResponse.Descriptor, + ProcessStatusCodePagesErrorResponse.Descriptor, ProcessLocalErrorResponse.Descriptor); /// diff --git a/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreHandlers.Device.cs b/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreHandlers.Device.cs index eeba9330..a549da28 100644 --- a/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreHandlers.Device.cs +++ b/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreHandlers.Device.cs @@ -43,10 +43,10 @@ namespace OpenIddict.Server.AspNetCore */ AttachHttpResponseCode.Descriptor, AttachCacheControlHeader.Descriptor, - ProcessHostRedirectionResponse.Descriptor, - ProcessStatusCodePagesErrorResponse.Descriptor, ProcessPassthroughErrorResponse.Descriptor, + ProcessStatusCodePagesErrorResponse.Descriptor, ProcessLocalErrorResponse.Descriptor, + ProcessHostRedirectionResponse.Descriptor, ProcessEmptyResponse.Descriptor); } } diff --git a/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreHandlers.Session.cs b/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreHandlers.Session.cs index 9ec38aef..760d814d 100644 --- a/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreHandlers.Session.cs +++ b/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreHandlers.Session.cs @@ -52,11 +52,11 @@ namespace OpenIddict.Server.AspNetCore RemoveCachedRequest.Descriptor, AttachHttpResponseCode.Descriptor, AttachCacheControlHeader.Descriptor, - ProcessQueryResponse.Descriptor, - ProcessHostRedirectionResponse.Descriptor, - ProcessStatusCodePagesErrorResponse.Descriptor, + ProcessRedirectionResponse.Descriptor, ProcessPassthroughErrorResponse.Descriptor, + ProcessStatusCodePagesErrorResponse.Descriptor, ProcessLocalErrorResponse.Descriptor, + ProcessHostRedirectionResponse.Descriptor, ProcessEmptyResponse.Descriptor); /// @@ -308,7 +308,7 @@ namespace OpenIddict.Server.AspNetCore .AddFilter() .AddFilter() .UseSingletonHandler() - .SetOrder(ProcessQueryResponse.Descriptor.Order - 1_000) + .SetOrder(ProcessRedirectionResponse.Descriptor.Order - 1_000) .Build(); /// @@ -344,7 +344,7 @@ namespace OpenIddict.Server.AspNetCore /// Contains the logic responsible of processing logout responses. /// Note: this handler is not used when the OpenID Connect request is not initially handled by ASP.NET Core. /// - public class ProcessQueryResponse : IOpenIddictServerHandler + public class ProcessRedirectionResponse : IOpenIddictServerHandler { /// /// Gets the default descriptor definition assigned to this handler. @@ -352,7 +352,7 @@ namespace OpenIddict.Server.AspNetCore public static OpenIddictServerHandlerDescriptor Descriptor { get; } = OpenIddictServerHandlerDescriptor.CreateBuilder() .AddFilter() - .UseSingletonHandler() + .UseSingletonHandler() .SetOrder(ProcessStatusCodePagesErrorResponse.Descriptor.Order - 1_000) .Build(); diff --git a/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreHandlers.cs b/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreHandlers.cs index 6ceaeee6..563635f3 100644 --- a/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreHandlers.cs +++ b/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreHandlers.cs @@ -820,58 +820,6 @@ namespace OpenIddict.Server.AspNetCore } } - /// - /// Contains the logic responsible of processing empty OpenID Connect responses that should trigger a host redirection. - /// Note: this handler is not used when the OpenID Connect request is not initially handled by ASP.NET Core. - /// - public class ProcessHostRedirectionResponse : IOpenIddictServerHandler - where TContext : BaseRequestContext - { - /// - /// Gets the default descriptor definition assigned to this handler. - /// - public static OpenIddictServerHandlerDescriptor Descriptor { get; } - = OpenIddictServerHandlerDescriptor.CreateBuilder() - .AddFilter() - .UseSingletonHandler>() - .SetOrder(ProcessJsonResponse.Descriptor.Order - 1_000) - .Build(); - - /// - /// Processes the event. - /// - /// The context associated with the event to process. - /// - /// A that can be used to monitor the asynchronous operation. - /// - public ValueTask HandleAsync([NotNull] TContext context) - { - if (context == null) - { - throw new ArgumentNullException(nameof(context)); - } - - // This handler only applies to ASP.NET Core requests. If the HTTP context cannot be resolved, - // this may indicate that the request was incorrectly processed by another server stack. - var response = context.Transaction.GetHttpRequest()?.HttpContext.Response; - if (response == null) - { - throw new InvalidOperationException("The ASP.NET Core HTTP request cannot be resolved."); - } - - var properties = context.Transaction.GetProperty(typeof(AuthenticationProperties).FullName); - if (properties != null && !string.IsNullOrEmpty(properties.RedirectUri)) - { - response.Redirect(properties.RedirectUri); - - context.Logger.LogInformation("The response was successfully returned as a 302 response."); - context.HandleRequest(); - } - - return default; - } - } - /// /// Contains the logic responsible of attaching an appropriate HTTP status code. /// Note: this handler is not used when the OpenID Connect request is not initially handled by ASP.NET Core. @@ -1410,6 +1358,58 @@ namespace OpenIddict.Server.AspNetCore } } + /// + /// Contains the logic responsible of processing empty OpenID Connect responses that should trigger a host redirection. + /// Note: this handler is not used when the OpenID Connect request is not initially handled by ASP.NET Core. + /// + public class ProcessHostRedirectionResponse : IOpenIddictServerHandler + where TContext : BaseRequestContext + { + /// + /// Gets the default descriptor definition assigned to this handler. + /// + public static OpenIddictServerHandlerDescriptor Descriptor { get; } + = OpenIddictServerHandlerDescriptor.CreateBuilder() + .AddFilter() + .UseSingletonHandler>() + .SetOrder(ProcessEmptyResponse.Descriptor.Order - 1_000) + .Build(); + + /// + /// Processes the event. + /// + /// The context associated with the event to process. + /// + /// A that can be used to monitor the asynchronous operation. + /// + public ValueTask HandleAsync([NotNull] TContext context) + { + if (context == null) + { + throw new ArgumentNullException(nameof(context)); + } + + // This handler only applies to ASP.NET Core requests. If the HTTP context cannot be resolved, + // this may indicate that the request was incorrectly processed by another server stack. + var response = context.Transaction.GetHttpRequest()?.HttpContext.Response; + if (response == null) + { + throw new InvalidOperationException("The ASP.NET Core HTTP request cannot be resolved."); + } + + var properties = context.Transaction.GetProperty(typeof(AuthenticationProperties).FullName); + if (properties != null && !string.IsNullOrEmpty(properties.RedirectUri)) + { + response.Redirect(properties.RedirectUri); + + context.Logger.LogInformation("The response was successfully returned as a 302 response."); + context.HandleRequest(); + } + + return default; + } + } + /// /// Contains the logic responsible of processing OpenID Connect responses that don't specify any parameter. /// Note: this handler is not used when the OpenID Connect request is not initially handled by ASP.NET Core. diff --git a/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreOptions.cs b/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreOptions.cs index 3ad63dad..933cc1df 100644 --- a/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreOptions.cs +++ b/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreOptions.cs @@ -35,8 +35,11 @@ namespace OpenIddict.Server.AspNetCore /// Gets or sets a boolean indicating whether OpenIddict should allow the rest of the request processing pipeline /// to be invoked when returning an error from the interactive authorization and logout endpoints. /// When this option is enabled, special logic must be added to these actions to handle errors, that can be - /// retrieved using + /// retrieved using . /// + /// + /// Important: the error pass-through mode cannot be used when the status code pages integration is enabled. + /// public bool EnableErrorPassthrough { get; set; } /// diff --git a/src/OpenIddict.Server.Owin/OpenIddictServerOwinHandlers.Device.cs b/src/OpenIddict.Server.Owin/OpenIddictServerOwinHandlers.Device.cs index b8a9c2fc..4303e840 100644 --- a/src/OpenIddict.Server.Owin/OpenIddictServerOwinHandlers.Device.cs +++ b/src/OpenIddict.Server.Owin/OpenIddictServerOwinHandlers.Device.cs @@ -43,9 +43,9 @@ namespace OpenIddict.Server.Owin */ AttachHttpResponseCode.Descriptor, AttachCacheControlHeader.Descriptor, - ProcessHostRedirectionResponse.Descriptor, ProcessPassthroughErrorResponse.Descriptor, ProcessLocalErrorResponse.Descriptor, + ProcessHostRedirectionResponse.Descriptor, ProcessEmptyResponse.Descriptor); } } diff --git a/src/OpenIddict.Server.Owin/OpenIddictServerOwinHandlers.Session.cs b/src/OpenIddict.Server.Owin/OpenIddictServerOwinHandlers.Session.cs index a88bdc4e..6d8bc112 100644 --- a/src/OpenIddict.Server.Owin/OpenIddictServerOwinHandlers.Session.cs +++ b/src/OpenIddict.Server.Owin/OpenIddictServerOwinHandlers.Session.cs @@ -52,10 +52,10 @@ namespace OpenIddict.Server.Owin RemoveCachedRequest.Descriptor, AttachHttpResponseCode.Descriptor, AttachCacheControlHeader.Descriptor, - ProcessQueryResponse.Descriptor, - ProcessHostRedirectionResponse.Descriptor, + ProcessRedirectionResponse.Descriptor, ProcessPassthroughErrorResponse.Descriptor, ProcessLocalErrorResponse.Descriptor, + ProcessHostRedirectionResponse.Descriptor, ProcessEmptyResponse.Descriptor); /// @@ -302,7 +302,7 @@ namespace OpenIddict.Server.Owin .AddFilter() .AddFilter() .UseSingletonHandler() - .SetOrder(ProcessQueryResponse.Descriptor.Order - 1_000) + .SetOrder(ProcessRedirectionResponse.Descriptor.Order - 1_000) .Build(); /// @@ -338,7 +338,7 @@ namespace OpenIddict.Server.Owin /// Contains the logic responsible of processing logout responses. /// Note: this handler is not used when the OpenID Connect request is not initially handled by OWIN. /// - public class ProcessQueryResponse : IOpenIddictServerHandler + public class ProcessRedirectionResponse : IOpenIddictServerHandler { /// /// Gets the default descriptor definition assigned to this handler. @@ -346,7 +346,7 @@ namespace OpenIddict.Server.Owin public static OpenIddictServerHandlerDescriptor Descriptor { get; } = OpenIddictServerHandlerDescriptor.CreateBuilder() .AddFilter() - .UseSingletonHandler() + .UseSingletonHandler() .SetOrder(ProcessPassthroughErrorResponse>.Descriptor.Order - 1_000) .Build(); diff --git a/src/OpenIddict.Server.Owin/OpenIddictServerOwinHandlers.cs b/src/OpenIddict.Server.Owin/OpenIddictServerOwinHandlers.cs index 0c9cf050..783b39c9 100644 --- a/src/OpenIddict.Server.Owin/OpenIddictServerOwinHandlers.cs +++ b/src/OpenIddict.Server.Owin/OpenIddictServerOwinHandlers.cs @@ -751,58 +751,6 @@ namespace OpenIddict.Server.Owin } } - /// - /// Contains the logic responsible of processing empty OpenID Connect responses that should trigger a host redirection. - /// Note: this handler is not used when the OpenID Connect request is not initially handled by OWIN. - /// - public class ProcessHostRedirectionResponse : IOpenIddictServerHandler - where TContext : BaseRequestContext - { - /// - /// Gets the default descriptor definition assigned to this handler. - /// - public static OpenIddictServerHandlerDescriptor Descriptor { get; } - = OpenIddictServerHandlerDescriptor.CreateBuilder() - .AddFilter() - .UseSingletonHandler>() - .SetOrder(ProcessJsonResponse.Descriptor.Order - 1_000) - .Build(); - - /// - /// Processes the event. - /// - /// The context associated with the event to process. - /// - /// A that can be used to monitor the asynchronous operation. - /// - public ValueTask HandleAsync([NotNull] TContext context) - { - if (context == null) - { - throw new ArgumentNullException(nameof(context)); - } - - // This handler only applies to OWIN requests. If The OWIN request cannot be resolved, - // this may indicate that the request was incorrectly processed by another server stack. - var response = context.Transaction.GetOwinRequest()?.Context.Response; - if (response == null) - { - throw new InvalidOperationException("The OWIN request cannot be resolved."); - } - - var properties = context.Transaction.GetProperty(typeof(AuthenticationProperties).FullName); - if (properties != null && !string.IsNullOrEmpty(properties.RedirectUri)) - { - response.Redirect(properties.RedirectUri); - - context.Logger.LogInformation("The response was successfully returned as a 302 response."); - context.HandleRequest(); - } - - return default; - } - } - /// /// Contains the logic responsible of attaching an appropriate HTTP status code. /// Note: this handler is not used when the OpenID Connect request is not initially handled by OWIN. @@ -1281,6 +1229,58 @@ namespace OpenIddict.Server.Owin } } + /// + /// Contains the logic responsible of processing empty OpenID Connect responses that should trigger a host redirection. + /// Note: this handler is not used when the OpenID Connect request is not initially handled by OWIN. + /// + public class ProcessHostRedirectionResponse : IOpenIddictServerHandler + where TContext : BaseRequestContext + { + /// + /// Gets the default descriptor definition assigned to this handler. + /// + public static OpenIddictServerHandlerDescriptor Descriptor { get; } + = OpenIddictServerHandlerDescriptor.CreateBuilder() + .AddFilter() + .UseSingletonHandler>() + .SetOrder(ProcessEmptyResponse.Descriptor.Order - 1_000) + .Build(); + + /// + /// Processes the event. + /// + /// The context associated with the event to process. + /// + /// A that can be used to monitor the asynchronous operation. + /// + public ValueTask HandleAsync([NotNull] TContext context) + { + if (context == null) + { + throw new ArgumentNullException(nameof(context)); + } + + // This handler only applies to OWIN requests. If The OWIN request cannot be resolved, + // this may indicate that the request was incorrectly processed by another server stack. + var response = context.Transaction.GetOwinRequest()?.Context.Response; + if (response == null) + { + throw new InvalidOperationException("The OWIN request cannot be resolved."); + } + + var properties = context.Transaction.GetProperty(typeof(AuthenticationProperties).FullName); + if (properties != null && !string.IsNullOrEmpty(properties.RedirectUri)) + { + response.Redirect(properties.RedirectUri); + + context.Logger.LogInformation("The response was successfully returned as a 302 response."); + context.HandleRequest(); + } + + return default; + } + } + /// /// Contains the logic responsible of processing OpenID Connect responses that don't specify any parameter. /// Note: this handler is not used when the OpenID Connect request is not initially handled by OWIN. diff --git a/test/OpenIddict.Server.IntegrationTests/OpenIddictServerIntegrationTests.Session.cs b/test/OpenIddict.Server.IntegrationTests/OpenIddictServerIntegrationTests.Session.cs index 913ffcfc..c8b1606e 100644 --- a/test/OpenIddict.Server.IntegrationTests/OpenIddictServerIntegrationTests.Session.cs +++ b/test/OpenIddict.Server.IntegrationTests/OpenIddictServerIntegrationTests.Session.cs @@ -531,6 +531,61 @@ namespace OpenIddict.Server.FunctionalTests Assert.Equal(new[] { "custom_value_1", "custom_value_2" }, (string[]) response["parameter_with_multiple_values"]); } + [Fact] + public async Task ApplyLogoutResponse_UsesPostLogoutRedirectUriWhenProvided() + { + // Arrange + await using var server = await CreateServerAsync(options => + { + options.EnableDegradedMode(); + + options.AddEventHandler(builder => + builder.UseInlineHandler(context => + { + context.Response["target_uri"] = context.PostLogoutRedirectUri; + + return default; + })); + }); + + await using var client = await server.CreateClientAsync(); + + // Act + var response = await client.PostAsync("/connect/logout", new OpenIddictRequest + { + PostLogoutRedirectUri = "http://www.fabrikam.com/path" + }); + + // Assert + Assert.Equal("http://www.fabrikam.com/path", (string) response["target_uri"]); + } + + [Fact] + public async Task ApplyLogoutResponse_ReturnsEmptyResponseWhenNoPostLogoutRedirectUriIsProvided() + { + // Arrange + await using var server = await CreateServerAsync(options => + { + options.EnableDegradedMode(); + + options.AddEventHandler(builder => + builder.UseInlineHandler(context => + { + context.Response["target_uri"] = context.PostLogoutRedirectUri; + + return default; + })); + }); + + await using var client = await server.CreateClientAsync(); + + // Act + var response = await client.PostAsync("/connect/logout", new OpenIddictRequest()); + + // Assert + Assert.Empty(response.GetParameters()); + } + [Fact] public async Task ApplyLogoutResponse_DoesNotSetStateWhenUserIsNotRedirected() {