search

bruno-garcia / Bazinga.AspNetCore.Authentication.Basic

Basic Authentication for Microsoft ASP.NET Core Authentication
MIT License
48 stars 13 forks source link

Allow for more complex Principal/Identity construction. #2

Closed marcrocny closed 6 years ago

marcrocny commented 6 years ago

The extension only allows for a bool response and automatically fills in a contextual ClaimsPrincipal/CliamsIdentity with just the username. There's no way (that I can see) to provide an extension to the claims included there without fully overriding BasicAuthenticationHandler.HandleAuthenticateAsync() which kind of obviates the purpose of the class.

Note also, for what it's worth: while I like (maybe prefer) the "direct injection" method used here, the more "aspnetcore-correct" way of doing this would appear to be adding the injected authentication method to an "*AuthenticationEvents" object that is configured either directly on AuthenticationSchemeOptions.Events or indirectly by setting the type on AuthenticationSchemeOptions.EventsType. The latter uses dependency injection to create the events-object anyway, but there you are.

bruno-garcia commented 6 years ago

Hi @marcrocny, thanks for your feedback! Looking at the Microsoft implementation for Twitter, for example, I don't see them invoking anything on the Events giving a chance to the application to add more claims to the ClaimsIdentity. It sets the 4 predefined claim types with values returning from 'ObtainAccessTokenAsync' call but that's it. Perhaps I'm missing something?

I wasn't aware of the EventsType hook when I wrote this. Perhaps it could use this API instead to be closer to the other Microsoft written handlers. What would you suggest?

bruno-garcia commented 6 years ago

I took a look at the code and it does calls into Events with the newly created ClaimsPrincipal.

That's the hook for the application to modify it or even reject the user by calling AuthenticationSucceededContext.Fail during CredentialsValidated

bruno-garcia commented 6 years ago

@marcrocny I'll close the issue for now. Feel free to reopen if my last comment doesn't solve your problem.

springy76 commented 6 years ago

I also needed to persist some additional claims which I get during DB lookup, so I slightly modified the code: my commit

bruno-garcia commented 6 years ago

I'd be happy to merge a pull request with your changes.

Although I'm not a big fan of passing a mutating collection like that, I see it solves the problem rather simply. And changing to code now to abandon the original idea of the generic argument for registration in favor of the EventsType approach is not really worth it as it'll break everyone using this for little value.