Skip to content

Release/2.0 #53

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

Merged
merged 4 commits into from
Jun 9, 2023
Merged

Release/2.0 #53

merged 4 commits into from
Jun 9, 2023

Conversation

RyuLindow
Copy link
Contributor

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.

@RyuLindow RyuLindow merged commit 25f46ee into master Jun 9, 2023
@merijng
Copy link

merijng commented Jul 3, 2023

@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
Copy link
Member

Hi @merijng.

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

@merijng
Copy link

merijng commented Jul 3, 2023

@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
Copy link
Member

@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
Copy link

merijng commented Jul 6, 2023

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants