Commit 0623ae19c792f1bc8b993c8f0c5bbb3efc473641
1 parent
23a10a5f
Exists in
master
and in
21 other branches
Refactory to new feature that show a specific message when a user inactive try login
Showing
3 changed files
with
54 additions
and
7 deletions
Show diff stats
app/controllers/public/account_controller.rb
@@ -46,8 +46,12 @@ class AccountController < ApplicationController | @@ -46,8 +46,12 @@ class AccountController < ApplicationController | ||
46 | 46 | ||
47 | self.current_user = plugins_alternative_authentication | 47 | self.current_user = plugins_alternative_authentication |
48 | 48 | ||
49 | - self.current_user ||= User.authenticate(params[:user][:login], params[:user][:password], environment) if params[:user] | ||
50 | - | 49 | + begin |
50 | + self.current_user ||= User.authenticate(params[:user][:login], params[:user][:password], environment) if params[:user] | ||
51 | + rescue NoosferoExceptions::UserInactive => e | ||
52 | + session[:notice] = e.message | ||
53 | + return | ||
54 | + end | ||
51 | if logged_in? | 55 | if logged_in? |
52 | check_join_in_community(self.current_user) | 56 | check_join_in_community(self.current_user) |
53 | 57 |
app/models/user.rb
@@ -120,11 +120,17 @@ class User < ActiveRecord::Base | @@ -120,11 +120,17 @@ class User < ActiveRecord::Base | ||
120 | 120 | ||
121 | validates_inclusion_of :terms_accepted, :in => [ '1' ], :if => lambda { |u| ! u.terms_of_use.blank? }, :message => N_('{fn} must be checked in order to signup.').fix_i18n | 121 | validates_inclusion_of :terms_accepted, :in => [ '1' ], :if => lambda { |u| ! u.terms_of_use.blank? }, :message => N_('{fn} must be checked in order to signup.').fix_i18n |
122 | 122 | ||
123 | + scope :has_login?, lambda { |login,email,environment_id| | ||
124 | + where('login = ? OR email = ?', login, email). | ||
125 | + where(environment_id: environment_id) | ||
126 | + } | ||
127 | + | ||
123 | # Authenticates a user by their login name or email and unencrypted password. Returns the user or nil. | 128 | # Authenticates a user by their login name or email and unencrypted password. Returns the user or nil. |
124 | def self.authenticate(login, password, environment = nil) | 129 | def self.authenticate(login, password, environment = nil) |
125 | environment ||= Environment.default | 130 | environment ||= Environment.default |
126 | - u = self.first :conditions => ['(login = ? OR email = ?) AND environment_id = ? AND activated_at IS NOT NULL', | ||
127 | - login, login, environment.id] # need to get the salt | 131 | + |
132 | + u = self.has_login?(login, login, environment.id) | ||
133 | + u = u.first if u.is_a?(ActiveRecord::Relation) | ||
128 | u && u.authenticated?(password) ? u : nil | 134 | u && u.authenticated?(password) ? u : nil |
129 | end | 135 | end |
130 | 136 | ||
@@ -237,6 +243,12 @@ class User < ActiveRecord::Base | @@ -237,6 +243,12 @@ class User < ActiveRecord::Base | ||
237 | end | 243 | end |
238 | 244 | ||
239 | def authenticated?(password) | 245 | def authenticated?(password) |
246 | + | ||
247 | + unless self.activated? | ||
248 | + message = _('The user "%{login}" is not active!') % {login: self.login} | ||
249 | + raise NoosferoExceptions::UserInactive.new(message, self) | ||
250 | + end | ||
251 | + | ||
240 | result = (crypted_password == encrypt(password)) | 252 | result = (crypted_password == encrypt(password)) |
241 | if (encryption_method != User.system_encryption_method) && result | 253 | if (encryption_method != User.system_encryption_method) && result |
242 | self.password_type = User.system_encryption_method.to_s | 254 | self.password_type = User.system_encryption_method.to_s |
@@ -275,8 +287,14 @@ class User < ActiveRecord::Base | @@ -275,8 +287,14 @@ class User < ActiveRecord::Base | ||
275 | # current password. | 287 | # current password. |
276 | # * Saves the record unless it is a new one. | 288 | # * Saves the record unless it is a new one. |
277 | def change_password!(current, new, confirmation) | 289 | def change_password!(current, new, confirmation) |
278 | - unless self.authenticated?(current) | ||
279 | - self.errors.add(:current_password, _('does not match.')) | 290 | + |
291 | + begin | ||
292 | + unless self.authenticated?(current) | ||
293 | + self.errors.add(:current_password, _('does not match.')) | ||
294 | + raise IncorrectPassword | ||
295 | + end | ||
296 | + rescue NoosferoExceptions::UserInactive => e | ||
297 | + self.errors.add(:current_password, e.message) | ||
280 | raise IncorrectPassword | 298 | raise IncorrectPassword |
281 | end | 299 | end |
282 | self.force_change_password!(new, confirmation) | 300 | self.force_change_password!(new, confirmation) |
@@ -393,3 +411,15 @@ class User < ActiveRecord::Base | @@ -393,3 +411,15 @@ class User < ActiveRecord::Base | ||
393 | Delayed::Job.enqueue(UserActivationJob.new(self.id), {:priority => 0, :run_at => (NOOSFERO_CONF['hours_until_user_activation_check'] || 72).hours.from_now}) | 411 | Delayed::Job.enqueue(UserActivationJob.new(self.id), {:priority => 0, :run_at => (NOOSFERO_CONF['hours_until_user_activation_check'] || 72).hours.from_now}) |
394 | end | 412 | end |
395 | end | 413 | end |
414 | + | ||
415 | +module NoosferoExceptions | ||
416 | + class UserInactive < ActiveRecord::ActiveRecordError | ||
417 | + attr_reader :user | ||
418 | + | ||
419 | + def initialize(message, user = nil) | ||
420 | + @user = user | ||
421 | + | ||
422 | + super(message) | ||
423 | + end | ||
424 | + end | ||
425 | +end |
test/functional/account_controller_test.rb
@@ -40,6 +40,14 @@ class AccountControllerTest < ActionController::TestCase | @@ -40,6 +40,14 @@ class AccountControllerTest < ActionController::TestCase | ||
40 | post :login, :user => { :login => 'fake', :password => 'fake' } | 40 | post :login, :user => { :login => 'fake', :password => 'fake' } |
41 | end | 41 | end |
42 | 42 | ||
43 | + should 'fail login if a user is inactive and show a warning message' do | ||
44 | + user = User.create!(login: 'testuser', email: 'test@email.com', password:'test', password_confirmation:'test', activation_code: nil) | ||
45 | + post :login, :user => { :login => 'testuser', :password => 'test' } | ||
46 | + | ||
47 | + assert_match 'not active', session[:notice] | ||
48 | + assert_nil session[:user] | ||
49 | + end | ||
50 | + | ||
43 | def test_should_fail_login_and_not_redirect | 51 | def test_should_fail_login_and_not_redirect |
44 | @request.env["HTTP_REFERER"] = 'bli' | 52 | @request.env["HTTP_REFERER"] = 'bli' |
45 | post :login, :user => {:login => 'johndoe', :password => 'bad password'} | 53 | post :login, :user => {:login => 'johndoe', :password => 'bad password'} |
@@ -273,8 +281,9 @@ class AccountControllerTest < ActionController::TestCase | @@ -273,8 +281,9 @@ class AccountControllerTest < ActionController::TestCase | ||
273 | assert_template 'invalid_change_password_code' | 281 | assert_template 'invalid_change_password_code' |
274 | end | 282 | end |
275 | 283 | ||
276 | - should 'require password confirmation correctly to enter new pasword' do | 284 | + should 'require password confirmation correctly to enter new password' do |
277 | user = create_user('testuser', :email => 'testuser@example.com', :password => 'test', :password_confirmation => 'test') | 285 | user = create_user('testuser', :email => 'testuser@example.com', :password => 'test', :password_confirmation => 'test') |
286 | + user.activate | ||
278 | change = ChangePassword.create!(:requestor => user.person) | 287 | change = ChangePassword.create!(:requestor => user.person) |
279 | 288 | ||
280 | post :new_password, :code => change.code, :change_password => { :password => 'onepass', :password_confirmation => 'another_pass' } | 289 | post :new_password, :code => change.code, :change_password => { :password => 'onepass', :password_confirmation => 'another_pass' } |
@@ -749,6 +758,8 @@ class AccountControllerTest < ActionController::TestCase | @@ -749,6 +758,8 @@ class AccountControllerTest < ActionController::TestCase | ||
749 | get :activate | 758 | get :activate |
750 | assert_nil assigns(:message) | 759 | assert_nil assigns(:message) |
751 | post :login, :user => {:login => 'testuser', :password => 'test123'} | 760 | post :login, :user => {:login => 'testuser', :password => 'test123'} |
761 | + | ||
762 | + assert_match 'not active', session[:notice] | ||
752 | assert_nil session[:user] | 763 | assert_nil session[:user] |
753 | end | 764 | end |
754 | 765 | ||
@@ -758,6 +769,8 @@ class AccountControllerTest < ActionController::TestCase | @@ -758,6 +769,8 @@ class AccountControllerTest < ActionController::TestCase | ||
758 | get :activate, :activation_code => 'wrongcode' | 769 | get :activate, :activation_code => 'wrongcode' |
759 | assert_nil assigns(:message) | 770 | assert_nil assigns(:message) |
760 | post :login, :user => {:login => 'testuser', :password => 'test123'} | 771 | post :login, :user => {:login => 'testuser', :password => 'test123'} |
772 | + | ||
773 | + assert_match 'not active', session[:notice] | ||
761 | assert_nil session[:user] | 774 | assert_nil session[:user] |
762 | end | 775 | end |
763 | 776 |