diff --git a/app/controllers/public/account_controller.rb b/app/controllers/public/account_controller.rb index 6c8b0b1..d44b5b7 100644 --- a/app/controllers/public/account_controller.rb +++ b/app/controllers/public/account_controller.rb @@ -141,23 +141,34 @@ class AccountController < ApplicationController end end - # The user requests a password change. She forgot her old password. - # - # Posts back. + include ForgotPasswordHelper + helper :forgot_password + def forgot_password if @plugins.dispatch(:allow_password_recovery).include?(false) redirect_back_or_default(:controller => 'home') session[:notice] = _("This environment doesn't allow password recovery.") end - @change_password = ChangePassword.new(params[:change_password]) - @change_password.environment_id = environment.id + + @change_password = ChangePassword.new if request.post? begin - @change_password.save! + requestors = fetch_requestors(params[:value]) + raise ActiveRecord::RecordNotFound if requestors.blank? || params[:value].blank? + + requestors.each do |requestor| + ChangePassword.create!(:requestor => requestor) + end render :action => 'password_recovery_sent' - rescue ActiveRecord::RecordInvalid => e - nil # just pass and render at the end of the action + rescue ActiveRecord::RecordNotFound + if params[:value].blank? + @change_password.errors.add_to_base(_('Can not recover user password with blank value.')) + else + @change_password.errors.add_to_base(_('Could not find any user with %s equal to "%s".') % [fields_label, params[:value]]) + end + rescue ActiveRecord::RecordInvald + @change_password.errors.add_to_base(_('Could not perform password recovery for the user.')) end end end diff --git a/app/helpers/forgot_password_helper.rb b/app/helpers/forgot_password_helper.rb new file mode 100644 index 0000000..49fef4e --- /dev/null +++ b/app/helpers/forgot_password_helper.rb @@ -0,0 +1,43 @@ +module ForgotPasswordHelper + def plugins_options + @plugins.dispatch(:change_password_fields) + end + + def user_fields + %w[login email] + plugins_options.select {|options| options[:model].to_sym == :user }.map { |options| options[:field].to_s } + end + + def person_fields + %w[] + plugins_options.select {|options| options[:model].to_sym == :person }.map { |options| options[:field].to_s } + end + + def fields + user_fields + person_fields + end + + def fields_label + labels = [ + _('Username'), + _('Email'), + ] + plugins_options.map { |options| options[:name] } + + last = labels.pop + label = labels.join(', ') + "#{label} #{_('or')} #{last}" + end + + def build_query(fields, value) + fields.map {|field| "#{field} = '#{value}'"}.join(' OR ') + end + + def fetch_requestors(value) + requestors = [] + person_query = build_query(person_fields, value) + user_query = build_query(user_fields, value) + + requestors += Person.where(person_query).where(:environment_id => environment.id) if person_fields.present? + requestors += User.where(user_query).where(:environment_id => environment.id).map(&:person) if user_fields.present? + requestors + end + +end diff --git a/app/models/change_password.rb b/app/models/change_password.rb index 89656c0..4811f3f 100644 --- a/app/models/change_password.rb +++ b/app/models/change_password.rb @@ -1,14 +1,9 @@ class ChangePassword < Task - settings_items :value - attr_accessor :password, :password_confirmation, :environment_id - - include Noosfero::Plugin::HotSpot + attr_accessor :password, :password_confirmation def self.human_attribute_name(attrib) case attrib.to_sym - when :value - _('Value') when :password _('Password') when :password_confirmation @@ -18,55 +13,7 @@ class ChangePassword < Task end end - def environment - (requestor.environment if requestor) || Environment.find_by_id(environment_id) - end - - def plugins_options - plugins.dispatch(:change_password_fields) - end - - def user_fields - %w[login email] + plugins_options.select {|options| options[:model].to_sym == :user }.map { |options| options[:field].to_s } - end - - def person_fields - %w[] + plugins_options.select {|options| options[:model].to_sym == :person }.map { |options| options[:field].to_s } - end - - def fields - user_fields + person_fields - end - - def fields_label - labels = [ - _('Username'), - _('Email'), - ] + plugins_options.map { |options| options[:name] } - - last = labels.pop - label = labels.join(', ') - "#{label} #{_('or')} #{last}" - end - - ################################################### - # validations for creating a ChangePassword task - - validates_presence_of :value, :environment_id, :on => :create, :message => _('must be filled in') - - validates_each :value, :on => :create do |data,attr,value| - unless data.value.blank? - users = data.find_users - if users.blank? - data.errors.add(:value, _('"%s" is not valid.') % value.to_s) - end - end - end - - before_validation do |change_password| - users = change_password.find_users - change_password.requestor ||= users.first.person if users.present? - end + validates_presence_of :requestor ################################################### # validations for updating a ChangePassword task @@ -76,18 +23,8 @@ class ChangePassword < Task validates_presence_of :password_confirmation, :on => :update, :if => lambda { |change| change.status != Task::Status::CANCELLED } validates_confirmation_of :password, :if => lambda { |change| change.status != Task::Status::CANCELLED } - def build_query(fields) - fields.map {|field| "#{field} = '#{value}'"}.join(' OR ') - end - - def find_users - results = [] - person_query = build_query(person_fields) - user_query = build_query(user_fields) - - results += Person.where(person_query).where(:environment_id => environment.id).map(&:user) - results += User.where(user_query).where(:environment_id => environment.id) - results + def environment + requestor.environment unless requestor.nil? end def title @@ -128,7 +65,7 @@ class ChangePassword < Task url = url_for(:host => hostname, :controller => 'account', :action => 'new_password', :code => code) lambda do - _("In order to change your password, please visit the following address:\n\n%s") % url + _("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 end end diff --git a/app/views/account/forgot_password.rhtml b/app/views/account/forgot_password.rhtml index da72d24..2aa98ea 100644 --- a/app/views/account/forgot_password.rhtml +++ b/app/views/account/forgot_password.rhtml @@ -2,16 +2,13 @@ <%= error_messages_for :change_password, :header_message => _('Instructions to password recovery could not be sent'), :message => nil %> -<% labelled_form_for :change_password, @change_password, :url => { :action => 'forgot_password' } do |f| %> - - <%= labelled_form_field @change_password.fields_label, text_field_tag('change_password[value]') %> - -
- <% button_bar do %> - <%= submit_button('send', _('Send instructions')) %> - <% end %> -
-<%= 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.')) %> - +<% form_tag do %> + <%= labelled_form_field fields_label, text_field_tag(:value) %> + +
+ <% button_bar do %> + <%= submit_button('send', _('Send instructions')) %> + <% end %> +
+ <%= 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.')) %> <% end %> - diff --git a/test/functional/account_controller_test.rb b/test/functional/account_controller_test.rb index 226e115..af994aa 100644 --- a/test/functional/account_controller_test.rb +++ b/test/functional/account_controller_test.rb @@ -216,20 +216,17 @@ class AccountControllerTest < ActionController::TestCase end should 'respond to forgotten password change request with login' do - change = ChangePassword.new - ChangePassword.expects(:new).with('value' => 'test').returns(change) - change.expects(:save!).returns(true) + create_user('test') - post :forgot_password, :change_password => { :value => 'test' } + post :forgot_password, :value => 'test' assert_template 'password_recovery_sent' end should 'respond to forgotten password change request with email' do change = ChangePassword.new - ChangePassword.expects(:new).with('value' => 'test@localhost.localdomain').returns(change) - change.expects(:save!).returns(true) + create_user('test', :email => 'test@localhost.localdomain') - post :forgot_password, :change_password => { :value => 'test@localhost.localdomain' } + post :forgot_password, :value => 'test@localhost.localdomain' assert_template 'password_recovery_sent' end @@ -276,7 +273,7 @@ class AccountControllerTest < ActionController::TestCase should 'require password confirmation correctly to enter new pasword' do user = create_user('testuser', :email => 'testuser@example.com', :password => 'test', :password_confirmation => 'test') - change = ChangePassword.create!(:value => 'testuser', :environment_id => Environment.default.id) + change = ChangePassword.create!(:requestor => user.person) post :new_password, :code => change.code, :change_password => { :password => 'onepass', :password_confirmation => 'another_pass' } assert_response :success @@ -862,7 +859,7 @@ class AccountControllerTest < ActionController::TestCase assert_response :redirect #Redirect on post action - post :forgot_password, :change_password => { :value => 'test' } + post :forgot_password, :value => 'test' assert_response :redirect end diff --git a/test/unit/change_password_test.rb b/test/unit/change_password_test.rb index 0d1c72a..c5ff7d5 100644 --- a/test/unit/change_password_test.rb +++ b/test/unit/change_password_test.rb @@ -4,36 +4,15 @@ class ChangePasswordTest < ActiveSupport::TestCase fixtures :environments - should 'validate' do - data = ChangePassword.new(:environment_id => Environment.default) - assert !data.valid? + def setup + @user = create_user('testuser', :password => 'test', :password_confirmation => 'test', :email => 'test@example.com') + @person = @user.person end - should 'require only a valid value' do - User.destroy_all - create_user('testuser', :email => 'test@example.com') - - data = ChangePassword.new - data.environment_id = Environment.default.id - assert !data.valid? - assert data.errors.invalid?(:value) - - data.value = 'testuser' - data.valid? - assert data.valid? - - data.value = 'test@example.com' - assert data.valid? - end + attr_accessor :user, :person should 'require correct passsword confirmation' do - create_user('testuser', :password => 'test', :password_confirmation => 'test', :email => 'test@example.com') - - change = ChangePassword.new - change.environment_id = Environment.default.id - change.value = 'testuser' - change.save! - + change = ChangePassword.create!(:requestor => person) change.status = Task::Status::FINISHED change.password = 'right' change.password_confirmation = 'wrong' @@ -46,16 +25,7 @@ class ChangePasswordTest < ActiveSupport::TestCase end should 'actually change password' do - User.destroy_all - person = create_user('testuser', :password => 'test', :password_confirmation => 'test', :email => 'test@example.com').person - - change = ChangePassword.new - change.environment_id = Environment.default.id - change.value = 'testuser' - change.save! - - change.expects(:requestor).returns(person).at_least_once - + change = ChangePassword.create!(:requestor => person) change.password = 'newpass' change.password_confirmation = 'newpass' change.finish @@ -64,14 +34,7 @@ class ChangePasswordTest < ActiveSupport::TestCase end should 'not require password and password confirmation when cancelling' do - User.destroy_all - person = create_user('testuser', :password => 'test', :password_confirmation => 'test', :email => 'test@example.com').person - - change = ChangePassword.new - change.environment_id = Environment.default.id - change.value = 'testuser' - change.save! - + change = ChangePassword.create!(:requestor => person) assert_nothing_raised do change.cancel end @@ -90,53 +53,22 @@ class ChangePasswordTest < ActiveSupport::TestCase p1 = create_user('sample-user', :password => 'test', :password_confirmation => 'test', :email => 'sample-user@test.com', :environment => e1).person p2 = create_user('sample-user', :password => 'test', :password_confirmation => 'test', :email => 'sample-user@test.com', :environment => e2).person - c1 = ChangePassword.create!(:value => 'sample-user', :environment_id => e1.id) - c2 = ChangePassword.create!(:value => 'sample-user', :environment_id => e2.id) + c1 = ChangePassword.create!(:requestor => p1) + c2 = ChangePassword.create!(:requestor => p2) assert_equal c1.requestor, p1 assert_equal c2.requestor, p2 end should 'have target notification description' do - person = create_user('testuser').person - - change = ChangePassword.create(:value => 'testuser', :environment_id => Environment.default.id) - + change = ChangePassword.create!(:requestor => person) assert_match(/#{change.requestor.name} wants to change its password/, change.target_notification_description) end should 'deliver task created message' do - person = create_user('testuser').person - - task = ChangePassword.create(:value => 'testuser', :environment_id => Environment.default.id) - + task = ChangePassword.create!(:requestor => person) email = TaskMailer.deliver_task_created(task) assert_match(/#{task.requestor.name} wants to change its password/, email.subject) end - should 'allow extra fields provided by plugins' do - class Plugin1 < Noosfero::Plugin - def change_password_fields - {:field => 'f1', :name => 'F1', :model => 'person'} - end - end - class Plugin2 < Noosfero::Plugin - def change_password_fields - [{:field => 'f2', :name => 'F2', :model => 'person'}, - {:field => 'f3', :name => 'F3', :model => 'person'}] - end - end - - environment = Environment.default - environment.enable_plugin(Plugin1) - environment.enable_plugin(Plugin2) - person = create_user('testuser').person - - change_password = ChangePassword.new(:environment_id => environment.id) - - assert_includes change_password.fields, 'f1' - assert_includes change_password.fields, 'f2' - assert_includes change_password.fields, 'f3' - end - end diff --git a/test/unit/forgot_password_helper_test.rb b/test/unit/forgot_password_helper_test.rb new file mode 100644 index 0000000..60d3120 --- /dev/null +++ b/test/unit/forgot_password_helper_test.rb @@ -0,0 +1,129 @@ +require File.dirname(__FILE__) + '/../test_helper' + +class ForgotPasswordHelperTest < ActionView::TestCase + include ForgotPasswordHelper + + def setup + @environment = Environment.default + @plugins = Noosfero::Plugin::Manager.new(@environment, self) + end + + attr_accessor :environment + + should 'allow extra fields provided by plugins' do + class Plugin1 < Noosfero::Plugin + def change_password_fields + {:field => 'f1', :name => 'F1', :model => 'person'} + end + end + class Plugin2 < Noosfero::Plugin + def change_password_fields + [{:field => 'f2', :name => 'F2', :model => 'person'}, + {:field => 'f3', :name => 'F3', :model => 'person'}] + end + end + + environment.enable_plugin(Plugin1) + environment.enable_plugin(Plugin2) + + assert_includes fields, 'f1' + assert_includes fields, 'f2' + assert_includes fields, 'f3' + end + + should 'allow extra person fields provided by plugins' do + class Plugin1 < Noosfero::Plugin + def change_password_fields + {:field => 'f1', :name => 'F1', :model => 'person'} + end + end + class Plugin2 < Noosfero::Plugin + def change_password_fields + [{:field => 'f2', :name => 'F2', :model => 'user'}, + {:field => 'f3', :name => 'F3', :model => 'person'}] + end + end + + environment.enable_plugin(Plugin1) + environment.enable_plugin(Plugin2) + + assert_includes person_fields, 'f1' + assert_not_includes person_fields, 'f2' + assert_includes person_fields, 'f3' + end + + should 'allow extra user fields provided by plugins' do + class Plugin1 < Noosfero::Plugin + def change_password_fields + {:field => 'f1', :name => 'F1', :model => 'user'} + end + end + class Plugin2 < Noosfero::Plugin + def change_password_fields + [{:field => 'f2', :name => 'F2', :model => 'person'}, + {:field => 'f3', :name => 'F3', :model => 'user'}] + end + end + + environment.enable_plugin(Plugin1) + environment.enable_plugin(Plugin2) + + assert_includes user_fields, 'f1' + assert_not_includes user_fields, 'f2' + assert_includes user_fields, 'f3' + end + + should 'add plugins fields labels to final label' do + class Plugin1 < Noosfero::Plugin + def change_password_fields + {:field => 'f1', :name => 'F1', :model => 'user'} + end + end + class Plugin2 < Noosfero::Plugin + def change_password_fields + [{:field => 'f2', :name => 'F2', :model => 'person'}, + {:field => 'f3', :name => 'F3', :model => 'user'}] + end + end + + environment.enable_plugin(Plugin1) + environment.enable_plugin(Plugin2) + + assert_match /F1/, fields_label + assert_match /F2/, fields_label + assert_match /F3/, fields_label + end + + should 'fetch requestors based on fields available' do + p1 = create_user('person1').person + p2 = create_user('person2').person + + requestors = fetch_requestors('person1') + assert_includes requestors, p1 + assert_not_includes requestors, p2 + + p3 = create_user('person3').person + p3.address = 'some address' + p3.save! + p4 = create_user('person4').person + p4.address = 'some address' + p4.save! + p5 = create_user('person5').person + p5.address = 'another address' + p5.save! + + self.stubs(:person_fields).returns(%w[address]) + requestors = fetch_requestors('some address') + assert_includes requestors, p3 + assert_includes requestors, p4 + end + + should 'not fetch people from other environments' do + p1 = create_user('person', :environment => fast_create(Environment)).person + p2 = create_user('person').person + + requestors = fetch_requestors('person') + assert_not_includes requestors, p1 + assert_includes requestors, p2 + end +end -- libgit2 0.21.2