Refactory to new feature that show a specific message when a user inactive try login

Michel Felipe · Luciano Prestes
1 parent c6a2cc53
Showing 5 changed files with 66 additions and 10 deletions Show diff stats
app/controllers/public/account_controller.rb
app/models/user.rb
test/functional/account_controller_test.rb
test/unit/change_password_test.rb
test/unit/user_test.rb
@@ -46,8 +46,12 @@ class AccountController &lt; ApplicationController
  
     self.current_user = plugins_alternative_authentication
  
-    self.current_user ||= User.authenticate(params[:user][:login], params[:user][:password], environment) if params[:user]
-
+    begin
+      self.current_user ||= User.authenticate(params[:user][:login], params[:user][:password], environment) if params[:user]
+    rescue User::UserNotActivated => e
+      session[:notice] = e.message
+      return
+    end
     if logged_in?
       check_join_in_community(self.current_user)
  
@@ -120,11 +120,17 @@ class User &lt; ActiveRecord::Base
  
   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
  
+  scope :has_login?, lambda { |login,email,environment_id|
+    where('login = ? OR email = ?', login, email).
+    where(environment_id: environment_id)
+  }
+
   # Authenticates a user by their login name or email and unencrypted password.  Returns the user or nil.
   def self.authenticate(login, password, environment = nil)
     environment ||= Environment.default
-    u = self.first :conditions => ['(login = ? OR email = ?) AND environment_id = ? AND activated_at IS NOT NULL',
-                                   login, login, environment.id] # need to get the salt
+
+    u = self.has_login?(login, login, environment.id)
+    u = u.first if u.is_a?(ActiveRecord::Relation)
     u && u.authenticated?(password) ? u : nil
   end
  
@@ -236,7 +242,23 @@ class User &lt; ActiveRecord::Base
     password.crypt(salt)
   end
  
+  class UserNotActivated < StandardError
+    attr_reader :user
+
+    def initialize(message, user = nil)
+      @user = user
+
+      super(message)
+    end
+  end
+
   def authenticated?(password)
+
+    unless self.activated?
+      message = _('The user "%{login}" is not activated! Please check your email to activate your user') % {login: self.login}
+      raise UserNotActivated.new(message, self)
+    end
+
     result = (crypted_password == encrypt(password))
     if (encryption_method != User.system_encryption_method) && result
       self.password_type = User.system_encryption_method.to_s
@@ -275,9 +297,15 @@ class User &lt; ActiveRecord::Base
   #   current password.
   # * Saves the record unless it is a new one.
   def change_password!(current, new, confirmation)
-    unless self.authenticated?(current)
-      self.errors.add(:current_password, _('does not match.'))
-      raise IncorrectPassword
+
+    begin
+      unless self.authenticated?(current)
+        self.errors.add(:current_password, _('does not match.'))
+        raise IncorrectPassword
+      end
+    rescue UserNotActivated => e
+      self.errors.add(:current_password, e.message)
+      raise UserNotActivated
     end
     self.force_change_password!(new, confirmation)
   end
@@ -40,6 +40,14 @@ class AccountControllerTest &lt; ActionController::TestCase
     post :login, :user => { :login => 'fake', :password => 'fake' }
   end
  
+  should 'fail login if a user is inactive and show a warning message' do
+    user = User.create!(login: 'testuser', email: 'test@email.com', password:'test', password_confirmation:'test', activation_code: nil)
+    post :login, :user => { :login => 'testuser', :password => 'test' }
+
+    assert_match 'not activated', session[:notice]
+    assert_nil session[:user]
+  end
+
   def test_should_fail_login_and_not_redirect
     @request.env["HTTP_REFERER"] = 'bli'
     post :login, :user => {:login => 'johndoe', :password => 'bad password'}
@@ -273,8 +281,9 @@ class AccountControllerTest &lt; ActionController::TestCase
     assert_template 'invalid_change_password_code'
   end
  
-  should 'require password confirmation correctly to enter new pasword' do
+  should 'require password confirmation correctly to enter new password' do
     user = create_user('testuser', :email => 'testuser@example.com', :password => 'test', :password_confirmation => 'test')
+    user.activate
     change = ChangePassword.create!(:requestor => user.person)
  
     post :new_password, :code => change.code, :change_password => { :password => 'onepass', :password_confirmation => 'another_pass' }
@@ -749,6 +758,8 @@ class AccountControllerTest &lt; ActionController::TestCase
     get :activate
     assert_nil assigns(:message)
     post :login, :user => {:login => 'testuser', :password => 'test123'}
+
+    assert_match 'not activated', session[:notice]
     assert_nil session[:user]
   end
  
@@ -758,6 +769,8 @@ class AccountControllerTest &lt; ActionController::TestCase
     get :activate, :activation_code => 'wrongcode'
     assert_nil assigns(:message)
     post :login, :user => {:login => 'testuser', :password => 'test123'}
+
+    assert_match 'not activated', session[:notice]
     assert_nil session[:user]
   end
  
@@ -30,7 +30,8 @@ class ChangePasswordTest &lt; ActiveSupport::TestCase
     change.password_confirmation = 'newpass'
     change.finish
  
-    assert User.find(person.user.id).authenticated?('newpass')
+    person.user.activate
+    assert person.user.authenticated?('newpass')
   end
  
   should 'not require password and password confirmation when cancelling' do
@@ -123,6 +123,7 @@ class UserTest &lt; ActiveSupport::TestCase
  
   def test_should_change_password
     user = create_user('changetest', :password => 'test', :password_confirmation => 'test', :email => 'changetest@example.com')
+    user.activate
     assert_nothing_raised do
       user.change_password!('test', 'newpass', 'newpass')
     end
@@ -132,6 +133,7 @@ class UserTest &lt; ActiveSupport::TestCase
  
   def test_should_give_correct_current_password_for_changing_password
     user = create_user('changetest', :password => 'test', :password_confirmation => 'test', :email => 'changetest@example.com')
+    user.activate
     assert_raise User::IncorrectPassword do
       user.change_password!('wrong', 'newpass', 'newpass')
     end
@@ -141,6 +143,8 @@ class UserTest &lt; ActiveSupport::TestCase
  
   should 'require matching confirmation when changing password by force' do
     user = create_user('changetest', :password => 'test', :password_confirmation => 'test', :email => 'changetest@example.com')
+    user.activate
+
     assert_raise ActiveRecord::RecordInvalid do
       user.force_change_password!('newpass', 'newpasswrong')
     end
@@ -153,6 +157,8 @@ class UserTest &lt; ActiveSupport::TestCase
     assert_nothing_raised  do
       user.force_change_password!('newpass', 'newpass')
     end
+
+    user.activate
     assert user.authenticated?('newpass')
   end
  
@@ -256,6 +262,7 @@ class UserTest &lt; ActiveSupport::TestCase
  
     # when the user logs in, her password must be reencrypted with the new
     # method
+    user.activate
     user.authenticated?('test')
  
     # and the new password must be saved back to the database
@@ -273,6 +280,7 @@ class UserTest &lt; ActiveSupport::TestCase
     User.expects(:system_encryption_method).returns(:md5).at_least_once
  
     # but the user provided the wrong password
+    user.activate
     user.authenticated?('WRONG_PASSWORD')
  
     # and then her password is not updated
@@ -520,7 +528,9 @@ class UserTest &lt; ActiveSupport::TestCase
  
   should 'not authenticate a not activated user' do
     user = new_user :login => 'testuser', :password => 'test123', :password_confirmation => 'test123'
-    assert_nil User.authenticate('testuser', 'test123')
+    assert_raises User::UserNotActivated do
+      User.authenticate('testuser', 'test123')
+    end
   end
  
   should 'have activation code but no activated at when created' do
...	...	@@ -46,8 +46,12 @@ class AccountController < ApplicationController
46	46
47	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 User::UserNotActivated => e
	52	+ session[:notice] = e.message
	53	+ return
	54	+ end
51	55	if logged_in?
52	56	check_join_in_community(self.current_user)
53	57
...	...
...	...	@@ -120,11 +120,17 @@ class User < ActiveRecord::Base
120	120
121	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	128	# Authenticates a user by their login name or email and unencrypted password. Returns the user or nil.
124	129	def self.authenticate(login, password, environment = nil)
125	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	134	u && u.authenticated?(password) ? u : nil
129	135	end
130	136
...	...	@@ -236,7 +242,23 @@ class User < ActiveRecord::Base
236	242	password.crypt(salt)
237	243	end
238	244
	245	+ class UserNotActivated < StandardError
	246	+ attr_reader :user
	247	+
	248	+ def initialize(message, user = nil)
	249	+ @user = user
	250	+
	251	+ super(message)
	252	+ end
	253	+ end
	254	+
239	255	def authenticated?(password)
	256	+
	257	+ unless self.activated?
	258	+ message = _('The user "%{login}" is not activated! Please check your email to activate your user') % {login: self.login}
	259	+ raise UserNotActivated.new(message, self)
	260	+ end
	261	+
240	262	result = (crypted_password == encrypt(password))
241	263	if (encryption_method != User.system_encryption_method) && result
242	264	self.password_type = User.system_encryption_method.to_s
...	...	@@ -275,9 +297,15 @@ class User < ActiveRecord::Base
275	297	# current password.
276	298	# * Saves the record unless it is a new one.
277	299	def change_password!(current, new, confirmation)
278		- unless self.authenticated?(current)
279		- self.errors.add(:current_password, _('does not match.'))
280		- raise IncorrectPassword
	300	+
	301	+ begin
	302	+ unless self.authenticated?(current)
	303	+ self.errors.add(:current_password, _('does not match.'))
	304	+ raise IncorrectPassword
	305	+ end
	306	+ rescue UserNotActivated => e
	307	+ self.errors.add(:current_password, e.message)
	308	+ raise UserNotActivated
281	309	end
282	310	self.force_change_password!(new, confirmation)
283	311	end
...	...
...	...	@@ -40,6 +40,14 @@ class AccountControllerTest < ActionController::TestCase
40	40	post :login, :user => { :login => 'fake', :password => 'fake' }
41	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 activated', session[:notice]
	48	+ assert_nil session[:user]
	49	+ end
	50	+
43	51	def test_should_fail_login_and_not_redirect
44	52	@request.env["HTTP_REFERER"] = 'bli'
45	53	post :login, :user => {:login => 'johndoe', :password => 'bad password'}
...	...	@@ -273,8 +281,9 @@ class AccountControllerTest < ActionController::TestCase
273	281	assert_template 'invalid_change_password_code'
274	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	285	user = create_user('testuser', :email => 'testuser@example.com', :password => 'test', :password_confirmation => 'test')
	286	+ user.activate
278	287	change = ChangePassword.create!(:requestor => user.person)
279	288
280	289	post :new_password, :code => change.code, :change_password => { :password => 'onepass', :password_confirmation => 'another_pass' }
...	...	@@ -749,6 +758,8 @@ class AccountControllerTest < ActionController::TestCase
749	758	get :activate
750	759	assert_nil assigns(:message)
751	760	post :login, :user => {:login => 'testuser', :password => 'test123'}
	761	+
	762	+ assert_match 'not activated', session[:notice]
752	763	assert_nil session[:user]
753	764	end
754	765
...	...	@@ -758,6 +769,8 @@ class AccountControllerTest < ActionController::TestCase
758	769	get :activate, :activation_code => 'wrongcode'
759	770	assert_nil assigns(:message)
760	771	post :login, :user => {:login => 'testuser', :password => 'test123'}
	772	+
	773	+ assert_match 'not activated', session[:notice]
761	774	assert_nil session[:user]
762	775	end
763	776
...	...
...	...	@@ -30,7 +30,8 @@ class ChangePasswordTest < ActiveSupport::TestCase
30	30	change.password_confirmation = 'newpass'
31	31	change.finish
32	32
33		- assert User.find(person.user.id).authenticated?('newpass')
	33	+ person.user.activate
	34	+ assert person.user.authenticated?('newpass')
34	35	end
35	36
36	37	should 'not require password and password confirmation when cancelling' do
...	...
...	...	@@ -123,6 +123,7 @@ class UserTest < ActiveSupport::TestCase
123	123
124	124	def test_should_change_password
125	125	user = create_user('changetest', :password => 'test', :password_confirmation => 'test', :email => 'changetest@example.com')
	126	+ user.activate
126	127	assert_nothing_raised do
127	128	user.change_password!('test', 'newpass', 'newpass')
128	129	end
...	...	@@ -132,6 +133,7 @@ class UserTest < ActiveSupport::TestCase
132	133
133	134	def test_should_give_correct_current_password_for_changing_password
134	135	user = create_user('changetest', :password => 'test', :password_confirmation => 'test', :email => 'changetest@example.com')
	136	+ user.activate
135	137	assert_raise User::IncorrectPassword do
136	138	user.change_password!('wrong', 'newpass', 'newpass')
137	139	end
...	...	@@ -141,6 +143,8 @@ class UserTest < ActiveSupport::TestCase
141	143
142	144	should 'require matching confirmation when changing password by force' do
143	145	user = create_user('changetest', :password => 'test', :password_confirmation => 'test', :email => 'changetest@example.com')
	146	+ user.activate
	147	+
144	148	assert_raise ActiveRecord::RecordInvalid do
145	149	user.force_change_password!('newpass', 'newpasswrong')
146	150	end
...	...	@@ -153,6 +157,8 @@ class UserTest < ActiveSupport::TestCase
153	157	assert_nothing_raised do
154	158	user.force_change_password!('newpass', 'newpass')
155	159	end
	160	+
	161	+ user.activate
156	162	assert user.authenticated?('newpass')
157	163	end
158	164
...	...	@@ -256,6 +262,7 @@ class UserTest < ActiveSupport::TestCase
256	262
257	263	# when the user logs in, her password must be reencrypted with the new
258	264	# method
	265	+ user.activate
259	266	user.authenticated?('test')
260	267
261	268	# and the new password must be saved back to the database
...	...	@@ -273,6 +280,7 @@ class UserTest < ActiveSupport::TestCase
273	280	User.expects(:system_encryption_method).returns(:md5).at_least_once
274	281
275	282	# but the user provided the wrong password
	283	+ user.activate
276	284	user.authenticated?('WRONG_PASSWORD')
277	285
278	286	# and then her password is not updated
...	...	@@ -520,7 +528,9 @@ class UserTest < ActiveSupport::TestCase
520	528
521	529	should 'not authenticate a not activated user' do
522	530	user = new_user :login => 'testuser', :password => 'test123', :password_confirmation => 'test123'
523		- assert_nil User.authenticate('testuser', 'test123')
	531	+ assert_raises User::UserNotActivated do
	532	+ User.authenticate('testuser', 'test123')
	533	+ end
524	534	end
525	535
526	536	should 'have activation code but no activated at when created' do
...	...