[PM-32424] Send Access Enumeration protection

src/Identity/IdentityServer/RequestValidators/SendAccess/SendAccessConstants.cs

@@ -72,14 +72,6 @@ public static class EmailOtpValidatorResults
         /// Represents the status indicating that both email and OTP are required, and the OTP has been sent.
         /// </summary>
         public const string EmailAndOtpRequired = "email_and_otp_required";
-        /// <summary>
-        /// Represents the status indicating that both email and OTP are required, and the OTP is invalid.
-        /// </summary>
-        public const string EmailOtpInvalid = "otp_invalid";
-        /// <summary>
-        /// For what ever reason the OTP was not able to be generated
-        /// </summary>
-        public const string OtpGenerationFailed = "otp_generation_failed";
     }
 
     /// <summary>

src/Identity/IdentityServer/RequestValidators/SendAccess/SendEmailOtpRequestValidator.cs

@@ -1,4 +1,5 @@
-using System.Security.Claims;
+using System.Globalization;
+using System.Security.Claims;
 using Bit.Core.Auth.Identity;
 using Bit.Core.Auth.Identity.TokenProviders;
 using Bit.Core.Services;
@@ -9,7 +10,12 @@
 
 namespace Bit.Identity.IdentityServer.RequestValidators.SendAccess;
 
+/**
+* The error responses here do not fully match the standard for OAuth with respect to Invalid Request vs Invalid Grant. This is intended to better protect
+* against enumeration. We return Invalid Request for all errors related to the email and OTP, even if in some cases Invalid Grant might be more appropriate.
+*/
 public class SendEmailOtpRequestValidator(
+    ILogger<SendEmailOtpRequestValidator> logger,
     IOtpTokenProvider<DefaultOtpTokenProviderOptions> otpTokenProvider,
     IMailService mailService) : ISendAuthenticationMethodValidator<EmailOtp>
 {
@@ -20,8 +26,7 @@ public class SendEmailOtpRequestValidator(
     private static readonly Dictionary<string, string> _sendEmailOtpValidatorErrorDescriptions = new()
     {
         { SendAccessConstants.EmailOtpValidatorResults.EmailRequired, $"{SendAccessConstants.TokenRequest.Email} is required." },
-        { SendAccessConstants.EmailOtpValidatorResults.EmailAndOtpRequired, $"{SendAccessConstants.TokenRequest.Email} and {SendAccessConstants.TokenRequest.Otp} are required." },
-        { SendAccessConstants.EmailOtpValidatorResults.EmailOtpInvalid, $"{SendAccessConstants.TokenRequest.Email} otp is invalid." },
+        { SendAccessConstants.EmailOtpValidatorResults.EmailAndOtpRequired, $"{SendAccessConstants.TokenRequest.Email} and {SendAccessConstants.TokenRequest.Otp} are required." }
     };
 
     public async Task<GrantValidationResult> ValidateRequestAsync(ExtensionGrantValidationContext context, EmailOtp authMethod, Guid sendId)
@@ -37,21 +42,14 @@ public async Task<GrantValidationResult> ValidateRequestAsync(ExtensionGrantVali
             return BuildErrorResult(SendAccessConstants.EmailOtpValidatorResults.EmailRequired);
         }
 
-        /*
-         * This is somewhat contradictory to our process where a poor shape means invalid_request and invalid
-         * data is invalid_grant.
-         * In this case the shape is correct and the data is invalid but to protect against enumeration we treat incorrect emails
-         * as invalid requests. The response for a request with a correct email which needs an OTP and a request
-         * that has an invalid email need to be the same otherwise an attacker could enumerate until a valid email is found.
-         */
         if (!authMethod.emails.Contains(email, StringComparer.OrdinalIgnoreCase))
         {
-            return BuildErrorResult(SendAccessConstants.EmailOtpValidatorResults.EmailAndOtpRequired);
+            return BuildErrorResult();
         }
 
         // get otp from request
         var requestOtp = request.Get(SendAccessConstants.TokenRequest.Otp);
-        var uniqueIdentifierForTokenCache = string.Format(SendAccessConstants.OtpToken.TokenUniqueIdentifier, sendId, email);
+        var uniqueIdentifierForTokenCache = string.Format(CultureInfo.InvariantCulture, SendAccessConstants.OtpToken.TokenUniqueIdentifier, sendId, email);
         if (string.IsNullOrEmpty(requestOtp))
         {
             // Since the request doesn't have an OTP, generate one
@@ -63,15 +61,16 @@ public async Task<GrantValidationResult> ValidateRequestAsync(ExtensionGrantVali
             // Verify that the OTP is generated
             if (string.IsNullOrEmpty(token))
             {
-                return BuildErrorResult(SendAccessConstants.EmailOtpValidatorResults.OtpGenerationFailed);
+                logger.LogError("Failed to generate OTP for SendAccess");
+                return BuildErrorResult();
             }
 
             await mailService.SendSendEmailOtpEmailAsync(
                 email,
                 token,
-                string.Format(SendAccessConstants.OtpEmail.Subject, token));
+                string.Format(CultureInfo.CurrentCulture, SendAccessConstants.OtpEmail.Subject, token));
 
-            return BuildErrorResult(SendAccessConstants.EmailOtpValidatorResults.EmailAndOtpRequired);
+            return BuildErrorResult();
         }
 
         // validate request otp
@@ -84,13 +83,18 @@ await mailService.SendSendEmailOtpEmailAsync(
         // If OTP is invalid return error result
         if (!otpResult)
         {
-            return BuildErrorResult(SendAccessConstants.EmailOtpValidatorResults.EmailOtpInvalid);
+            return BuildErrorResult();
         }
 
         return BuildSuccessResult(sendId, email!);
     }
 
-    private static GrantValidationResult BuildErrorResult(string error)
+    /// <summary>
+    /// Build the error response for the SendEmailOtpRequestValidator.
+    /// </summary>
+    /// <param name="error">The error code to use for the validation result. This is defaulted to EmailAndOtpRequired if not specified because it is the most common response.</param>
+    /// <returns>A GrantValidationResult representing the error.</returns>
+    private static GrantValidationResult BuildErrorResult(string error = SendAccessConstants.EmailOtpValidatorResults.EmailAndOtpRequired)
     {
         switch (error)
         {
@@ -102,14 +106,6 @@ private static GrantValidationResult BuildErrorResult(string error)
                     {
                         { SendAccessConstants.SendAccessError, error }
                     });
-            case SendAccessConstants.EmailOtpValidatorResults.EmailOtpInvalid:
-                return new GrantValidationResult(
-                    TokenRequestErrors.InvalidGrant,
-                    errorDescription: _sendEmailOtpValidatorErrorDescriptions[error],
-                    new Dictionary<string, object>
-                    {
-                        { SendAccessConstants.SendAccessError, error }
-                    });
             default:
                 return new GrantValidationResult(
                     TokenRequestErrors.InvalidRequest,

test/Identity.IntegrationTest/RequestValidation/SendAccess/SendEmailOtpReqestValidatorIntegrationTests.cs

@@ -2,6 +2,7 @@
 using Bit.Core.Services;
 using Bit.Core.Tools.Models.Data;
 using Bit.Core.Tools.SendFeatures.Queries.Interfaces;
+using Bit.Identity.IdentityServer.RequestValidators.SendAccess;
 using Bit.IntegrationTestCommon.Factories;
 using Duende.IdentityModel;
 using NSubstitute;
@@ -132,7 +133,7 @@ public async Task SendAccess_EmailOtpProtectedSend_ValidOtp_ReturnsAccessToken()
     }
 
     [Fact]
-    public async Task SendAccess_EmailOtpProtectedSend_InvalidOtp_ReturnsInvalidGrant()
+    public async Task SendAccess_EmailOtpProtectedSend_InvalidOtp_ReturnsInvalidRequest()
     {
         // Arrange
         var sendId = Guid.NewGuid();
@@ -170,8 +171,8 @@ public async Task SendAccess_EmailOtpProtectedSend_InvalidOtp_ReturnsInvalidGran
 
         // Assert
         var content = await response.Content.ReadAsStringAsync();
-        Assert.Contains(OidcConstants.TokenErrors.InvalidGrant, content);
-        Assert.Contains("email otp is invalid", content);
+        Assert.Contains(OidcConstants.TokenErrors.InvalidRequest, content);
+        Assert.Contains($"{SendAccessConstants.TokenRequest.Email} and {SendAccessConstants.TokenRequest.Otp} are required.", content);
     }
 
     [Fact]

test/Identity.Test/IdentityServer/SendAccess/SendConstantsSnapshotTests.cs

@@ -50,8 +50,6 @@ public void EmailOtpValidatorResults_Constants_HaveCorrectValues()
         // Assert
         Assert.Equal("email_required", SendAccessConstants.EmailOtpValidatorResults.EmailRequired);
         Assert.Equal("email_and_otp_required", SendAccessConstants.EmailOtpValidatorResults.EmailAndOtpRequired);
-        Assert.Equal("otp_invalid", SendAccessConstants.EmailOtpValidatorResults.EmailOtpInvalid);
-        Assert.Equal("otp_generation_failed", SendAccessConstants.EmailOtpValidatorResults.OtpGenerationFailed);
     }
 
     [Fact]

test/Identity.Test/IdentityServer/SendAccess/SendEmailOtpRequestValidatorTests.cs

@@ -1,4 +1,5 @@
-using Bit.Core.Auth.Identity;
+using System.Globalization;
+using Bit.Core.Auth.Identity;
 using Bit.Core.Auth.Identity.TokenProviders;
 using Bit.Core.Services;
 using Bit.Core.Tools.Models.Data;
@@ -96,7 +97,7 @@ public async Task ValidateRequestAsync_EmailWithoutOtp_GeneratesAndSendsOtp(
             Request = tokenRequest
         };
 
-        var expectedUniqueId = string.Format(SendAccessConstants.OtpToken.TokenUniqueIdentifier, sendId, email);
+        var expectedUniqueId = string.Format(CultureInfo.InvariantCulture, SendAccessConstants.OtpToken.TokenUniqueIdentifier, sendId, email);
 
         sutProvider.GetDependency<IOtpTokenProvider<DefaultOtpTokenProviderOptions>>()
             .GenerateTokenAsync(
@@ -181,7 +182,7 @@ public async Task ValidateRequestAsync_ValidOtp_ReturnsSuccess(
 
         emailOtp = emailOtp with { emails = [email] };
 
-        var expectedUniqueId = string.Format(SendAccessConstants.OtpToken.TokenUniqueIdentifier, sendId, email);
+        var expectedUniqueId = string.Format(CultureInfo.InvariantCulture, SendAccessConstants.OtpToken.TokenUniqueIdentifier, sendId, email);
 
         sutProvider.GetDependency<IOtpTokenProvider<DefaultOtpTokenProviderOptions>>()
             .ValidateTokenAsync(
@@ -216,7 +217,7 @@ await sutProvider.GetDependency<IMailService>()
     }
 
     [Theory, BitAutoData]
-    public async Task ValidateRequestAsync_InvalidOtp_ReturnsInvalidGrant(
+    public async Task ValidateRequestAsync_InvalidOtp_ReturnsInvalidRequest(
         SutProvider<SendEmailOtpRequestValidator> sutProvider,
         [AutoFixture.ValidatedTokenRequest] ValidatedTokenRequest tokenRequest,
         EmailOtp emailOtp,
@@ -233,7 +234,7 @@ public async Task ValidateRequestAsync_InvalidOtp_ReturnsInvalidGrant(
 
         emailOtp = emailOtp with { emails = [email] };
 
-        var expectedUniqueId = string.Format(SendAccessConstants.OtpToken.TokenUniqueIdentifier, sendId, email);
+        var expectedUniqueId = string.Format(CultureInfo.InvariantCulture, SendAccessConstants.OtpToken.TokenUniqueIdentifier, sendId, email);
 
         sutProvider.GetDependency<IOtpTokenProvider<DefaultOtpTokenProviderOptions>>()
             .ValidateTokenAsync(invalidOtp,
@@ -247,8 +248,8 @@ public async Task ValidateRequestAsync_InvalidOtp_ReturnsInvalidGrant(
 
         // Assert
         Assert.True(result.IsError);
-        Assert.Equal(OidcConstants.TokenErrors.InvalidGrant, result.Error);
-        Assert.Equal("email otp is invalid.", result.ErrorDescription);
+        Assert.Equal(OidcConstants.TokenErrors.InvalidRequest, result.Error);
+        Assert.Equal($"{SendAccessConstants.TokenRequest.Email} and {SendAccessConstants.TokenRequest.Otp} are required.", result.ErrorDescription);
 
         // Verify OTP validation was attempted
         await sutProvider.GetDependency<IOtpTokenProvider<DefaultOtpTokenProviderOptions>>()
@@ -265,8 +266,9 @@ public void Constructor_WithValidParameters_CreatesInstance()
         // Arrange
         var otpTokenProvider = Substitute.For<IOtpTokenProvider<DefaultOtpTokenProviderOptions>>();
         var mailService = Substitute.For<IMailService>();
+        var logger = Substitute.For<Microsoft.Extensions.Logging.ILogger<SendEmailOtpRequestValidator>>();
         // Act
-        var validator = new SendEmailOtpRequestValidator(otpTokenProvider, mailService);
+        var validator = new SendEmailOtpRequestValidator(logger, otpTokenProvider, mailService);
 
         // Assert
         Assert.NotNull(validator);