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

fix: issue with phone number lib being imported when not used #1

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dhruuuuuv
Copy link

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

tbc

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.

@dhruuuuuv dhruuuuuv added the bug Something isn't working label Aug 23, 2023
Copy link
Member

@benhowes benhowes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does the right thing already, I have just made loads of really nitty suggestions, sorry!

two_factor/views/profile.py Outdated Show resolved Hide resolved
two_factor/views/profile.py Outdated Show resolved Hide resolved
two_factor/views/profile.py Outdated Show resolved Hide resolved
@dhruuuuuv dhruuuuuv force-pushed the dhruv/add-phonenumber-compatibility branch from 0fba898 to 0a8d128 Compare August 23, 2023 13:00
@dhruuuuuv dhruuuuuv force-pushed the dhruv/add-phonenumber-compatibility branch from 0a8d128 to c480de6 Compare August 23, 2023 14:00
@dhruuuuuv dhruuuuuv force-pushed the dhruv/add-phonenumber-compatibility branch from c480de6 to a51ed39 Compare August 23, 2023 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants