planetterp / PlanetTerp

a community for students at the university of maryland
https://planetterp.com
GNU Affero General Public License v3.0
40 stars 20 forks source link

add admin permission, move from is_staff to permission system #126

Closed tybug closed 1 year ago

tybug commented 1 year ago

See inline comment for details:

        # planetterp admins are a level between staff users and normal users:
        # admins can view the admin panel and take all actions theiren, but
        # cannot view the django admin panel.
        # Essentially, this role is for site admins which should not have access
        # to the prod db, which the django admin panel grants to a moderate
        # degree.

I did try adding a User.is_planetterp_admin field first, but ran into issues with request.user.is_planetterp_admin erroring when type(user) is AnonymousUser. This wasn't an issue with request.user.is_staff because is_staff is a builtin field on AbstractUser, while we only set is_planetterp_admin on our own User : AbstractUser. Frankly, this seems like bad design by django.

Not happy about using the django permissions system - it has its own warts[^1] - but it seemed the easiest and most flexible way to achieve this.

All current staff accounts are also superusers, which have every permission automatically, so there's no need to do anything on the migration side of things.

To assign this permission, use the django admin panel.

[^1]: permissions are inherently tied to a model, so there's no way to declare a site-wide permission like "is an admin". The next best place for it to live is under the User model, which looks strange. Nothing breaks, but it's counterintuitive when adding permissions to users.

nsandler1 commented 1 year ago

To clarify, this PR creates one new permission that only excludes the ability to view the django admin panel. Is there a new role for admins who have full access or will that stay as a standard superuser?

tybug commented 1 year ago

I can't imagine a scenario in which we want a distinction between "can access everything including django admin" and "superuser", so I would think that stays as a superuser.

nsandler1 commented 1 year ago

I can see the name being a source of confusion down the road so I think the name of the permission should change. Since this will be used by site moderators, "moderator" is more fitting. What do you think?

tybug commented 1 year ago

was thinking the same, I agree "moderator" is a better name here

nsandler1 commented 1 year ago

Second pass is good! I didn't encounter any errors and everything appears to work. I'm good to merge once the above things have been addressed.

tybug commented 1 year ago

merge at your discretion, as I did not change the enum suggested above