From 2a9cb6c10befffbc8cea8795b6dd6960990a92c9 Mon Sep 17 00:00:00 2001 From: x4d3 Date: Tue, 29 Nov 2016 18:26:30 +0000 Subject: [PATCH 1/2] [MNOE-209] Unlimited failed attempts of login user session is not returning either not found, either a user with password_not valid if the password is valid Resource needed to be returned properly to be able to be updated in the devise-3.5.10/lib/devise/models/lockable.rb --- api/spec/requests/devise/authentication_spec.rb | 2 +- core/app/models/mno_enterprise/user.rb | 12 ++++++------ core/lib/devise/strategies/remote_authenticatable.rb | 12 +++++++----- 3 files changed, 14 insertions(+), 12 deletions(-) diff --git a/api/spec/requests/devise/authentication_spec.rb b/api/spec/requests/devise/authentication_spec.rb index 782c8f096..fdd53d72a 100644 --- a/api/spec/requests/devise/authentication_spec.rb +++ b/api/spec/requests/devise/authentication_spec.rb @@ -3,7 +3,7 @@ module MnoEnterprise RSpec.describe "Remote Authentication", type: :request do - let(:user) { build(:user) } + let(:user) { build(:user, password_valid: true) } before { api_stub_for(get: "/users/#{user.id}", response: from_api(user)) } before { api_stub_for(put: "/users/#{user.id}", response: from_api(user)) } diff --git a/core/app/models/mno_enterprise/user.rb b/core/app/models/mno_enterprise/user.rb index 09b89f812..9abc1178c 100644 --- a/core/app/models/mno_enterprise/user.rb +++ b/core/app/models/mno_enterprise/user.rb @@ -51,7 +51,7 @@ class User < BaseResource :last_sign_in_ip, :confirmation_token, :confirmed_at, :confirmation_sent_at, :unconfirmed_email, :failed_attempts, :unlock_token, :locked_at, :name, :surname, :company, :phone, :phone_country_code, :geo_country_code, :geo_state_code, :geo_city, :website, :orga_on_create, :sso_session, :current_password_required, :admin_role, - :api_key, :api_secret, :developer, :kpi_enabled, :external_id, :meta_data + :api_key, :api_secret, :developer, :kpi_enabled, :external_id, :meta_data, :password_valid define_model_callbacks :validation #required by Devise @@ -94,14 +94,14 @@ class User < BaseResource # The auth_hash includes an email and password # Return nil in case of failure def self.authenticate(auth_hash) - u = self.post("user_sessions", auth_hash) - + # retro compatibilty issue: previous version were returning nil if the user was not found or if the password was invalid + # see https://maestrano.atlassian.net/browse/MNOE-209 + # we now validate that the password is valid in /core/lib/devise/strategies/remote_authenticatable.rb + u = self.post("user_sessions", auth_hash.merge(validate_password: true)) if u && u.id u.clear_attribute_changes! - return u end - - nil + return u end diff --git a/core/lib/devise/strategies/remote_authenticatable.rb b/core/lib/devise/strategies/remote_authenticatable.rb index 279c6b5ef..d8275bbe1 100644 --- a/core/lib/devise/strategies/remote_authenticatable.rb +++ b/core/lib/devise/strategies/remote_authenticatable.rb @@ -1,19 +1,20 @@ module Devise module Strategies class RemoteAuthenticatable < Authenticatable - + # def valid? # true || params[scope] # end - + # For an example check : https://github.com/plataformatec/devise/blob/master/lib/devise/strategies/database_authenticatable.rb # Method called by warden to authenticate a resource. def authenticate! # authentication_hash doesn't include the password auth_params = params[scope] - + + # mapping.to is a wrapper over the resource model - resource = mapping.to.new + resource = mapping.to.authenticate(auth_params) return fail! unless resource @@ -24,9 +25,10 @@ def authenticate! # # If the block returns true the resource will be loged in # If the block returns false the authentication will fail! - if validate(resource){ resource = resource.remote_authentication(auth_params) } + if validate(resource){ resource.password_valid } success!(resource) end + end end end From b4709977969369dd4cdb5981d76f4ae5aca1d313 Mon Sep 17 00:00:00 2001 From: x4d3 Date: Tue, 14 Nov 2017 15:40:13 +0000 Subject: [PATCH 2/2] Apply code review --- core/lib/devise/strategies/remote_authenticatable.rb | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/core/lib/devise/strategies/remote_authenticatable.rb b/core/lib/devise/strategies/remote_authenticatable.rb index d8275bbe1..7dc2229b5 100644 --- a/core/lib/devise/strategies/remote_authenticatable.rb +++ b/core/lib/devise/strategies/remote_authenticatable.rb @@ -14,7 +14,7 @@ def authenticate! # mapping.to is a wrapper over the resource model - resource = mapping.to.authenticate(auth_params) + resource = mapping.to.new.remote_authentication(auth_params) return fail! unless resource @@ -25,10 +25,9 @@ def authenticate! # # If the block returns true the resource will be loged in # If the block returns false the authentication will fail! - if validate(resource){ resource.password_valid } + if validate(resource) { resource.password_valid } success!(resource) end - end end end @@ -37,10 +36,10 @@ def authenticate! Warden::Strategies.add :remote_authenticatable, Devise::Strategies::RemoteAuthenticatable Devise.add_module :remote_authenticatable, strategy: true, controller: :sessions, route: :session -Warden::Manager.after_authentication do |user,auth,opts| +Warden::Manager.after_authentication do |user, auth, opts| Rails.cache.delete(['user', user.to_key]) if user end -Warden::Manager.before_logout do |user,auth,opts| +Warden::Manager.before_logout do |user, auth, opts| Rails.cache.delete(['user', user.to_key]) if user end