ueberauth / ueberauth_identity

A username/password Strategy for Überauth
MIT License
80 stars 21 forks source link

doc error, ( I think ) #23

Closed kswope closed 5 years ago

kswope commented 6 years ago

At the end of the README, in the "Calling" section

Depending on the configured url you can initial the request through:

/auth/identity

Shouldn't this be

/auth/identity/callback

In addition, shouldn't this just be shared with the real oauth callbacks and be function matched somewhere on %{provider: strategy} ? Otherwise, if its on its own, its not really a callback, just a controller method you are submitting to. ( The ueberauth example does what I'm suggesting )

yordis commented 6 years ago

In general,

/auth/:strategy_name initialize the workflow based on the strategy name. /auth/:strategy_name/callback received the information from the workflow.

I am not sure if these questions are answered on Ueberauth already @ueberauth/core ?! I am sure that this is documented.

kswope commented 6 years ago

The docs are wrong are they not? There is no /auth/identity in the 'workflow'.

yordis commented 6 years ago

identity comes from the name of the strategy so by you injecting the ueberauth_identity strategy you will have identity available.

Sorry if I am misunderstanding something.

kswope commented 6 years ago

Sorry if I am misunderstanding something.

You are, but its no big deal, its just a doc thing.

In ueberauth the user clicks

GET /auth/:strategy_name 

And that starts the flow, I think we agree with that.

However, in ueberauth_identity

GET /auth/:strategy_name 

Doesn't seem to exist, there is no route for it, if you go to

GET /auth/identity

I only get an error, something about request/2 not there

Which makes sense, because

/auth/:strategy_name 

Is a GET route, and when you submit a login form for ueberauth_identity, you would not be making a GET request.

I'm just pointing out in the docs, references to

/auth/identity

are not correct.

yordis commented 6 years ago

From the README

  1. Create the request and callback routes if you haven't already:

    scope "/auth", MyApp do
      pipe_through :browser
    
      get "/:provider", AuthController, :request
      get "/:provider/callback", AuthController, :callback
      post "/identity/callback", AuthController, :identity_callback
    end

Did you add the proper routing on your app?!

kswope commented 6 years ago

Where is the

POST /auth/identity

In those routes?

As in the example https://github.com/ueberauth/ueberauth_example/blob/master/lib/ueberauth_example_web/templates/auth/request.html.eex

<%= form_tag @callback_url, method: "post" do %>
yordis commented 6 years ago

@kswope there is not POST /auth/identity but POST /auth/identity/callback

The POST to the callback is done by the OAuth2 workflow, no by you. You just initialize the workflow by doing GET /auth/:provider_name

kswope commented 6 years ago

The POST to the callback is done by the OAuth2 workflow, no by you. You just initialize the workflow by doing GET /auth/:provider_name

There is no way that is correct. You are saying to start the ueberauth_identity workflow you have to submit a form (with a password) to a GET??? You never submit a form using a GET, its a violation of REST principles and will get you in big trouble, and you also cannot guard against CSRF attacks.

There is further evidence you are wrong, in the ueberauth_identity example, the form, as I pointed out, is a POST

<%= form_tag @callback_url, method: "post" do %>

ueberauth_identity seems to have a 1 stage workflow, unlike ueberauth itself, which is a two stage workflow.

yordis commented 6 years ago

@kswope you are right, I forgot the context of talking about identity provider.

kswope commented 6 years ago

Ok, so back to the original post, I'm correct, right? Doc error? I'm passionate about this because I lost a couple hours trying to figure it all out, not including this thread :)

doomspork commented 6 years ago

@kswope a PR clearing up the documentation is always welcomed! 😁

lessless commented 5 years ago

this was closed with #25

thanks @kswope