Aspnetcore: IAuthenticationSchemeProvider needs a TryAddScheme()

Created on 9 Oct 2017 · 11Comments · Source: dotnet/aspnetcore

Looking at this example/test to dynamically add authentication schemes at runtime:

https://github.com/aspnet/Security/blob/0959c941b40086b131b1e01d304fc23bb887fdc8/test/Microsoft.AspNetCore.Authentication.Test/DynamicSchemeTests.cs#L137-L143

If the endpoint is invoked twice, it will result in an InvalidOperationException:
https://github.com/aspnet/HttpAbstractions/blob/dev/src/Microsoft.AspNetCore.Authentication.Core/AuthenticationSchemeProvider.cs#L121

Instead perhaps this method should be bool TryAddScheme(AuthenticationScheme scheme) so users don't have to wrap it in a try-catch.

Also it appears that the underlying _map should be a ConcurrentDictionary?

Done area-security enhancement

Source

damianh

All 11 comments

While we're looking at this API why is AddScheme and RemoveScheme synchronous but GetSchemeAsync asynchronous?

damianh on 9 Oct 2017

This is a good suggestion but the best way to fix it would be a breaking change, so we will consider this in the 3.0 milestone. For now we recommend using a workaround, such as trying to catch the exception.

Eilon on 12 Oct 2017

👍1

Closing because it seems that implementors of the current interface could add their own interface with this new method, down-cast, and call it where needed.

Eilon on 13 Dec 2018

😕1 👎1

Re-opening based on offline discussion:

Extra small. It would be one new overload of this method that doesn’t throw:
https://github.com/aspnet/HttpAbstractions/blob/bc7092a32b1943c7f17439e419d3f66cd94ce9bd/src/Microsoft.AspNetCore.Authentication.Core/AuthenticationSchemeProvider.cs#L136-L156
https://github.com/aspnet/HttpAbstractions/blob/bc7092a32b1943c7f17439e419d3f66cd94ce9bd/src/Microsoft.AspNetCore.Authentication.Abstractions/IAuthenticationSchemeProvider.cs#L72

We might want to wait for C# 8 default interface implementations though (later in 3.0).