search

jazzband / django-two-factor-auth

Complete Two-Factor Authentication for Django providing the easiest integration into most Django projects.
MIT License
1.67k stars 446 forks source link

Make phonenumber plugin really a plugin #469

Open claudep opened 2 years ago

claudep commented 2 years ago

In commit 07dac39d35783, we laid the foundation for a new plugin architecture, and the PhoneNumber capability was moved to a plugin. However, there are still places in the main code that expect the phone plugin to be activated.

This is some meta issue to make the phonenumber plugin really optional.

sevdog commented 1 year ago

There is also an import from phonedevice plugin in views.core:

https://github.com/jazzband/django-two-factor-auth/blob/22aaab5c702d44d26ee8bf83c0190aca997e0331/two_factor/views/core.py#L38

Which breaks when importing anything from views package if phonenumbers is not installed:

  File ".virtualenvs/lib/python3.9/site-packages/two_factor/views/__init__.py", line 1, in <module>
    from .core import (
  File ".virtualenvs/lib/python3.9/site-packages/two_factor/views/core.py", line 38, in <module>
    from two_factor.plugins.phonenumber.utils import get_available_phone_methods
  File ".virtualenvs/lib/python3.9/site-packages/two_factor/plugins/phonenumber/utils.py", line 3, in <module>
    import phonenumbers
ModuleNotFoundError: No module named 'phonenumbers'
claudep commented 1 year ago

Sure, probably get_available_phone_methods should be moved (back?) in two_factor.utils?

sevdog commented 1 year ago

Sure, probably get_available_phone_methods should be moved (back?) in two_factor.utils?

Since that method does not rely on other code in the phonenumber plugin it could work.

But IMO it would be better to find a way to have the SetupCompleteView to get extra context using the registry.

Also, now that I was looking for that method (and other imports from that plugin), there is an import from phone plugin also in two_factor.views.profile (which is also imported in __init__ file):

https://github.com/jazzband/django-two-factor-auth/blob/83ce8005f76d37a2b00d5ccda03819619a9ee669/two_factor/views/profile.py#L10-L12

To have a nice plugin system also the ProfileView should get its data from the registry. Otherwive there it would just be hardcoded and not plugin-based.

claudep commented 1 year ago

Please feel free to suggest a PR with your ideas!

swiss-knight commented 1 year ago

Hello,

may I link this SO thread without necessarily opening a new issue as it may be related?

claudep commented 1 year ago

The above SO thread should be fixed by 2ff98b514d42db97d7c9fdabe2face86fc6f8188

peymanslh commented 1 year ago

@claudep I don't know about the current situation of this issue, Is there anything that I can help with?

claudep commented 1 year ago

@peymanslh, I hope @sevdog will continue working on this and suggest a patch.

sevdog commented 1 year ago

If I will work on this I would like to do a quite heavy rework to have a real plugin-based app, where every OTP method is not mandatory (while currently TOTP and Static are not threated as such). This will require some time and may lead to a long PR.