devise-two-factor icon indicating copy to clipboard operation
devise-two-factor copied to clipboard

Remove duplicate regist stratery

Open pdkproitf opened this issue 7 years ago • 1 comments

I'm working on an app use two_factor_backupable of devise-two-factor. It really helpful, thank for that!

I detected there are two problems may need to be fixed.

Problem 01: Remove duplicate of registertwo_factor_authenticatable strategy.

The gem registers two_factor_authenticatable strategy on generations.

  • The first one is add two_factor_authenticatable into model.
  • The second one is inject strategy into warden config. screen shot 2018-08-28 at 1 03 07 am

Problem 02: It happens again with two_factor_backupable in another way.

The installation instruction of two_factor_backupable in README.md file requires to add two_factor_backupable to both model and Devise initializer.

Checking on waden, it is duplicated.

screen shot 2018-08-27 at 5 04 35 pm So I think the requirement to add `:two_factor_backupable` into devise initializer is redundant.

Why it should be fixed?

By default, thedevise gem will look through and run all of the strategies. So the strategy will be executed 2 times. It definitely takes time and may generate the unexpected problem.

EX: The devise-two-factor force cascade to the next strategy if the current one fails. This is the cause of failed attempt increase multi-time on one user request.

Ruby version: ruby 2.5.1p57 Rails version: Rails 5.2.0

pdkproitf avatar Aug 27 '18 18:08 pdkproitf

This looks like a good catch! And I can confirm this finding.

Before:

Warden::Proxy:70296438841060 @config={:default_scope=>:admin, :scope_defaults=>{}, :default_strategies=>{:admin=>[
:two_factor_backupable,
:two_factor_authenticatable,
:two_factor_backupable,
:two_factor_authenticatable,
:rememberable
]}, :intercept_401=>false, :failure_app=>#<Devise::Delegator:0x00007fde51e18c78>}

After removing the calls from the initializer (but still listing them in the model):

@config={:default_scope=>:admin, :scope_defaults=>{}, :default_strategies=>{:admin=>[
:two_factor_backupable,
:two_factor_authenticatable,
:rememberable
]}, :intercept_401=>false, :failure_app=>#<Devise::Delegator:0x00007fa304fef660>}

This gem is missing an app with tests. If it had one, you could proof with tests, that your changes work as expected.

mediafinger avatar Aug 01 '19 13:08 mediafinger