ref: remove PickledObjectField from Authenticator model (#93515)

<!-- Describe your PR here. -->

Author: asottile-sentry

migrations_lockfile.txt

@@ -25,7 +25,7 @@ preprod: 0004_add_django_jsonfield
 
 replays: 0001_squashed_0005_drop_replay_index
 
-sentry: 0928_move_notifications_models
+sentry: 0929_no_pickle_authenticator
 
 social_auth: 0001_squashed_0002_default_auto_field
 

pyproject.toml

@@ -185,6 +185,7 @@ module = [
     "sentry.api.serializers.rest_framework.group_notes",
     "sentry.audit_log.services.*",
     "sentry.auth.access",
+    "sentry.auth.authenticators.recovery_code",
     "sentry.auth.manager",
     "sentry.auth.services.*",
     "sentry.auth.view",

src/sentry/auth/authenticators/recovery_code.py

@@ -1,14 +1,19 @@
+from __future__ import annotations
+
 import hmac
 from base64 import b32encode
 from binascii import hexlify
 from hashlib import sha1
 from os import urandom
+from typing import TYPE_CHECKING, Any
 
-from django.utils.encoding import force_bytes
 from django.utils.translation import gettext_lazy as _
 
 from .base import AuthenticatorInterface
 
+if TYPE_CHECKING:
+    from sentry.users.models.authenticator import Authenticator
+
 
 class RecoveryCodeInterface(AuthenticatorInterface):
     """A backup interface that is based on static recovery codes."""
@@ -26,20 +31,20 @@ class RecoveryCodeInterface(AuthenticatorInterface):
     remove_button = None
     is_backup_interface = True
 
-    def __init__(self, authenticator=None) -> None:
-        AuthenticatorInterface.__init__(self, authenticator)
+    def __init__(self, authenticator: Authenticator | None = None) -> None:
+        super().__init__(authenticator)
 
-    def get_codes(self):
+    def get_codes(self) -> list[str]:
         rv = []
         if self.is_enrolled():
-            h = hmac.new(key=force_bytes(self.config["salt"]), msg=None, digestmod=sha1)
+            h = hmac.new(key=self.config["salt"].encode(), msg=None, digestmod=sha1)
             for x in range(10):
-                h.update(("%s|" % x).encode("utf-8"))
-                rv.append(b32encode(h.digest())[:8].decode("utf-8"))
+                h.update(f"{x}|".encode())
+                rv.append(b32encode(h.digest())[:8].decode())
         return rv
 
-    def generate_new_config(self):
-        salt = hexlify(urandom(16))
+    def generate_new_config(self) -> dict[str, Any]:
+        salt = hexlify(urandom(16)).decode()
         return {"salt": salt, "used": 0}
 
     def regenerate_codes(self, save: bool = True) -> None:
@@ -52,7 +57,7 @@ def regenerate_codes(self, save: bool = True) -> None:
         if save:
             self.authenticator.save()
 
-    def validate_otp(self, otp) -> bool:
+    def validate_otp(self, otp: str) -> bool:
         mask = self.config["used"]
         code = otp.strip().replace("-", "").upper()
         for idx, ref_code in enumerate(self.get_codes()):

src/sentry/migrations/0929_no_pickle_authenticator.py

@@ -0,0 +1,44 @@
+# Generated by Django 5.2.1 on 2025-06-13 15:44
+
+from django.db import migrations
+
+import sentry.users.models.authenticator
+from sentry.new_migrations.migrations import CheckedMigration
+from sentry.new_migrations.monkey.special import SafeRunSQL
+
+
+class Migration(CheckedMigration):
+    # This flag is used to mark that a migration shouldn't be automatically run in production.
+    # This should only be used for operations where it's safe to run the migration after your
+    # code has deployed. So this should not be used for most operations that alter the schema
+    # of a table.
+    # Here are some things that make sense to mark as post deployment:
+    # - Large data migrations. Typically we want these to be run manually so that they can be
+    #   monitored and not block the deploy for a long period of time while they run.
+    # - Adding indexes to large tables. Since this can take a long time, we'd generally prefer to
+    #   run this outside deployments so that we don't block them. Note that while adding an index
+    #   is a schema change, it's completely safe to run the operation after the code has deployed.
+    # Once deployed, run these manually via: https://develop.sentry.dev/database-migrations/#migration-deployment
+
+    is_post_deployment = True
+
+    dependencies = [
+        ("sentry", "0928_move_notifications_models"),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name="authenticator",
+            name="config",
+            field=sentry.users.models.authenticator.AuthenticatorConfig(),
+        ),
+        SafeRunSQL(
+            """
+            ALTER TABLE auth_authenticator ALTER COLUMN config TYPE jsonb USING config::jsonb;
+            """,
+            reverse_sql="""
+            ALTER TABLE auth_authenticator ALTER COLUMN config TYPE text;
+            """,
+            hints={"tables": ["auth_authenticator"]},
+        ),
+    ]

src/sentry/testutils/helpers/backups.py

@@ -372,7 +372,7 @@ def create_exhaustive_user(
             first_seen=datetime(2012, 4, 5, 3, 29, 45, tzinfo=UTC),
             last_seen=datetime(2012, 4, 5, 3, 29, 45, tzinfo=UTC),
         )
-        Authenticator.objects.create(user=user, type=1)
+        Authenticator.objects.create(user=user, type=1, config={})
 
         if is_admin:
             self.add_user_permission(user, "users.admin")

src/sentry/users/models/authenticator.py

@@ -6,6 +6,8 @@
 
 from django.contrib.auth.models import AnonymousUser
 from django.db import models
+from django.db.backends.base.base import BaseDatabaseWrapper
+from django.db.models.expressions import Expression
 from django.utils import timezone
 from django.utils.functional import cached_property
 from django.utils.translation import gettext_lazy as _
@@ -28,7 +30,6 @@
     FlexibleForeignKey,
     control_silo_model,
 )
-from sentry.db.models.fields.picklefield import PickledObjectField
 from sentry.db.models.manager.base import BaseManager
 from sentry.hybridcloud.models.outbox import ControlOutboxBase
 from sentry.hybridcloud.outbox.base import ControlOutboxProducingModel
@@ -128,11 +129,11 @@ def bulk_users_have_2fa(self, user_ids: list[int]) -> dict[int, bool]:
         return {id: id in authenticators for id in user_ids}
 
 
-class AuthenticatorConfig(PickledObjectField):
+class AuthenticatorConfig(models.JSONField):
     def _is_devices_config(self, value: Any) -> bool:
         return isinstance(value, dict) and "devices" in value
 
-    def get_db_prep_value(self, value: Any, *args: Any, **kwargs: Any) -> str | None:
+    def get_db_prep_value(self, value: Any, *args: Any, **kwargs: Any) -> Any:
         if self._is_devices_config(value):
             # avoid mutating the original object
             value = copy.deepcopy(value)
@@ -143,8 +144,10 @@ def get_db_prep_value(self, value: Any, *args: Any, **kwargs: Any) -> str | None
 
         return super().get_db_prep_value(value, *args, **kwargs)
 
-    def to_python(self, value: Any) -> Any | None:
-        ret = super().to_python(value)
+    def from_db_value(
+        self, value: str | None, expression: Expression, connection: BaseDatabaseWrapper
+    ) -> Any | None:
+        ret = super().from_db_value(value, expression, connection)
         if self._is_devices_config(ret):
             for device in ret["devices"]:
                 if isinstance(device["binding"], str):

tests/sentry/api/endpoints/test_organization_member_index.py

@@ -359,8 +359,8 @@ def test_2fa_enabled_query(self):
 
         # Two authenticators to ensure the user list is distinct
         with assume_test_silo_mode(SiloMode.CONTROL):
-            Authenticator.objects.create(user_id=member_2fa.user_id, type=1)
-            Authenticator.objects.create(user_id=member_2fa.user_id, type=2)
+            Authenticator.objects.create(user_id=member_2fa.user_id, type=1, config={})
+            Authenticator.objects.create(user_id=member_2fa.user_id, type=2, config={})
 
         response = self.get_success_response(
             self.organization.slug, qs_params={"query": "has2fa:true"}

tests/sentry/hybridcloud/rpc/test_rpc_model.py

@@ -29,7 +29,7 @@ def _get_rpc_model_subclasses(self) -> set[type[RpcModel]]:
 
     def test_rpc_model_equals_method(self) -> None:
         orm_user = self.create_user()
-        Authenticator.objects.create(user=orm_user, type=1)
+        Authenticator.objects.create(user=orm_user, type=1, config={})
 
         user1 = user_service.get_user(orm_user.id)
         user2 = user_service.get_user(orm_user.id)

tests/sentry/users/api/endpoints/test_user_authenticator_details.py

@@ -294,7 +294,7 @@ def test_delete_superuser(self):
         user = self.create_user(email="[email protected]", is_superuser=True)
 
         with override_options({"sms.twilio-account": "twilio-account"}):
-            auth = Authenticator.objects.create(type=2, user=user)  # sms
+            auth = Authenticator.objects.create(type=2, user=user, config={})  # sms
             available_auths = Authenticator.objects.all_interfaces_for_user(
                 user, ignore_backup=True
             )
@@ -313,7 +313,7 @@ def test_delete_staff(self):
         staff_user = self.create_user(email="[email protected]", is_staff=True)
 
         with override_options({"sms.twilio-account": "twilio-account"}):
-            auth = Authenticator.objects.create(type=2, user=staff_user)  # sms
+            auth = Authenticator.objects.create(type=2, user=staff_user, config={})  # sms
             available_auths = Authenticator.objects.all_interfaces_for_user(
                 staff_user, ignore_backup=True
             )
@@ -330,7 +330,7 @@ def test_delete_staff(self):
 
     def test_cannot_delete_without_superuser_or_staff(self):
         user = self.create_user(email="[email protected]", is_superuser=False, is_staff=False)
-        auth = Authenticator.objects.create(type=3, user=user)  # u2f
+        auth = Authenticator.objects.create(type=3, user=user, config={})  # u2f
 
         actor = self.create_user(email="[email protected]", is_superuser=False, is_staff=False)
         self.login_as(user=actor)

tests/sentry/users/api/serializers/test_user.py

@@ -21,7 +21,7 @@ def test_simple(self):
         assert result["has2fa"] is False
 
         Authenticator.objects.create(
-            type=available_authenticators(ignore_backup=True)[0].type, user=user
+            type=available_authenticators(ignore_backup=True)[0].type, user=user, config={}
         )
 
         result = serialize(user)
@@ -63,7 +63,7 @@ def test_simple(self):
             auth_provider=auth_provider, ident=user.email, user=user
         )
         auth = Authenticator.objects.create(
-            type=available_authenticators(ignore_backup=True)[0].type, user=user
+            type=available_authenticators(ignore_backup=True)[0].type, user=user, config={}
         )
 
         result = serialize(user, user, DetailedUserSerializer())
@@ -113,7 +113,7 @@ def setUp(self):
             auth_provider=auth_provider, ident=self.user.email, user=self.user
         )
         self.auth = Authenticator.objects.create(
-            type=available_authenticators(ignore_backup=True)[0].type, user=self.user
+            type=available_authenticators(ignore_backup=True)[0].type, user=self.user, config={}
         )
 
     def test_simple(self):

tests/sentry/users/models/test_authenticator.py

@@ -1,10 +1,13 @@
+from django.db import connection
+from django.db.models.expressions import Expression
 from fido2.ctap2 import AuthenticatorData
 from fido2.utils import sha256
 
 from sentry.auth.authenticators.recovery_code import RecoveryCodeInterface
 from sentry.auth.authenticators.totp import TotpInterface
 from sentry.auth.authenticators.u2f import create_credential_object
 from sentry.testutils.cases import TestCase
+from sentry.testutils.pytest.fixtures import django_db_all
 from sentry.testutils.silo import control_silo_test
 from sentry.users.models.authenticator import Authenticator, AuthenticatorConfig
 
@@ -39,6 +42,7 @@ def test_bulk_users_have_2fa(self):
         }
 
 
+@django_db_all
 def test_authenticator_config_compatibility():
     field_json = AuthenticatorConfig()
 
@@ -71,4 +75,6 @@ def test_authenticator_config_compatibility():
         ]
     }
 
-    assert field_json.to_python(field_json.get_db_prep_value(value)) == value
+    encoded = field_json.get_db_prep_value(value, connection=connection)
+    encoded_s = encoded.dumps(encoded.adapted)
+    assert field_json.from_db_value(encoded_s, Expression("config"), connection) == value

tests/sentry/users/models/test_user.py

@@ -225,8 +225,8 @@ def test_simple(self):
 
         Authenticator.objects.get(user=from_user, type=1)
         to_auth_dup = Authenticator.objects.get(user=to_user, type=1)
-        from_auth_uniq = Authenticator.objects.create(user=from_user, type=2)
-        to_auth_uniq = Authenticator.objects.create(user=to_user, type=3)
+        from_auth_uniq = Authenticator.objects.create(user=from_user, type=2, config={})
+        to_auth_uniq = Authenticator.objects.create(user=to_user, type=3, config={})
 
         from_user.merge_to(to_user)