Remove `User.is_staff` and `User.is_superuser`

[iaindillingham]

What's the purpose of User.is_staff? How does it relate to the CoreDeveloper role?

[mikerkelly]

From a quick look I think it does nothing. CoreDeveloper seems intended to be like a User with is_staff and is_superuser. But there is no relationship in code. Probably these are leftovers from a time when we either used django.contrib.admin or intended to mirror some of its User fields and behaviours. We don't use that app now. I think we don't use User.is_user or User.is_superuser for anything and they can/should be removed. Detail:

In the django.contrib.admin app, is_staff allows a user to access its auto-generated admin site and read models and perhaps view custom views there (eg reports) but cannot create/update/delete model instances without further permissions. is_superuser users are given CRUD permissions on all models.

We don't include django.contrib.admin in INSTALLED_APPS in job-server/settings.py, however. Our staff pages are similar-in-concept but not using that code at all as far as I can see.

References to is_staff in our job-server code:

templates/base.html:    <meta name="is_staff" content="{% if user_can_view_staff_area %}true{% else %}false{% endif %}">

jobserver/models/user.py:    is_staff = models.BooleanField(default=False)
jobserver/api/releases.py:            staff=user.is_staff,

tests/integration/test_auth.py:    assert not user.is_staff
tests/unit/jobserver/models/test_user.py:    assert su.is_staff
tests/unit/jobserver/api/test_releases.py:    token_login_user.is_staff = True

• base.html -- not used, could it be removed?
• models/user.py -- defining the field.
• releases.py -- build_level4_user() -- but would only achieve something if any other code referred to this.
• tests -- of the above.

None of these appear to have any effect. Maybe at one time there was more code that referenced these fields but it has been removed without removing the fields. Or maybe we once used django.contrib.admin and those fields drove its behaviour.

Similarly, we define is_superuser on the User model but without custom code using it, which I can't see, it doesn't do anything in the absence of django.contrib.admin.

Note: this should be tested carefully if removed -- if this is all wrong, high potential impact! Perhaps some other installed app refers to these fields that I'm unaware of?

[mikerkelly]

Worth checking if the Airlock team/code would be affected by this -- they use this API for authentication.

[mikerkelly]

This commit added these fields but they weren't used at the time, and we had already removed django.contrib.admin in #950. I've skipped around a couple of times in the repo and can't see that they've ever been used.

I looked in job-runner and airlock I see no reference to that string in either repo (except in this JS assets file, which I think is not relevant, not completely sure)

I've asked in #team-rap if it affects them in any way, from the Slack thread:

For auth, Airlock calls the Level4AuthenticationAPI and Level4AuthorisationAPI views, and it receives the results of build_level4_user to determine whether a user is an output checker, and what workspaces they can see. None of that makes use of is_staff or is_superuser, so I think you're good from an airlock perspective

[mikerkelly]

I further checked the history of this with: git log -G 'is_staff' --all --patch --stat --pretty=fuller > is_staff_log.txt ... and have seen nothing that contradicts my previous comments. We can go ahead and do this, I think.

[iaindillingham]

Alas, base.html is a red herring. We use the same name, but for a different thing.

https://github.com/opensafely-core/job-server/blob/8656ca1c7e298d7d61a6d55a785085f7597aad83/templates/base.html#L26

Above, meta.content is unrelated to user.is_staff.^1