diff --git a/src/Identity/IdentityServer/RequestValidators/SendAccess/SendAccessConstants.cs b/src/Identity/IdentityServer/RequestValidators/SendAccess/SendAccessConstants.cs index f38a4a880f1d..3ed76a93d05c 100644 --- a/src/Identity/IdentityServer/RequestValidators/SendAccess/SendAccessConstants.cs +++ b/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. /// public const string EmailAndOtpRequired = "email_and_otp_required"; - /// - /// Represents the status indicating that both email and OTP are required, and the OTP is invalid. - /// - public const string EmailOtpInvalid = "otp_invalid"; - /// - /// For what ever reason the OTP was not able to be generated - /// - public const string OtpGenerationFailed = "otp_generation_failed"; } /// diff --git a/src/Identity/IdentityServer/RequestValidators/SendAccess/SendEmailOtpRequestValidator.cs b/src/Identity/IdentityServer/RequestValidators/SendAccess/SendEmailOtpRequestValidator.cs index 02442d8c7e0b..90c41193f7c6 100644 --- a/src/Identity/IdentityServer/RequestValidators/SendAccess/SendEmailOtpRequestValidator.cs +++ b/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 logger, IOtpTokenProvider otpTokenProvider, IMailService mailService) : ISendAuthenticationMethodValidator { @@ -20,8 +26,7 @@ public class SendEmailOtpRequestValidator( private static readonly Dictionary _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 ValidateRequestAsync(ExtensionGrantValidationContext context, EmailOtp authMethod, Guid sendId) @@ -37,21 +42,14 @@ public async Task 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 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) + /// + /// Build the error response for the SendEmailOtpRequestValidator. + /// + /// The error code to use for the validation result. This is defaulted to EmailAndOtpRequired if not specified because it is the most common response. + /// A GrantValidationResult representing the error. + 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 - { - { SendAccessConstants.SendAccessError, error } - }); default: return new GrantValidationResult( TokenRequestErrors.InvalidRequest, diff --git a/test/Identity.IntegrationTest/RequestValidation/SendAccess/SendEmailOtpReqestValidatorIntegrationTests.cs b/test/Identity.IntegrationTest/RequestValidation/SendAccess/SendEmailOtpReqestValidatorIntegrationTests.cs index 9250859de119..63d8829bc341 100644 --- a/test/Identity.IntegrationTest/RequestValidation/SendAccess/SendEmailOtpReqestValidatorIntegrationTests.cs +++ b/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] diff --git a/test/Identity.Test/IdentityServer/SendAccess/SendConstantsSnapshotTests.cs b/test/Identity.Test/IdentityServer/SendAccess/SendConstantsSnapshotTests.cs index 76a40410c9e8..41458f22dc9f 100644 --- a/test/Identity.Test/IdentityServer/SendAccess/SendConstantsSnapshotTests.cs +++ b/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] diff --git a/test/Identity.Test/IdentityServer/SendAccess/SendEmailOtpRequestValidatorTests.cs b/test/Identity.Test/IdentityServer/SendAccess/SendEmailOtpRequestValidatorTests.cs index 5001ac88da31..7b5f1fc45132 100644 --- a/test/Identity.Test/IdentityServer/SendAccess/SendEmailOtpRequestValidatorTests.cs +++ b/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>() .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>() .ValidateTokenAsync( @@ -216,7 +217,7 @@ await sutProvider.GetDependency() } [Theory, BitAutoData] - public async Task ValidateRequestAsync_InvalidOtp_ReturnsInvalidGrant( + public async Task ValidateRequestAsync_InvalidOtp_ReturnsInvalidRequest( SutProvider 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>() .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>() @@ -265,8 +266,9 @@ public void Constructor_WithValidParameters_CreatesInstance() // Arrange var otpTokenProvider = Substitute.For>(); var mailService = Substitute.For(); + var logger = Substitute.For>(); // Act - var validator = new SendEmailOtpRequestValidator(otpTokenProvider, mailService); + var validator = new SendEmailOtpRequestValidator(logger, otpTokenProvider, mailService); // Assert Assert.NotNull(validator);