jazzband / django-two-factor-auth

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

On a clean install, migrations are not in sync with code #587

Closed jessamynsmith closed 1 year ago

jessamynsmith commented 1 year ago

Expected Behavior

I would expect that when creating a new project, installing django-two-factor-auth, and running migrate, that no further migrations would need to be made for the two factor app.

Current Behavior

When installing django-two-factor-auth and running makemigrations, it is generating a migration for the two_factor app:

(venv) % python manage.py migrate      
Operations to perform:
  Apply all migrations: admin, auth, contenttypes, my_app, otp_email, otp_static, otp_totp, sessions, two_factor
Running migrations:
  Applying otp_email.0001_initial... OK
  Applying otp_email.0002_sidechanneldevice_email... OK
  Applying otp_email.0003_emaildevice_email... OK
  Applying otp_email.0004_throttling... OK
  Applying otp_static.0001_initial... OK
  Applying otp_static.0002_throttling... OK
  Applying otp_totp.0001_initial... OK
  Applying otp_totp.0002_auto_20190420_0723... OK
  Applying two_factor.0001_initial... OK
  Applying two_factor.0002_auto_20150110_0810... OK
  Applying two_factor.0003_auto_20150817_1733... OK
  Applying two_factor.0004_auto_20160205_1827... OK
  Applying two_factor.0005_auto_20160224_0450... OK
  Applying two_factor.0006_phonedevice_key_default... OK
  Applying two_factor.0007_auto_20201201_1019... OK
  Applying two_factor.0008_delete_phonedevice... OK
(venv) % python manage.py makemigrations
Migrations for 'two_factor':
  venv/lib/python3.11/site-packages/two_factor/migrations/0009_initial.py
    - Create model PhoneDevice

Possible Solution

If the issue is that a migration is missing, generate and commit it. If the problem is that a model is in the wrong app, perhaps remove it from that app. Note: this issue was introduced in version 1.15.0; if I install 1.14.0 there are no migration issues.

Steps to Reproduce (for bugs)

  1. Follow the installation instructions for django-two-factor auth
  2. Run: python manage.py makemigrations

Context

This issue can be avoided by only ever generating migrations for my own apps, but that is a bit tedious.

Your Environment

rikva commented 1 year ago

Same problem here after upgrading from 1.14.0 to 1.15.0. Downgraded back to 1.14.0 for now until this migration is added.

erlendgit commented 1 year ago

I notice a similar migration named 0008_delete_phonedevice.py. My guess is that the model is to be phased out.

cmile commented 1 year ago

It is not phased out, it has been moved to plugins/phonenumber/models.py, but Django does not seem to look for the migration in plugins/phonenumber/migrations and thinks it needs to create a new one.

sephii commented 1 year ago

Adding two_factor.plugins.phonenumber to INSTALLED_APPS kind of fixes the problem (but then you’ll have phone number authentication enabled).

claudep commented 1 year ago

Should be fixed by 2ff98b514d42db97d7c9fdabe2face86fc6f8188

jaap3 commented 1 year ago

I'm still getting a ghost migration in CI on a project with django-two-factor-auth (on a dependabot PR to upgrade to 1.15.1):

./manage.py makemigrations --dry-run --no-input --check
Migrations for 'two_factor':
  /opt/hostedtoolcache/Python/3.8.16/x64/lib/python3.8/site-packages/two_factor/migrations/0002_phonedevice.py
    - Create model PhoneDevice

Not sure what makes this "phonedevice" model appear?

claudep commented 1 year ago

Ah yes, it looks like #596 is still needed to avoid that (as currently phonenumber plugin views are imported in base urls.py, which is incidentally registering the Phone model in the apps registry).

lukewiwa commented 1 year ago

From experience this is an issue because the new Django default for DEFAULT_AUTO_FIELD in the settings file is "django.db.models.BigAutoField" whereas the phonenumber app has not defined a default. So there is a mismatch and an extra migration file needs to be generated.

If you change your settings DEFAULT_AUTO_FIELD to "django.db.models.AutoField" then the migrations work fine. If we run migrations with this change enabled https://github.com/jazzband/django-two-factor-auth/commit/008dd920f39812e8e59563fd657cd0a4edfe5f2a then it works fine.

So we probably need another release to make sure this is fixed for all new users.

mgax commented 1 year ago

Can confirm, with django-two-factor-auth master, when I run makemigrations in my project, no extra migrations are being generated.

claudep commented 1 year ago

Thanks for testing that!

mgax commented 1 year ago

Thanks for testing that!

Sure thing! But I think it introduced another issue: https://github.com/jazzband/django-two-factor-auth/issues/627.