Skip to content
This repository

Issue testing password change implementation using FactoryGirl #150

Closed
xlprojects opened this Issue November 11, 2011 · 9 comments

2 participants

XL Projects INC. Marc Lainez
XL Projects INC.

Hey there, hope someone can help me.

I've created a change password method for the User model as follow

def update_password(current_password, new_password, new_password_confirmation)
  if new_password == new_password_confirmation
    if !User.authenticate(self.username, current_password).nil?
      return self.change_password!(new_password)
    else
      return false
    end
  else
    return false
  end
end

Here is my User factory

  FactoryGirl.define do
    factory :user do
      first_name            "Bob"
      last_name             "Sinclar"
      date_of_birth         "03/12/1982"
      genre                 "M"
      sequence(:username)   { |n| "bsinclar#{n}" }
      email                 { "#{username}@example.com" }
      password              "secret"
      salt                  "asdasdastr4325234324sdfds"
      activation_state      "active"
      admin                 false

      after_build do |u|
        u.crypted_password = Sorcery::CryptoProviders::BCrypt.encrypt(u.password, u.salt)
      end

      factory :admin_user do
        admin true
      end
    end
  end

Here are my rspec tests

require 'spec_helper'

describe User do
  subject { Factory(:user) }

  describe :update_password do
    let!(:bob) {Factory(:user, password: "password", password_confirmation: "password" )}

    it "updates the password if everything is passed correctly" do
      bob.update_password("password","new_pass","new_pass").should be_true
    end

    it "throws an error if you enter the incorrect current password" do
      bob.update_password("bad_password","new_pass","new_pass").should be_false
    end

    it "throws an error if the new password isn't confirmed properly" do
      bob.update_password("password","new_pass","bad_new_pass").should be_false
    end
  end
end

I obtain F.. but in fact it seems like even if I try to authentify with the password "password", it complains that the password is not valid.
Using pry, I was able to investigate about the user object in the test (here 'bob') and it seems that the salt gets changed upon user creation... I am not sure why or how... This is what I get with some tryouts:

pry(#<RSpec::Core::ExampleGroup::Nested_1::Nested_1>)> bob
=> #<User id: 39, first_name: "Bob", last_name: "Sinclar", date_of_birth: "03/12/1982", genre: "M", username: "bsinclar1", email: "bsinclar1@example.com", crypted_password: "$2a$10$2c6n5H5g7jNpvUX50iXcdeAfOLdaC8TnhcHQddeClkFh...", salt: "7a5c5bfa820ba61284e614c6098ea6709eb385bf", admin: false, created_at: "2011-11-12 04:47:30", updated_at: "2011-11-12 04:47:30", remember_me_token: nil, remember_me_token_expires_at: nil, activation_state: "pending", activation_token: "4c78a2f8f9c157c9dd7706f8aa21749ccd1ba2d9", activation_token_expires_at: nil, reset_password_token: nil, reset_password_token_expires_at: nil, reset_password_email_sent_at: nil, failed_logins_count: 0, lock_expires_at: nil, last_login_at: nil, last_logout_at: nil, last_activity_at: nil>
pry(#<RSpec::Core::ExampleGroup::Nested_1::Nested_1>)> bob.salt
=> "7a5c5bfa820ba61284e614c6098ea6709eb385bf"
pry(#<RSpec::Core::ExampleGroup::Nested_1::Nested_1>)> bob.crypted_password
=> "$2a$10$2c6n5H5g7jNpvUX50iXcdeAfOLdaC8TnhcHQddeClkFhpnm9Zr4MC"
pry(#<RSpec::Core::ExampleGroup::Nested_1::Nested_1>)> Sorcery::CryptoProviders::BCrypt.encrypt("password", bob.salt)
=> "$2a$10$RekZ5aMdXRK8zGKk7N5QQuMXDKzEQ.UGm9LhA2KvQe9jAd58X1RkG"
pry(#<RSpec::Core::ExampleGroup::Nested_1::Nested_1>)> Sorcery::CryptoProviders::BCrypt.encrypt("password", "asdasdastr4325234324sdfds")
=> "$2a$10$RFsoUOAiernoegn1ibftzuF5w.rlVtXZoyTQ9iwHda8jDQb5FUOcq"

I'm quite confused and would appreciate any help to help me debug my test suite... the change password functionality works fine.... I just want it tested thoroughly but FactoryGirl seems to not cope nicely with sorcery... thoughts?

Marc Lainez
Collaborator

Hi,

There is an example in rails with password reset there: https://github.com/NoamB/sorcery-example-app
Maybe you'll find some answers there.

But there is something bothering me in your approach. Why would you want to explicitly set the password salt and crypt your password in your factory. It's done automatically by sorcery in a before save hook, check the code in https://github.com/NoamB/sorcery/blob/master/lib/sorcery/model.rb lines 80 and 172

I'd remove the "salt" line and the "after_build" block from your factory. Can you do that and tell us how it works ?

Good luck

XL Projects INC.

Hey Marc, thanks for your answer. Here the goal is to provide the ability to change their password from their profile page, not to reset it when they forgot their credentials.
In a nutshell, you need to validate that they know their current password and that they confirm their new password.

From the testing rails 3 wiki page, I got that you needed to use the Bcrypt sorcery helper from the fixture...

Per your advice I now have:
user_spec.rb

require 'spec_helper'

describe User do
  subject { Factory(:user) }

  describe :update_password do
    let!(:bob) {Factory(:user)}

    it "updates the password if everything is passed correctly" do
      bob.update_password("password","new_pass","new_pass").should be_true
    end

    it "throws an error if you enter the incorrect current password" do
      bob.update_password("bad_password","new_pass","new_pass").should be_false
    end

    it "throws an error if the new password isn't confirmed properly" do
      bob.update_password("password","new_pass","bad_new_pass").should be_false
    end
  end
end

users.rb (factory)

FactoryGirl.define do
  factory :user do
    first_name            "Bob"
    last_name             "Sinclar"
    date_of_birth         "03/12/1982"
    genre                 "M"
    sequence(:username)   { |n| "bsinclar#{n}" }
    email                 { "#{username}@example.com" }
    password              "password"
    password_confirmation "password"
    activation_state      "active"
    admin                 false

    factory :admin_user do
      admin true
    end
  end
end

but that gives me the same results... I thought maybe it's because I didn't do, in my rspec test:

before(:each) do
  login_user(bob)
end

but when I do that I get:

  1) User update_password updates the password if everything is passed correctly
     Failure/Error: login_user(bob)
     NoMethodError:
       undefined method `login_user' for nil:NilClass
     # ./spec/models/user_spec.rb:10:in `block (3 levels) in <top (required)>'

Not sure what is going wrong here...

Marc Lainez
Collaborator

If it can't find the login_user helper it's probably because you forgot to include Sorcery::TestHelpers::Rails in your spec_helper or in your spec file. But that's not related to the initial problem.

You don't need that login_user in a before each. Try changing:

if !User.authenticate(self.username, current_password).nil?
    return self.change_password!(new_password)

with

if user = User.authenticate(self.username, current_password)
    user.change_password!(new_password)

And I'm pretty sure it will work. The change_password! method is an instance method not a class method, and you need to apply it to the output of your authenticate call.

XL Projects INC.

I do have, in the spec_helper.rb file:

RSpec.configure do |config|
  config.mock_with :rspec
  config.use_transactional_fixtures = true
  config.include Factory::Syntax::Methods
  config.include Sorcery::TestHelpers::Rails
end

The weird thing is that the my update_password method works in the app... I just can't seem to make testing work... I will try what you say but here is my full User model:

class User < ActiveRecord::Base
  authenticates_with_sorcery!

  attr_accessible :username, :email, :password, :password_confirmation, :first_name, :last_name, :date_of_birth, :genre

  validates_confirmation_of :password
  validates_presence_of :password, :on => :create
  validates_presence_of :username, :email, :first_name, :last_name, :date_of_birth, :genre
  validates_uniqueness_of :username, :email

  def is_admin?
    self.admin
  end

  def update_password(current_password, new_password, new_password_confirmation)
    if new_password == new_password_confirmation
      if !User.authenticate(self.username, current_password).nil?
        return self.change_password!(new_password)
      else
        return false
      end
    else
      return false
    end
  end
end
Marc Lainez
Collaborator

The issue is probably coming from your validates_confirmation_of :password

The change_password method only sets the password field not the confirmation. If you check at the example app https://github.com/NoamB/sorcery-example-app/blob/master/app/controllers/password_resets_controller.rb#L29
It sets the new password confirmation before calling the change_password! method to handle validation.

In your case, I guess you do that in your controller action, either through mass assignment or explicitly. That's why it works in the real app.

But in the tests the password confirmation will never match your new password as you don't set it. So if you want to keep your actual code just add this line before your return self.change_password! and you should be fine:

self.password_confirmation = new_password_confirmation
XL Projects INC.

I do have the password confirmation in my factory:

FactoryGirl.define do
  factory :user do
    first_name            "Bob"
    last_name             "Sinclar"
    date_of_birth         "03/12/1982"
    genre                 "M"
    sequence(:username)   { |n| "bsinclar#{n}" }
    email                 { "#{username}@example.com" }
    password              "password"
    password_confirmation "password"
    activation_state      "active"
    admin                 false

    factory :admin_user do
      admin true
    end
  end
end

I tried as you said but without success...

def update_password(current_password, new_password, new_password_confirmation)
    if new_password == new_password_confirmation
      if !User.authenticate(self.username, current_password).nil?
        self.password_confirmation = new_password_confirmation
        return self.change_password!(new_password)
      else
        return false
      end
    else
      return false
    end
  end

On thing I notice using pry, is that when I look at the user bob created with my factory, the activation state is set to "pending" (see the last code bit on my first post)... even though I set it explicitly to "active" in my factory... do you think the user_activation module could be interfering?

Thanks for the help by the way, I really appreciate...

Marc Lainez
Collaborator

Yes you have password confirmation in your factory, but not for the new password. Validation gets triggered each time you save the object and the change_password! method has a call to the save method. And if you look at the change_password! method code, it only sets the password field, not it's confirmation, it means you have to set it yourself if you want your tests to pass.

Good point about the activation thing. I think trying to set it up in the factory won't work, there is a before_create setup_activation on user being called that will overwrite your factory attributes. You can activate it in a before block

before :each do
    bob.activate!
end
XL Projects INC.

Awesome, it works! Thanks so much for the help... Here is my code in case anyone is interested... Marc, do you think I could have done things differently? always opened for suggestions.

models/user.rb

class User < ActiveRecord::Base
  authenticates_with_sorcery!

  attr_accessible :username, :email, :password, :password_confirmation, :first_name, :last_name, :date_of_birth, :genre

  validates_confirmation_of :password
  validates_presence_of :password, :on => :create
  validates_presence_of :username, :email, :first_name, :last_name, :date_of_birth, :genre
  validates_uniqueness_of :username, :email

  def is_admin?
    self.admin
  end

  def update_password(current_password, new_password, new_password_confirmation)
    if new_password == new_password_confirmation
      if !User.authenticate(self.username, current_password).nil?
        self.password_confirmation = new_password_confirmation #required for test purpose
        return self.change_password!(new_password)
      else
        return false
      end
    else
      return false
    end
  end
end

controllers/user_controller.rb (not ideal to use the update method but it was the quickest way)

...
def update
    if params[:commit] == 'Update Password'
      respond_to do |format|
        format.js {
          if @user.update_password(params[:user][:current_password], params[:user][:password], params[:user][:password_confirmation])
            flash[:success] = "Password has been updated successfully."
          else
            flash[:error] = "There was an error and the password was not updated. Please try again."
          end
          render action: :update
        }
      end
    else
      if @user.update_attributes(params[:user])
        redirect_to profile_url, :success => 'Profile was successfully updated.'
      else
        redirect_to profile_url, :error => 'There was an error and the profile was not updated.'
      end
    end
  end
...

specs/factories/users.rb

FactoryGirl.define do
  factory :user do
    first_name            "Bob"
    last_name             "Sinclar"
    date_of_birth         "03/12/1982"
    genre                 "M"
    sequence(:username)   { |n| "bsinclar#{n}" }
    email                 { "#{username}@example.com" }
    password              "password"
    password_confirmation "password"
    activation_state      "active"
    admin                 false

    factory :admin_user do
      admin true
    end
  end
end

specs/models/users.rb

require 'spec_helper'

describe User do
  subject { Factory(:user) }

  describe :update_password do
    let!(:bob) {Factory(:user)}

    before(:each) do
      bob.activate!
    end

    it "updates the password if everything is passed correctly" do
      bob.update_password("password","new_pass","new_pass").should be_true
    end

    it "throws an error if you enter the incorrect current password" do
      bob.update_password("bad_password","new_pass","new_pass").should be_false
    end

    it "throws an error if the new password isn't confirmed properly" do
      bob.update_password("password","new_pass","bad_new_pass").should be_false
    end
  end
end
Marc Lainez
Collaborator

Well, I would indeed do a few things differently, but many of them are details...

I'd remove the outer if because the change_password! method will have validation that does the same thing. The outer else is not needed, it means the update_password won't return false if auth fails but nil which will have the same outcome for your controller. So that's alright.

I'd replace !auth.nil? with auth.present? it's more elegant and self readable, and also more rails-like.

I'd also remove the inner else because a ruby method automatically returns the result of the last call, here change_password! will return true or false, which is what we want isn't it?

For that reason, skip the return statements, it feels more like java than ruby when you do that, and it adds some unneeded verbosity in ruby code... well that's what I think, I don't want to start a coding guidelines debate here :)

I guess it could look like that:

  def update_password(current_password, new_password, new_password_confirmation)
    if User.authenticate(self.username, current_password).present?
      self.password_confirmation = new_password_confirmation #required for test purpose
      self.change_password!(new_password)
    end
  end

I would also use a different resource for PasswordChanges. Password changes would be handled through a controller whose sole responsibility would be to handle password changes, nothing more. Leaving the users update action simple and with no additional if statement. But I guess that's just how I like it, to make it more readable and understandable...

You could also remove the activation_state attribute from the factory as it's not relevant anymore.

And the subject block in your spec is not needed.

Voilà :) You asked for feedback, I tried to be as complete as possible. Glad you made it work, I guess this issue can be closed now.

Marc Lainez mlainez closed this November 13, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.