umbraco / UmbracoIdentityExtensions

Code files & installation that enables easy extensibility points for ASP.Net Identity and the Umbraco back office
MIT License
38 stars 23 forks source link

Release/2.0 #53

Closed RyuLindow closed 1 year ago

RyuLindow commented 1 year ago

This is not exactly a security issue, as not all external identity providers support client secrets so that property was not made optional which means that you as a developer can decide for yourself whether you want to use one.

merijng commented 1 year ago

@RyuLindow @bergmania I've just tested this.

I believe this issue is not fixed yet.

You've made it possible to add a client secret through the ConfigureBackOfficeAzureActiveDirectoryAuth extension. However, I can still fill in any client secret that I want. It is not getting checked.

I don't know how Umbraco is handling this under water, but the UseOpenIdConnectAuthentication extension is part of package Microsoft.Owin.Security.OpenIdConnect. Maybe the issue relies there?

bergmania commented 1 year ago

Hi @merijng.

It is the actual provider that needs to verify the secret. The packages can't do that.

merijng commented 1 year ago

@bergmania That's true. However, Umbraco 8 is not doing anything with the token right now.

In Umbraco 8 I do see a .well-known/openid-configuration lookup, which is correct. After the /authorize (login) call, I would expect Umbraco to call the /token endpoint with the code send by the provider. It is not happening.

Below you can see my fiddler calls. Sorry for the blurring.

image

When you configure it in Umbraco 10, with an incorrect client secret, you'll get an unauthorized error on the token endpoint. This is correct :)

I am quite sure our provider is working correctly, because I tested the provider for both Umbraco 10 and 8.

bergmania commented 1 year ago

@merijng, I have no idea where that code is executed. Maybe you have an idea. If so feel free to make a PR. Im not sure if this is in Umbraco, in this package or in ASPNET.Identity

merijng commented 1 year ago

@bergmania @RyuLindow, I've added an extra pull request.

Owin can handle tokens, but I don't have the time to resolve the compatibility issue between Umbraco, Owin, and ASP.NET Identity. By using notifications, I've fixed the issue. Please read the considerations in my pull request.