[Django] #32664: Cannot run makemigrations management command without a SECRET_KEY
Django
-------------------------------------+-------------------------------------
Reporter: François | Owner: François Freitag
Freitag |
Type: | Status: assigned
Cleanup/optimization |
Component: Core | Version: dev
(Other) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
I believe #29324 intended to fix this issue.
Steps to reproduce:
{{{
$ cd $(mktemp -d)
$ python -m venv venv
$ source venv/bin/activate
$ pip install 'Django>=3.2'
$ python -m django startproject foo
$ sed -ri '/SECRET_KEY/d' foo/foo/settings.py # Remove SECRET_KEY from
settings
$ PYTHONPATH=foo DJANGO_SETTINGS_MODULE="foo.settings" python -m django
makemigrations --check
}}}
The output is attached.
--
Ticket URL: <https://code.djangoproject.com/ticket/32664>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: | Freitag
Cleanup/optimization | Status: assigned
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* Attachment "err.txt" added.
Django
-------------------------------------+-------------------------------------
Type: | Freitag
Component: Core (Other) | Version: dev
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Florian Apolloner (added)
* status: assigned => closed
* resolution: => needsinfo
Comment:
#29324 fix this issue for management commands that **do not rely** on the
`SECRET_KEY`, as far as I'm aware `check` is not one of them. Have you
tried with a custom management command?
--
Ticket URL: <https://code.djangoproject.com/ticket/32664#comment:1>
Django
-------------------------------------+-------------------------------------
Type: | Freitag
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: needsinfo =>
Comment:
I am using the `makemigrations` command with the `--check` toggle to
verify no new migrations are needed. I don’t think it needs a
`SECRET_KEY`?
--
Ticket URL: <https://code.djangoproject.com/ticket/32664#comment:2>
Django
-------------------------------------+-------------------------------------
Type: | Freitag
Cleanup/optimization | Status: new
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
Here’s a patch that solves the issue for me:
https://github.com/django/django/pull/14282
That may help describe and reproduce the issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/32664#comment:3>
Django
-------------------------------------+-------------------------------------
Type: | Freitag
Cleanup/optimization | Status: new
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
I'm not sure about fixing this. It's a reasonable assumption that
`SECRET_KEY` is necessary for all built-in commands. We cannot not
guarantee that `makemigrations` or any other built-in command will work in
the future without a `SECRET_KEY`.
--
Ticket URL: <https://code.djangoproject.com/ticket/32664#comment:4>
Django
-------------------------------------+-------------------------------------
Type: | Freitag
Cleanup/optimization | Status: new
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by François Freitag):
> We cannot not guarantee that makemigrations or any other built-in
command will work in the future without a SECRET_KEY.
That’s true. I’m not asking for all management commands to work without a
`SECRET_KEY`, but for the commands to fail only when the `SECRET_KEY` is
accessed.
My goal is to avoid defining a `SECRET_KEY` in environments that do not
need it. That’s the same goal as #29324 and corresponding release note
mention https://docs.djangoproject.com/en/3.2/releases/3.2/#security:
> running management commands that do not rely on the SECRET_KEY without
needing to provide a value.
My project (the same as Jon in #29324) works around the limitation by
generating a random string for `SECRET_KEY` when none is available. It is
annoying to maintain logic to create values for unused settings.
A regression in this area isn’t a big deal. IMO, maintaining it on a best-
effort basis is sufficient, and can help simplifying the running on
management commands for other projects.
--
Ticket URL: <https://code.djangoproject.com/ticket/32664#comment:5>
Django
-------------------------------------+-------------------------------------
Type: | Freitag
Cleanup/optimization | Status: new
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: François Freitag (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/32664#comment:6>
Django
-------------------------------------+-------------------------------------
Type: | Freitag
Cleanup/optimization | Status: new
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Florian Apolloner):
I think the proper fix would be to move
`PasswordResetTokenGenerator.secret` to a property so it is only accessed
when actually needed. This would also help tests that change the
`SECRET_KEY` and then wonder why `default_token_generator` does not pick
it up. `Lazy*` should be used as a last resort.
--
Ticket URL: <https://code.djangoproject.com/ticket/32664#comment:7>
Django
-------------------------------------+-------------------------------------
Type: | Freitag
Cleanup/optimization | Status: new
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by François Freitag):
Thanks for the suggestion, it is a good improvement!
I updated the patch https://github.com/django/django/pull/14282 to reflect
it.
Please let me know if that’s what you had in mind?
--
Ticket URL: <https://code.djangoproject.com/ticket/32664#comment:8>
Django
-------------------------------------+-------------------------------------
Type: | Freitag
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: François Freitag => François Freitag
* status: new => assigned
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/32664#comment:9>
Django
-------------------------------------+-------------------------------------
Type: | Freitag
Cleanup/optimization | Status: assigned
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/32664#comment:10>
Django
-------------------------------------+-------------------------------------
Type: | Freitag
Component: Core (Other) | Version: dev
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"6b0b3eafd6674305f7b09dbeb1aa0d8d8bb4df57" 6b0b3ea]:
{{{
#!CommitTicketReference repository=""
revision="6b0b3eafd6674305f7b09dbeb1aa0d8d8bb4df57"
Fixed #32664 -- Made PasswordResetTokenGenerator.secret validation lazy.
Django apps initialization to run management command triggers the admin
autodiscovery. Importing django.contrib.auth.tokens creates an instance
of PasswordResetTokenGenerator which required a SECRET_KEY.
For several management commands, the token generator is unused. It
should only complain about a missing SECRET_KEY when it is used.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32664#comment:11>
Django
-------------------------------------+-------------------------------------
Type: | Freitag
Cleanup/optimization | Status: closed
Component: Core (Other) | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Michael Manganiello):
Considering Django 3.2
[https://docs.djangoproject.com/en/3.2/releases/3.2/#security release
notes] mention:
> The `SECRET_KEY` setting is now checked for a valid value upon first
access, rather than when settings are first loaded. This enables running
management commands that do not rely on the `SECRET_KEY` without needing
to provide a value. As a consequence of this, calling `configure()`
without providing a valid `SECRET_KEY`, and then going on to access
`settings.SECRET_KEY` will now raise an `ImproperlyConfigured` exception.
Has it been considered to port this fix to the 3.2 branch? As it is today,
`PasswordResetTokenGenerator` initialization breaks the execution of
management commands when trying to follow the recommendation provided by
the release notes.
--
Ticket URL: <https://code.djangoproject.com/ticket/32664#comment:12>
Django
-------------------------------------+-------------------------------------
Type: | Freitag
Cleanup/optimization | Status: closed
Component: Core (Other) | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
Replying to [comment:12 Michael Manganiello]:
> Has it been considered to port this fix to the 3.2 branch? As it is
today, `PasswordResetTokenGenerator` initialization breaks the execution
of management commands when trying to follow the recommendation provided
by the release notes.
This release note is rather about custom management commands. It's
arguable (as you see from the above discussion) that build-in commands
that rely on `django.contrib.auth` module should not require the
`SECRET_KEY`. However, we decided to do the best we can to make
`SECRET_KEY` validation lazy also for built-in management commands. This
ticket is a cleanup not a bug in 948a874425e7d999950a8fa3b6598d9e34a4b861
that's why it doesn't qualify for a backport.
--
Ticket URL: <https://code.djangoproject.com/ticket/32664#comment:13>