Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Question on alwaysIssueNewRefreshToken, how is my saveToken() suppose to know it was from a refresh_token grant type? #673

Open
HappyZombies opened this issue Jan 11, 2021 · 2 comments

Comments

@HappyZombies
Copy link

When alwaysIssueNewRefreshToken is set to false, the revokeToken() method is never ran, and as such the saveToken() method is called directly without removing the older token (makes sense).

So in my example, saveToken() is called, and in it I have a database query that executes an INSERT to my database. My query fails since in the schema I made, it is expecting both refreshToken and access token, but when alwaysIssueNewRefreshToken is false, I don't get a new refresh_token and only a new access_token, so I have to now run an UPDATE on my db query....Now yes, I can update my saveToken() method to be flexible for this, but the problem is that there is really no way (expect seeing that I have no refresh token) for me to know if saveToken() is being called to save a brand new token or to simply update a new token. I guess I can do

if(!token.refreshToken) run UPDATE query instead

But shouldn't there be some sort of flag or maybe a updateToken() method that runs instead? I guess I could run an upsert query instead, but now saveToken() isn't really "saving a token" now, it's doing an upsert or conditional insert if it doesn't have a refresh_token.

TL;DR -- Maybe incorporate an optional 'updateToken()' method that runs when alwaysIssueNewRefreshToken is false?

@HappyZombies
Copy link
Author

In addition, since I only retrieve the access_token from 'saveToken()', I won't be able to fully return the entire token object since I am missing the refresh_token, https://oauth2-server.readthedocs.io/en/latest/model/spec.html?highlight=getRefreshToken#savetoken-token-client-user-callback

The return values expect to contain the refresh_token when I can't provide it because it's not being returned.

What I think needs to happen is instead of only returning access_token in saveToken() during a refresh_token grant, to also include the existing refresh_token that wasn't deleted. This way we can return the refresh_token (which is just the same) and also so we don't lose the reference of the tokens we need to update.

https://github.com/oauthjs/node-oauth2-server/blob/master/lib/grant-types/refresh-token-grant-type.js#L161 here instead of not including the refresh_token if alwaysIssueNewRefreshToken is false, we should include the refresh_token that was not revoked/removed. I can create an MR if needed, just let me know if this is something you guys would like to see.

I understand that I can add certain checks and change a few things on my side, but then I am not leveragling this API at all and instead finding patches/fixes for things that should be supported.

Let me know if you guys would like me to make an MR for this if you deem it necessary/needed.

@fjeddy
Copy link

fjeddy commented Feb 7, 2022

but the problem is that there is really no way (expect seeing that I have no refresh token) for me to know if saveToken() is being called to save a brand new token or to simply update a new token. I guess I can do

It is exactly how you know if it's an update or a creation, the lack of a refreshToken... The library is doing it's data validation in the background, so you can safely assume that it's an update if no refreshToken is present.

Having a seperate updateToken function would just split the logic into two functions, besides that, it would work exactly the same way. You can achieve this yourself by creating the logic.

saveToken: async (token, client, user) => {
  if (refreshToken) this.updateToken(token, client, user)
  else this.createToken(token, client, user)
}

Just a FYI: It sounds like you are storing the accessToken and refreshToken in the same table, you could ask yourself, why not combine getAccessToken and getRefreshToken ? Your setup is "abnormal", as the common practice is to store accessTokens and refreshTokens in two seperate tables, apart from each other.

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

No branches or pull requests

2 participants