Commit 94d199c1da9865092f83bc170d9fc9aac6bd30d0
1 parent
abd673e1
Exists in
master
and in
22 other branches
ActionItem78: implementing password recovery
git-svn-id: https://svn.colivre.coop.br/svn/noosfero/trunk@640 3f533792-8f58-4932-b0fe-aaf55b0a4547
Showing
7 changed files
with
97 additions
and
13 deletions
Show diff stats
app/controllers/public/account_controller.rb
| @@ -70,19 +70,43 @@ class AccountController < PublicController | @@ -70,19 +70,43 @@ class AccountController < PublicController | ||
| 70 | end | 70 | end |
| 71 | end | 71 | end |
| 72 | 72 | ||
| 73 | - # posts back | 73 | + # The user requests a password change. She forgot her old password. |
| 74 | + # | ||
| 75 | + # Posts back. | ||
| 74 | def forgot_password | 76 | def forgot_password |
| 75 | @change_password = ChangePassword.new(params[:change_password]) | 77 | @change_password = ChangePassword.new(params[:change_password]) |
| 76 | if request.post? | 78 | if request.post? |
| 77 | begin | 79 | begin |
| 78 | - @change_password.confirm! | 80 | + @change_password.save! |
| 79 | render :action => 'password_recovery_sent' | 81 | render :action => 'password_recovery_sent' |
| 80 | - rescue Exception => e | 82 | + rescue ActiveRecord::RecordInvalid => e |
| 81 | nil # just pass and render at the end of the action | 83 | nil # just pass and render at the end of the action |
| 82 | end | 84 | end |
| 83 | end | 85 | end |
| 84 | end | 86 | end |
| 85 | 87 | ||
| 88 | + # The user has a code for a ChangePassword request object. | ||
| 89 | + # | ||
| 90 | + # Posts back. | ||
| 91 | + def new_password | ||
| 92 | + @change_password = ChangePassword.find_by_code(params[:code]) | ||
| 93 | + | ||
| 94 | + unless @change_password | ||
| 95 | + render :action => 'invalid_change_password_code' | ||
| 96 | + return | ||
| 97 | + end | ||
| 98 | + | ||
| 99 | + if request.post? | ||
| 100 | + begin | ||
| 101 | + @change_password.update_attributes!(params[:change_password]) | ||
| 102 | + @change_password.finish | ||
| 103 | + render :action => 'new_password_ok' | ||
| 104 | + rescue ActiveRecord::RecordInvalid => e | ||
| 105 | + nil # just render new_password | ||
| 106 | + end | ||
| 107 | + end | ||
| 108 | + end | ||
| 109 | + | ||
| 86 | protected | 110 | protected |
| 87 | 111 | ||
| 88 | before_filter :load_profile_for_user | 112 | before_filter :load_profile_for_user |
app/models/change_password.rb
| @@ -8,12 +8,18 @@ class ChangePassword < Task | @@ -8,12 +8,18 @@ class ChangePassword < Task | ||
| 8 | end | 8 | end |
| 9 | 9 | ||
| 10 | attr_accessor :login, :email, :password, :password_confirmation | 10 | attr_accessor :login, :email, :password, :password_confirmation |
| 11 | + N_('ChangePassword|Login') | ||
| 12 | + N_('ChangePassword|Email') | ||
| 13 | + N_('ChangePassword|Password') | ||
| 14 | + N_('ChangePassword|Password Confirmation') | ||
| 11 | 15 | ||
| 12 | ################################################### | 16 | ################################################### |
| 13 | # validations for creating a ChangePassword task | 17 | # validations for creating a ChangePassword task |
| 14 | 18 | ||
| 15 | validates_presence_of :login, :email, :on => :create | 19 | validates_presence_of :login, :email, :on => :create |
| 16 | 20 | ||
| 21 | + validates_presence_of :requestor_id | ||
| 22 | + | ||
| 17 | validates_format_of :email, :on => :create, :with => Noosfero::Constants::EMAIL_FORMAT, :if => (lambda { |obj| !obj.email.blank? }) | 23 | validates_format_of :email, :on => :create, :with => Noosfero::Constants::EMAIL_FORMAT, :if => (lambda { |obj| !obj.email.blank? }) |
| 18 | 24 | ||
| 19 | validates_each :login, :on => :create do |data,attr,value| | 25 | validates_each :login, :on => :create do |data,attr,value| |
| @@ -29,7 +35,7 @@ class ChangePassword < Task | @@ -29,7 +35,7 @@ class ChangePassword < Task | ||
| 29 | end | 35 | end |
| 30 | end | 36 | end |
| 31 | 37 | ||
| 32 | - before_create do |change_password| | 38 | + before_validation_on_create do |change_password| |
| 33 | change_password.requestor = Person.find_by_identifier(change_password.login) | 39 | change_password.requestor = Person.find_by_identifier(change_password.login) |
| 34 | end | 40 | end |
| 35 | 41 | ||
| @@ -47,7 +53,7 @@ class ChangePassword < Task | @@ -47,7 +53,7 @@ class ChangePassword < Task | ||
| 47 | end | 53 | end |
| 48 | 54 | ||
| 49 | def perform | 55 | def perform |
| 50 | - user = User.find_by_login(self.login) | 56 | + user = self.requestor.user |
| 51 | user.force_change_password!(self.password, self.password_confirmation) | 57 | user.force_change_password!(self.password, self.password_confirmation) |
| 52 | end | 58 | end |
| 53 | 59 |
| @@ -0,0 +1,11 @@ | @@ -0,0 +1,11 @@ | ||
| 1 | +<h2><%= _('Enter new password') %></h2> | ||
| 2 | + | ||
| 3 | +<%= error_messages_for :change_password %> | ||
| 4 | + | ||
| 5 | +<% form_for(:change_password) do |f| %> | ||
| 6 | + | ||
| 7 | + <%= labelled_form_field(_('Enter new password'), (f.password_field :password)) %> | ||
| 8 | + <%= labelled_form_field(_('Confirm the new password'), (f.password_field :password_confirmation)) %> | ||
| 9 | + <%= submit_tag _('Change password') %> | ||
| 10 | + | ||
| 11 | +<% end %> |
| @@ -0,0 +1,9 @@ | @@ -0,0 +1,9 @@ | ||
| 1 | +<h2><%= _('Password changed sucessfully') %></h2> | ||
| 2 | + | ||
| 3 | +<p> | ||
| 4 | +<%= _('%s, your new password was successfully installed.') % @change_password.requestor.identifier %> | ||
| 5 | +</p> | ||
| 6 | + | ||
| 7 | +<p> | ||
| 8 | +<%= _("You can <a href='%s'>login</a> now.") % url_for(:action => 'login') %> | ||
| 9 | +</p> |
test/functional/account_controller_test.rb
| @@ -183,11 +183,43 @@ class AccountControllerTest < Test::Unit::TestCase | @@ -183,11 +183,43 @@ class AccountControllerTest < Test::Unit::TestCase | ||
| 183 | end | 183 | end |
| 184 | 184 | ||
| 185 | should 'respond to forgotten password change request' do | 185 | should 'respond to forgotten password change request' do |
| 186 | - flunk 'not implemented yet' | 186 | + change = ChangePassword.new |
| 187 | + ChangePassword.expects(:new).returns(change) | ||
| 188 | + change.expects(:save!).returns(true) | ||
| 189 | + | ||
| 190 | + post :forgot_password | ||
| 191 | + assert_template 'password_recovery_sent' | ||
| 192 | + end | ||
| 193 | + | ||
| 194 | + should 'provide interface for entering new password' do | ||
| 195 | + change = ChangePassword.new | ||
| 196 | + ChangePassword.expects(:find_by_code).with('osidufgiashfkjsadfhkj99999').returns(change) | ||
| 197 | + | ||
| 198 | + get :new_password, :code => 'osidufgiashfkjsadfhkj99999' | ||
| 199 | + assert_equal change, assigns(:change_password) | ||
| 187 | end | 200 | end |
| 188 | 201 | ||
| 189 | - should 'provide interface for entering new password to replace forgotten one' do | ||
| 190 | - flunk 'not implemented yet' | 202 | + should 'actually change password after entering new password' do |
| 203 | + change = ChangePassword.new | ||
| 204 | + ChangePassword.expects(:find_by_code).with('osidufgiashfkjsadfhkj99999').returns(change) | ||
| 205 | + | ||
| 206 | + requestor = mock | ||
| 207 | + requestor.stubs(:identifier).returns('joe') | ||
| 208 | + change.stubs(:requestor).returns(requestor) | ||
| 209 | + change.expects(:update_attributes!).with({'password' => 'newpass', 'password_confirmation' => 'newpass'}) | ||
| 210 | + change.expects(:finish) | ||
| 211 | + | ||
| 212 | + post :new_password, :code => 'osidufgiashfkjsadfhkj99999', :change_password => { :password => 'newpass', :password_confirmation => 'newpass' } | ||
| 213 | + | ||
| 214 | + assert_template 'new_password_ok' | ||
| 215 | + end | ||
| 216 | + | ||
| 217 | + should 'require a valid change_password code' do | ||
| 218 | + ChangePassword.destroy_all | ||
| 219 | + | ||
| 220 | + assert_raise RuntimeError do | ||
| 221 | + get :new_password, :code => 'dontexist' | ||
| 222 | + end | ||
| 191 | end | 223 | end |
| 192 | 224 | ||
| 193 | protected | 225 | protected |
test/unit/change_password_test.rb
| @@ -74,21 +74,18 @@ class ChangePasswordTest < Test::Unit::TestCase | @@ -74,21 +74,18 @@ class ChangePasswordTest < Test::Unit::TestCase | ||
| 74 | 74 | ||
| 75 | should 'actually change password' do | 75 | should 'actually change password' do |
| 76 | User.destroy_all | 76 | User.destroy_all |
| 77 | - User.create!(:login => 'testuser', :password => 'test', :password_confirmation => 'test', :email => 'test@example.com') | 77 | + person = User.create!(:login => 'testuser', :password => 'test', :password_confirmation => 'test', :email => 'test@example.com').person |
| 78 | 78 | ||
| 79 | change = ChangePassword.new | 79 | change = ChangePassword.new |
| 80 | change.login = 'testuser' | 80 | change.login = 'testuser' |
| 81 | change.email = 'test@example.com' | 81 | change.email = 'test@example.com' |
| 82 | change.save! | 82 | change.save! |
| 83 | 83 | ||
| 84 | - user = User.new | ||
| 85 | - User.expects(:find_by_login).with('testuser').returns(user) | ||
| 86 | - user.expects(:force_change_password!).with('newpass', 'newpass') | 84 | + change.expects(:requestor).returns(person).at_least_once |
| 87 | 85 | ||
| 88 | change.password = 'newpass' | 86 | change.password = 'newpass' |
| 89 | change.password_confirmation = 'newpass' | 87 | change.password_confirmation = 'newpass' |
| 90 | change.finish | 88 | change.finish |
| 91 | - | ||
| 92 | end | 89 | end |
| 93 | 90 | ||
| 94 | end | 91 | end |