Github/authentik
1
0
Fork 0
mirror of https://github.com/goauthentik/authentik.git synced 2026-08-30 18:51:39 -07:00
Code Issues Packages Projects Releases Wiki Activity

core: address review feedback on password devices

Browse Source
This commit is contained in:
Dominic R
2026-08-26 13:07:30 -04:00
parent 513d48be54
commit f534bfef22
8 changed files with 75 additions and 100 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
authentik/core/migrations/0064_remove_user_password.py → authentik/core/migrations/0065_remove_user_password.py
View File

@@ -6,11 +6,14 @@ from django.db import migrations, models
class Migration(migrations.Migration):
dependencies = [
("authentik_core", "0063_actor"),
("authentik_stages_password", "0012_migrate_user_passwords"),
("authentik_core", "0064_user_authentik_c_usernam_2f0e4b_idx"),
("authentik_stages_password", "0011_passworddevice"),
]
operations = [
# The columns stay in the database until a future release so this release can be
# downgraded, but nothing writes them anymore, so they have to become nullable for
# user inserts to keep working.
migrations.AlterField(
model_name="user",
name="password",

66
authentik/core/models.py
View File

@@ -370,10 +370,8 @@ class User(SerializerModel, AttributesMixin, AbstractUser):
# (This knowingly violates the Liskov substitution principle. It is better to fail loudly.)
user_permissions = None
# Hash staged by the `password` setter, written to the password device on save.
_pending_password_hash: str | None = None
# Change date staged alongside the hash, written to the password device on save.
_pending_password_change_date: datetime | None = None
# Set by the `password` setter when the password device has unsaved changes.
_password_device_dirty = False
uuid = models.UUIDField(default=uuid4, editable=False, unique=True)
name = models.TextField(help_text=_("User's display name."))
@@ -413,14 +411,15 @@ class User(SerializerModel, AttributesMixin, AbstractUser):
return self.username
def save(self, *args, **kwargs):
if self._pending_password_hash is None:
if self._password_device_dirty:
# The user and their password device hold what used to be a single row, so
# they have to be written together.
with transaction.atomic():
super().save(*args, **kwargs)
self.password_device.save()
self._password_device_dirty = False
else:
super().save(*args, **kwargs)
return
# The user and their password device hold what used to be a single row, so they
# have to be written together.
with transaction.atomic():
super().save(*args, **kwargs)
self._save_pending_password()
@staticmethod
def default_path() -> str:
@@ -580,8 +579,6 @@ class User(SerializerModel, AttributesMixin, AbstractUser):
Declaring this property also removes the `password` field that would otherwise be
inherited from Django's AbstractBaseUser, so the hash has a single home.
"""
if self._pending_password_hash is not None:
return self._pending_password_hash
try:
return self.password_device.password
except ObjectDoesNotExist:
@@ -589,37 +586,26 @@ class User(SerializerModel, AttributesMixin, AbstractUser):
@password.setter
def password(self, password_hash: str):
"""Stage a password hash. As with any other field, `save()` persists it."""
self._pending_password_hash = password_hash
"""Set a password hash on the password device. As with any other field, `save()`
persists it."""
from authentik.stages.password.models import PasswordDevice
try:
device = self.password_device
except ObjectDoesNotExist:
device = PasswordDevice(user=self, name="Password")
self.password_device = device
device.password = password_hash
self._password_device_dirty = True
@property
def password_change_date(self) -> datetime:
"""Date the user's password was last changed, stored on their password device."""
if self._pending_password_change_date is not None:
return self._pending_password_change_date
try:
return self.password_device.password_change_date
except ObjectDoesNotExist:
return self.date_joined
def _save_pending_password(self):
"""Write a staged password hash to this user's password device."""
from authentik.stages.password.models import PasswordDevice
if self._pending_password_hash is None:
return
defaults = {"password": self._pending_password_hash}
if self._pending_password_change_date is not None:
defaults["password_change_date"] = self._pending_password_change_date
device, _ = PasswordDevice.objects.update_or_create(
user=self,
defaults=defaults,
create_defaults={**defaults, "name": "Password"},
)
self.password_device = device
self._pending_password_hash = None
self._pending_password_change_date = None
def set_password(self, raw_password, signal=True, sender=None, request=None):
if self.pk and signal:
from authentik.core.signals import password_changed
@@ -627,8 +613,9 @@ class User(SerializerModel, AttributesMixin, AbstractUser):
if not sender:
sender = self
password_changed.send(sender=sender, user=self, password=raw_password, request=request)
self._pending_password_change_date = now()
return super().set_password(raw_password)
result = super().set_password(raw_password)
self.password_device.password_change_date = now()
return result
def set_password_from_hash(self, password_hash: str, signal=True, sender=None, request=None):
"""Set password directly from a pre-hashed value.
@@ -646,7 +633,7 @@ class User(SerializerModel, AttributesMixin, AbstractUser):
sender = self
password_hash_changed.send(sender=sender, user=self, request=request)
self.password = password_hash
self._pending_password_change_date = now()
self.password_device.password_change_date = now()
def check_password(self, raw_password: str) -> bool:
"""
@@ -660,7 +647,8 @@ class User(SerializerModel, AttributesMixin, AbstractUser):
# Password hash upgrades shouldn't be considered password changes, so only the
# device is written and password_change_date is left alone.
self.password = make_password(raw_password)
self._save_pending_password()
self.password_device.save()
self._password_device_dirty = False
return check_password(raw_password, self.password, setter)

19
authentik/core/tests/test_users.py
View File

@@ -3,6 +3,7 @@
from unittest.mock import patch
from django.contrib.auth.hashers import make_password
from django.db import IntegrityError, transaction
from django.http import HttpRequest
from django.test.testcases import TestCase
@@ -82,19 +83,21 @@ class TestUsers(TestCase):
user = User.objects.create(username=generate_id())
self.assertEqual(user.locale(), "")
def test_password_stored_on_device(self):
"""Test a new user's password is written to their password device"""
user = User.objects.create_user(username=generate_id(), password="initial") # nosec
self.assertEqual(user.password, PasswordDevice.objects.get(user=user).password)
self.assertTrue(User.objects.get(pk=user.pk).check_password("initial"))
def test_password_change_updates_device(self):
"""Test changing a password updates the device instead of adding another one"""
"""Test changing a password updates the user's single password device"""
user = User.objects.create_user(username=generate_id(), password="initial") # nosec
user.set_password("changed")
user.save()
self.assertEqual(PasswordDevice.objects.filter(user=user).count(), 1)
self.assertTrue(User.objects.get(pk=user.pk).check_password("changed"))
user = User.objects.get(pk=user.pk)
self.assertTrue(user.check_password("changed"))
self.assertFalse(user.check_password("initial"))
def test_second_password_device_rejected(self):
"""Test the database only allows one password device per user"""
user = User.objects.create_user(username=generate_id(), password="initial") # nosec
with self.assertRaises(IntegrityError), transaction.atomic():
PasswordDevice.objects.create(user=user, name="Password", password="second")
def test_password_staged_until_save(self):
"""Test a password is only written to the device once the user is saved"""

4
authentik/enterprise/providers/ssf/signals.py
View File

@@ -131,6 +131,9 @@ device_type_map = {
@receiver(post_save)
def ssf_device_post_save(sender: type[Model], instance: Device, created: bool, **_):
# A password device holds the user's password, not a second factor, so it must not
# emit CAEP credential-change events for authenticators. Password changes are already
# reported by ssf_password_changed_cred_change.
if not isinstance(instance, Device) or isinstance(instance, PasswordDevice):
return
if not instance.confirmed:
@@ -158,6 +161,7 @@ def ssf_device_post_save(sender: type[Model], instance: Device, created: bool, *
@receiver(post_delete)
def ssf_device_post_delete(sender: type[Model], instance: Device, **_):
# See ssf_device_post_save: password devices are not authenticators.
if not isinstance(instance, Device) or isinstance(instance, PasswordDevice):
return
if not instance.confirmed:

9
authentik/lib/migrations.py
View File

@@ -41,11 +41,9 @@ def fallback_names(app: str, model: str, field: str):
return migrator
def progress_bar(iterable: Iterable, total: int | None = None):
def progress_bar(iterable: Iterable):
"""Call in a loop to create terminal progress bar
https://stackoverflow.com/questions/3173320/text-progress-bar-in-the-console
`total` must be given for iterables without a length, such as generators."""
https://stackoverflow.com/questions/3173320/text-progress-bar-in-the-console"""
prefix = "Writing: "
suffix = " finished"
@@ -54,8 +52,7 @@ def progress_bar(iterable: Iterable, total: int | None = None):
fill = "█"
print_end = "\r"
if total is None:
total = len(iterable)
total = len(iterable)
if total < 1:
return

22
authentik/stages/password/migrations/0011_passworddevice.py
View File

@@ -1,4 +1,4 @@
# Generated by Django 5.2.17 on 2026-08-13 00:31
# Generated by Django 5.2.17 on 2026-08-26 16:35
import django.db.models.deletion
import django.utils.timezone
@@ -51,6 +51,26 @@ class Migration(migrations.Migration):
"verbose_name": "Password Device",
"verbose_name_plural": "Password Devices",
"abstract": False,
"indexes": [
models.Index(
fields=["password_change_date"], name="authentik_s_passwor_cf6692_idx"
)
],
},
),
migrations.RunSQL(
sql="""
INSERT INTO authentik_stages_password_passworddevice (
created, last_updated, name, confirmed, password, password_change_date, user_id
)
SELECT now(), now(), 'Password', TRUE, password, password_change_date, id
FROM authentik_core_user;
""",
reverse_sql="""
UPDATE authentik_core_user u
SET password = d.password, password_change_date = d.password_change_date
FROM authentik_stages_password_passworddevice d
WHERE d.user_id = u.id;
""",
),
]

44
authentik/stages/password/migrations/0012_migrate_user_passwords.py
View File

@@ -1,44 +0,0 @@
from django.apps.registry import Apps
from django.db import migrations
from django.db.backends.base.schema import BaseDatabaseSchemaEditor
from authentik.lib.migrations import progress_bar
from authentik.lib.utils.db import chunked_queryset
def create_password_devices(apps: Apps, schema_editor: BaseDatabaseSchemaEditor):
User = apps.get_model("authentik_core", "User")
PasswordDevice = apps.get_model("authentik_stages_password", "PasswordDevice")
db_alias = schema_editor.connection.alias
users = User.objects.using(db_alias).only(
"pk", "password", "password_change_date", "last_login"
)
print("\nMigrating user passwords to password devices, this might take a couple of minutes...")
batch = []
for user in progress_bar(chunked_queryset(users), total=users.count()):
batch.append(
PasswordDevice(
user_id=user.pk,
name="Password",
password=user.password,
password_change_date=user.password_change_date,
last_used=user.last_login,
)
)
if len(batch) >= 1000:
PasswordDevice.objects.using(db_alias).bulk_create(batch)
batch = []
if batch:
PasswordDevice.objects.using(db_alias).bulk_create(batch)
class Migration(migrations.Migration):
dependencies = [
("authentik_stages_password", "0011_passworddevice"),
]
# Irreversible on purpose: the User.password column is no longer written once devices
# exist, so reversing would revert users to stale or missing password hashes.
operations = [migrations.RunPython(create_password_devices)]

4
authentik/stages/password/models.py
View File

@@ -103,6 +103,10 @@ class PasswordDevice(Device):
password = models.CharField(max_length=128)
password_change_date = models.DateTimeField(default=now)
def __str__(self):
return str(self.name) or str(self.user_id)
class Meta(Device.Meta):
verbose_name = _("Password Device")
verbose_name_plural = _("Password Devices")
indexes = [models.Index(fields=["password_change_date"])]