Scope set to null when calling RefreshTokenAsync

mayhammf commented 1 year ago

Describe the problem

Scope is always set to null for refresh token requests RefreshTokenAsync and it is not possible to include it via the extraParameter arg

What was the expected behavior?

It is possible to set the scope for refresh token requests.

Reproduction

Call RefreshTokenAsync
Observe that no scopes were defined (depending on the setup your Auth0, the claims included in the Id token will vary)
Attempt to set the scope via the extraParameter arg

You get an exception:

System.InvalidOperationException: Duplicate parameter: scope
at IdentityModel.Client.Parameters.AddOptional (System.String key, System.String value, System.Boolean allowDuplicates) [0x0001f] in D:\a\IdentityModel\IdentityModel\src\Client\Messages\Parameters.cs:131 
at IdentityModel.Client.HttpClientTokenRequestExtensions.RequestRefreshTokenAsync (System.Net.Http.HttpMessageInvoker client, IdentityModel.Client.RefreshTokenRequest request, System.Threading.CancellationToken cancellationToken) [0x0004d] in D:\a\IdentityModel\IdentityModel\src\Client\Extensions\HttpClientTokenRequestExtensions.cs:117 
at IdentityModel.OidcClient.OidcClient.RefreshTokenAsync (System.String refreshToken, IdentityModel.Client.Parameters backChannelParameters, System.Threading.CancellationToken cancellationToken) [0x0014c] in /_/src/OidcClient/OidcClient.cs:331 
at Auth0.OidcClient.Auth0ClientBase.RefreshTokenAsync (System.String refreshToken, System.Object extraParameters, System.Threading.CancellationToken cancellationToken) [0x0009d] in <34f276a24a504fc099620fe62e28d814>:0

Environment

Android and iOS

Version of this library used: Android: 3.2.6. iOS: 3.3.6
IdentityModel.OidcClient subdependency resolved to 4.0.0 or 5.2.0

Cause

This happens because the Scope property in RefreshTokenRequest ends up being null when created by the IdentityModel.OidcClient.OidcClient.RefreshTokenAsync method (in version 4.0.0 there is no option to set the scope from outside, while in version 5.2.0, there is a scope arg that can be used)

See here for version 4.0.0: https://github.com/IdentityModel/IdentityModel.OidcClient/blob/4.0.0/src/OidcClient/OidcClient.cs#L319 See here for version 5.2.0: https://github.com/IdentityModel/IdentityModel.OidcClient/blob/5.2.0/src/OidcClient/OidcClient.cs#L320 Notice that the scope gets passed into the RefreshTokenRequest and since the scope arg is not being set in the RefreshTokenAsync call in Auth0ClientBase, the scope end up being set to null.

Then theRequestRefreshTokenAsync method in the HttpClientTokenRequestExtensions attempts to set the scope to what is defined in the RefreshTokenRequest (which happens to be null due the reason explained above) See here: https://github.com/IdentityModel/IdentityModel/blob/4.0.0/src/Client/Extensions/HttpClientTokenRequestExtensions.cs#L97

When trying to set it using the extraParameter arg, the Duplicate parameter: scope exception is thrown as the RequestRefreshTokenAsync method in the HttpClientTokenRequestExtensions attempts to set the scope.

Possible fix: In version 4.0.0, there is simply no way to set the scope, and i think that is way they ended up adding the scope arg in version 5.2.0 (might be easier said than done) A possible fix could be to update the IdentityModel.OidcClient dependency to 5.2.0 and make it possible to set it via the RefreshTokenAsync in the `Auth0ClientBase and pass the scope further.

mayhammf commented 1 year ago

The only workaround at the moment is to set a custom BackchannelHandler and intercept the /token reqeust where the scope parameter is missing and inject it that way.

frederikprijck commented 1 year ago

Thanks, I believe it makes sense that it is added as an extra parameter to the IdentityModel.OidcClient's RefreshToken method, and not read from the initial options, as typically you only need scope here when you want to de-scope the Access Token. The Refresh Token should already be connected to the original scopes, so there is no reason to sent along scopes unless you want to de-scope it.

De-scoping would be where you originally issued a token for a certain set of scopes, but you want to use the refresh token to get an access token for a subset of those original scopes.

Can you verify that descoping is what you are looking for?

mayhammf commented 1 year ago

Correct. The requirements we have force us to define the scope again for requests with refresh grant type.

We have been using the extra parameter option in the previous version (3.2.0). And due to the changes in the subdependencies, the extra parameter option started throwing the duplicate parameter exception.

In version 4.0.0 of the IdentityModel, the scope will always be set to null, and i think the author of the package made that realizatdion between version 4.0.0 and 5.2.0 and thus ended up adding the scope as an arg to the RequestRefreshTokenAsync method

frederikprijck commented 1 year ago

Thanks, we can have a look into this and how and if we can provide a solution here, even if just bumping the dependency (with or without bumping our major version, will need to look into that).

Just to ensure we are saying the same thing, you aren't looking to pass scope to be able to access it in a Rule or anything, right?

mayhammf commented 1 year ago

Thank you for looking into it. The reason is that we need the scope parameter to be included in the request that ends up going out of our system, the parameter is then used by the owner of the Auth0 client to perform certain processes and return the correct claims.

In principle, the extra parameters options should not be interfered with and i would say that it is an issue in the IdentityModel dependency where they blindly set the scope parameter without checking that it is already set https://github.com/IdentityModel/IdentityModel/blob/5.1.0/src/Client/Extensions/HttpClientTokenRequestExtensions.cs#L111

frederikprijck commented 1 year ago

The reason is that we need to scope parameter to be included in the request that end up going out of our system, the parameter is then used by the owner of the Auth0 client to perform certain processes and return the correct claims.

Can you elaborate on this? I want to ensure we aren't using scope for purposes other than it is intended.

When u initially login with Scope: A AND B, you get a Refresh Token for Scope A AND B. You can pass Scope A OR B when consuming that refresh token, and it will return another refresh token only for Scope A OR B.

If you don't want this, and would be planning to send A AND B, I would argue Scope is not intended for this, and probably want to look into using a different, custom parameter that doesnt conflict with what's defined in the specification.

Regardless, this is something we need to look into to support, but saying the above mostly to ensure you don't encounter issues by using scope for things other than it's intended for.

mayhammf commented 1 year ago

Both cases are applicable here and are relevant to our system: A OR B: this is the de-scoping scenario that you mentioned and, in the current version of the library, it is not possible to do that (due to the behavior explained in the description of the issue) A AND B: we currently need to pass the same scopes again, but based on your description, this is an issue which the owner of the Auth0 client must correct (aka: new "refreshed" tokens must retain the original scope if otherwise isn't requested)

frederikprijck commented 1 year ago

Thanks, that makes sense. Just want to ensure you don't run into issues.

Happy to review a PR if you want to provide one, else I can add this to our backlog to try and get to as soon as possible, but can not make any timeframe promises.

mayhammf commented 1 year ago

I would be happy to create a PR but it's not gonna be a simple one; there isn't a straightforward way to set the scope in the version 4.0.0 of the IdentityModel.OidcClient package which is the version used by your library. I can update the dependency to version 5.2.0 (where the method signature accepts the scope out-of-the-box) but that will either get the PR rejected or will require extensive testing as it is a major version upgrade.

We currently hacked our way around the issue by setting our own backchannel http handler via the Auth0ClientOptions to something like this: (ugly hack alret)

public class BackchannelHttpMessageHandler : HttpClientHandler
    {
        private readonly string scope;
        public BackchannelHttpMessageHandler(string scope)
        {
            this.scope = scope;
        }

        protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
        {
            if (request is IdentityModel.Client.ProtocolRequest protocolRequest)
            {
                var isTokenRequest =
                    protocolRequest.Method == HttpMethod.Post
                    &&
                    protocolRequest.RequestUri.AbsolutePath.EndsWith("/token");

                if (isTokenRequest)
                {
                    var content = await request.Content.ReadAsStringAsync();
                    var parameters = HttpUtility.ParseQueryString(content);
                    var isRefreshGrantType = parameters["grant_type"] == "refresh_token";
                    var scopeIsMissing = parameters["scope"].IsEmpty();

                    if (isRefreshGrantType && scopeIsMissing)
                    {
                        parameters["scope"] = this.scope;
                        var dict = new Dictionary<string, string>();
                        foreach (string key in parameters.AllKeys) dict.Add(key, parameters[key]);

                        request.Content = new FormUrlEncodedContent(dict);
                    }
                }

            }

            return await base.SendAsync(request, cancellationToken);
        }
    }

mayhammf commented 1 year ago

Side note: The package Auth0.OidcClient.Core 3.2.6 declares IdentityModel.OidcClient (>= 4.0.0) which could potentially be problematic as it is essentially stating that the latest 5.2.0 version is allowed, and that is not the case at least for the RefreshTokenAsync method where the signature referenced in this library no longer exists. The upgrade could easily occur unintentionally if another dependency references the higher version as sub-dependency.

frederikprijck commented 1 year ago

👋 I opened a PR to address this, can you have a look and see if it would work for you?

This should be non-breaking, assuming everything else keeps working as it did with v4 (which it looks like it does, but I will still need to verify).

saamerm commented 1 year ago

Is anyone having an issue while creating an archive? https://github.com/auth0/auth0-oidc-client-net/issues/249 I made sure that the OIDC package version that gets installed was v4.0.0, and strangely all my other functions work perfectly fine

frederikprijck commented 1 year ago

That doesnt seem to related to eachother. Let's keep the communication in one place (in the issue you reported) please.

frederikprijck commented 1 year ago

This got incorrectly closed through the wrong commit due to unfortunate wording in the PR description, my bad.

This will be resolved through #255, which does need #254 .

auth0 / auth0-oidc-client-net