aspnetcore: Avoid CA2201 (Generic Exception) Violations

Is there an existing issue for this?

I have searched the existing issues

Is your feature request related to a problem? Please describe the problem.

#46212 was closed by a comment describing ways to override how RemoteAuthenticationHandler handles remote failure, but this doesn’t help from an observability/telemetry standpoint. I imagine many teams consider any thrown System.Exception to be a critical error. We could work around it for RemoteAuthenticationHandler, but it would be better if the platform avoided new Exception().

Describe the solution you’d like

Resolve all CA2201 violations, or at least those related to System.Exception, in non-test/benchmark code by throwing a more specific exception.

Additional context

This feels like it could be up-for-grabs if there’s project guidance on scoping custom exceptions.

About this issue

Original URL
State: closed
Created a year ago
Comments: 15 (15 by maintainers)

Most upvoted comments

Don’t re-use it, that’s already an exception that could happen in this code path, such as from providers that make external requests to retrieve or verify tokens (OAuth, OIDC). Getting this exception type for a different reason would be confusing. While I like re-using exceptions, this one is too specific to System.Security.Authentication components. I think we’re better off with our own uniquely named exception here.

Tratcher on Apr 28, 2023

API Review Notes:

We like keeping naming it something other than AuthenticationException to avoid confusion
- The proposal of AuthenticationFailureException sounds good to us.
We also like leaving it unsealed in case people want to customize it.
Considering where it’s used, the Microsoft.AspNetCore.Authentication.Abstractions project makes sense.
And the Exception base type also seems reasonable.

// Microsoft.AspNetCore.Authentication.Abstractions.dll
namespace Microsoft.AspNetCore.Authentication;

public class AuthenticationFailureException : Exception
{
    public AuthenticationFailureException(string? message);

    public AuthenticationFailureException(string? message, Exception? innerException);
}

API Approved as proposed!

halter73 on May 2, 2023

@halter73 please note https://github.com/dotnet/aspnetcore/issues/47708#issuecomment-1522276638 has been updated slightly to align with current implementation.

dahlbyk on Apr 30, 2023

Agreed it is almost surely acceptable to change the NRE as it’s so unlikely anyone is catching it. We have replaced those in numerous public API lower down the stack without incident.

WRT how I generated the list - I added the code to the root editorconfig (where it should go long term) and ran eng\build.cmd. Perhaps that doesn’t build everything.

danmoseley on Apr 25, 2023