Refresh token not revoked even though logs say it was

[JarrodJ83]

Issue / Steps to reproduce the problem

If the ClientId of the app trying to revoke the token does not match the ClientId of the token then the token is not revoked. However, the result is true and the logs indicate it was removed. The issue is in TokenRevocationResponseGenerator:

var token = await RefreshTokenStore.GetRefreshTokenAsync(validationResult.Token);

            if (token != null)
            {
                if (token.ClientId == validationResult.Client.ClientId)
                {
                    Logger.LogDebug("Refresh token revoked");
                    await RefreshTokenStore.RemoveRefreshTokenAsync(validationResult.Token);
                    await ReferenceTokenStore.RemoveReferenceTokensAsync(token.SubjectId, token.ClientId);
                }
                else
                {
                    Logger.LogWarning("Client {clientId} tried to revoke a refresh token belonging to a different client: {clientId}", validationResult.Client.ClientId, token.ClientId);
                }

                return true;
            }

            return false;

I'm not sure what the desired behavior is here. If it is to just warn the user but still revoke the token then we should always do the revoke and issue the warning if the clientIds differ. If the desired behavior is to prevent the revocation if the clientIds don't match then we need to return false after issuing the warning.

In my case (and perhaps this is not the right way to do this) I am building an admin app that we will be able to use internally to revoke refresh tokens if necessary. I secure this app with the IdP using client credentials. In this case my clientids will not match but I would want the app to be able to delete refresh tokens of other clients.

I'm happy to fix this if someone can give me direction on which way it should be.

Steps to Reproduce

Try and revoke a token using a different client than the one that issued the token.

Relevant parts of the log file

Warning: Client "Mobile_Code" tried to revoke a refresh token belonging to a different client: "Mobile_Code"
Information: Token successfully revoked
Information: 200 "POST" "/connect/revocation"

[leastprivilege]

The spec says the result of a revocation request must always be success to prevent token scanning attacks.

[JarrodJ83]

Thanks for pointing this out, @leastprivilege . I should have read the spec before posting. After reading it this all makes sense.

That said I don't believe the logs are accurately representing what is happening on the server. Here is what is being logged currently:

Warning: Client "Mobile_Code" tried to revoke a refresh token belonging to a different client: "Mobile_Code"
Information: Token successfully revoked
Information: 200 "POST" "/connect/revocation"

Here is something like I would expect to see:

Warning: Client "Mobile_Code" tried to revoke a refresh token belonging to a different client: "Mobile_Code". Tokens will not be revoked.
Information: Token revocation process succeeded
Information: 200 "POST" "/connect/revocation"

At this point it is clear to those who haven't read all the specs completely that this is desired behavior. What do you think?

Also, in the warning line the same clientId is used in the log template so it doesn't log both clientIds:
Logger.LogWarning("Client {clientId} tried to revoke a refresh token belonging to a different client: {clientId}", validationResult.Client.ClientId, token.ClientId);

results in:

Warning: Client "Mobile_Code" tried to revoke a refresh token belonging to a different client: "Mobile_Code"

This is is just about clarity at this point. Let me know if you'd like me to make the above suggestions. Otherwise feel free to close this issue!

[leastprivilege]

OK - want to send a PR?

[JarrodJ83]

Will do!

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.