Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refine authenticate_by security guarantee [ci-skip] #43997

Merged

Conversation

jonathanhefner
Copy link
Member

@jonathanhefner jonathanhefner commented Dec 25, 2021

We are not in a position to guarantee exactly how long authenticate_by will take. For example, if the find_by query is not backed by an index, the database will need to perform a full table scan, and the query time will vary based on where the record is in the heap, if it is found at all. Therefore, we should be more specific about what we guarantee.

@consu
Copy link

consu commented Dec 25, 2021

in case of a failed login attemp you should avoid:

  • increasing a failed attempt counter, for existing users
  • create additional logging, for existing users
  • load an existing user to execute additional actions, like mailings
  • start displaying captachas, only if the user exists
  • don't use expensive default_scope
  • don't use a before action, that loads the specific user in every case
  • check your after_initialize callbacks, if they
  • if you increasing BCrypt::Engine.cost, you need to rehash the password, creating a new user will take more time, in this case.
  • this is only tested for bcrypt, so don't override has_secure_password ...

We are not in a position to guarantee exactly how long `authenticate_by`
will take.  For example, if the `find_by` query is not backed by an
index, the database will need to perform a full table scan, and the
query time will vary based on where the record is in the heap, if it is
found at all.  Therefore, we should be more specific about what we
guarantee.
@jonathanhefner jonathanhefner changed the title Document database query caveat for authenticate_by [ci-skip] Refine authenticate_by security guarantee [ci-skip] Dec 25, 2021
@jonathanhefner
Copy link
Member Author

Thank you for the feedback, @consu! Those are good points to keep in mind, though I think many of them fall outside the scope of authenticate_by's API documentation. Still, in the interest of managing expectations, I have removed all mention of "same amount of time" from the documentation.

@jonathanhefner jonathanhefner merged commit 94c28ac into rails:main Dec 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants