Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Jun 19, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions home/migrations/0011_user_is_planetterp_admin.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
from django.db import migrations


class Migration(migrations.Migration):

dependencies = [
('home', '0010_initial_data'),
]

operations = [
migrations.AlterModelOptions(
name='user',
options={'permissions': [('admin', 'Can take any planetterp admin actions')]},
),
]
15 changes: 11 additions & 4 deletions home/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -331,11 +331,9 @@ class User(AbstractUser):
objects = UserManager()

send_review_email = BooleanField(default=True)

# accounts which are from ourumd are given an unusable password so nobody
# can log in to the accounts
from_ourumd = BooleanField(default=False)

username = CharField(
max_length=22,
unique=True,
Expand All @@ -351,7 +349,6 @@ class User(AbstractUser):
"unique": "A user with that username already exists."
}
)

email = EmailField(
unique=True,
null=True,
Expand All @@ -366,7 +363,6 @@ class User(AbstractUser):
)
}
)

password = CharField(
max_length=128,
validators=[
Expand All @@ -377,6 +373,17 @@ class User(AbstractUser):
}
)

class Meta:
# 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.
permissions = [
("admin", "Can take any planetterp admin actions")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"moderator" is more fitting here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've gone with mod, as moderator was quite long.

]

# Workaround to force CharField to store empty values as NULL instead of ''
# https://stackoverflow.com/a/38621160
def save(self, *args, **kwargs):
Expand Down
8 changes: 4 additions & 4 deletions home/tables/columns.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ def grade_to_element(self, grade):

def render(self, value: dict):
review = value.pop("review")
is_staff = value.pop("is_staff")
is_planetterp_admin = value.pop("is_planetterp_admin")
Copy link
Member

@nsandler1 nsandler1 Jun 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be involved in having this permission have the user's is_staff=True but is_superuser=False? This would allow us to continue to use the is_staff check everywhere except in places where we only want superusers, in which case we'd use is_superuser. This would also make it easier to search for users based on if they're staff or not.

EDIT: I realize that is_staff will grants access to the django admin panel, so unless there's a way around that, this idea is moot.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as you said, the issue here is the admin panel. is_staff is also a very stiff permission and would not allow us to split things into multiple permissions in the future if we wanted.


column_html = ""
if review.professor.slug:
Expand Down Expand Up @@ -80,7 +80,7 @@ def render(self, value: dict):

# wrap long usernames to avoid increasing the information column width
column_html += '<span style="white-space: normal; word-break: break-all;">'
if is_staff and review.user:
if is_planetterp_admin and review.user:
if review.anonymous:
column_html += '''
<span class="noselect" data-toggle="tooltip" data-placement="right" title="This reviewer posted anonymously">
Expand Down Expand Up @@ -110,12 +110,12 @@ def render(self, value: dict):
if review.created_at.date() >= date(2020, 3, 10) and review.created_at.date() <= date(2021, 8, 30):
column_html += ' <i class="fas fa-head-side-mask" data-toggle="tooltip" data-placement="right" title="This review was submitted while most classes were online during the COVID-19 pandemic. It may not be indicative of a regular semester."></i>'

if not review.user or (review.anonymous and not is_staff):
if not review.user or (review.anonymous and not is_planetterp_admin):
username = "Anonymous"
else:
username = review.user.username

if is_staff:
if is_planetterp_admin:
column_html += '''
<br/ >
<span>
Expand Down
4 changes: 2 additions & 2 deletions home/tables/reviews_table.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ def get_data(self, reviews: QuerySet[Review]):
if ReviewsTableColumn.INFORMATION in self.columns:
formatted_data['information'] = {
"review": review,
"is_staff": self.request.user.is_staff
"is_planetterp_admin": self.request.user.has_perm("home.admin")
}
if ReviewsTableColumn.REVIEW in self.columns:
formatted_data['review'] = {"review": review}
Expand Down Expand Up @@ -87,7 +87,7 @@ def __init__(self, reviews, request, *args, **kwargs):
)

self.columns = [ReviewsTableColumn.INFORMATION, ReviewsTableColumn.REVIEW]
if request.user.is_staff:
if request.user.has_perm("home.admin"):
self.columns.append(ReviewsTableColumn.ACTION)

kwargs = {"empty_text": empty_text}
Expand Down
4 changes: 2 additions & 2 deletions home/templates/base_main.html
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
<link rel="stylesheet" type="text/css" href="{% static 'css/progressbar.css' %}">
<link rel="stylesheet" type="text/css" href="{% static 'fontawesome-5.15.3/css/all.min.css' %}">

{% if user and user.is_staff %}
{% if user and perms.home.admin %}
<script type="text/javascript" src="{% static 'js/admin-action.js' %}"></script>
{% endif %}

Expand Down Expand Up @@ -151,7 +151,7 @@
</ul>

<ul class="navbar-nav navbar-right mr-3">
{% if user.is_staff %}
{% if perms.home.admin %}
<li class="nav-item">
<a style="color: #ffc107;" class="nav-link" href="{% url 'admin' %}#reviews" title="Admin Page">
<span id="unverified_count" class="badge badge-pill badge-warning">{% unverified_count %}</span>
Expand Down
2 changes: 1 addition & 1 deletion home/templates/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ <h4>Recent reviews</h4>
django can't handle parenthesis in if statements for order of operations :/ so
split into multiple cases
-->
{% if user.is_staff and review.user %}
{% if perms.home.admin and review.user %}
{{ review.user.username }}
{% elif review.user and not review.anonymous %}
{{ review.user.username }}
Expand Down
4 changes: 2 additions & 2 deletions home/templates/professor.html
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
<div class="container mw-100 px-5">
<div class="row">
<div class="col-xs-12 col-sm-6 col-md-6 offset-md-1">
{% if user.is_staff %}
{% if perms.home.admin %}
<div class="professor-header input-group">
<h1 class="input-group">
<span id="professor-name"><strong>{{ professor.name }}</strong></span>
Expand Down Expand Up @@ -130,7 +130,7 @@ <h4 class="modal-title" id="grades-modal-label">Grades</h4>
<script type="text/javascript">
var num_reviews = parseInt("{{ num_reviews }}");
var average_rating = {% if professor.average_rating %} {{ professor.average_rating }} {% else %} null {% endif %};
var is_admin = {{ user.is_staff|yesno:"true,false" }};
var is_admin = {{ perms.home.admin|yesno:"true,false" }};

$(function() {
initializeRateYo(3, "review");
Expand Down
8 changes: 3 additions & 5 deletions home/views/admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
from django.views import View
from django.http import JsonResponse
from django.template.context_processors import csrf
from django.contrib.auth.mixins import UserPassesTestMixin
from django.contrib.auth.mixins import PermissionRequiredMixin
from django.urls import reverse
from django.utils.safestring import mark_safe

Expand All @@ -21,10 +21,8 @@
from home.utils import send_email, _ttl_cache
from planetterp import config

class Admin(UserPassesTestMixin, View):

def test_func(self):
return self.request.user.is_staff
class Admin(PermissionRequiredMixin, View):
permission_required = "home.admin"
Copy link
Member

@nsandler1 nsandler1 Jun 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts on making an enum to store the string value? I assume the string won't change, but I think it's more pleasant to deal with an enum than with a string. Also, this would give us more flexibility in naming/updating it.

Copy link
Contributor Author

@tybug tybug Jun 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't work as well as you might hope because there's no easy way to use this enum value in templates. Currently django populates a global perms variable and does some magic for perms checking:

{% if perms.home.admin %}
...
{% endif %}

I don't even know how this would work with a custom enum. In addition, if we did want to reference Permissinon in templates, we would have to populate the template with our Permission enum, which requires a custom context processor.

I'm not sure the added complexity is worthwhile here.


def get(self, request):
reviews = (
Expand Down
12 changes: 5 additions & 7 deletions home/views/basic.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
from django.views import View
from django.views.generic import TemplateView, RedirectView, ListView
from django.http import HttpResponse, Http404
from django.contrib.auth.mixins import UserPassesTestMixin
from django.contrib.auth.mixins import PermissionRequiredMixin

from home.models import Organization, Professor, Course, Review, Grade, User
from home.tables.reviews_table import VerifiedReviewsTable, ProfileReviewsTable
Expand Down Expand Up @@ -128,22 +128,20 @@ def post(self, request):

return JsonResponse(context)

class RecomputeTTLCache(UserPassesTestMixin, View):
def test_func(self):
return self.request.user.is_staff
class RecomputeTTLCache(PermissionRequiredMixin, View):
permission_required = "home.admin"

def post(self, _request):
recompute_ttl_cache()
return HttpResponse()

class UserProfile(UserPassesTestMixin, View):
class UserProfile(PermissionRequiredMixin, View):
# as all accounts have the option to still leave anonymous reviews, only
# allow admins to view individual user profiles for now.
#
# We may want to allow people to view a subset of other user's profiles
# in the future, which would show only the public reviews of that user.
def test_func(self):
return self.request.user.is_staff
permission_required = "home.admin"

def get(self, request, user_id):
# if a user clicks on a link to their own profile, redirect them to
Expand Down
2 changes: 1 addition & 1 deletion home/views/endpoints.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ def get(self, request):
value_dict['name'] = result.name

data = {
"label": f"{result}" if request.user.is_staff else f"{result.name}",
"label": f"{result}" if request.user.has_perm("home.admin") else f"{result.name}",
"result": value_dict
}

Expand Down
2 changes: 1 addition & 1 deletion home/views/professor.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ def get(self, request, slug):
"num_reviews": reviews.count()
}

if request.user.is_staff:
if request.user.has_perm("home.admin"):
edit_professor_form = ProfessorUpdateForm(professor, instance=professor)
unverify_professor_form = ProfessorUnverifyForm(professor.pk)
merge_professor_form = ProfessorMergeForm(request)
Expand Down