Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BEEEP] [PM-2844] Add custom error codes for server API exceptions #70

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 29 additions & 12 deletions src/Api/Utilities/ExceptionHandlerFilterAttribute.cs
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
using Bit.Api.Models.Public.Response;
using System.Reflection;
using Bit.Api.Models.Public.Response;
using Bit.Core;
using Bit.Core.Exceptions;
using Bit.Core.Resources;
using Microsoft.AspNetCore.Mvc;
using Microsoft.AspNetCore.Mvc.Filters;
using Microsoft.Extensions.Localization;
using Microsoft.IdentityModel.Tokens;
using Stripe;
using InternalApi = Bit.Core.Models.Api;
Expand All @@ -19,7 +23,7 @@ public ExceptionHandlerFilterAttribute(bool publicApi)

public override void OnException(ExceptionContext context)
{
var errorMessage = "An error has occurred.";
var errorMessage = GetFormattedMessageFromErrorCode(context);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Consider adding a null check for context before calling GetFormattedMessageFromErrorCode


var exception = context.Exception;
if (exception == null)
Expand All @@ -44,10 +48,6 @@ public override void OnException(ExceptionContext context)
internalErrorModel = new InternalApi.ErrorResponseModel(badRequestException.ModelState);
}
}
else
{
errorMessage = badRequestException.Message;
}
}
else if (exception is StripeException stripeException && stripeException?.StripeError?.Type == "card_error")
{
Expand All @@ -65,12 +65,10 @@ public override void OnException(ExceptionContext context)
}
else if (exception is GatewayException)
{
errorMessage = exception.Message;
context.HttpContext.Response.StatusCode = 400;
}
else if (exception is NotSupportedException && !string.IsNullOrWhiteSpace(exception.Message))
{
errorMessage = exception.Message;
context.HttpContext.Response.StatusCode = 400;
}
else if (exception is ApplicationException)
Expand All @@ -79,17 +77,17 @@ public override void OnException(ExceptionContext context)
}
else if (exception is NotFoundException)
{
errorMessage = "Resource not found.";
errorMessage = GetFormattedMessageFromErrorCode(context, ErrorCodes.ResourceNotFound);
context.HttpContext.Response.StatusCode = 404;
}
else if (exception is SecurityTokenValidationException)
{
errorMessage = "Invalid token.";
errorMessage = GetFormattedMessageFromErrorCode(context, ErrorCodes.InvalidToken);
context.HttpContext.Response.StatusCode = 403;
}
else if (exception is UnauthorizedAccessException)
{
errorMessage = "Unauthorized.";
errorMessage = GetFormattedMessageFromErrorCode(context, ErrorCodes.Unauthorized);
context.HttpContext.Response.StatusCode = 401;
}
else if (exception is ConflictException)
Expand All @@ -114,7 +112,7 @@ public override void OnException(ExceptionContext context)
{
var logger = context.HttpContext.RequestServices.GetRequiredService<ILogger<ExceptionHandlerFilterAttribute>>();
logger.LogError(0, exception, exception.Message);
errorMessage = "An unhandled server error has occurred.";
errorMessage = GetFormattedMessageFromErrorCode(context, ErrorCodes.UnhandledError);
context.HttpContext.Response.StatusCode = 500;
}

Expand All @@ -136,4 +134,23 @@ public override void OnException(ExceptionContext context)
context.Result = new ObjectResult(errorModel);
}
}

private string GetFormattedMessageFromErrorCode(ExceptionContext context, string alternativeErrorCode = null)
{
var localizerFactory = context.HttpContext.RequestServices.GetRequiredService<IStringLocalizerFactory>();

var assemblyName = new AssemblyName(typeof(SharedResources).GetTypeInfo().Assembly.FullName);
var localizer = localizerFactory.Create("ErrorMessages", assemblyName.Name);

var errorCode = alternativeErrorCode ?? context.Exception.Message;
var errorMessage = localizer[errorCode];
Comment on lines +145 to +146
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Using Exception.Message as errorCode could lead to unexpected behavior. Consider using a default error code instead


// Get localized error message for the exception message
if (errorMessage.ResourceNotFound is false)
{
return $"({errorCode}) {localizer[errorCode]}";
}
Comment on lines +149 to +152
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: This logic might return '(ErrorMessage) ErrorMessage' for non-code errors. Consider refactoring to handle this case


return context.Exception.Message;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Returning Exception.Message as fallback may expose sensitive information. Consider using a generic error message

}
}
3 changes: 2 additions & 1 deletion src/Api/Vault/Controllers/SyncController.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using Bit.Api.Vault.Models.Response;
using Bit.Core;
using Bit.Core.Entities;
using Bit.Core.Enums;
using Bit.Core.Enums.Provider;
Expand Down Expand Up @@ -59,7 +60,7 @@ public async Task<SyncResponseModel> Get([FromQuery] bool excludeDomains = false
var user = await _userService.GetUserByPrincipalAsync(User);
if (user == null)
{
throw new BadRequestException("User not found.");
throw new BadRequestException(ErrorCodes.UserNotFound);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Consider using NotFoundException instead of BadRequestException for user not found scenario

}

var organizationUserDetails = await _organizationUserRepository.GetManyDetailsByUserAsync(user.Id,
Expand Down
13 changes: 13 additions & 0 deletions src/Core/Constants.cs
Original file line number Diff line number Diff line change
Expand Up @@ -46,3 +46,16 @@ public static List<string> GetAllKeys()
.ToList();
}
}

public static class ErrorCodes
{
public const string Error = "ERR000";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: ERR000 seems too generic. Consider using a more specific name like 'GenericError' or 'UnspecifiedError'

public const string OrganizationNotFound = "ERR001";
public const string OrganizationCannotUsePolicies = "ERR002";
public const string UserNotFound = "ERR003";
public const string ResourceNotFound = "ERR004";
public const string InvalidToken = "ERR005";
public const string Unauthorized = "ERR006";
public const string PolicyRequiredByTrustedDeviceEncryption = "ERR007";
public const string UnhandledError = "ERR500";
}
Comment on lines +50 to +61
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Consider adding a comment explaining the purpose of this class and how it should be used

13 changes: 12 additions & 1 deletion src/Core/Core.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@
<EmbeddedResource Include="licensing.cer" />
<EmbeddedResource Include="licensing_dev.cer" />
<EmbeddedResource Include="MailTemplates\Handlebars\**\*.hbs" />
<EmbeddedResource Update="Resources\ErrorMessages.en.resx">
<Generator>ResXFileCodeGenerator</Generator>
<LastGenOutput>ErrorMessages.en.Designer.cs</LastGenOutput>
</EmbeddedResource>
</ItemGroup>

<ItemGroup>
Expand Down Expand Up @@ -60,7 +64,14 @@
</ItemGroup>

<ItemGroup>
<Folder Include="Resources\" />
<Folder Include="Properties\" />
</ItemGroup>

<ItemGroup>
<Compile Update="Resources\ErrorMessages.en.Designer.cs">
<DesignTime>True</DesignTime>
<AutoGen>True</AutoGen>
<DependentUpon>ErrorMessages.en.resx</DependentUpon>
</Compile>
</ItemGroup>
</Project>
102 changes: 102 additions & 0 deletions src/Core/Resources/ErrorMessages.en.Designer.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

48 changes: 48 additions & 0 deletions src/Core/Resources/ErrorMessages.en.resx
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
<?xml version="1.0" encoding="utf-8"?>

<root>
<xsd:schema id="root" xmlns="" xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns:msdata="urn:schemas-microsoft-com:xml-msdata">
<xsd:element name="root" msdata:IsDataSet="true">

</xsd:element>
</xsd:schema>
<resheader name="resmimetype">
<value>text/microsoft-resx</value>
</resheader>
<resheader name="version">
<value>1.3</value>
</resheader>
<resheader name="reader">
<value>System.Resources.ResXResourceReader, System.Windows.Forms, Version=2.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>
</resheader>
<resheader name="writer">
<value>System.Resources.ResXResourceWriter, System.Windows.Forms, Version=2.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>
</resheader>
<data name="ERR001" xml:space="preserve">
<value>Organization not found.</value>
</data>
<data name="ERR002" xml:space="preserve">
<value>This organization cannot use policies.</value>
</data>
<data name="ERR003" xml:space="preserve">
<value>User not found.</value>
</data>
<data name="ERR000" xml:space="preserve">
<value>An error has occurred.</value>
</data>
<data name="ERR004" xml:space="preserve">
<value>Resource not found.</value>
</data>
<data name="ERR005" xml:space="preserve">
<value>Invalid token.</value>
</data>
<data name="ERR006" xml:space="preserve">
<value>Unauthorized.</value>
</data>
<data name="ERR500" xml:space="preserve">
<value>An unhandled server error has occurred.</value>
</data>
<data name="ERR007" xml:space="preserve">
<value>Trusted device encryption is on and requires this policy.</value>
</data>
</root>
4 changes: 2 additions & 2 deletions src/Core/Services/Implementations/PolicyService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ public async Task SaveAsync(Policy policy, IUserService userService, IOrganizati

if (!org.UsePolicies)
{
throw new BadRequestException("This organization cannot use policies.");
throw new BadRequestException(ErrorCodes.OrganizationCannotUsePolicies);
}

// Handle dependent policy checks
Expand Down Expand Up @@ -281,7 +281,7 @@ private async Task RequiredBySsoTrustedDeviceEncryptionAsync(Organization org)
var ssoConfig = await _ssoConfigRepository.GetByOrganizationIdAsync(org.Id);
if (ssoConfig?.GetData()?.MemberDecryptionType == MemberDecryptionType.TrustedDeviceEncryption)
{
throw new BadRequestException("Trusted device encryption is on and requires this policy.");
throw new BadRequestException(ErrorCodes.PolicyRequiredByTrustedDeviceEncryption);
}
}
}