Skip to content
This repository has been archived by the owner on Nov 19, 2024. It is now read-only.

Scope property mapping in GetOpenIdConnectConfigurationAsync #45

Closed
wants to merge 1 commit into from

Conversation

hybrid2102
Copy link

What issue does this PR address?

When calling GetOpenIdConnectConfigurationAsync the resulting object doesn't contain the Scope property.
I added the necessary mapping, then used the new property in the HTTP request for RefreshAccessTokenAsync.

In relation with #43

Important: Any code or remarks in your Pull Request are under the following terms:

If You provide us with any comments, bug reports, feedback, enhancements, or modifications proposed or suggested by You for the Software, such Feedback is provided on a non-confidential basis (notwithstanding any notice to the contrary You may include in any accompanying communication), and Licensor shall have the right to use such Feedback at its discretion, including, but not limited to the incorporation of such suggested changes into the Software. You hereby grant Licensor a perpetual, irrevocable, transferable, sublicensable, nonexclusive license under all rights necessary to incorporate and use your Feedback for any purpose, including to make and sell any products and services.

(see our license, section 7)

@brockallen
Copy link
Member

Hmm, this is now confusing... Am I correct in my observation that we have Scope on the normal ASP.NET Core's OIDC config, and then again here as a seemingly redundant setting, except that it's not and only used on refresh token renewals?

@hybrid2102
Copy link
Author

Hmm, this is now confusing... Am I correct in my observation that we have Scope on the normal ASP.NET Core's OIDC config, and then again here as a seemingly redundant setting, except that it's not and only used on refresh token renewals?

In my own encounters, I've found that requesting a fresh access token from ADFS using a refresh token, without specifying any scopes in the request, results in a scope-less token that doesn't align with my specific scenario's requirements.

@brockallen
Copy link
Member

What I find odd is that the scope is normally presented in the original authorize request, but then on the token renewal request you are expected to re-request the same scopes, right? I can see that if you wanted to down-scope the access token, but is no scope is presented I'd expect the same scopes as the original grant to be issued. Anyway, that's just a side comment on ADFS.

As for this PR and code change -- can what you need/want be achieved by getting the scopes from the original OIDC options? That's what I'm looking for, so we don't introduce a new setting that is not obvious what it does and how it differs from the other OIDC options scope collection.

@hybrid2102
Copy link
Author

What I find odd is that the scope is normally presented in the original authorize request, but then on the token renewal request you are expected to re-request the same scopes, right? I can see that if you wanted to down-scope the access token, but is no scope is presented I'd expect the same scopes as the original grant to be issued. Anyway, that's just a side comment on ADFS.

I agree with you, unfortunately our ADFS installation is a black-box to me.

As for this PR and code change -- can what you need/want be achieved by getting the scopes from the original OIDC options? That's what I'm looking for, so we don't introduce a new setting that is not obvious what it does and how it differs from the other OIDC options scope collection.

This is exactly what I wanted to achieve with the code submitted in this PR, maybe I missed something?

@josephdecock
Copy link
Member

The downside to this is that it adds a scope property on the main client configuration. This is potentially confusing, because most identity providers don't need this parameter and so users don't generally need to set it, and because it has a very specific use case but a very general name. Is there a way to get at the OIDC handler's config at the point of refresh, rather than storing it in the config object?

@brockallen
Copy link
Member

It looks like you have a workaround already possible: You can dervice from OpenIdConnectConfigurationService, and reimplment IOpenIdConnectConfigurationService. Call the base, and then add your additional scopes to the result. I think this should be possible, yes?

@hybrid2102
Copy link
Author

It looks like you have a workaround already possible: You can dervice from OpenIdConnectConfigurationService, and reimplment IOpenIdConnectConfigurationService. Call the base, and then add your additional scopes to the result. I think this should be possible, yes?

“Thank you for the suggestion—I’ll give it a try when I have some free time!” 🙌

@josephdecock
Copy link
Member

Looks like we have a solution so I'll close this, but feel free to reopen or follow up if necessary!

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

Successfully merging this pull request may close these issues.

3 participants