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

Fix Github provider #159

Merged
merged 1 commit into from
Apr 28, 2018
Merged

Fix Github provider #159

merged 1 commit into from
Apr 28, 2018

Conversation

jmschneider
Copy link
Contributor

Fix issue with using jsonMiddleware for form encoded data. Update documentation to indicate that client_secret is required.

// Json parser
const jsonMiddleware = bodyParser.json()
// Form data parser
const formMiddleware = bodyParser.urlencoded()
Copy link
Member

Choose a reason for hiding this comment

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

This line is affecting all providers! If formMiddleware is required we can add an optional parameter like addAuthorize(strategy, { json: true } = {}) and conditionally use either form for json middleware

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, the addAuthorize function is only called by the github provider. The problem is the _handleCallback method in the oauth2 scheme. It looks like it used to send json data but at some point was changed to send form data. Whenever this was changed, the serverMiddleware was not updated to use the form middleware. Currently, I could not get the github provider to work without this change.

As a side not, this was an issue I ran into with the laravel.passport provider as well and was a major reason I did not use the addAuthorize function. If we also had the authorization request send the full req.body instead of just req.body.code it could potentially be reused for the laravel.passport provider.

Copy link
Member

@pi0 pi0 left a comment

Choose a reason for hiding this comment

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

pending to prevent breaking changes

@jmschneider
Copy link
Contributor Author

After looking at the oauth2 scheme, I think it might make sense to add redirect_uri, response_type and grant_typeto the data sent by the server middleware. If it is all data that is intentionally included in the front end authentication token request it would make sense that we would also want to include it in the backend authentication token request.

@pi0
Copy link
Member

pi0 commented Apr 28, 2018

Merging as a hotfix because

Currently, the addAuthorize function is only called by the github provider.

@pi0 pi0 merged commit 8b1819f into nuxt-community:dev Apr 28, 2018
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

Successfully merging this pull request may close these issues.

2 participants