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 all GitHub errors #58

Merged
merged 3 commits into from
Jun 1, 2020

Conversation

chasers
Copy link
Contributor

@chasers chasers commented May 19, 2020

This should handle #57

@@ -234,18 +237,27 @@ defmodule Ueberauth.Strategy.Github do
conn = put_private(conn, :github_token, token)
# Will be better with Elixir 1.3 with/else
case Ueberauth.Strategy.Github.OAuth.get(token, "/user") do
{:ok, %OAuth2.Response{status_code: 401, body: _body}} ->
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we fine with a generalized error here or do we actually want to keep the Token Error with unauthorized error when status code is 401? This could potentially be a breaking change if some consumers are depending on the Token Error. Let me know what you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In production for me 401s are coming back as {:error, %OAuth2.Response{status_code: 401, body: _body}} (see my example in #57) which is why my app is blowing up here so I'm not even sure {:ok, %OAuth2.Response{status_code: 401, body: _body}} will match anything anymore. Maybe this has to do with my Oauth2 version? I tried to reproduce this with Github manually but I'm not even sure how to do that.

But if you want me to include it anyways I will... Just let me know!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Meh I added it back just in case.

@doomspork doomspork requested a review from Hanspagh May 26, 2020 19:42
@doomspork
Copy link
Member

@Hanspagh do you want to take a peek at this now that it's been updated?

@Hanspagh Hanspagh merged commit d513791 into ueberauth:master Jun 1, 2020
@Hanspagh
Copy link
Contributor

Hanspagh commented Jun 1, 2020

This looks all good to me thanks 👍

1 similar comment
@Hanspagh
Copy link
Contributor

Hanspagh commented Jun 1, 2020

This looks all good to me thanks 👍

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.

3 participants