Skip to content

Commit

Permalink
Remove User.is_staff and User.is_superuser fields
Browse files Browse the repository at this point in the history
The User.is_staff and User.is_superuser models fields were not used
anywhere so they and related code have been removed.

We once used django.contrib.admin but no longer do (instead having a
completely custom staff app). The User model has, for some time,
sub-classed AbstractBaseUser, not AbstractUser, to be more
customizable. The removed User fields are present on AbstractUser
but not AbstractBaseUser. They were only present on our User model
because django.contrib.admin required them to be present,
when we used it.

UserManager needed a custom create_user function adding, as
django.contrib.auth.models.UserManager.create_user sets a default
for is_staff and is_superuser. This may be a hint we should be using
BaseUserManager over UserManager. See comments and #4627.

The `create_user` management command does not touch these fields
explicitly, so no changes there.
  • Loading branch information
mikerkelly committed Oct 1, 2024
1 parent edc734a commit d15bc66
Show file tree
Hide file tree
Showing 6 changed files with 55 additions and 22 deletions.
1 change: 0 additions & 1 deletion jobserver/api/releases.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
@@ -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",
),
]
25 changes: 17 additions & 8 deletions jobserver/models/user.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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):
Expand Down Expand Up @@ -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
Expand Down
1 change: 0 additions & 1 deletion tests/integration/test_auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 0 additions & 4 deletions tests/unit/jobserver/api/test_releases.py
Original file line number Diff line number Diff line change
Expand Up @@ -1671,7 +1671,6 @@ def test_level4tokenauthenticationapi_success(
},
}, # should not include workspace3
"output_checker": False,
"staff": False,
}


Expand All @@ -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()
Expand Down Expand Up @@ -1721,7 +1719,6 @@ def test_level4tokenauthenticationapi_success_privileged(
},
}, # should not include workspace3
"output_checker": True,
"staff": True,
}


Expand Down Expand Up @@ -1866,7 +1863,6 @@ def test_level4authorisationapi_success(
},
}, # should not include workspace3
"output_checker": False,
"staff": False,
}


Expand Down
26 changes: 18 additions & 8 deletions tests/unit/jobserver/models/test_user.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
from django.utils import timezone

from jobserver.authorization.roles import (
CoreDeveloper,
InteractiveReporter,
OutputChecker,
ProjectCollaborator,
Expand Down Expand Up @@ -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

0 comments on commit d15bc66

Please sign in to comment.