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.

Sign up for GitHub

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

Jump to bottom

Handle client side flow for google auth. #35

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Handle client side flow for google auth. #35

wants to merge 1 commit into from

Conversation

rohanpujaris
Copy link

Use ueberauth_google with client side flow, when user just passes token to server.

@rohanpujaris rohanpujaris force-pushed the support_for_google_client_side_flow branch from 1367e38 to 9ae213d Compare April 11, 2017 16:44
hassox
hassox reviewed Jun 13, 2017
View reviewed changes
Copy link
Member

@hassox hassox left a comment

Choose a reason for hiding this comment

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

Overall looks good. 1 question about the return of the oauth call.

lib/ueberauth/strategy/google.ex
@@ -124,7 +131,7 @@ defmodule Ueberauth.Strategy.Google do
resp = Ueberauth.Strategy.Google.OAuth.get(token, path)

case resp do
{:ok, %OAuth2.Response{status_code: 401, body: _body}} ->
Copy link
Member

Choose a reason for hiding this comment

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

Does a 401 here return with an :error in the tuple? I think when I was testing it it still responded with an :ok

Copy link
Author

@rohanpujaris rohanpujaris Jun 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@hassox Yep, it returned an :error when I tested it. I will retest it and confirm.

Copy link
Author

@rohanpujaris rohanpujaris Jun 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@hassox I have verified the response again. It returns with and :error. Below is the returned response.

{:error,
 %OAuth2.Response{body: %{"error" => "invalid_token",
    "error_description" => "Invalid Credentials"},
  headers: [{"vary", "X-Origin"},
   {"www-authenticate",
    "Bearer realm=\"https://accounts.google.com/\", error=invalid_token"},
   {"content-type", "application/json; charset=UTF-8"},
   {"date", "Wed, 14 Jun 2017 06:47:11 GMT"},
   {"expires", "Wed, 14 Jun 2017 06:47:11 GMT"},
   {"cache-control", "private, max-age=0"},
   {"x-content-type-options", "nosniff"}, {"x-frame-options", "SAMEORIGIN"},
   {"x-xss-protection", "1; mode=block"}, {"server", "GSE"},
   {"alt-svc", "quic=\":443\"; ma=2592000; v=\"38,37,36,35\""},
   {"accept-ranges", "none"}, {"vary", "Origin,Accept-Encoding"},
   {"transfer-encoding", "chunked"}], status_code: 401}}

Copy link
Member

Choose a reason for hiding this comment

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

The latest oauth2 version has changed so that any successful HTTP status codes (200..399) returns {:ok, resp} and all others returns {:error, resp}.

@hassox hassox mentioned this pull request Jun 26, 2017
Closed
yordis
yordis reviewed Sep 14, 2017
View reviewed changes
lib/ueberauth/strategy/google.ex
@doc """
Handles the callback for Google client side flow.
"""
def handle_callback!(%Plug.Conn{params: %{"token" => token}} = conn) do
Copy link
Member

Choose a reason for hiding this comment

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

@hassox could we add the parameter as binary as well? We technically don't need the Conn so if I add the logic of the oauth callbacks outside of an phoenix project I dont need to carry on the Conn everywhere.

The same for other strategies

Copy link
Member

Choose a reason for hiding this comment

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

@yordis I'm not sure I follow when you say binary. Are you asking that this match be changed from %Plug.Conn{params: %{"token" => token}} to %{params: %{"token" => token}}?

Copy link
Member

Choose a reason for hiding this comment

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

@blakedietz could you make this change based on @yordis's comment below?

Copy link

Choose a reason for hiding this comment

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

@rohanpujaris the param that holds the JWT is called credential I think: https://developers.google.com/identity/gsi/web/reference/js-reference#credential

@blakedietz
Copy link

Any updates on this? I’m running into issues with my spa + api because of this.

@doomspork doomspork requested a review from a team October 25, 2017 18:29
@doomspork
Copy link
Member

@blakedietz Sorry for the delays, I'll look over things and see where they stand. In the mean time @yordis or @hassox do you have any further questions or concerns?

@yordis
Copy link
Member

yordis commented Oct 25, 2017

@doomspork as long as I don't depends of Plug.Conn because when you use GraphQL/Absinthe I don't have access to such of thing so.

@rohanpujaris
Copy link
Author

@doomspork @yordis Should I proceed with removing %Plug.Conn{} reference on this PR. reference is also present here -> https://github.com/rohanpujaris/ueberauth_google/blob/master/lib/ueberauth/strategy/google.ex#L34

Let me know if we want to remove it from above code as well?

@yordis
Copy link
Member

yordis commented Nov 29, 2017

@rohanpujaris dont worry about it for now.

@doomspork doomspork requested review from a team and removed request for a team November 29, 2017 20:52
@yordis
Copy link
Member

yordis commented Nov 29, 2017

@ueberauth/developers just roll with what we have right now so when we introduce token workflow we go back to all the providers and we take care of the use case

@scrogson
Copy link
Member

scrogson commented Nov 30, 2017 •
edited
Loading

@yordis why would we want to remove the Conn ever? If you want to fetch the user without the Conn just call Ueberauth.Strategy.Google.OAuth.get/2 (which is already public) and you're done.

@yordis
Copy link
Member

yordis commented Nov 30, 2017

@scrogson yes but, details like those

and returning the same ueberauth data structure back is why I wouldn't do the request myself.

Outside, yes just call the API directly.

@benonymus
Copy link

is it not planned to have this merged?

@doomspork
Copy link
Member

@rohanpujaris / @ueberauth/core — where is this PR at? What do we need to do to finish getting this merged? @yordis how does this impact the proposed changes you'd like to make?

@yordis
Copy link
Member

yordis commented Oct 23, 2019

There are multiple ways to implement this, the only issue going for this route is the dependency with Plug.Conn which I guess is fine for now, but people in Absinthe land and other situations where they don't have a Plug.Conn can't use the feature.

@sergiotapia
Copy link

Can we get this merged?

@luka-TU
Copy link

luka-TU commented Sep 6, 2021

Hi! What is the plan for this PR?

@yordis
Copy link
Member

yordis commented Jul 8, 2022

@ueberauth/developers I completely lost track of this, my apologies. Any thoughts?

@rohanpujaris
Copy link
Author

I am no longer working on Elixir and PhoenixFramework. If anybody wants to take this forward, feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luka-TU luka-TU luka-TU left review comments

@yordis yordis yordis left review comments

@doomspork doomspork doomspork left review comments

@scrogson scrogson scrogson left review comments

@hassox hassox hassox left review comments

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@rohanpujaris @blakedietz @doomspork @yordis @scrogson @benonymus @sergiotapia @luka-TU @hassox