Skip to content

Conversation

Namburgesas
Copy link

Using refresh_token grant type to get a new access token was not working because of this issue. Client object key was "id" instead of "clientId". As a result, client ID check always failed because undefined was always returned.

@thomseddon
Copy link
Member

Yep, all the example models use clientId not id so this should be fixed. It's potentially a big breaking change so we'll need to make a minor version bump.

Please can you squash this into a single commit?

@Namburgesas
Copy link
Author

Squashed.

Copy link
Contributor

@mjsalinger mjsalinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mjsalinger mjsalinger merged commit af6741e into oauthjs:dev Aug 27, 2018
@adieuadieu
Copy link

adieuadieu commented Aug 28, 2018

This doesn't seem right. In the documentation, the return shape of the getClient() model method says it should be id.

Moreover, if one changes the getClient() method to include the clientId property instead of id, the authorization_code grant-type stops working because it still expects client.id:

if (code.client.id !== client.id) {
throw new InvalidGrantError('Invalid grant: authorization code is invalid');
}

@maxtruxa
Copy link
Member

maxtruxa commented Aug 28, 2018

I have to agree with @adieuadieu. Looking through the changes again I have no idea why I approved this PR a few months ago.

Client object key was "id" instead of "clientId".

Since getRefreshToken is a model function that has to be implemented by the user, it looks like OP just returned a malformed object (with clientId: '...' instead of client: {id: '...'} as correctly stated in the documentation).

@Namburgesas
Copy link
Author

My fault, I was unaware that the documentation specified that client: {id: '...'} is the proper form. Shall we back out this change and fix the example models instead?

As mentioned by @thomseddon, the example models use client: {clientId: '...'} instead of client: {id: '...'}. Upon closer inspection, they also seem to have other issues, including returning client.clientSecret unnecessarily.
See https://github.com/oauthjs/express-oauth-server/blob/master/examples/postgresql/model.js

@mjsalinger
Copy link
Contributor

I'll back out this change.. Anyone want to tackle updating the examples? If not, i can probably get to it next week..

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

Successfully merging this pull request may close these issues.

5 participants