dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.2k stars 9.93k forks source link

Denying authorization access to a Blazor webassembly client #28344

Open jayrulez opened 3 years ago

jayrulez commented 3 years ago

Describe the bug

I am using Openiddict as an OpenId Connect server with a blazor webassembly client.

If I attempt to access a protected route in the client, it redirects me to the oidc server (based on Openiddict) for auth*n. If I provide access to the blazor client then it works as expected.

However, If I deny access to the blazor client then I believe the RemoteAuthenticatorViewCore is behaving incorrectly. The expected behavior is that the client is redirected to the login failed callback route where the error message returned by the oidc server (in this case: "The authorization was denied by the end user.") is displayed to the user.

However, the client stays on this view:

image

I think the issue is in this method: https://github.com/dotnet/aspnetcore/blob/648c15dbe90fb8c113f7c6b4adeb40d9e10494f6/src/Components/WebAssembly/WebAssembly.Authentication/src/RemoteAuthenticatorViewCore.cs#L245

I'm not having a good time with debugging a blazor webassembly client so I cannot confirm this but I think this method is hitting one of the cases that throws an exception or the empty RemoteAuthenticationStatus.OperationCompleted case.

The login callback preview shows this: image

So I am leaning to the former.

To Reproduce

You can reproduce the issue by running this sample project here: https://github.com/openiddict/openiddict-samples/tree/dev/samples/Balosar

It doesn't require any setup so it should take just a few minutes.

Just click the "Fetch Data" component link. It will redirect you to the auth server for. you can then create an account and login. (I suggest creating an account beforehand or disabling email requirement for sign in as it breaks the flow by default.). Anyway, once you have an account you can attempt to authorize the client. When it prompts for consent, deny the client and you will be returned to the view in the first screenshot.

I first contacted @kevinchalet about this issue. He says as I expected that it is not an issue with Openiddict.

Further technical details

$ dotnet --info .NET SDK (reflecting any global.json): Version: 5.0.100 Commit: 5044b93829

Runtime Environment: OS Name: Windows OS Version: 10.0.19042 OS Platform: Windows RID: win10-x64 Base Path: C:\Program Files\dotnet\sdk\5.0.100\

kevinchalet commented 3 years ago

I took a brief look and...

If I had to guess what's happening, I'd say that's because the Blazor WASM authentication stack doesn't properly extract the authorization error response from the URI fragment.

jayrulez commented 3 years ago

The issue seems to be exactly what @kevinchalet suggested.

https://github.com/dotnet/aspnetcore/blob/648c15dbe90fb8c113f7c6b4adeb40d9e10494f6/src/Components/WebAssembly/WebAssembly.Authentication/src/Interop/AuthenticationService.ts#L236

State extraction only considers query params.

kevinchalet commented 3 years ago

Surprisingly, non-errored authorization responses returned using the fragment response mode are correctly parsed and things work just fine (yet, these responses also include the same state parameter). Not sure why it doesn't work with errored responses πŸ˜•

javiercn commented 3 years ago

@jayrulez thanks for contacting us.

Thanks everyone for digging into this. It seems like there is a bug here. If you want to send a PR for it, we'll be happy to accept it.

kevinchalet commented 3 years ago

@javiercn I wish I could help, but with the incoming release of OpenIddict 3.0 (expected to ship later this month), I sadly don't have much free time ATM πŸ˜…

@jayrulez wanna give it a try? πŸ˜ƒ

javiercn commented 3 years ago

@kevinchalet no problem.

We'll see if someone wants to pick it up and otherwise we'll consider doing the fix ourselves.

josercadena commented 3 years ago

@javiercn Hey πŸ‘‹ i took a quick look into this. I gave prevalence to the state value coming from a fragment (you know, in case there was a state param as well)... would that make any sense at all? πŸ˜…

javiercn commented 3 years ago

@josercadena thanks for the contribution.

We'll take a look and comment on the PR.

ghost commented 3 years ago

Thanks for contacting us. We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We will evaluate the request when we are planning the work for the next milestone. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

ghost commented 3 years ago

Hello,

any news on this. It seems the PR was closed without a merge. I think this is a major issue and needs a fix soon.

regards Kai

ghost commented 3 years ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

JeanFTP commented 2 years ago

Hi, Any update on this issue? Why has it not been fixed yet ? Thanks

Krangth commented 1 year ago

https://github.com/dotnet/aspnetcore/pull/28454

Grizzlly commented 1 year ago

Hello! Any news on this one?

ghost commented 9 months ago

Thanks for contacting us.

We're moving this issue to the .NET 9 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

Lepollo3000 commented 7 months ago

I think this should be one of their priorities.