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);