-
Notifications
You must be signed in to change notification settings - Fork 16
Auth #1
base: master
Are you sure you want to change the base?
Auth #1
Changes from 68 commits
65f49a9
e84f9be
eff8376
d517b54
64ed8f9
d4ea2b4
b677abc
1befca8
6625019
153b7ce
fe52685
3e30e6d
5a2f0ed
d3b7a93
653d4ed
ba4b093
c157742
f137822
5b550b8
e7dd9e1
4e02461
39ccccb
3558bac
bdef99d
e61a7aa
fc2c735
76ff10a
44a6bac
7c07c02
085dd92
0e3efd9
fcf4a8b
9113466
0712d9c
9c22870
19470c8
4796235
f801205
2a7616a
b53f616
6049142
97bd003
7f3a55e
f698bc8
ef46ae1
d3aca05
326db9c
4abf899
f3a1571
8655c00
5fbf0ba
13de852
6637c85
ead269e
c73cba4
01194c3
288c67a
9b1a270
cd2ab05
9912c79
25d083d
fbd52ec
ecc8eb5
6ae63ab
e51d52a
bb13f6b
1eebcae
4113391
2b92a22
3003342
6f2ef5d
83d5deb
78cb46e
57bf7df
ed0e210
4664c37
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,20 +1,40 @@ | ||
# Demo | ||
|
||
To start your Phoenix server: | ||
## Spec | ||
|
||
* Install dependencies with `mix deps.get` | ||
* Create and migrate your database with `mix ecto.setup` | ||
* Install Node.js dependencies with `npm install --prefix assets` | ||
* Start Phoenix endpoint with `mix phx.server` | ||
We will have two database tables: "users" and "users_tokens": | ||
|
||
Now you can visit [`localhost:4000`](http://localhost:4000) from your browser. | ||
* "users" will have the "hashed_password", "email" and "confirmed_at" fields plus timestamps | ||
* "users_tokens" will have "token", "context", "sent_to", "inserted_at" fields | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have you considered moving away from pre-required/specific database tables? Consider the scenario where the user table already exists, so all they need to do is to add the missing columns and use your logic. This would virtually enable absolute flexibility and stay out of the end-user/dev way. For example, the way I look at this current implementation is that the spec assumes there will be a password always (which is not true). For example, you could consider replacing the I recently worked on a passwordless authentication solution, and it's unfortunate that most (if not all) the existing authentication systems are strictly following patterns that will always stay in your developer way and most probably enforce some pretty painful UX on the end-user side. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I personally have no interest in covering all possible use cases. :) I have done that, it didn't work. I intend this solution to be pretty much as is. If you want to move away from what it does, you can either:
In regards to JWT and in general non DB-backed tokens, we discussed this extensively, and we believe that DB tokens are more secure and provide better UI, as they allow you to expire and track them individually or collectively. In any case, I don't guarantee this generator provides the best UI possible, if you believe that passwordless is the best way to go for certain apps, then I look forward to a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for explaining this @josevalim I arrived here having in mind Devise as a context. I'm glad this is a more specific solution vs. all-case one. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, in case you didn't see it, there is more context here: https://dashbit.co/blog/a-new-authentication-solution-for-phoenix I was not expecting folks to find this PR outside of the blog post. :) |
||
|
||
Ready to run in production? Please [check our deployment guides](https://hexdocs.pm/phoenix/deployment.html). | ||
On sign in, we need to renew the session and delete the CSRF token. The password should be limited to 80 characters, the email to 160. | ||
|
||
## Learn more | ||
Confirmation, resetting passwords, remember me, and session management will be all based on tokens. Tokens are generated randomly using `:crypto.strong_rand_bytes/1` and then hashed using sha2/sha3. The hashing helps protect against timing attacks. The hashed token is the one stored in the database, the original token is not stored. | ||
|
||
* Official website: https://www.phoenixframework.org/ | ||
* Guides: https://hexdocs.pm/phoenix/overview.html | ||
* Docs: https://hexdocs.pm/phoenix | ||
* Forum: https://elixirforum.com/c/phoenix-forum | ||
* Source: https://github.com/phoenixframework/phoenix | ||
Whenever a token is generated, a context has to be given (such as "session", "reset", etc). The context is used to avoid one token being used against its original purpose. Verifying the token will hash the token and look-up its hashed result in the database. Verification also takes a ttl. The current date is compared against the token `inserted_at + ttl` and the token is deemed as expired if enough time has passed. | ||
|
||
For confirming e-mail addresses, we will generate a token, and e-mail it to the user. We will store the hashed token, the context ("confirm"), and store the confirmation e-mail under the `sent_to` column. Once the user clicks the link, we will verify the token, and see if the current user e-mail matches the one in the `sent_to` column. Once the token is used, it is deleted. Note we won't automatically sign the user in after confirmation - this protects us from someone getting access to the account via confirmation tokens. This also allows us to set long expiry dates in the tokens. | ||
|
||
For resetting the password, the process is very similar. Once the link is clicked, it will go to the reset password page. The reset password page will ask for the new password and for the password confirmation. Once the user sends the form, we will verify the token and proceed to reset the password. Resetting the password will delete all tokens. Generally speaking, changing the password always deletes all tokens. Resetting the password won't sign the user in - so if anyone intercepts the token, they cannot gain access to the system as they are missing the e-mail/username. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Hi @josevalim. This is super helpful! I just want a bit of clarification here: if we're worried about the token being leaked (for confirmation/password reset) then shouldn't we also be worried that we're advocating sending this token in the body of plain text email with a relatively long expiry (1 week)? We can never guarantee secure transmission of email, and in the case of an intercepted email an attacker would also have access to the email address, so that is enough data for an account takeover using the password reset functionality as they have the 2 bits of info required (token + email) and can set their own password and login immediately regardless of whether we do it for them or not. So I guess my questions are: a) why would we take the UX-hit of not logging users in automatically given the most likely token leak scenario is via email, where the attacker would have access to the email address anyway? b) would it be wise to shorten the token expiry here, to reduce the interception window? Django uses a default of 3 days, and allows automatic login after reset but both are configurable. Laravel uses a much stricter one hour window, but also automatically auths after resets. I guess you could say people can change it but the wording of the text above: "This also allows us to set long expiry dates in the tokens." seems to suggest people should push for longer token expiry. It's late here and I may have missed something super obvious but thought I'd ask anyway. Thanks! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think those are good considerations. I will reduce the initial period is a safe call. Thanks! |
||
|
||
For changing the e-mail, the user will put the new e-mail in a field. This will create a new token with change:CURRENT_EMAIL as context and the new e-mail stored in the `sent_to` column. A hashed token will be sent to the new e-mail. Once the user clicks on the e-mail link, the raw token will be hashed and we will change the user to the new e-mail as long as the hashed token and old email match to the currently signed in user. Once this is done, the token is deleted. This token will have a short expiry date by default. | ||
|
||
## Pending for generators | ||
|
||
mix phx.gen.auth Account User users ...extrafields... | ||
|
||
The authentication mechanism should be an option. Default to bcrypt for Unix systems, pdkdf2 for Windows systems. The line to config/test.exs must always be added. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Though all are decent, Argon2(id) might be the better choice for new platforms. Bcrypt is starting to show flaws (and Pbkdf2 has been the weaker choice for a long time now). If I didn't conform to NIST/OWASP recommendations for Pow, then I would have gone with Argon2 as the default. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, that's why I was going with Bcrypt too. Have they updated their recommendation at all? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh I see. Then this is good. AFAIK bcrypt is still default recommendation by OWASP and Pbkdf2 or Balloon by NIST. |
||
|
||
## License | ||
|
||
Copyright 2020 Dashbit | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at [http://www.apache.org/licenses/LICENSE-2.0](http://www.apache.org/licenses/LICENSE-2.0) | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,3 +20,6 @@ config :demo, DemoWeb.Endpoint, | |
|
||
# Print only warnings and errors during test | ||
config :logger, level: :warn | ||
|
||
# Only in tests, remove the complexity from the password hashing algorithm | ||
config :bcrypt_elixir, :log_rounds, 1 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should support all three comeonin adapters. We should default to bcrypt except on Windows which should use pkbdf2. Each adapter will require a different line here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Argon2 is recommended here. Any reason not to use it as the default? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I am aware folks will have different opinions and takes on the password hashing, that's why I prefer to follow recommendations from OWASP and similar and, AFAIK, it is still bcrypt. If someone has any updated insights or data on that, I would love to hear. :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems that the recommendation is still
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. any thoughts on passwordless auth? it solves a lot of hassle in pass world like reset, change, forgot, keeps email as source of truth and solve password hashing issues There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In my opinion, passwordless auth should be avoided most of the time. I guess it depends on your user base, but in my experience, many people use their work email to sign up for services, even on non work related sites. Unless you can verify their identity some other way during sign up, that means they will lose access to their accounts when they change jobs, since you can't trust email update requests from any other email address. This is especially important if your project falls under GDPR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
even in that cenario, its safer passwordless since the emails belongs to the company and that could be abused by bad actors, I manage and managed the email myself a couple of times, its usual to help someone to recover stuff left in company emails they dont own anymore, passwords dont solve this issue, specially for car rentals, and some other kind of stuff they get important purchase info by mail, the mistake is to use a email you dont own, and when you know is passwordless you think twice to use one you do not own. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Someone is welcome to implement There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. any opinions on https://github.com/zanderxyz/veil? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where do you see this spec going? Will it be in the Readme of phx_gen_auth, in the moduledoc of generated code, or just in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I originally thought it would only be part of this PR but I think you will be more capable of assessing it. If you feel like there is important information in here, then maybe some of it should be added to
mix phx.gen.auth
docs OR added to the module docs of the related modules, especially theuser
anduser_token
ones. WDYT?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. I'll look for a good place to put it. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding this as
@moduledoc
is something that I wanted to suggest after runningmix phx.gen.auth
in my project. It would make it so much easier to read through the code, especially considering the fact that when it's generated via mix it's a single commit. I personally found it a bit hard to figure out where to start reading the generated code.