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

Handle errors in OAuth endpoint #27

Merged
merged 3 commits into from
Nov 3, 2019
Merged

Conversation

frangio
Copy link
Contributor

@frangio frangio commented Nov 3, 2019

Fixes #26.

I've added a test and a fix. Let me know if you had something else in mind.

Jest is reporting missing branch coverage because I'm not testing the default value in case the response does not include an error_description. I introduced the default value just in case, but the documentation I could find seems to always include error_description, so maybe it's not necessary.

const {
data: { access_token: token, scope }
} = await request(route, parameters);
} = response;
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of throwing a custom error, I'd simulate the behavior that would occur if the server responded with a 400 status code. I've created https://github.com/octokit/request-error.js for cases such as this.

The code would look sth like this

throw new RequestError(`${response.data.error_description} (${response.data.error})`, 400, {
  headers: response.headers,
  request: request.endpoint(route, parameters)
});

That way the error can be handled just like other errors such as server outages or request timeouts

Copy link
Contributor Author

@frangio frangio Nov 3, 2019

Choose a reason for hiding this comment

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

I implemented this change but used response.status (which will be 200) rather than 400... Because it doesn't sound like a good idea to fake the status code like that. Let me know if you disagree.

Edit: Reading more carefully I see that you had explicitly suggested emulating a 400 status code. I don't have enough context to understand what is or isn't a good idea here. I'll just do whatever you tell me.

@frangio
Copy link
Contributor Author

frangio commented Nov 3, 2019

There is a type error that should be fixed by octokit/request-error.js#17.

@gr2m gr2m self-assigned this Nov 3, 2019
@gr2m gr2m force-pushed the login-oauth-error branch from f4d7618 to a8d36c4 Compare November 3, 2019 21:12
@gr2m
Copy link
Contributor

gr2m commented Nov 3, 2019

I've rebased on latest master and bumped @octokit/request-error to the latest version. Once everything is green I'll go ahead and merge

@gr2m
Copy link
Contributor

gr2m commented Nov 3, 2019

Could you do me a favor and apply the same fix to @octokit/auth-oauth-app.js? The same error was reported over there, too, and the very same fix is required: octokit/auth-oauth-app.js#16

@gr2m gr2m merged commit 01b32fd into octokit:master Nov 3, 2019
@octokitbot
Copy link
Collaborator

🎉 This PR is included in version 2.4.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

Cannot read property 'split' of undefined
3 participants