diff --git a/jobserver/api/releases.py b/jobserver/api/releases.py index 9914324fc..808f5b216 100644 --- a/jobserver/api/releases.py +++ b/jobserver/api/releases.py @@ -608,7 +608,6 @@ def build_level4_user(user): # list the explicit workspaces that the user has this permission # for. output_checker=has_role(user, OutputChecker), - staff=user.is_staff, ) ) # we must validate this or DRF will refuse to serializer it. diff --git a/jobserver/migrations/0007_remove_user_is_staff_remove_user_is_superuser.py b/jobserver/migrations/0007_remove_user_is_staff_remove_user_is_superuser.py new file mode 100644 index 000000000..478043def --- /dev/null +++ b/jobserver/migrations/0007_remove_user_is_staff_remove_user_is_superuser.py @@ -0,0 +1,20 @@ +# Generated by Django 5.1.1 on 2024-09-30 14:13 + +from django.db import migrations + + +class Migration(migrations.Migration): + dependencies = [ + ("jobserver", "0006_remove_user_socialauth_token_type"), + ] + + operations = [ + migrations.RemoveField( + model_name="user", + name="is_staff", + ), + migrations.RemoveField( + model_name="user", + name="is_superuser", + ), + ] diff --git a/jobserver/models/user.py b/jobserver/models/user.py index 42c1ad569..8e19cbddf 100644 --- a/jobserver/models/user.py +++ b/jobserver/models/user.py @@ -14,7 +14,7 @@ from django.utils.functional import cached_property from sentry_sdk import capture_message -from ..authorization import CoreDeveloper, InteractiveReporter +from ..authorization import InteractiveReporter from ..authorization.fields import RolesArrayField from ..hash_utils import hash_user_pat @@ -47,11 +47,22 @@ def order_by_name(self): class UserManager(DjangoUserManager.from_queryset(UserQuerySet)): use_in_migrations = True - def create_superuser(self, email, password, **extra_fields): - username = email - return super().create_superuser( - username, email, password, roles=[CoreDeveloper] - ) + def create_user(self, username, email=None, password=None, **extra_fields): + # User extends AbstractBaseUser and doesn't have is_staff or + # is_superuser fields as UserManager expects. So we call the private + # UserManager._create_user method to bypass UserManager.create_user + # accessing those fields. This may now break if the UserManager + # implementation changes. It would hopefully break loudly, at least. + # + # Why have a manager at all? We think we want the special handling from + # UserManager_create_user that normalizes the username and email and + # hashes the password. Possibly, we don't need some or all of those. + # Possibly, we could extend BaseUserManager instead and do some of + # those transformations in our code. Or, possibly, we don't need + # create_user or get_or_create_user at all and could just have those + # behaviours on User.create, if they're needed. Ref #4627. + # https://github.com/opensafely-core/job-server/issues/4627 + return super()._create_user(username, email, password, **extra_fields) def get_or_create_user(username, email, fullname, update_fields=None): @@ -124,8 +135,6 @@ class User(AbstractBaseUser): fullname = models.TextField(default="") is_active = models.BooleanField(default=True) - is_staff = models.BooleanField(default=False) - is_superuser = models.BooleanField(default=False) date_joined = models.DateTimeField("date joined", default=timezone.now) # PATs are generated for bot users. They can only be generated via a shell diff --git a/tests/integration/test_auth.py b/tests/integration/test_auth.py index 2c05c994e..7ae19da75 100644 --- a/tests/integration/test_auth.py +++ b/tests/integration/test_auth.py @@ -77,7 +77,6 @@ def test_login_pipeline(client, slack_messages): assert user.fullname == "Test User" assert user.email == "test@example.com" assert user.is_active - assert not user.is_staff assert user.roles == [] # ensure social auth extra data populated diff --git a/tests/unit/jobserver/api/test_releases.py b/tests/unit/jobserver/api/test_releases.py index ac490c940..37a9986e9 100644 --- a/tests/unit/jobserver/api/test_releases.py +++ b/tests/unit/jobserver/api/test_releases.py @@ -1671,7 +1671,6 @@ def test_level4tokenauthenticationapi_success( }, }, # should not include workspace3 "output_checker": False, - "staff": False, } @@ -1681,7 +1680,6 @@ def test_level4tokenauthenticationapi_success_privileged( # enable privileges for user token_login_user.roles.append(OutputChecker) token_login_user.roles.append(CoreDeveloper) - token_login_user.is_staff = True token_login_user.save() project = ProjectFactory() @@ -1721,7 +1719,6 @@ def test_level4tokenauthenticationapi_success_privileged( }, }, # should not include workspace3 "output_checker": True, - "staff": True, } @@ -1866,7 +1863,6 @@ def test_level4authorisationapi_success( }, }, # should not include workspace3 "output_checker": False, - "staff": False, } diff --git a/tests/unit/jobserver/models/test_user.py b/tests/unit/jobserver/models/test_user.py index 4806e6b62..dd3c2ee2f 100644 --- a/tests/unit/jobserver/models/test_user.py +++ b/tests/unit/jobserver/models/test_user.py @@ -6,7 +6,6 @@ from django.utils import timezone from jobserver.authorization.roles import ( - CoreDeveloper, InteractiveReporter, OutputChecker, ProjectCollaborator, @@ -334,10 +333,21 @@ def test_get_or_create_user_update_fields(update_fields): assert user.fullname == expected_fullname -def test_create_superuser(): - su = User.objects.create_superuser(email="test@test.test", password="hunter2") - assert su.username == "test@test.test" - assert su.email == "test@test.test" - assert su.is_staff - assert su.is_superuser - assert CoreDeveloper in su.roles +def test_create_user(): + """Test of the custom UserManager.create_user() method.""" + username = "Ⅳan" # Note unicode character. + email = "test@TeSt.TEST" # Note uppercase in domain. + password = "hunter2" + user = User.objects.create_user(username=username, email=email, password=password) + + # It's not completely clear that we need all of these behaviours from + # django.contrib.auth.models.UserManager for our Users. Ref #4627. + # https://github.com/opensafely-core/job-server/issues/4627 + # But we should test them, as the function does them (for now?). + + # Username unicode gets normalized. + assert user.username == "IVan" + # E-mail gets normalized by lower-casing the domain. + assert user.email == "test@test.test" + # Password is not stored in plaintext. + assert user.password != password