ueberauth / guardian

Elixir Authentication
MIT License
3.44k stars 381 forks source link

Suggestion for version 0.X -> 1.X upgrade guide, re: null client auth tokens #573

Closed darrenklein closed 5 years ago

darrenklein commented 5 years ago

Hello, thanks very much for all of your work on this terrific library. I just wanted to write to make note of an issue that I encountered when upgrading from Guardian 0.14 -> 1.1 that I don't think was mentioned in your upgrade guide. Briefly - when using version 0.14, a null token in the authorization header on the request connection would work for requests to unprotected controller actions, but in version 1.1 the presence of a null token for this header would cause an error; the header needs to be entirely absent from the request in order for the request to an unprotected controller action to work. Ok, a little more detail -

I'm using my Phoenix app as a "pure" API which is consumed by two ReactJS apps running on separate servers. While most of the Phoenix app's controller actions need Guardian's protection, there are a handful that don't. When using Guardian 0.14, my router pipeline was set up like so:

  pipeline :api do
    plug CORSPlug, [origin: Application.get_env(:my_app, :client_url)]
    plug :accepts, ["json"]
    plug Guardian.Plug.VerifyHeader, realm: "Bearer"
    plug Guardian.Plug.LoadResource
  end

and I flagged unprotected controller actions in the controllers themselves using on a controller-by-controller basis using:

plug Guardian.Plug.EnsureAuthenticated when not action in [:index, :reset_password]

On the client side, I was setting the headers for all of my requests like so:

const headers = () => {
        const token = JSON.parse(localStorage.getItem("token")),
            reqHeaders = {
                "Accept": "application/json",
                "Content-Type": "application/json"
                                "Authorization": ``Bearer: ${token}`
            }

        return reqHeaders
    }

which, in certain circumstances, was setting the value of the Authorization header as Bearer: "null" - basically, any request to the server outside of an authenticated session. Ok, maybe not best practice, but it worked for any request made to an unprotected action.

After upgrading Guardian, my router pipeline was changed to:

  pipeline :api do
    plug CORSPlug, [origin: Application.get_env(:my_app, :client_url)]
    plug :accepts, ["json"]
    plug MyApp.Guardian.AuthPipeline
  end

and I continued to use the controller-based method of excluding specific actions from Guardian protection - however, I now found that requests to unprotected controller actions were raising an exception:

(Protocol.UndefinedError) protocol String.Chars not implemented for %ArgumentError{message: "argument error: [\"null\"]"}. This protocol is implemented for: Atom, BitString, Date, DateTime, Decimal, Ecto.Date, Ecto.DateTime, Ecto.Time, Float, Integer, List, NaiveDateTime, Postgrex.Copy, Postgrex.Query, Postgrex.Stream, Time, UAParser.Device, UAParser.OperatingSystem, UAParser.UA, UAParser.Version, URI, Version, Version.Requirement

relating to changes made to Guardian.Plug.VerifyHeader - lib/guardian/plug/verify_header.ex:98: Guardian.Plug.VerifyHeader.call/2

Anyway, the fix for this was pretty simple - I just changed my client code to conditionally include the Authorize header:

const headers = () => {
        const token = JSON.parse(localStorage.getItem("token")),
            reqHeaders = {
                "Accept": "application/json",
                "Content-Type": "application/json"
            }

        if (token) {
            reqHeaders["Authorization"] = `Bearer: ${token}`
        }

        return reqHeaders
    }

and now it all worked perfectly.

This is probably best (or better) practice anyway, but regardless, it does illustrate a potential breaking change that users may encounter when upgrading from version 0.X -> 1.X - only add an Authorization header for requests that need to be authorized.

Hanspagh commented 5 years ago

Thank you for the heads up and very clear instructions. Maybe we should add this to the upgrade guide?

darrenklein commented 5 years ago

Great, thanks for giving this your attention. Keep up the great work!