update_failed_logins_count is called twice when login failed

[kissrobber]

Failed_logins_count is incremented by 2 when login failed.

login failed request SQL log

CACHE User Load (0.0ms)  SELECT  `users`.* FROM `users` WHERE `users`.`type` IN ('User') AND `users`.`deleted_at` IS NULL AND `users`.`email` = '[email protected]' ORDER BY `users`.`id` ASC LIMIT 1  [["LIMIT", 1]]
SQL (5.8ms)  UPDATE `users` SET `failed_logins_count` = COALESCE(`failed_logins_count`, 0) + 1 WHERE `users`.`type` IN ('User') AND `users`.`id` = 1702
User Load (0.7ms)  SELECT  `users`.* FROM `users` WHERE `users`.`type` IN ('User') AND `users`.`deleted_at` IS NULL AND `users`.`email` = '[email protected]' ORDER BY `users`.`id` ASC LIMIT 1
SQL (3.2ms)  UPDATE `users` SET `failed_logins_count` = COALESCE(`failed_logins_count`, 0) + 1 WHERE `users`.`type` IN ('User') AND `users`.`id` = 1702

Config.after_failed_login and Config.after_login are set doubly

(byebug) Rails.application.config.sorcery.submodules
[:user_activation, :reset_password, :activity_logging, :brute_force_protection, :external]
(byebug) Sorcery::Controller::Config.after_login
[:register_login_time_to_db, :register_last_ip_address, :reset_failed_logins_count!, :register_login_time_to_db, :register_last_ip_address, :reset_failed_logins_count!]
(byebug) Sorcery::Controller::Config.after_failed_login
[:update_failed_logins_count!, :update_failed_logins_count!]

[joshbuker]

I was under the impression that ActionController::Base and ActionController::API didn't inherit from each other in any way. Seems weird that including the controller in each separately would be the cause of a callback duplication.

@kissrobber would you mind helping me understand what's going wrong here?

[kissrobber]

in rails
Both defined?(ActionController::API) and defined?(ActionController::Base) are true.
https://github.com/rails/rails/tree/master/actionpack/lib/action_controller

in sorcery
Both ActionController::API.send(:include, Sorcery::Controller) and ActionController::Base.send(:include, Sorcery::Controller) fire Sorcery::Controller.included
https://github.com/Sorcery/sorcery/blob/master/lib/sorcery/controller.rb#L3
↓
https://github.com/Sorcery/sorcery/blob/master/lib/sorcery/controller/submodules/brute_force_protection.rb#L11

[kissrobber]

Sorry I closed this PR by mistake.

[joshbuker]

@kissrobber That shouldn't cause a duplication issue though, API and Base don't inherit each other as far as I can tell. This change would break applications that use API for a subset of controllers, and Base for the rest.

Can you provide a minimal example controller so I can replicate this issue and try it out?

[kissrobber]

API and Base don't inherit each other

That doesn't matter here.

In the original code,
both the if conditions are evaluated as true regardless whether they inherit each other or not.

      if defined?(ActionController::API) # -> true
        ActionController::API.send(:include, Sorcery::Controller) # <- always called
      end

      if defined?(ActionController::Base) # -> true
        ActionController::Base.send(:include, Sorcery::Controller) # <- always called
        ActionController::Base.helper_method :current_user
        ActionController::Base.helper_method :logged_in?
      end

Then both ActionController::*.send(:include, Sorcery::Controller) is called always.

So, this called twice

          Config.after_login << :reset_failed_logins_count!
          Config.after_failed_login << :update_failed_logins_count!

https://github.com/Sorcery/sorcery/blob/master/lib/sorcery/controller/submodules/brute_force_protection.rb#L11

The Config sorcery/lib/sorcery/controller/config.rb is shared with them.

Can you provide a minimal example controller so I can replicate this issue and try it out?

You mean it has been confirmed that this issue doesn't occur in your code?
In that case, I will do that sometime soon 👍

[joshbuker]

Ahh, I understand the issue now. I thought it was some weird inheritance, but it's just the self.included code not safe-guarding against being included by different controllers. You're right, although I think the better solution here is updating the self.included method to not double down if it gets included multiple times. I'll also double check the others and fix those as well.

Don't worry about the example, I was just misunderstanding the problem.

[joshbuker]

Closes #172

[petevanwesep]

Is @kissrobber's solution a reasonable monkey patch for this issue? Need a stopgap upgrade in response to CVE-2019-5418 and CVE-2019-5419. Thanks!

[joshbuker]

Sorry for the delay, will merge this into master after CI passes.

Not fully convinced this solves all the potential issues with double inclusion, but I confirmed locally that this prevents the double callbacks for the controller submodules.