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

Improve remember token logins #334

Merged
merged 1 commit into from Aug 16, 2013

Conversation

Projects
None yet
3 participants
@gylaz
Copy link
Contributor

gylaz commented Aug 9, 2013

  • Don't allow blank remember_token (Fixes #333)
  • Add validation of remember_token to avoid blanks
  • Rename denies_controller_spec.rb -> permissions_controller_spec.rb
Clearance.configuration.user_model.find_by_remember_token token
if remember_token.present?
@current_user ||= Clearance.configuration.user_model.
where(remember_token: remember_token).first

This comment has been minimized.

@croaky

croaky Aug 9, 2013

Contributor

Maybe break each of these dots onto their own lines or extract another private method for it like user_from_remember_token.

@@ -49,6 +49,7 @@ module Validations
unless: :email_optional?

validates :password, presence: true, unless: :skip_password_validation?
validates :remember_token, presence: true

This comment has been minimized.

@croaky

croaky Aug 9, 2013

Contributor

This will mean in suites where users existed before Clearance, the developer will need to backfill remember_tokens. Probably worth creating an example migration in the wiki so people know how to do that.

This comment has been minimized.

@gylaz

gylaz Aug 11, 2013

Contributor

Not sure if this is really necessary, but I added this to match the db constraint.

If we leave this in: Is this something that needs to be generated during clearance install, if the users table exists?

This comment has been minimized.

@gylaz

gylaz Aug 16, 2013

Contributor

I'm going to remove this line from this PR and open a new PR that addresses the issue.

@@ -17,7 +17,7 @@
post :create, session: { email: user.email, password: user.password }

expect(response).to render_template(:new)
expect(flash[:notice]).to match /^Bad email or password/
expect(flash[:notice]).to match(/^Bad email or password/)

This comment has been minimized.

@croaky

croaky Aug 9, 2013

Contributor

Yup. You a syntastic user?

This comment has been minimized.

@gylaz

gylaz Aug 9, 2013

Contributor

Haha! I'm trying it out.

@croaky

This comment has been minimized.

Copy link
Contributor

croaky commented Aug 9, 2013

Couple of comments but otherwise looks good if Travis passes it. Would like another thoughtbot dev to take a look at it as well.

@JoelQ

This comment has been minimized.

Copy link
Contributor

JoelQ commented Aug 16, 2013

Looks good to me 👍

Don't allow logins with blank remember_token
* Rename denies_controller_spec.rb -> permissions_controller_spec.rb
* Fixes #333

@gylaz gylaz merged commit 932dac9 into master Aug 16, 2013

1 check was pending

default The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment