fix: issue with PhoneDevice being imported when not enabled

[dhruuuuuv]

Description

Crash was being caused by assuming phonenumbers was being used, even when not enabled. This PR checks to see if the library is included, for backwards compatibility, before including it.

Motivation and Context

When you view the 2FA "profile" page, it calls backup_phones() directly from the phone plugin, which causes a from .models import PhoneDevice

This then means that the PhoneDevice model is known to django's model registry and it gets associated with the two_factor rather than two_factor.plugins.phonenumber.

It does not cause a problem right after because the PhoneDevice sits on the end of the list of available models for checking to see if a user has 2FA.

Later on though in device_classes, when you look at a user with no 2FA methods it crashes because it tries to look up data on that non-existent table.

In short: After the first call to backup_phones(), the django process then erroneously has PhoneDevice in it's model registry. After that, django_otp.device_classes picks up PhoneDevice as one of the available options and any time devices_for_user is called, we see the error.

Open Issues

https://stackoverflow.com/questions/73104958/no-such-table-two-factor-phonedevice-when-using-django-two-factor-auth-1-14-0

How Has This Been Tested?

Tested accessing profile page without phone number 2FA enabled.

Screenshots (if appropriate):

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

[claudep]

Ideally, if we could have a test to avoid any regression, it would be great. The test might be more difficult to write than the fix 😓

[benhowes]

@claudep updated to add tests for with/without the phonenumber plugin enabled.

[codecov]

Codecov Report

Merging #648 (7473099) into master (7339c3c) will increase coverage by 0.05%.
Report is 1 commits behind head on master.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #648      +/-   ##
==========================================
+ Coverage   95.38%   95.44%   +0.05%     
==========================================
  Files          75       76       +1     
  Lines        3252     3293      +41     
  Branches      372      264     -108     
==========================================
+ Hits         3102     3143      +41     
  Misses        119      119              
  Partials       31       31              

Files Changed	Coverage Δ
tests/test_views_profile.py	100.00% <100.00%> (ø)
two_factor/views/profile.py	100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[claudep]

I'm not convinced this will be sufficient, because you still import backup_phones and get_available_phone_methods at the top of the file, and there is an import phonenumbers in plugins/phonenumber/utils.py. Isn't the idea to avoid the phonenumbers import?

[benhowes]

It's working because the PhoneDevice model is only imported by backup_phones at runtime here.

We did consider adding the import inside the block which only runs when phonenumbers is enabled and certainly could move it there if that's considered cleaner, but it does not seem to be required with the current implementation of backup_phones. Happy to change it around if you'd rather have it that way?

Edit: I see what you mean now - that's not the problem that this PR seeks to fix, it's simply fixing that device_classes() starts including PhoneDevice in the list of classes after the model has been imported, even though the phonenumber plugin is not enabled which causes a 500 error.

[claudep]

Ah, thanks for the clarification.