Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove duplicate regist stratery #135

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

pdkproitf
Copy link

@pdkproitf pdkproitf commented Aug 27, 2018

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

@mediafinger
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants