ueberauth / guardian

Elixir Authentication
MIT License
3.44k stars 381 forks source link

Using halt on auth_error #401

Closed uzarubin closed 7 years ago

uzarubin commented 7 years ago

I would always get (Plug.Conn.AlreadySentError) the response was already sent when I tried to use the example auth_error as the error_handler inside my pipeline.

It wasn't until I added |> halt that the issue was fixed. See below for an example.

defmodule MyApp.AuthErrorHandler do
  import Plug.Conn

  def auth_error(conn, {type, reason}, _opts) do
    body = Poison.encode!(%{message: to_string(type)})
    # send_resp(conn, 401, body)  <--- This line causes the error

    send_resp(conn, 401, body) |> halt # this makes the error go away  
  end
end

I was wondering if this is something that I messed up in my own configuration, or there was a forgotten halt in the documentation?

oskar1233 commented 7 years ago

I suggest pasting your configuration as well.

Answering you question: halt shouldn't be needed here, docs aren't wrong on this.

uzarubin commented 7 years ago

@oskar1233 I was actually following your example on some of these things. Here is the config from my project:

defmodule Wire.Guardian.Pipelines.JsonAuth do
   use Guardian.Plug.Pipeline, otp_app: :wire,
                              module: Wire.Guardian,
                              error_handler: Wire.Guardian.ErrorHandler

  plug Guardian.Plug.VerifyHeader
  plug Guardian.Plug.LoadResource, allow_blank: false
end

And here is my router file:

pipeline :api do
    plug Wire.Guardian.Pipelines.JsonAuth
  end

 scope "/api", WireWeb do
    pipe_through :api

    get "/invite-tokens/:token_value", BusinessAdmin.InvitesController, :show, param: :token_value
  end

Here is a stack trace with a uuid token

[debug] Processing with WireWeb.BusinessAdmin.InvitesController.show/2
  Parameters: %{"token_value" => "00b18e1f-bfa0-49f3-972d-24d6b0e7ef34"}
  Pipelines: [:api]
[error] #PID<0.806.0> running WireWeb.Endpoint terminated
Server: localhost:4000 (http)
Request: GET /api/invite-tokens/00b18e1f-bfa0-49f3-972d-24d6b0e7ef34
** (exit) an exception was raised:
    ** (Plug.Conn.AlreadySentError) the response was already sent
        (phoenix) lib/phoenix/controller.ex:529: Phoenix.Controller.put_new_layout/2
        (wire) lib/wire_web/controllers/business_admin/invites_controller.ex:1: WireWeb.BusinessAdmin.InvitesController.phoenix_controller_pipeline/2
        (wire) lib/wire_web/endpoint.ex:1: WireWeb.Endpoint.instrument/4
        (phoenix) lib/phoenix/router.ex:278: Phoenix.Router.__call__/1
        (wire) lib/wire_web/endpoint.ex:1: WireWeb.Endpoint.plug_builder_call/2
        (wire) lib/plug/debugger.ex:99: WireWeb.Endpoint."call (overridable 3)"/2
        (wire) lib/wire_web/endpoint.ex:1: WireWeb.Endpoint.call/2
        (plug) lib/plug/adapters/cowboy/handler.ex:15: Plug.Adapters.Cowboy.Handler.upgrade/4
        (cowboy)/wire/deps/cowboy/src/cowboy_protocol.erl:442: :cowboy_protocol.execute/4

Edit: I believe I have found the problem that is causing the error.

# verify_header.ex
 @spec call(Plug.Conn.t, Keyword.t) :: Plug.Conn.t
    def call(conn, opts) do
      with nil <- GPlug.current_token(conn, opts),
           {:ok, token} <- fetch_token_from_header(conn, opts),
           module <- Pipeline.fetch_module!(conn, opts),
           claims_to_check <- Keyword.get(opts, :claims, %{}),
           key <- storage_key(conn, opts),
           {:ok, claims} <- Guardian.decode_and_verify(module, token, claims_to_check, opts) do

        conn
        |> GPlug.put_current_token(token, key: key)
        |> GPlug.put_current_claims(claims, key: key)
      else
        :no_token_found -> conn
        {:error, reason} ->
          conn
          |> Pipeline.fetch_error_handler!(opts)
          |> apply(:auth_error, [conn, {:invalid_token, reason}, opts])
          # There needs to be a |> halt here otherwise the connection will continue. 

        _ -> conn
      end
    end
uzarubin commented 7 years ago

Going to go ahead and close this.

hauptbenutzer commented 6 years ago

I'm sorry to say, this does prevent me from using guardian 1.x with absinthe (or any such API-framework, I would imagine). The problem here is, that in a GraphQL Query some requested fields might require authentication/authorization before resolving, while some don't. To make the distinction, the plug pipeline has to first reach the Absinthe plug. This, due to the halt in the Guardian plugs, cannot happen at the moment.

In fact, there is no way (as far as I can tell) to use the here-mentioned Guardian plugs, and still reach the app logic (absinthe or anything else, really) without manually calling it in the ErrorHandler module. Making the halt a configurable behaviour, would probably alleviate this issue.

ratbag98 commented 6 years ago

I've reached this thread probably for the same reason as @hauptbenutzer - in the middle of upgrading my Absinthe-powered API to use Guardian 1.x and I'm crashing out of my auth error handler if the user isn't logged in.

mpoeter commented 6 years ago

Same problem here! Please let the user decide whether to halt or not.

Apelsinka223 commented 5 years ago

@hauptbenutzer @ratbag98 @mpoeter have you solved this problem somehow?

ratbag98 commented 5 years ago

Sorry @Apelsinka223 , I dropped Guardian the day after posting my “me too” and coded my own very simple system that does what we need and no more.

Apelsinka223 commented 5 years ago

@doomspork is there any chance that halt would become configurable?

mpoeter commented 5 years ago

@Apelsinka223 I simply created my own VerifyHeader module by copying the code from the Guarding module and removing the halt call.

hauptbenutzer commented 5 years ago

@Apelsinka223 We simply built a wrapper module that would unhalt the connection: https://gist.github.com/hauptbenutzer/bcc528c622317aabf0b8c5ee1890c536 Not pretty, but it does the job.

Apelsinka223 commented 5 years ago

@hauptbenutzer looks like best solution, thank you!

DmitryKK commented 5 years ago

I have same problem. Why was the issue closed?

yordis commented 5 years ago

@DmitryKK the person who opened the issue closed it 🤷‍♂

I will suggest to open a new issue and share your particular situation.

oskar1233 commented 5 years ago

@DmitryKK @yordis Configurable halt has been merged in https://github.com/ueberauth/guardian/pull/617.