Skip to content

Conversation

LilyFirefly
Copy link

Add to TokenAuthentication.authenticate_credentials.

Add to `TokenAuthentication.authenticate_credentials`.
@LilyFirefly
Copy link
Author

@meshy @kevinetienne Review?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 9ff3f29 on update-last-login into 9ad71ad on master.

@meshy
Copy link
Contributor

meshy commented Apr 7, 2015

👍

@meshy
Copy link
Contributor

meshy commented Apr 7, 2015

Looks good to me. Are there any other similar signals that we should be connecting?

@LilyFirefly
Copy link
Author

Perhaps user_login_failed and user_logged_out.

@meshy
Copy link
Contributor

meshy commented Apr 7, 2015

🆒

@LilyFirefly
Copy link
Author

I think there's no good reason to connect user_login_failed. The credentials argument it expects is cleaned by django to avoid leaking sensitive information. But the only information we have to pass to user_login_failed is the token, which is sensitive.

@meshy
Copy link
Contributor

meshy commented Apr 7, 2015

Surely there would be no token on a login attempt, only an email and password?

* Obtaining an Auth Token corresponds more closely to logging in than
  using an Auth Token.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling b2fe7b3 on update-last-login into 9ad71ad on master.

@LilyFirefly
Copy link
Author

Updated. I didn't need to add user_login_failed, because the authenticate function is called in AuthTokenSerializer.validate.

@LilyFirefly LilyFirefly added the bug label Apr 7, 2015
@LilyFirefly LilyFirefly self-assigned this Apr 7, 2015
@meshy
Copy link
Contributor

meshy commented Apr 7, 2015

Looks good to me :) Happy when travis is

LilyFirefly pushed a commit that referenced this pull request Apr 7, 2015
Connect user_logged_in signal
@LilyFirefly LilyFirefly merged commit 0dac76b into master Apr 7, 2015
@LilyFirefly LilyFirefly deleted the update-last-login branch April 7, 2015 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants