Auth #1
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,20 +1,40 @@ | ||
| # Demo | ||
|
|
||
| ## Spec | ||
|
|
||
| We will have two database tables: "users" and "users_tokens": | ||
|
|
||
| * "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. 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. 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. 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. 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. :) |
||
|
|
||
| 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. | ||
|
|
||
| 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. | ||
|
|
||
| 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.
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. 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. 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. Right, that's why I was going with Bcrypt too. Have they updated their recommendation at all? There was a problem hiding this comment. 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. 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. Argon2 is recommended here. Any reason not to use it as the default? There was a problem hiding this comment. There was a problem hiding this comment. 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. It seems that the recommendation is still
There was a problem hiding this comment. 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.
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.
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. Someone is welcome to implement There was a problem hiding this comment. 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.authdocs OR added to the module docs of the related modules, especially theuseranduser_tokenones. 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
@moduledocis something that I wanted to suggest after runningmix phx.gen.authin 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.