restructure UserSerializer.create and UserSerializer.update

authentik/core/api/users.py

@@ -182,6 +182,8 @@ class UserSerializer(ModelSerializer):
         return RoleSerializer(instance.roles, many=True).data
 
     def __init__(self, *args, **kwargs):
+        """Setting password and permissions directly are allowed only in blueprints. The `create`
+        and `update` methods are adjusted accordingly."""
         super().__init__(*args, **kwargs)
         if SERIALIZER_CONTEXT_BLUEPRINT in self.context:
             self.fields["password"] = CharField(required=False, allow_null=True)
@@ -192,40 +194,47 @@ class UserSerializer(ModelSerializer):
             )
 
     def create(self, validated_data: dict) -> User:
-        """If this serializer is used in the blueprint context, we allow for
-        directly setting a password. However should be done via the `set_password`
-        method instead of directly setting it like rest_framework."""
         password = validated_data.pop("password", None)
         password_hash = validated_data.pop("password_hash", None)
-        self._validate_password_inputs(password, password_hash)
-        perms_qs = Permission.objects.filter(
-            codename__in=[x.split(".")[1] for x in validated_data.pop("permissions", [])]
-        ).values_list("content_type__app_label", "codename")
-        perms_list = [f"{ct}.{name}" for ct, name in list(perms_qs)]
+        permissions = validated_data.pop("permissions", [])
+        if SERIALIZER_CONTEXT_BLUEPRINT in self.context:
+            self._validate_password_inputs(password, password_hash)
+
         instance: User = super().create(validated_data)
-        self._set_password(instance, password, password_hash)
-        instance.assign_perms_to_managed_role(perms_list)
+
+        if SERIALIZER_CONTEXT_BLUEPRINT in self.context:
+            self._set_password(instance, password, password_hash)
+            perms_qs = Permission.objects.filter(
+                codename__in=[x.split(".")[1] for x in permissions]
+            ).values_list("content_type__app_label", "codename")
+            perms_list = [f"{ct}.{name}" for ct, name in list(perms_qs)]
+            instance.assign_perms_to_managed_role(perms_list)
+
+        self._ensure_password_not_empty(instance)
         return instance
 
     def update(self, instance: User, validated_data: dict) -> User:
-        """Same as `create` above, set the password directly if we're in a blueprint
-        context"""
         password = validated_data.pop("password", None)
         password_hash = validated_data.pop("password_hash", None)
-        self._validate_password_inputs(password, password_hash)
-        perms_qs = Permission.objects.filter(
-            codename__in=[x.split(".")[1] for x in validated_data.pop("permissions", [])]
-        ).values_list("content_type__app_label", "codename")
-        perms_list = [f"{ct}.{name}" for ct, name in list(perms_qs)]
+        permissions = validated_data.pop("permissions", [])
+        if SERIALIZER_CONTEXT_BLUEPRINT in self.context:
+            self._validate_password_inputs(password, password_hash)
+
         instance = super().update(instance, validated_data)
-        self._set_password(instance, password, password_hash)
-        instance.assign_perms_to_managed_role(perms_list)
+
+        if SERIALIZER_CONTEXT_BLUEPRINT in self.context:
+            self._set_password(instance, password, password_hash)
+            perms_qs = Permission.objects.filter(
+                codename__in=[x.split(".")[1] for x in permissions]
+            ).values_list("content_type__app_label", "codename")
+            perms_list = [f"{ct}.{name}" for ct, name in list(perms_qs)]
+            instance.assign_perms_to_managed_role(perms_list)
+
+        self._ensure_password_not_empty(instance)
         return instance
 
     def _validate_password_inputs(self, password: str | None, password_hash: str | None):
         """Validate mutually-exclusive password inputs before any model mutation."""
-        if SERIALIZER_CONTEXT_BLUEPRINT not in self.context:
-            return
         if password is not None and password_hash is not None:
             raise ValidationError(_("Cannot set both password and password_hash. Use only one."))
         if password_hash is None:
@@ -239,20 +248,21 @@ class UserSerializer(ModelSerializer):
             ) from exc
 
     def _set_password(self, instance: User, password: str | None, password_hash: str | None = None):
-        """Set password of user if we're in a blueprint context, and if it's an empty
-        string then use an unusable password. Supports both plaintext password and
-        pre-hashed password via password_hash parameter."""
-        if SERIALIZER_CONTEXT_BLUEPRINT in self.context:
-            if password_hash is not None:
-                instance.set_password_from_hash(password_hash)
-                instance.save()
-                return
-            elif password:
-                instance.set_password(password)
-                instance.save()
-                return
+        """Set password from plain text or hash"""
+        if password_hash is not None:
+            instance.set_password_from_hash(password_hash)
+            instance.save()
+            return
+        elif password:
+            instance.set_password(password)
+            instance.save()
+            return
 
-        if not instance.password:
+    def _ensure_password_not_empty(self, instance: User):
+        """In principle there should be no issue with storing an empty string in the password field,
+        since Django will treat that as an unusable password. However, let's make it explicit just
+        to be extra sure."""
+        if len(instance.password) == 0:
             instance.set_unusable_password()
             instance.save()