Commit bdb0e5a855c17ab2b4384d4db6fdf855424ffd45

Authored by Rodrigo Souto
1 parent 90a1b0b2

forgot_password: allow multiple matches of users

Also, moving requestor match logic to a helper instead of inside the
change_password model.
app/controllers/public/account_controller.rb
@@ -141,23 +141,34 @@ class AccountController < ApplicationController @@ -141,23 +141,34 @@ class AccountController < ApplicationController
141 end 141 end
142 end 142 end
143 143
144 - # The user requests a password change. She forgot her old password.  
145 - #  
146 - # Posts back. 144 + include ForgotPasswordHelper
  145 + helper :forgot_password
  146 +
147 def forgot_password 147 def forgot_password
148 if @plugins.dispatch(:allow_password_recovery).include?(false) 148 if @plugins.dispatch(:allow_password_recovery).include?(false)
149 redirect_back_or_default(:controller => 'home') 149 redirect_back_or_default(:controller => 'home')
150 session[:notice] = _("This environment doesn't allow password recovery.") 150 session[:notice] = _("This environment doesn't allow password recovery.")
151 end 151 end
152 - @change_password = ChangePassword.new(params[:change_password])  
153 - @change_password.environment_id = environment.id 152 +
  153 + @change_password = ChangePassword.new
154 154
155 if request.post? 155 if request.post?
156 begin 156 begin
157 - @change_password.save! 157 + requestors = fetch_requestors(params[:value])
  158 + raise ActiveRecord::RecordNotFound if requestors.blank? || params[:value].blank?
  159 +
  160 + requestors.each do |requestor|
  161 + ChangePassword.create!(:requestor => requestor)
  162 + end
158 render :action => 'password_recovery_sent' 163 render :action => 'password_recovery_sent'
159 - rescue ActiveRecord::RecordInvalid => e  
160 - nil # just pass and render at the end of the action 164 + rescue ActiveRecord::RecordNotFound
  165 + if params[:value].blank?
  166 + @change_password.errors.add_to_base(_('Can not recover user password with blank value.'))
  167 + else
  168 + @change_password.errors.add_to_base(_('Could not find any user with %s equal to "%s".') % [fields_label, params[:value]])
  169 + end
  170 + rescue ActiveRecord::RecordInvald
  171 + @change_password.errors.add_to_base(_('Could not perform password recovery for the user.'))
161 end 172 end
162 end 173 end
163 end 174 end
app/helpers/forgot_password_helper.rb 0 → 100644
@@ -0,0 +1,43 @@ @@ -0,0 +1,43 @@
  1 +module ForgotPasswordHelper
  2 + def plugins_options
  3 + @plugins.dispatch(:change_password_fields)
  4 + end
  5 +
  6 + def user_fields
  7 + %w[login email] + plugins_options.select {|options| options[:model].to_sym == :user }.map { |options| options[:field].to_s }
  8 + end
  9 +
  10 + def person_fields
  11 + %w[] + plugins_options.select {|options| options[:model].to_sym == :person }.map { |options| options[:field].to_s }
  12 + end
  13 +
  14 + def fields
  15 + user_fields + person_fields
  16 + end
  17 +
  18 + def fields_label
  19 + labels = [
  20 + _('Username'),
  21 + _('Email'),
  22 + ] + plugins_options.map { |options| options[:name] }
  23 +
  24 + last = labels.pop
  25 + label = labels.join(', ')
  26 + "#{label} #{_('or')} #{last}"
  27 + end
  28 +
  29 + def build_query(fields, value)
  30 + fields.map {|field| "#{field} = '#{value}'"}.join(' OR ')
  31 + end
  32 +
  33 + def fetch_requestors(value)
  34 + requestors = []
  35 + person_query = build_query(person_fields, value)
  36 + user_query = build_query(user_fields, value)
  37 +
  38 + requestors += Person.where(person_query).where(:environment_id => environment.id) if person_fields.present?
  39 + requestors += User.where(user_query).where(:environment_id => environment.id).map(&:person) if user_fields.present?
  40 + requestors
  41 + end
  42 +
  43 +end
app/models/change_password.rb
1 class ChangePassword < Task 1 class ChangePassword < Task
2 2
3 - settings_items :value  
4 - attr_accessor :password, :password_confirmation, :environment_id  
5 -  
6 - include Noosfero::Plugin::HotSpot 3 + attr_accessor :password, :password_confirmation
7 4
8 def self.human_attribute_name(attrib) 5 def self.human_attribute_name(attrib)
9 case attrib.to_sym 6 case attrib.to_sym
10 - when :value  
11 - _('Value')  
12 when :password 7 when :password
13 _('Password') 8 _('Password')
14 when :password_confirmation 9 when :password_confirmation
@@ -18,55 +13,7 @@ class ChangePassword &lt; Task @@ -18,55 +13,7 @@ class ChangePassword &lt; Task
18 end 13 end
19 end 14 end
20 15
21 - def environment  
22 - (requestor.environment if requestor) || Environment.find_by_id(environment_id)  
23 - end  
24 -  
25 - def plugins_options  
26 - plugins.dispatch(:change_password_fields)  
27 - end  
28 -  
29 - def user_fields  
30 - %w[login email] + plugins_options.select {|options| options[:model].to_sym == :user }.map { |options| options[:field].to_s }  
31 - end  
32 -  
33 - def person_fields  
34 - %w[] + plugins_options.select {|options| options[:model].to_sym == :person }.map { |options| options[:field].to_s }  
35 - end  
36 -  
37 - def fields  
38 - user_fields + person_fields  
39 - end  
40 -  
41 - def fields_label  
42 - labels = [  
43 - _('Username'),  
44 - _('Email'),  
45 - ] + plugins_options.map { |options| options[:name] }  
46 -  
47 - last = labels.pop  
48 - label = labels.join(', ')  
49 - "#{label} #{_('or')} #{last}"  
50 - end  
51 -  
52 - ###################################################  
53 - # validations for creating a ChangePassword task  
54 -  
55 - validates_presence_of :value, :environment_id, :on => :create, :message => _('must be filled in')  
56 -  
57 - validates_each :value, :on => :create do |data,attr,value|  
58 - unless data.value.blank?  
59 - users = data.find_users  
60 - if users.blank?  
61 - data.errors.add(:value, _('"%s" is not valid.') % value.to_s)  
62 - end  
63 - end  
64 - end  
65 -  
66 - before_validation do |change_password|  
67 - users = change_password.find_users  
68 - change_password.requestor ||= users.first.person if users.present?  
69 - end 16 + validates_presence_of :requestor
70 17
71 ################################################### 18 ###################################################
72 # validations for updating a ChangePassword task 19 # validations for updating a ChangePassword task
@@ -76,18 +23,8 @@ class ChangePassword &lt; Task @@ -76,18 +23,8 @@ class ChangePassword &lt; Task
76 validates_presence_of :password_confirmation, :on => :update, :if => lambda { |change| change.status != Task::Status::CANCELLED } 23 validates_presence_of :password_confirmation, :on => :update, :if => lambda { |change| change.status != Task::Status::CANCELLED }
77 validates_confirmation_of :password, :if => lambda { |change| change.status != Task::Status::CANCELLED } 24 validates_confirmation_of :password, :if => lambda { |change| change.status != Task::Status::CANCELLED }
78 25
79 - def build_query(fields)  
80 - fields.map {|field| "#{field} = '#{value}'"}.join(' OR ')  
81 - end  
82 -  
83 - def find_users  
84 - results = []  
85 - person_query = build_query(person_fields)  
86 - user_query = build_query(user_fields)  
87 -  
88 - results += Person.where(person_query).where(:environment_id => environment.id).map(&:user)  
89 - results += User.where(user_query).where(:environment_id => environment.id)  
90 - results 26 + def environment
  27 + requestor.environment unless requestor.nil?
91 end 28 end
92 29
93 def title 30 def title
@@ -128,7 +65,7 @@ class ChangePassword &lt; Task @@ -128,7 +65,7 @@ class ChangePassword &lt; Task
128 url = url_for(:host => hostname, :controller => 'account', :action => 'new_password', :code => code) 65 url = url_for(:host => hostname, :controller => 'account', :action => 'new_password', :code => code)
129 66
130 lambda do 67 lambda do
131 - _("In order to change your password, please visit the following address:\n\n%s") % url 68 + _("In order to change your password, please visit the following address:\n\n%s\n\nIf you did not required any change to your password just desconsider this email.") % url
132 end 69 end
133 end 70 end
134 71
app/views/account/forgot_password.rhtml
@@ -2,16 +2,13 @@ @@ -2,16 +2,13 @@
2 2
3 <%= error_messages_for :change_password, :header_message => _('Instructions to password recovery could not be sent'), :message => nil %> 3 <%= error_messages_for :change_password, :header_message => _('Instructions to password recovery could not be sent'), :message => nil %>
4 4
5 -<% labelled_form_for :change_password, @change_password, :url => { :action => 'forgot_password' } do |f| %>  
6 -  
7 - <%= labelled_form_field @change_password.fields_label, text_field_tag('change_password[value]') %>  
8 -  
9 -<div>  
10 - <% button_bar do %>  
11 - <%= submit_button('send', _('Send instructions')) %>  
12 - <% end %>  
13 -</div>  
14 -<%= content_tag(:small,_('After clicking the button above, you will receive an email with a link to a page where you will be able to create a new password.')) %>  
15 - 5 +<% form_tag do %>
  6 + <%= labelled_form_field fields_label, text_field_tag(:value) %>
  7 +
  8 + <div>
  9 + <% button_bar do %>
  10 + <%= submit_button('send', _('Send instructions')) %>
  11 + <% end %>
  12 + </div>
  13 + <%= content_tag(:small,_('After clicking the button above, you will receive an email with a link to a page where you will be able to create a new password.')) %>
16 <% end %> 14 <% end %>
17 -  
test/functional/account_controller_test.rb
@@ -216,20 +216,17 @@ class AccountControllerTest &lt; ActionController::TestCase @@ -216,20 +216,17 @@ class AccountControllerTest &lt; ActionController::TestCase
216 end 216 end
217 217
218 should 'respond to forgotten password change request with login' do 218 should 'respond to forgotten password change request with login' do
219 - change = ChangePassword.new  
220 - ChangePassword.expects(:new).with('value' => 'test').returns(change)  
221 - change.expects(:save!).returns(true) 219 + create_user('test')
222 220
223 - post :forgot_password, :change_password => { :value => 'test' } 221 + post :forgot_password, :value => 'test'
224 assert_template 'password_recovery_sent' 222 assert_template 'password_recovery_sent'
225 end 223 end
226 224
227 should 'respond to forgotten password change request with email' do 225 should 'respond to forgotten password change request with email' do
228 change = ChangePassword.new 226 change = ChangePassword.new
229 - ChangePassword.expects(:new).with('value' => 'test@localhost.localdomain').returns(change)  
230 - change.expects(:save!).returns(true) 227 + create_user('test', :email => 'test@localhost.localdomain')
231 228
232 - post :forgot_password, :change_password => { :value => 'test@localhost.localdomain' } 229 + post :forgot_password, :value => 'test@localhost.localdomain'
233 assert_template 'password_recovery_sent' 230 assert_template 'password_recovery_sent'
234 end 231 end
235 232
@@ -276,7 +273,7 @@ class AccountControllerTest &lt; ActionController::TestCase @@ -276,7 +273,7 @@ class AccountControllerTest &lt; ActionController::TestCase
276 273
277 should 'require password confirmation correctly to enter new pasword' do 274 should 'require password confirmation correctly to enter new pasword' do
278 user = create_user('testuser', :email => 'testuser@example.com', :password => 'test', :password_confirmation => 'test') 275 user = create_user('testuser', :email => 'testuser@example.com', :password => 'test', :password_confirmation => 'test')
279 - change = ChangePassword.create!(:value => 'testuser', :environment_id => Environment.default.id) 276 + change = ChangePassword.create!(:requestor => user.person)
280 277
281 post :new_password, :code => change.code, :change_password => { :password => 'onepass', :password_confirmation => 'another_pass' } 278 post :new_password, :code => change.code, :change_password => { :password => 'onepass', :password_confirmation => 'another_pass' }
282 assert_response :success 279 assert_response :success
@@ -862,7 +859,7 @@ class AccountControllerTest &lt; ActionController::TestCase @@ -862,7 +859,7 @@ class AccountControllerTest &lt; ActionController::TestCase
862 assert_response :redirect 859 assert_response :redirect
863 860
864 #Redirect on post action 861 #Redirect on post action
865 - post :forgot_password, :change_password => { :value => 'test' } 862 + post :forgot_password, :value => 'test'
866 assert_response :redirect 863 assert_response :redirect
867 end 864 end
868 865
test/unit/change_password_test.rb
@@ -4,36 +4,15 @@ class ChangePasswordTest &lt; ActiveSupport::TestCase @@ -4,36 +4,15 @@ class ChangePasswordTest &lt; ActiveSupport::TestCase
4 4
5 fixtures :environments 5 fixtures :environments
6 6
7 - should 'validate' do  
8 - data = ChangePassword.new(:environment_id => Environment.default)  
9 - assert !data.valid? 7 + def setup
  8 + @user = create_user('testuser', :password => 'test', :password_confirmation => 'test', :email => 'test@example.com')
  9 + @person = @user.person
10 end 10 end
11 11
12 - should 'require only a valid value' do  
13 - User.destroy_all  
14 - create_user('testuser', :email => 'test@example.com')  
15 -  
16 - data = ChangePassword.new  
17 - data.environment_id = Environment.default.id  
18 - assert !data.valid?  
19 - assert data.errors.invalid?(:value)  
20 -  
21 - data.value = 'testuser'  
22 - data.valid?  
23 - assert data.valid?  
24 -  
25 - data.value = 'test@example.com'  
26 - assert data.valid?  
27 - end 12 + attr_accessor :user, :person
28 13
29 should 'require correct passsword confirmation' do 14 should 'require correct passsword confirmation' do
30 - create_user('testuser', :password => 'test', :password_confirmation => 'test', :email => 'test@example.com')  
31 -  
32 - change = ChangePassword.new  
33 - change.environment_id = Environment.default.id  
34 - change.value = 'testuser'  
35 - change.save!  
36 - 15 + change = ChangePassword.create!(:requestor => person)
37 change.status = Task::Status::FINISHED 16 change.status = Task::Status::FINISHED
38 change.password = 'right' 17 change.password = 'right'
39 change.password_confirmation = 'wrong' 18 change.password_confirmation = 'wrong'
@@ -46,16 +25,7 @@ class ChangePasswordTest &lt; ActiveSupport::TestCase @@ -46,16 +25,7 @@ class ChangePasswordTest &lt; ActiveSupport::TestCase
46 end 25 end
47 26
48 should 'actually change password' do 27 should 'actually change password' do
49 - User.destroy_all  
50 - person = create_user('testuser', :password => 'test', :password_confirmation => 'test', :email => 'test@example.com').person  
51 -  
52 - change = ChangePassword.new  
53 - change.environment_id = Environment.default.id  
54 - change.value = 'testuser'  
55 - change.save!  
56 -  
57 - change.expects(:requestor).returns(person).at_least_once  
58 - 28 + change = ChangePassword.create!(:requestor => person)
59 change.password = 'newpass' 29 change.password = 'newpass'
60 change.password_confirmation = 'newpass' 30 change.password_confirmation = 'newpass'
61 change.finish 31 change.finish
@@ -64,14 +34,7 @@ class ChangePasswordTest &lt; ActiveSupport::TestCase @@ -64,14 +34,7 @@ class ChangePasswordTest &lt; ActiveSupport::TestCase
64 end 34 end
65 35
66 should 'not require password and password confirmation when cancelling' do 36 should 'not require password and password confirmation when cancelling' do
67 - User.destroy_all  
68 - person = create_user('testuser', :password => 'test', :password_confirmation => 'test', :email => 'test@example.com').person  
69 -  
70 - change = ChangePassword.new  
71 - change.environment_id = Environment.default.id  
72 - change.value = 'testuser'  
73 - change.save!  
74 - 37 + change = ChangePassword.create!(:requestor => person)
75 assert_nothing_raised do 38 assert_nothing_raised do
76 change.cancel 39 change.cancel
77 end 40 end
@@ -90,53 +53,22 @@ class ChangePasswordTest &lt; ActiveSupport::TestCase @@ -90,53 +53,22 @@ class ChangePasswordTest &lt; ActiveSupport::TestCase
90 p1 = create_user('sample-user', :password => 'test', :password_confirmation => 'test', :email => 'sample-user@test.com', :environment => e1).person 53 p1 = create_user('sample-user', :password => 'test', :password_confirmation => 'test', :email => 'sample-user@test.com', :environment => e1).person
91 p2 = create_user('sample-user', :password => 'test', :password_confirmation => 'test', :email => 'sample-user@test.com', :environment => e2).person 54 p2 = create_user('sample-user', :password => 'test', :password_confirmation => 'test', :email => 'sample-user@test.com', :environment => e2).person
92 55
93 - c1 = ChangePassword.create!(:value => 'sample-user', :environment_id => e1.id)  
94 - c2 = ChangePassword.create!(:value => 'sample-user', :environment_id => e2.id) 56 + c1 = ChangePassword.create!(:requestor => p1)
  57 + c2 = ChangePassword.create!(:requestor => p2)
95 58
96 assert_equal c1.requestor, p1 59 assert_equal c1.requestor, p1
97 assert_equal c2.requestor, p2 60 assert_equal c2.requestor, p2
98 end 61 end
99 62
100 should 'have target notification description' do 63 should 'have target notification description' do
101 - person = create_user('testuser').person  
102 -  
103 - change = ChangePassword.create(:value => 'testuser', :environment_id => Environment.default.id)  
104 - 64 + change = ChangePassword.create!(:requestor => person)
105 assert_match(/#{change.requestor.name} wants to change its password/, change.target_notification_description) 65 assert_match(/#{change.requestor.name} wants to change its password/, change.target_notification_description)
106 end 66 end
107 67
108 should 'deliver task created message' do 68 should 'deliver task created message' do
109 - person = create_user('testuser').person  
110 -  
111 - task = ChangePassword.create(:value => 'testuser', :environment_id => Environment.default.id)  
112 - 69 + task = ChangePassword.create!(:requestor => person)
113 email = TaskMailer.deliver_task_created(task) 70 email = TaskMailer.deliver_task_created(task)
114 assert_match(/#{task.requestor.name} wants to change its password/, email.subject) 71 assert_match(/#{task.requestor.name} wants to change its password/, email.subject)
115 end 72 end
116 73
117 - should 'allow extra fields provided by plugins' do  
118 - class Plugin1 < Noosfero::Plugin  
119 - def change_password_fields  
120 - {:field => 'f1', :name => 'F1', :model => 'person'}  
121 - end  
122 - end  
123 - class Plugin2 < Noosfero::Plugin  
124 - def change_password_fields  
125 - [{:field => 'f2', :name => 'F2', :model => 'person'},  
126 - {:field => 'f3', :name => 'F3', :model => 'person'}]  
127 - end  
128 - end  
129 -  
130 - environment = Environment.default  
131 - environment.enable_plugin(Plugin1)  
132 - environment.enable_plugin(Plugin2)  
133 - person = create_user('testuser').person  
134 -  
135 - change_password = ChangePassword.new(:environment_id => environment.id)  
136 -  
137 - assert_includes change_password.fields, 'f1'  
138 - assert_includes change_password.fields, 'f2'  
139 - assert_includes change_password.fields, 'f3'  
140 - end  
141 -  
142 end 74 end
test/unit/forgot_password_helper_test.rb 0 → 100644
@@ -0,0 +1,129 @@ @@ -0,0 +1,129 @@
  1 +require File.dirname(__FILE__) + '/../test_helper'
  2 +
  3 +class ForgotPasswordHelperTest < ActionView::TestCase
  4 + include ForgotPasswordHelper
  5 +
  6 + def setup
  7 + @environment = Environment.default
  8 + @plugins = Noosfero::Plugin::Manager.new(@environment, self)
  9 + end
  10 +
  11 + attr_accessor :environment
  12 +
  13 + should 'allow extra fields provided by plugins' do
  14 + class Plugin1 < Noosfero::Plugin
  15 + def change_password_fields
  16 + {:field => 'f1', :name => 'F1', :model => 'person'}
  17 + end
  18 + end
  19 + class Plugin2 < Noosfero::Plugin
  20 + def change_password_fields
  21 + [{:field => 'f2', :name => 'F2', :model => 'person'},
  22 + {:field => 'f3', :name => 'F3', :model => 'person'}]
  23 + end
  24 + end
  25 +
  26 + environment.enable_plugin(Plugin1)
  27 + environment.enable_plugin(Plugin2)
  28 +
  29 + assert_includes fields, 'f1'
  30 + assert_includes fields, 'f2'
  31 + assert_includes fields, 'f3'
  32 + end
  33 +
  34 + should 'allow extra person fields provided by plugins' do
  35 + class Plugin1 < Noosfero::Plugin
  36 + def change_password_fields
  37 + {:field => 'f1', :name => 'F1', :model => 'person'}
  38 + end
  39 + end
  40 + class Plugin2 < Noosfero::Plugin
  41 + def change_password_fields
  42 + [{:field => 'f2', :name => 'F2', :model => 'user'},
  43 + {:field => 'f3', :name => 'F3', :model => 'person'}]
  44 + end
  45 + end
  46 +
  47 + environment.enable_plugin(Plugin1)
  48 + environment.enable_plugin(Plugin2)
  49 +
  50 + assert_includes person_fields, 'f1'
  51 + assert_not_includes person_fields, 'f2'
  52 + assert_includes person_fields, 'f3'
  53 + end
  54 +
  55 + should 'allow extra user fields provided by plugins' do
  56 + class Plugin1 < Noosfero::Plugin
  57 + def change_password_fields
  58 + {:field => 'f1', :name => 'F1', :model => 'user'}
  59 + end
  60 + end
  61 + class Plugin2 < Noosfero::Plugin
  62 + def change_password_fields
  63 + [{:field => 'f2', :name => 'F2', :model => 'person'},
  64 + {:field => 'f3', :name => 'F3', :model => 'user'}]
  65 + end
  66 + end
  67 +
  68 + environment.enable_plugin(Plugin1)
  69 + environment.enable_plugin(Plugin2)
  70 +
  71 + assert_includes user_fields, 'f1'
  72 + assert_not_includes user_fields, 'f2'
  73 + assert_includes user_fields, 'f3'
  74 + end
  75 +
  76 + should 'add plugins fields labels to final label' do
  77 + class Plugin1 < Noosfero::Plugin
  78 + def change_password_fields
  79 + {:field => 'f1', :name => 'F1', :model => 'user'}
  80 + end
  81 + end
  82 + class Plugin2 < Noosfero::Plugin
  83 + def change_password_fields
  84 + [{:field => 'f2', :name => 'F2', :model => 'person'},
  85 + {:field => 'f3', :name => 'F3', :model => 'user'}]
  86 + end
  87 + end
  88 +
  89 + environment.enable_plugin(Plugin1)
  90 + environment.enable_plugin(Plugin2)
  91 +
  92 + assert_match /F1/, fields_label
  93 + assert_match /F2/, fields_label
  94 + assert_match /F3/, fields_label
  95 + end
  96 +
  97 + should 'fetch requestors based on fields available' do
  98 + p1 = create_user('person1').person
  99 + p2 = create_user('person2').person
  100 +
  101 + requestors = fetch_requestors('person1')
  102 + assert_includes requestors, p1
  103 + assert_not_includes requestors, p2
  104 +
  105 + p3 = create_user('person3').person
  106 + p3.address = 'some address'
  107 + p3.save!
  108 + p4 = create_user('person4').person
  109 + p4.address = 'some address'
  110 + p4.save!
  111 + p5 = create_user('person5').person
  112 + p5.address = 'another address'
  113 + p5.save!
  114 +
  115 + self.stubs(:person_fields).returns(%w[address])
  116 + requestors = fetch_requestors('some address')
  117 + assert_includes requestors, p3
  118 + assert_includes requestors, p4
  119 + end
  120 +
  121 + should 'not fetch people from other environments' do
  122 + p1 = create_user('person', :environment => fast_create(Environment)).person
  123 + p2 = create_user('person').person
  124 +
  125 + requestors = fetch_requestors('person')
  126 + assert_not_includes requestors, p1
  127 + assert_includes requestors, p2
  128 + end
  129 +end