move consent message store loading to after request validation // fixes…

…: #199

(cherry picked from commit a08a533)

src/IdentityServer/Endpoints/AuthorizeCallbackEndpoint.cs

@@ -1,13 +1,11 @@
 // Copyright (c) Duende Software. All rights reserved.
 // See LICENSE in the project root for license information.
 
-using System.Collections.Specialized;
 using System.Net;
 using System.Threading.Tasks;
 using Duende.IdentityServer.Configuration;
 using Duende.IdentityServer.Endpoints.Results;
 using Duende.IdentityServer.Hosting;
-using Duende.IdentityServer.Models;
 using Duende.IdentityServer.ResponseHandling;
 using Duende.IdentityServer.Services;
 using Duende.IdentityServer.Stores;
@@ -20,9 +18,6 @@ namespace Duende.IdentityServer.Endpoints
 {
     internal class AuthorizeCallbackEndpoint : AuthorizeEndpointBase
     {
-        private readonly IConsentMessageStore _consentResponseStore;
-        private readonly IAuthorizationParametersMessageStore _authorizationParametersMessageStore;
-
         public AuthorizeCallbackEndpoint(
             IEventService events,
             ILogger<AuthorizeCallbackEndpoint> logger,
@@ -33,10 +28,8 @@ public AuthorizeCallbackEndpoint(
             IUserSession userSession,
             IConsentMessageStore consentResponseStore,
             IAuthorizationParametersMessageStore authorizationParametersMessageStore = null)
-            : base(events, logger, options, validator, interactionGenerator, authorizeResponseGenerator, userSession)
+            : base(events, logger, options, validator, interactionGenerator, authorizeResponseGenerator, userSession, consentResponseStore, authorizationParametersMessageStore)
         {
-            _consentResponseStore = consentResponseStore;
-            _authorizationParametersMessageStore = authorizationParametersMessageStore;
         }
 
         public override async Task<IEndpointResult> ProcessAsync(HttpContext context)
@@ -50,39 +43,13 @@ public override async Task<IEndpointResult> ProcessAsync(HttpContext context)
             Logger.LogDebug("Start authorize callback request");
 
             var parameters = context.Request.Query.AsNameValueCollection();
-            if (_authorizationParametersMessageStore != null)
-            {
-                var messageStoreId = parameters[Constants.AuthorizationParamsStore.MessageStoreIdParameterName];
-                var entry = await _authorizationParametersMessageStore.ReadAsync(messageStoreId);
-                parameters = entry?.Data.FromFullDictionary() ?? new NameValueCollection();
-
-                await _authorizationParametersMessageStore.DeleteAsync(messageStoreId);
-            }
-
             var user = await UserSession.GetUserAsync();
-            var consentRequest = new ConsentRequest(parameters, user?.GetSubjectId());
-            var consent = await _consentResponseStore.ReadAsync(consentRequest.Id);
-
-            if (consent != null && consent.Data == null)
-            {
-                return await CreateErrorResultAsync("consent message is missing data");
-            }
 
-            try
-            {
-                var result = await ProcessAuthorizeRequestAsync(parameters, user, consent?.Data);
+            var result = await ProcessAuthorizeRequestAsync(parameters, user, true);
 
-                Logger.LogTrace("End Authorize Request. Result type: {0}", result?.GetType().ToString() ?? "-none-");
+            Logger.LogTrace("End Authorize Request. Result type: {0}", result?.GetType().ToString() ?? "-none-");
 
-                return result;
-            }
-            finally
-            {
-                if (consent != null)
-                {
-                    await _consentResponseStore.DeleteAsync(consentRequest.Id);
-                }
-            }
+            return result;
         }
     }
 }

src/IdentityServer/Endpoints/AuthorizeEndpoint.cs

@@ -13,6 +13,7 @@
 using Duende.IdentityServer.Extensions;
 using Microsoft.AspNetCore.Http;
 using Microsoft.Extensions.Logging;
+using Duende.IdentityServer.Stores;
 
 namespace Duende.IdentityServer.Endpoints
 {
@@ -25,8 +26,10 @@ public AuthorizeEndpoint(
            IAuthorizeRequestValidator validator,
            IAuthorizeInteractionResponseGenerator interactionGenerator,
            IAuthorizeResponseGenerator authorizeResponseGenerator,
-           IUserSession userSession)
-            : base(events, logger, options, validator, interactionGenerator, authorizeResponseGenerator, userSession)
+           IUserSession userSession,
+           IConsentMessageStore consentResponseStore,
+           IAuthorizationParametersMessageStore authorizationParametersMessageStore = null)
+            : base(events, logger, options, validator, interactionGenerator, authorizeResponseGenerator, userSession, consentResponseStore, authorizationParametersMessageStore)
         {
         }
 
@@ -55,7 +58,7 @@ public override async Task<IEndpointResult> ProcessAsync(HttpContext context)
             }
 
             var user = await UserSession.GetUserAsync();
-            var result = await ProcessAuthorizeRequestAsync(values, user, null);
+            var result = await ProcessAuthorizeRequestAsync(values, user);
 
             Logger.LogTrace("End authorize request. result type: {0}", result?.GetType().ToString() ?? "-none-");
 

src/IdentityServer/Endpoints/AuthorizeEndpointBase.cs

@@ -1,4 +1,4 @@
-// Copyright (c) Duende Software. All rights reserved.
+// Copyright (c) Duende Software. All rights reserved.
 // See LICENSE in the project root for license information.
 
 using System;
@@ -18,6 +18,7 @@
 using Duende.IdentityServer.Extensions;
 using Microsoft.AspNetCore.Http;
 using Microsoft.Extensions.Logging;
+using Duende.IdentityServer.Stores;
 
 namespace Duende.IdentityServer.Endpoints
 {
@@ -32,14 +33,19 @@ internal abstract class AuthorizeEndpointBase : IEndpointHandler
 
         private readonly IAuthorizeRequestValidator _validator;
 
+        private readonly IConsentMessageStore _consentResponseStore;
+        private readonly IAuthorizationParametersMessageStore _authorizationParametersMessageStore;
+
         protected AuthorizeEndpointBase(
             IEventService events,
             ILogger<AuthorizeEndpointBase> logger,
             IdentityServerOptions options,
             IAuthorizeRequestValidator validator,
             IAuthorizeInteractionResponseGenerator interactionGenerator,
             IAuthorizeResponseGenerator authorizeResponseGenerator,
-            IUserSession userSession)
+            IUserSession userSession,
+            IConsentMessageStore consentResponseStore,
+            IAuthorizationParametersMessageStore authorizationParametersMessageStore = null)
         {
             _events = events;
             _options = options;
@@ -48,6 +54,8 @@ protected AuthorizeEndpointBase(
             _interactionGenerator = interactionGenerator;
             _authorizeResponseGenerator = authorizeResponseGenerator;
             UserSession = userSession;
+            _consentResponseStore = consentResponseStore;
+            _authorizationParametersMessageStore = authorizationParametersMessageStore;
         }
 
         protected ILogger Logger { get; private set; }
@@ -56,7 +64,7 @@ protected AuthorizeEndpointBase(
 
         public abstract Task<IEndpointResult> ProcessAsync(HttpContext context);
 
-        internal async Task<IEndpointResult> ProcessAuthorizeRequestAsync(NameValueCollection parameters, ClaimsPrincipal user, ConsentResponse consent)
+        internal async Task<IEndpointResult> ProcessAuthorizeRequestAsync(NameValueCollection parameters, ClaimsPrincipal user, bool checkConsentResponse = false)
         {
             if (user != null)
             {
@@ -67,6 +75,15 @@ internal async Task<IEndpointResult> ProcessAuthorizeRequestAsync(NameValueColle
                 Logger.LogDebug("No user present in authorize request");
             }
 
+            if (checkConsentResponse && _authorizationParametersMessageStore != null)
+            {
+                var messageStoreId = parameters[Constants.AuthorizationParamsStore.MessageStoreIdParameterName];
+                var entry = await _authorizationParametersMessageStore.ReadAsync(messageStoreId);
+                parameters = entry?.Data.FromFullDictionary() ?? new NameValueCollection();
+
+                await _authorizationParametersMessageStore.DeleteAsync(messageStoreId);
+            }
+
             // validate request
             var result = await _validator.ValidateAsync(parameters, user);
             if (result.IsError)
@@ -78,35 +95,61 @@ internal async Task<IEndpointResult> ProcessAuthorizeRequestAsync(NameValueColle
                     result.ErrorDescription);
             }
 
-            var request = result.ValidatedRequest;
-            LogRequest(request);
+            string consentRequestId = null;
 
-            // determine user interaction
-            var interactionResult = await _interactionGenerator.ProcessInteractionAsync(request, consent);
-            if (interactionResult.IsError)
-            {
-                return await CreateErrorResultAsync("Interaction generator error", request, interactionResult.Error, interactionResult.ErrorDescription, false);
-            }
-            if (interactionResult.IsLogin)
+            try
             {
-                return new LoginPageResult(request);
+                Message<ConsentResponse> consent = null;
+
+                if (checkConsentResponse)
+                {
+                    var consentRequest = new ConsentRequest(result.ValidatedRequest.Raw, user?.GetSubjectId());
+                    consentRequestId = consentRequest.Id;
+                    consent = await _consentResponseStore.ReadAsync(consentRequestId);
+
+                    if (consent != null && consent.Data == null)
+                    {
+                        return await CreateErrorResultAsync("consent message is missing data");
+                    }
+                }
+
+                var request = result.ValidatedRequest;
+                LogRequest(request);
+
+                // determine user interaction
+                var interactionResult = await _interactionGenerator.ProcessInteractionAsync(request, consent?.Data);
+                if (interactionResult.IsError)
+                {
+                    return await CreateErrorResultAsync("Interaction generator error", request, interactionResult.Error, interactionResult.ErrorDescription, false);
+                }
+                if (interactionResult.IsLogin)
+                {
+                    return new LoginPageResult(request);
+                }
+                if (interactionResult.IsConsent)
+                {
+                    return new ConsentPageResult(request);
+                }
+                if (interactionResult.IsRedirect)
+                {
+                    return new CustomRedirectResult(request, interactionResult.RedirectUrl);
+                }
+
+                var response = await _authorizeResponseGenerator.CreateResponseAsync(request);
+
+                await RaiseResponseEventAsync(response);
+
+                LogResponse(response);
+
+                return new AuthorizeResult(response);
             }
-            if (interactionResult.IsConsent)
+            finally
             {
-                return new ConsentPageResult(request);
+                if (consentRequestId != null)
+                {
+                    await _consentResponseStore.DeleteAsync(consentRequestId);
+                }
             }
-            if (interactionResult.IsRedirect)
-            {
-                return new CustomRedirectResult(request, interactionResult.RedirectUrl);
-            }
-
-            var response = await _authorizeResponseGenerator.CreateResponseAsync(request);
-
-            await RaiseResponseEventAsync(response);
-
-            LogResponse(response);
-
-            return new AuthorizeResult(response);
         }
 
         protected async Task<IEndpointResult> CreateErrorResultAsync(

src/IdentityServer/Models/Messages/ConsentRequest.cs

@@ -1,4 +1,4 @@
-// Copyright (c) Duende Software. All rights reserved.
+// Copyright (c) Duende Software. All rights reserved.
 // See LICENSE in the project root for license information.
 
 

test/IdentityServer.IntegrationTests/Common/BrowserHandler.cs

@@ -1,4 +1,4 @@
-// Copyright (c) Duende Software. All rights reserved.
+// Copyright (c) Duende Software. All rights reserved.
 // See LICENSE in the project root for license information.
 
 

test/IdentityServer.IntegrationTests/Endpoints/Authorize/JwtRequestAuthorizeTests.cs

@@ -1281,13 +1281,69 @@ public async Task prompt_login_should_allow_user_to_login_and_complete_authoriza
                 });
             var response = await _mockPipeline.BrowserClient.GetAsync(url);
 
-            // this simulates the login page returning to the returnUrl whichi is the authorize callback page
+            // this simulates the login page returning to the returnUrl which is the authorize callback page
             _mockPipeline.BrowserClient.AllowAutoRedirect = false;
             response = await _mockPipeline.BrowserClient.GetAsync(IdentityServerPipeline.BaseUrl + _mockPipeline.LoginReturnUrl);
             response.StatusCode.Should().Be(HttpStatusCode.Redirect);
             response.Headers.Location.ToString().Should().StartWith("https://client/callback");
             response.Headers.Location.ToString().Should().Contain("id_token=");
             response.Headers.Location.ToString().Should().Contain("state=state123");
         }
+
+        [Theory]
+        [InlineData((Type) null)]
+        [InlineData(typeof(QueryStringAuthorizationParametersMessageStore))]
+        [InlineData(typeof(DistributedCacheAuthorizationParametersMessageStore))]
+        [Trait("Category", Category)]
+        public async Task prompt_login_should_allow_user_to_consent_and_complete_authorization(Type storeType)
+        {
+            if (storeType != null)
+            {
+                _mockPipeline.OnPostConfigureServices += services =>
+                {
+                    services.AddTransient(typeof(IAuthorizationParametersMessageStore), storeType);
+                };
+                _mockPipeline.Initialize();
+            }
+
+            _client.RequireConsent = true;
+
+            await _mockPipeline.LoginAsync("bob");
+
+            var requestJwt = CreateRequestJwt(
+                issuer: _client.ClientId,
+                audience: IdentityServerPipeline.BaseUrl,
+                credential: new X509SigningCredentials(TestCert.Load()),
+                claims: new[] {
+                    new Claim("client_id", _client.ClientId),
+                    new Claim("response_type", "id_token"),
+                    new Claim("scope", "openid profile"),
+                    new Claim("redirect_uri", "https://client/callback"),
+            });
+
+            var url = _mockPipeline.CreateAuthorizeUrl(
+                clientId: _client.ClientId,
+                responseType: "id_token",
+                state: "state123",
+                nonce: "nonce123",
+                extra: new
+                {
+                    request = requestJwt
+                });
+
+            _mockPipeline.ConsentResponse = new ConsentResponse()
+            {
+                ScopesValuesConsented = new string[] { "openid", "profile" }
+            };
+            _mockPipeline.BrowserClient.StopRedirectingAfter = 2;
+
+            var response = await _mockPipeline.BrowserClient.GetAsync(url);
+
+
+            response.StatusCode.Should().Be(HttpStatusCode.Redirect);
+            response.Headers.Location.ToString().Should().StartWith("https://client/callback");
+            response.Headers.Location.ToString().Should().Contain("id_token=");
+            response.Headers.Location.ToString().Should().Contain("state=state123");
+        }
     }
 }