Auth

[josevalim]

This is a complete auth system on top of a brand new Phoenix app. It includes:

1. Session-based authentication with remember me cookies
2. User registration with e-mail confirmation
3. Safe changing of e-mail (requires the current password and reconfirmation)
4. Safe changing of password(requires the current password)
5. Reset password

Sessions and tokens are kept in a separate table, which give users full server side control of when to expires sessions, tokens, etc. For more details, see the README in this PR.

EDIT I believe this PR has fulfilled its purpose, so I will go ahead and lock this discussion. Please give the mix phx.gen.auth generator by @aaronrenner a try, as we plan to eventually make it part of Phoenix. Thanks everyone for the feedback! 💯

[josevalim]

@aaronrenner I have written some notes we will most likely want to add to the docs of the generator itself:

## Notes about the generated authentication system

### Password hashing

The password hashing mechanism defaults to `bcrypt` for
Unix systems and `pdkdf2` for Windows systems. Both
systems use [the Comeonin interface](hexdocs.pm/comeonin/).

### Forbidding access

The generated code ships with an auth module with handful
plugs that fetch the current account, requires authentication
and so on. For instance, for an app named Demo which invoked
`mix phx.gen.auth Accounts User users`, you will find a module
named `DemoWeb.UserAuth` with plugs such as:

  * `fetch_current_user` - fetches the current user information if
    available

  * `require_authenticated_user` - must be invoked after
    `fetch_current_user` and requires that a current exists and is
    authenticated

  * `redirect_if_user_is_not_authenticated` - used for the few
    pages that must not be available to authenticated users

### Confirmation

The generated functionality ships with an account confirmation
mechanism, where users have to confirm their account, typically
by e-mail. However, the generated code does not forbid users
from using the application if their accounts have not yet been
confirmed. You can trivially add this functionality by customizing
the plugs generated in the Auth module.

### Notifiers

The generated code is not integrated with any system to send
SMSs or e-mails for confirming accounts, reseting passwords,
etc. Instead it simply logs a message to the terminal. It is
your responsibility to integrate with the proper system after
generation.

### Tracking sessions

All sessions and tokens are tracked in a separate table. This
allows you to track how many sessions are active for each account.
You could even expose this information to users if desired.

Note that whenever the password changes (either via reset password
or directly), all tokens are deleted and the user has to login
again on all devices.

### Enumeration attacks

An enumeration attack allows an attacker to enumerate all e-mails
registered in the application. The generated authentication code
protect against enumeration attacks on all endpoints, except in
the registration and update e-mail forms. If your application is
really sensitive to enumeration attacks, you need to implement
your own registration workflow, which tends to be very different
from the workflow for most applications.

### Case sensitiveness

The e-mail lookup is made to be case insensitive. Case insensitive
lookups are the default in MySQL and MSSQL but require the
citext extension in Postgres.

### Concurrent tests

The generated tests run concurrently if you are using a database
that supports concurrent tests (Postgres or MSSQL with snapshot
isolation level).

[josevalim]

All tests are in. I am officially done!

[aaronrenner]

@josevalim This is awesome! I'll continue putting this into generators today. If you don't mind, I'll continue submitting small changes to this PR as I find issues in order to keep the diff between what the generators create and this branch as small as possible.

[josevalim]

@aaronrenner the PRs are very welcome. I have also updated the documentation text above. It is probably better to not copy the text for now but rather at the end, when we will be sure it will stop changing. :)

[justinmayhew]

Thanks @josevalim and team for the interesting blog post and open sourcing this!

I have one small nitpick: I think it'd be a good idea to rename "encrypt" to "hash" everywhere we're referring to a hashed password.

[josevalim]

Good catch @mayhewj! It has been fixed.

[markevans]

Thanks for the work on this this is fantastic (and very timely for me!)

Just a general question (probably to @aaronrenner ) - for mix phx.gen.auth will you be separating the generator into chunks of functionality, e.g.

• registration
• login
• password reset
• remember me

Maybe there's a better separation than the above, but certainly as a user of this, it would help me understand what I was doing more if I could generate in chunks, rather than one big generator that did everything at once. Also I may/may not need all of the chunks.

Of course having a generator that did everything at once as well would be good. Just a thought.

[josevalim]

Unfortunately breaking it apart introduces a lot of complexity because you can't really compose an authentication system from individual parts. For example, resetting the password also confirms the account. If we break the generator apart, now we need to be amending code so depending on the order you add password reset and confirmation, everything still works out the same.

There is a lot of benefit in simplicity and that's what we are going for. :)

[markevans]

Unfortunately breaking it apart introduces a lot of complexity because you can't really compose an authentication system from individual parts. For example, resetting the password also confirms the account. If we break the generator apart, now we need to be amending code so depending on the order you add password reset and confirmation, everything still works out the same.

There is a lot of benefit in simplicity and that's what we are going for. :)

makes sense - thanks for the reply 👍

[joshchernoff]

What are your thoughts about padding the password to its max length to attempt to address the TLS Bicycle Attack? https://en.wikipedia.org/wiki/Bicycle_attack I realize most brute force attacks could be mitigated by fail2ban and or rate limits so I wonder if its even worth trying to address the TLS Bicycle Attack.

[josevalim]

@joshchernoff yeah. There are some things that are out of the scope for this PR because they come with their own complexity, such as rate limiting and client-side behaviour (strength checks, padding, etc).

[aaronrenner]

@josevalim Regarding this section of the docs

Concurrent tests

The generated tests run concurrently if you are using a database that supports concurrent tests (Postgres or MSSQL with snapshot isolation level).

Is MSSQL's snapshot isolation level something that should be enabled by default in new phoenix applications? If so, I'd be happy to submit a PR to phoenix to that adds it to config/test.exs by default.

config :demo_mssql, DemoMssql.Repo,
  username: "sa",
  password: "some!Password",
  database: "demo_mssql_test#{System.get_env("MIX_TEST_PARTITION")}",
  hostname: "localhost",
  pool: Ecto.Adapters.SQL.Sandbox,
+ set_allow_snapshot_isolation: :on

[josevalim]

Good catch! Please do send a PR to Phoenix. :)

[aaronrenner]

@josevalim I'm getting some deadlock errors when running tests with the Ecto.Adapters.Tds adapter, set_allow_snapshot_isolation: :on and async: true.

       1) test GET /users/confirm/:token confirms the given token once (DemoMssqlWeb.UserConfirmationControllerTest)
          test/demo_mssql_web/controllers/user_confirmation_controller_test.exs:59
          ** (Tds.Error) Line 1 (Error 1205): Transaction (Process ID 61) was deadlocked on lock resources with another process and has been chosen as the deadlock victim. Rerun the transaction.
          code: conn = get(conn, Routes.user_confirmation_path(conn, :confirm, token))
          stacktrace:
            (ecto_sql 3.4.2) lib/ecto/adapters/sql.ex:612: Ecto.Adapters.SQL.raise_sql_call_error/1
            (ecto_sql 3.4.2) lib/ecto/adapters/sql.ex:545: Ecto.Adapters.SQL.execute/5
            (ecto 3.4.0) lib/ecto/multi.ex:606: Ecto.Multi.apply_operation/4
            (ecto 3.4.0) lib/ecto/multi.ex:585: Ecto.Multi.apply_operation/5
            (elixir 1.10.2) lib/enum.ex:2111: Enum."-reduce/3-lists^foldl/2-0-"/3
            (ecto 3.4.0) lib/ecto/multi.ex:569: anonymous fn/5 in Ecto.Multi.apply_operations/5
            (ecto_sql 3.4.2) lib/ecto/adapters/sql.ex:894: anonymous fn/3 in Ecto.Adapters.SQL.checkout_or_transaction/4
            (db_connection 2.2.1) lib/db_connection.ex:1427: DBConnection.run_transaction/4
            (ecto 3.4.0) lib/ecto/repo/transaction.ex:20: Ecto.Repo.Transaction.transaction/4
            (demo_mssql 0.1.0) lib/demo_mssql/accounts.ex:277: DemoMssql.Accounts.confirm_user/1
            (demo_mssql 0.1.0) lib/demo_mssql_web/controllers/user_confirmation_controller.ex:31: DemoMssqlWeb.UserConfirmationController.confirm/2
            (demo_mssql 0.1.0) lib/demo_mssql_web/controllers/user_confirmation_controller.ex:1: DemoMssqlWeb.UserConfirmationController.action/2
            (demo_mssql 0.1.0) lib/demo_mssql_web/controllers/user_confirmation_controller.ex:1: DemoMssqlWeb.UserConfirmationController.phoenix_controller_pipeline/2
            (phoenix 1.5.0-dev) lib/phoenix/router.ex:352: Phoenix.Router.__call__/2
            (demo_mssql 0.1.0) lib/demo_mssql_web/endpoint.ex:1: DemoMssqlWeb.Endpoint.plug_builder_call/2
            (demo_mssql 0.1.0) lib/demo_mssql_web/endpoint.ex:1: DemoMssqlWeb.Endpoint.call/2
            (phoenix 1.5.0-dev) lib/phoenix/test/conn_test.ex:225: Phoenix.ConnTest.dispatch/5
            test/demo_mssql_web/controllers/user_confirmation_controller_test.exs:65: (test)
     
     21:41:27.792 [error] Tds.Protocol (#PID<0.405.0>) disconnected: ** (DBConnection.TransactionError) transaction is not started
     
     
       2) test PUT /users/reset_password/:token resets password once (DemoMssqlWeb.UserResetPasswordControllerTest)
          test/demo_mssql_web/controllers/user_reset_password_controller_test.exs:77
          ** (Tds.Error) Line 1 (Error 1205): Transaction (Process ID 67) was deadlocked on lock resources with another process and has been chosen as the deadlock victim. Rerun the transaction.
          code: put(conn, Routes.user_reset_password_path(conn, :update, token), %{
          stacktrace:
            (ecto_sql 3.4.2) lib/ecto/adapters/sql.ex:612: Ecto.Adapters.SQL.raise_sql_call_error/1
            (ecto_sql 3.4.2) lib/ecto/adapters/sql.ex:545: Ecto.Adapters.SQL.execute/5
            (ecto 3.4.0) lib/ecto/multi.ex:606: Ecto.Multi.apply_operation/4
            (ecto 3.4.0) lib/ecto/multi.ex:585: Ecto.Multi.apply_operation/5
            (elixir 1.10.2) lib/enum.ex:2111: Enum."-reduce/3-lists^foldl/2-0-"/3
            (ecto 3.4.0) lib/ecto/multi.ex:569: anonymous fn/5 in Ecto.Multi.apply_operations/5
            (ecto_sql 3.4.2) lib/ecto/adapters/sql.ex:894: anonymous fn/3 in Ecto.Adapters.SQL.checkout_or_transaction/4
            (db_connection 2.2.1) lib/db_connection.ex:1427: DBConnection.run_transaction/4
            (ecto 3.4.0) lib/ecto/repo/transaction.ex:20: Ecto.Repo.Transaction.transaction/4
            (demo_mssql 0.1.0) lib/demo_mssql/accounts.ex:345: DemoMssql.Accounts.reset_user_password/2
            (demo_mssql 0.1.0) lib/demo_mssql_web/controllers/user_reset_password_controller.ex:36: DemoMssqlWeb.UserResetPasswordController.update/2
            (demo_mssql 0.1.0) lib/demo_mssql_web/controllers/user_reset_password_controller.ex:1: DemoMssqlWeb.UserResetPasswordController.action/2
            (demo_mssql 0.1.0) lib/demo_mssql_web/controllers/user_reset_password_controller.ex:1: DemoMssqlWeb.UserResetPasswordController.phoenix_controller_pipeline/2
            (phoenix 1.5.0-dev) lib/phoenix/router.ex:352: Phoenix.Router.__call__/2
            (demo_mssql 0.1.0) lib/demo_mssql_web/endpoint.ex:1: DemoMssqlWeb.Endpoint.plug_builder_call/2
            (demo_mssql 0.1.0) lib/demo_mssql_web/endpoint.ex:1: DemoMssqlWeb.Endpoint.call/2
            (phoenix 1.5.0-dev) lib/phoenix/test/conn_test.ex:225: Phoenix.ConnTest.dispatch/5
            test/demo_mssql_web/controllers/user_reset_password_controller_test.exs:79: (test)
     
     ........
     
       3) test POST /users/confirm sends a new confirmation token (DemoMssqlWeb.UserConfirmationControllerTest)
          test/demo_mssql_web/controllers/user_confirmation_controller_test.exs:22
          ** (Tds.Error) Line 1 (Error 1205): Transaction (Process ID 64) was deadlocked on lock resources with another process and has been chosen as the deadlock victim. Rerun the transaction.
          code: assert Repo.get_by!(Accounts.UserToken, user_id: user.id).context == "confirm"
          stacktrace:
            (ecto_sql 3.4.2) lib/ecto/adapters/sql.ex:612: Ecto.Adapters.SQL.raise_sql_call_error/1
            (ecto_sql 3.4.2) lib/ecto/adapters/sql.ex:545: Ecto.Adapters.SQL.execute/5
            (ecto 3.4.0) lib/ecto/repo/queryable.ex:192: Ecto.Repo.Queryable.execute/4
            (ecto 3.4.0) lib/ecto/repo/queryable.ex:17: Ecto.Repo.Queryable.all/3
            (ecto 3.4.0) lib/ecto/repo/queryable.ex:120: Ecto.Repo.Queryable.one!/3
            test/demo_mssql_web/controllers/user_confirmation_controller_test.exs:30: (test)
     
     .21:41:30.315 [error] GenServer #PID<0.615.0> terminating
     ** (RuntimeError) Ecto SQL sandbox transaction was already committed/rolled back.
     
     The sandbox works by running each test in a transaction and closing thetransaction afterwards. However, the transaction has already terminated.Your test code is likely committing or rolling back transactions manually,either by invoking procedures or running custom SQL commands.
     
     One option is to manually checkout a connection without a sandbox:
     
         Ecto.Adapters.SQL.Sandbox.checkout(repo, sandbox: false)
     
     But remember you will have to undo any database changes performed by such tests.
     
         (ecto_sql 3.4.2) lib/ecto/adapters/sql/sandbox.ex:517: Ecto.Adapters.SQL.Sandbox.pre_checkin/4
         (db_connection 2.2.1) lib/db_connection/ownership/proxy.ex:221: DBConnection.Ownership.Proxy.pool_done/5
         (stdlib 3.10) gen_server.erl:637: :gen_server.try_dispatch/4
         (stdlib 3.10) gen_server.erl:711: :gen_server.handle_msg/6
         (stdlib 3.10) proc_lib.erl:249: :proc_lib.init_p_do_apply/3
     Last message: {:DOWN, #Reference<0.2394866164.2520252417.235517>, :process, #PID<0.614.0>, :shutdown}
     .....................................................
     
     Finished in 13.9 seconds
     99 tests, 3 failures
     
     Randomized with seed 858931

It's also happening in CI here: https://github.com/aaronrenner/phx_gen_auth/runs/566396334?check_suite_focus=true#step:9:135. In order to try this out you can

1. check out this branch: https://github.com/aaronrenner/phx_gen_auth/tree/ar-mssql-async
2. start the sql-server docker container: mcr.microsoft.com/mssql/server:2019-latest
3. run mix test --trace test/mix/tasks/phx_gen_auth/integration_test.exs:104 (the test app is generated in test_apps/demo_mssql if you want to explore it)

Any thoughts on how to easily resolve this, or should we hold off on automatically adding async: true to tests for mssql?

[josevalim]

Any thoughts on how to easily resolve this, or should we hold off on automatically adding async: true to tests for mssql?

Yes, let's hold off. :)

[ghenry]

Is this ready for playing with then?

[samgaw]

I've been on fence since @josevalim’s blog post whether or not to throw this out there. And I still am 😃 but from a security point of view I think there’s genuine benefit in introducing a starting point that covers auth history, specifically login events.

What are the thoughts on adding either an additional table, or storing it as an embedded schema alongside User, to track the last n auth attempts along the lines of:

%{
  seen_at: :utc_datetime
  ip_addr: :string
  success: :boolean
}

There had been previous discussion around rate-limiting & fail2ban to protect against brute force attacks, which I agree is outside of the scope for standardised auth. However that only covers so much, and a user is a much better source for providing security context than either e.g. I did login from that ASN once but I don’t recognise the 2 sessions associated with that ASN.

Likewise if the session was linked to a history event, either by storing the IP with the session or referencing the session id in the history record; the developer can give the user an option of killing a session from an unrecognised network.

Happy to do the leg work based on feedback and push up if there’s any interest in.

[josevalim]

Hi @samgaw! As with other feature requests, I believe this is generally very useful, but my goal with this PR and my idea for the generators is to provide a minimum working set.

Hopefully this PR will also help move the discussion forward, so in the future we won't be talking about how to setup auth, but rather how to expand auth with more strict concerns, such as rate-limiting and the valuable topics you have brought up above.

Thanks!

[samgaw]

Yeah, that's why I kept going back and forth on whether to suggest it. I didn't want to make you have to explain what minimum is again 😞

Cheers!

[ghenry]

Should a general starter audit log be added? That's what you mean @samgaw right?

I did a show on this and it's a massive value add out of the box.

https://www.se-radio.net/2019/10/episode-383-neil-madden-on-securing-your-api/

[samgaw]

@ghenry Exactly. It's the basis for brute force rate limiting, user visibility, session revoking etc. My thinking was that if there'a a common initial structure, more advanced lib adoption gets much easier. But I've read enough on @josevalim's responses to increased scope to appreciate where he's coming from and trust his judgement.

What I wasn't sure about was whether it should be an array on the User row, or a separate table. But implementation wise it's pretty simple to have flexibility and throw it in after the fact:

def drop_audit_event(list, key, limit \\ 10, forget \\ 1) do
  case list_size = Enum.count(list) do
    ^list_size when list_size > limit ->
      drop_by = (list_size - limit) + forget
      Enum.sort_by(list, &(&1[key]), {:asc, Date})
      |> Enum.drop(drop_by)

    ^list_size when list_size == limit ->
      Enum.sort_by(list, &(&1[key]), {:asc, Date})
      |> Enum.drop(forget)

    _ ->
      list
  end
end

iex > history = [
  %{seen_at: ~U[2020-04-09 09:56:56Z], ip_addr: {192, 0, 2, 1}, success: true},
  %{seen_at: ~U[2020-04-09 10:07:38Z], ip_addr: {192, 0, 2, 1}, success: true},
  %{seen_at: ~U[2020-04-08 07:01:34Z], ip_addr: {192, 0, 2, 55}, success: true},
  %{seen_at: ~U[2020-04-09 10:07:38Z], ip_addr: {10, 0, 0, 2}, success: false},
  %{seen_at: ~U[2020-04-09 10:07:38Z], ip_addr: {192, 0, 2, 3}, success: true}
]

iex > drop_audit_event(history, :seen_at, 5)
[
  %{ip_addr: {192, 0, 2, 1}, seen_at: ~U[2020-04-09 09:56:56Z], success: true},
  %{ip_addr: {192, 0, 2, 1}, seen_at: ~U[2020-04-09 10:07:38Z], success: true},
  %{ip_addr: {10, 0, 0, 2}, seen_at: ~U[2020-04-09 10:07:38Z], success: false},
  %{ip_addr: {192, 0, 2, 3}, seen_at: ~U[2020-04-09 10:07:38Z], success: true}
]

[josevalim]

@samgaw the app I am working has auditing built on top of this and it is done with a separate table. We will probably write about it, but I am less confident of having a general structure that works for mostly everyone when it comes to auditing.

[ghenry]

@samgaw the app I am working has auditing built on top of this and it is done with a separate table. We will probably write about it, but I am less confident of having a general structure that works for mostly everyone when it comes to auditing.

Yeah, it's a very tricky thing to generalise. I presume it's easy to create Plug anyway for these?

[josevalim]

I am personally not working at the plug level, as the auditing is tied to actions happening within the app. If you are only loading a page with non sensitive information, nothing is "audited".

[ijunaid8989]

when this will be merged and be available to use?

[josevalim]

It will never be merged. You can use this repo and merge the PR locally if you want. Or use the generator done by @aaronrenner: https://github.com/aaronrenner/phx_gen_auth/

[joshchernoff]

Since I've seen you added some logic for LiveView I hope its appropriate to ask this question here.

I'm working on a simple Blog using Elixir / Phoenix. I'm in the process of converting my Blog's show view into a LiveView.

Before converting it to a LiveView the fetch_current_user plug worked as you would expect. In the controller of my Post show.

I would pluck the current_user from the assigns and pass it along to my Blog context when fetching the post. In the context I would then use Bodyguard as a way to gate non-published (IE draft) posts from non admin users. Nothing fancy.

Something simple like so.

Post
    |> Repo.by_slug(slug)
    |> from(preload: ^preload)
    |> Bodyguard.scope(current_user)
    |> Repo.one!()

Now that I'm converting the show view to a LiveView and I'm no longer pulling the current_user from the assigns in the controller. Instead in the mount of my LiveView for the show I see I have access to the user_token via the session which I can use to look up the user at that moment. What I'm observing at the moment is that the fetch_current_user (which I still need for the other views) is doing the work of looking up the user of the user_token only to have LiveView do it all over again resulting in two user queries. At this point I started to think that I may want to have different pipelines for LiveView vs standard views.

My question is: What guidance can you given someone reading this pr about how to fetch the current_user in LiveView.

[josevalim]

First of all, you are extremely correct that when moving to LiveView, all of the authentication/authorization should be moved to mount.

Regarding the user, keep in mind that the LiveView receives on mount the same session as the connection, so you only need to do this:

if user = session["user_token"] && Accounts.get_user_by_session_token(session["user_token"]) do
  assign(socket, current_user: user)
else
  redirect(socket, to: Routes.user_session_path(socket, :new)
end

[stefanchrobot]

@josevalim

Regarding the user, keep in mind that the LiveView receives on mount the same session as the connection, so you only need to do this

Is there a way to have a kind of "plug" for this so as not to repeat this in every mount/3?

[michalmuskala]

Were there any thoughts about some simple hashing of passwords client-side before sending them to server?

The primary motivation in here wouldn't be additional security, but preventing cleartext passwords from leaking in transit - logged in some file or appearing in some error report or crashdump. In general carefully checking all the places so that does not happen is very error prone.