Skip to content
This repository has been archived by the owner on Jul 30, 2024. It is now read-only.

Remove password hashing for each token login #753

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

LOUISLCE
Copy link

@LOUISLCE LOUISLCE commented Feb 9, 2018

Hi,
The token request loader was critically slowing down login process according to #731 and #740 .

This remove hashing the user password at every request, which is pretty useless given that
the token is already signed by the serializer (and as such is hard to fake).

Thanks!

@@ -247,7 +247,7 @@ def _request_loader(request):
data = _security.remember_token_serializer.loads(
token, max_age=_security.token_max_age)
user = _security.datastore.find_user(id=data[0])
if user and verify_hash(data[1], user.password):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the reason to call verify_hash was to make sure that the token is invalidated once a user changes her password. Can you propose any solution that allows users to invalidate the remember token?

@briancappello
Copy link
Contributor

just thinking out loud here, i wonder if it could work to store verified tokens in the database, and whenever the user changes their password, clear out the cache. so whenever User.get_auth_token() gets called, it adds the token to the cache on the model, unless max age is None and a token already exists, in which case no need to generate a new one. and in the request loader, we're already doing a database lookup anyway, so any extra work should be minimal compared to the time saved in the most common case (ie that the hash has already been verified). it seems to me the most brittle part is the fact that the user model can't commit itself, and thus any code that calls get_auth_token needs to remember to commit the user instance (altho even in the case that that doesn't happen, the worst that happens would be a fallback to the current behaviour, ie verifying the hash every time)

@jwag956
Copy link
Collaborator

jwag956 commented May 8, 2019

Please see some ideas in: #771

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

4 participants