DPoP nonce updates from review discussion

src/Duende.AccessTokenManagement.OpenIdConnect/ConfigureOpenIdConnectOptions.cs

@@ -78,9 +78,7 @@ public void Configure(string? name, OpenIdConnectOptions options)
             options.Events.OnAuthorizationCodeReceived = CreateCallback(options.Events.OnAuthorizationCodeReceived);
             options.Events.OnTokenValidated = CreateCallback(options.Events.OnTokenValidated);
 
-            var logger = _loggerFactory.CreateLogger<DPoPProofTokenHandler>();
-
-            options.BackchannelHttpHandler = new DPoPProofTokenHandler(_dPoPProofService, _dPoPNonceStore, _httpContextAccessor, logger)
+            options.BackchannelHttpHandler = new AuthorizationServerDPoPHandler(_dPoPProofService, _dPoPNonceStore, _httpContextAccessor, _loggerFactory)
             {
                 InnerHandler = options.BackchannelHttpHandler ?? new HttpClientHandler()
             };

src/Duende.AccessTokenManagement.OpenIdConnect/DPoPProofTokenHandler.cs

@@ -1,36 +1,41 @@
-// Copyright (c) Brock Allen & Dominick Baier. All rights reserved.
+// Copyright (c) Brock Allen & Dominick Baier. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See LICENSE in the project root for license information.
 
 using Microsoft.AspNetCore.Authentication;
 using Microsoft.AspNetCore.Http;
 using Microsoft.Extensions.Logging;
+using System.Net;
 using System.Net.Http;
 using System.Threading;
 using System.Threading.Tasks;
 
 namespace Duende.AccessTokenManagement.OpenIdConnect;
 
 /// <summary>
-/// Delegating handler that injects the DPoP proof token. This is intended to be
-/// used on the OIDC authentication handler's backchannel http client.
+/// Delegating handler that injects the DPoP proof token into requests that are
+/// made to the authorization server. This is intended to be used on the OIDC
+/// authentication handler's backchannel http client.
 /// </summary>
-public class DPoPProofTokenHandler : DelegatingHandler
+internal class AuthorizationServerDPoPHandler : DelegatingHandler
 {
     private readonly IDPoPProofService _dPoPProofService;
     private readonly IDPoPNonceStore _dPoPNonceStore;
     private readonly IHttpContextAccessor _httpContextAccessor;
-    private readonly ILogger<DPoPProofTokenHandler> _logger;
+    private readonly ILogger<AuthorizationServerDPoPHandler> _logger;
 
-    internal DPoPProofTokenHandler(
+    internal AuthorizationServerDPoPHandler(
         IDPoPProofService dPoPProofService,
         IDPoPNonceStore dPoPNonceStore,
         IHttpContextAccessor httpContextAccessor,
-        ILogger<DPoPProofTokenHandler> logger)
+        ILoggerFactory loggerFactory)
     {
         _dPoPProofService = dPoPProofService;
         _dPoPNonceStore = dPoPNonceStore;
         _httpContextAccessor = httpContextAccessor;
-        _logger = logger;
+        // We depend on the logger factory, rather than the logger itself, since
+        // the type parameter of the logger (referencing this class) will not
+        // always be accessible.
+        _logger = loggerFactory.CreateLogger<AuthorizationServerDPoPHandler>();
     }
 
     /// <inheritdoc/>
@@ -39,32 +44,38 @@ protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage
         await SetDPoPProofTokenAsync(request, cancellationToken).ConfigureAwait(false);
         var response = await base.SendAsync(request, cancellationToken).ConfigureAwait(false);
 
-        // The authorization server might send us a new nonce in a successful
-        // request, indicating that we should use the new nonce in future.
+        // The authorization server might send us a new nonce on either a success or failure
         var dPoPNonce = response.GetDPoPNonce();
 
         if (dPoPNonce != null)
         {
-            _logger.LogDebug("The authorization server has supplied a new nonce");
-
-            await _dPoPNonceStore.StoreNonceAsync(new DPoPNonceContext
-            {
-                Url = request.GetDPoPUrl(),
-                Method = request.Method.ToString(),
-            }, dPoPNonce);
-
-            // But the authorization server might also send a failure response, and expect us to retry 
-            if (response.StatusCode == System.Net.HttpStatusCode.BadRequest)
+            // If the response code is a bad request, we can infer that we
+            // should retry with the new nonce. The server should have also set
+            // the error: use_dpop_nonce, but there's no need to incur the cost
+            // of parsing the json and checking for that, as we would only
+            // receive the nonce http header when that error was set.
+            // Authorization servers might preemptively send a new nonce, but
+            // the spec specifically says to do that on a success (and we handle
+            // that case in the else block)
+            //
+            // TL;DR - presence of nonce and 400 response code is enough to
+            // trigger a retry
+            if (response.StatusCode == HttpStatusCode.BadRequest)
             {
-
-                // REVIEW: Is it good enough to check the status code and
-                // existence of the new nonce? Should we parse the response, and
-                // look for the "use_dpop_nonce" value in the error property?
-
                 _logger.LogDebug("Request failed (bad request). Retrying request with new DPoP proof token that includes the new nonce");
                 response.Dispose();
                 await SetDPoPProofTokenAsync(request, cancellationToken, dPoPNonce).ConfigureAwait(false);
                 return await base.SendAsync(request, cancellationToken).ConfigureAwait(false);
+            } 
+            else if (response.StatusCode == HttpStatusCode.OK)
+            {
+                _logger.LogDebug("The authorization server has supplied a new nonce on a successful response, which will be stored and used in future requests to the authorization server");
+
+                await _dPoPNonceStore.StoreNonceAsync(new DPoPNonceContext
+                {
+                    Url = request.GetDPoPUrl(),
+                    Method = request.Method.ToString(),
+                }, dPoPNonce);
             }
         }