wrong typing for revokeToken argument

[jorenvandeweyer]

Specify your setup

• Operating System:
• Node version:
• npm version:
• version of @node-oauth/oauth2-server
• which OAuth2 workflow:
• at which workflow step does the error occur:

Describe the bug

Typescript type seems to be wrong for the revokeToken argument token

https://github.com/node-oauth/node-oauth2-server/blob/b9d4093eea7644fde3b982a46922dcef7dcae75c/index.d.ts#L371

Expected behavior

Should be only RefreshToken

[jankapunkt]

Hm, callback should also be not included but that was already fixed :thinking: Let me check, which branch contains the most recent docs right now

[jankapunkt]

Okay I found that types also still contain callback arguments, so we should definitely remove them for development and furthermore 5.x release

[shrihari-prakash]

@jankapunkt we already removed callbacks in 5.x I remember?

[jankapunkt]

@shrihari-prakash thanks for the reminder! @jorenvandeweyer in that case we only need to change the token param, the rest gets updated with merging #194 automatically

[shrihari-prakash]

211

[jankapunkt]

@jorenvandeweyer @shrihari-prakash the types file still contains revokeToken instead of refreshToken in #194 . Should this be handled in #194 ? I mean, once it's released it will be "pinned" in this version and may cause confusion.

One more PR to fix this?

[jorenvandeweyer]

Yes, this should be fixed bedore releasing it

[shrihari-prakash]

@jankapunkt you mean we remove the Token and only have RefreshToken right?

[shrihari-prakash]

I have a doubt, why should it be refresh token? As I understand, the function also revokes access tokens?

[jorenvandeweyer]

The refresh token type is passed as argument not the access token type, where do you see the access token type being passed? @shrihari-prakash

[shrihari-prakash]

@jorenvandeweyer the type Token already contains both refresh and access tokens. The function revokeToken is used to clear both refresh and access tokens. I think it is rather correct that we remove type RefreshToken and keep type Token in the method revokeToken.

[shrihari-prakash]

I don't think there is a separate function to clear access tokens at the moment.

[shrihari-prakash]

Or on the flip side, we could also remove Token type from argument and add properties accessToken and accessTokenExpiresAt to the RefreshToken type. As you can see, there is nothing related to access tokens currently on that type:

[jankapunkt]

@jorenvandeweyer @shrihari-prakash since I am not a TypeScript wizard at all I might ask you to find a solution that may be the best future-proof approach. It can be breaking, since 5.x is breaking anyway. Based on what you will come up with I will be available for review :-)

[shrihari-prakash]

Actually, I am not sure why we have two token interfaces in the first place.

For me, we should remove the type RefreshToken from the types and keep only Token. The difference between these types is only related to accessToken and an accessToken is always present in every refresh token anyway (@jankapunkt correct me if I am wrong), so better to add them. What do you think @jorenvandeweyer ?

For reference, the two types:

/**
     * An interface representing the token(s) and associated data.
     */
    interface Token {
        accessToken: string;
        accessTokenExpiresAt?: Date | undefined;
        refreshToken?: string | undefined;
        refreshTokenExpiresAt?: Date | undefined;
        scope?: string[] | undefined;
        client: Client;
        user: User;
        [key: string]: any;
    }

    /**
     * An interface representing the refresh token and associated data.
     */
    interface RefreshToken {
        refreshToken: string;
        refreshTokenExpiresAt?: Date | undefined;
        scope?: string[] | undefined;
        client: Client;
        user: User;
        [key: string]: any;
    }

[jankapunkt]

Just want to add that this design is mainly from the former oauthjs/oauth2-server repository, which is why I think we should consider the most future-proof approach here, even if breaking.

[shrihari-prakash]

Just want to add that this design is mainly from the former oauthjs/oauth2-server repository, which is why I think we should consider the most future-proof approach here, even if breaking.

@jankapunkt , that reminds me, I guess we really need a guide to upgrade to 5.X apart from changelog. We did this migration as we know the changes done, but it might hard for someone who is coming from oauthjs/oauth2-server (which I suppose accounts to a large portion of the people coming to this repository as that one is not maintained).

[jankapunkt]

100% agree on a migration guide here!

[shrihari-prakash]

Hello @jorenvandeweyer did you get a chance to revisit this?

[jorenvandeweyer]

Yes!

After a closer look we can conclude that the revokeToken function argument should have the same type as the return type of the getRefreshToken function.

https://github.com/node-oauth/node-oauth2-server/blob/1c409269c71e70449b7f79c45cb77a9687bfe15b/lib/grant-types/refresh-token-grant-type.js#L56-L57

Based on the type this function returns, I suggest we remove the Token type and keep the RefreshToken

https://github.com/node-oauth/node-oauth2-server/blob/1c409269c71e70449b7f79c45cb77a9687bfe15b/index.d.ts#L357

For me, we should remove the type RefreshToken from the types and keep only Token. The difference between these types is only related to accessToken and an accessToken is always present in every refresh token anyway (@jankapunkt correct me if I am wrong), so better to add them. What do you think @jorenvandeweyer ?

@shrihari-prakash Revoking the access token or refresh token by the user is not documented in RFC6749. It only covers that refresh token should be deleted when refreshing it. So I think we should currently not extend this token type.

[shrihari-prakash]

Hello @jorenvandeweyer , I am okay to keep any one of Token or RefreshToken type and remove the other. But the token type we decide to keep should contain the fields accessToken and accessTokenExpiresAt, even if we decide to keep RefreshToken. Because the entire point of the refresh token is to return an access token along with it.

Because if accessToken does not exist in this type, I don't see how the developer implementing the OAuth model can clear access tokens if they are using a key-value storage. Of course the actual code will pass the accessToken key, but it should also be a part of the types.

About "Revoking the access token or refresh token by the user is not documented in RFC6749", I was mentioning about the function in the OAuth model that clears off the refresh token (revokeToken). It is not user invoked yes.

[jankapunkt]

@jorenvandeweyer @shrihari-prakash the PR #247 is merged, how to we want to continue from here?

[shrihari-prakash]

Hello @jankapunkt , @jorenvandeweyer ,

The current state of types after #247 looks good from the OAuth spec point of view. However, it does not accurately represent what is actually passed to the function revokeToken.

Consider this scenario where someone implementing the OAuth model is using a key value store like Redis/Memcached as their token storage.

Basically, the model should implement the functions getAccessToken and getRefreshToken where you get the access token and refresh token objects by the string value passed. In such cases, when calling the saveToken function, the token should be saved twice, one with access token as the key and one with refresh token as the key where the access token key has a short expiry and the refresh token a longer one.

Now when the refresh token needs to be revoked (which obviously means that the associated access token also needs to be revoked), the latest merge of #247 means that access token will not be a part of the argument anymore. Now the refresh token from Redis/Memcached is cleared but the access token will be left dangling as the accessToken key is not a part of the RefreshToken type.

Of course, this is just from the types point of view and the actual code does pass the token with the accessToken property included. So no functionality is affected although there is a difference in the argument of revokeToken specified in the types file and the actual code.

Also about

we can conclude that the revokeToken function argument should have the same type as the return type of the getRefreshToken function.

It is also important that the argument types passed to saveToken and revokeToken are the same as they work synchronously where revokeToken deletes the token saved by saveToken.

I will let you to decide what is your preference on that. I am okay with also the current one if you want to go ahead with it as it doesn't affect the functionality.

[jankapunkt]

I'm not involved enough with Typescript to make a meaningful addition here, which is why I solely let this be on you both.

[jorenvandeweyer]

@shrihari-prakash The spec does not mention the revocation of a previously issued access token when using a refresh token.

In many cases it would be even impossible to revoke previously issued access token, think of short lived JWT tokens that are not saved in the database.

When you look at popular oAuth2 services they even declare access token revocation is not possible at all. (source)

To conclude I think we should not require or suggest to pass the access token in the getRefreshToken function by extending the type nor should we implement a getAccessToken function.

[jankapunkt]

Hey @shrihari-prakash @jorenvandeweyer since this is done, the only thing left is a migration guide. I opened a discussion for this here: https://github.com/node-oauth/node-oauth2-server/discussions/248

Let's give it some thoughts and I will craft a guide until end of this week.