diff --git a/docs/usage.md b/docs/usage.md index ab71f16a1..c26d13fd3 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -252,7 +252,7 @@ permissions can be granted to limit access to certain parts of the UI (and corre #### Superusers -Superusers can access all parts of the front and backend application as well as any and all objects. +Superusers can access all parts of the front and backend application as well as any and all objects. Superuser status can only be granted by another superuser. #### Admin Status diff --git a/src-ui/src/app/components/common/edit-dialog/user-edit-dialog/user-edit-dialog.component.spec.ts b/src-ui/src/app/components/common/edit-dialog/user-edit-dialog/user-edit-dialog.component.spec.ts index 560a259f4..9ffa1ea95 100644 --- a/src-ui/src/app/components/common/edit-dialog/user-edit-dialog/user-edit-dialog.component.spec.ts +++ b/src-ui/src/app/components/common/edit-dialog/user-edit-dialog/user-edit-dialog.component.spec.ts @@ -160,4 +160,23 @@ describe('UserEditDialogComponent', () => { }) expect(component.currentUserIsSuperUser).toBeTruthy() }) + + it('should disable superuser option if current user is not superuser', () => { + const control: AbstractControl = component.objectForm.get('is_superuser') + permissionsService.initialize([], { + id: 99, + username: 'user99', + is_superuser: false, + }) + component.ngOnInit() + expect(control.disabled).toBeTruthy() + + permissionsService.initialize([], { + id: 99, + username: 'user99', + is_superuser: true, + }) + component.ngOnInit() + expect(control.disabled).toBeFalsy() + }) }) diff --git a/src-ui/src/app/components/common/edit-dialog/user-edit-dialog/user-edit-dialog.component.ts b/src-ui/src/app/components/common/edit-dialog/user-edit-dialog/user-edit-dialog.component.ts index 819d075b5..7ba0f5ceb 100644 --- a/src-ui/src/app/components/common/edit-dialog/user-edit-dialog/user-edit-dialog.component.ts +++ b/src-ui/src/app/components/common/edit-dialog/user-edit-dialog/user-edit-dialog.component.ts @@ -60,6 +60,11 @@ export class UserEditDialogComponent ngOnInit(): void { super.ngOnInit() this.onToggleSuperUser() + if (!this.currentUserIsSuperUser) { + this.objectForm.get('is_superuser').disable() + } else { + this.objectForm.get('is_superuser').enable() + } } getCreateTitle() { diff --git a/src/documents/tests/test_admin.py b/src/documents/tests/test_admin.py index a32a31adf..3f27c80f3 100644 --- a/src/documents/tests/test_admin.py +++ b/src/documents/tests/test_admin.py @@ -1,4 +1,7 @@ +import types + from django.contrib.admin.sites import AdminSite +from django.contrib.auth.models import User from django.test import TestCase from django.utils import timezone @@ -6,6 +9,7 @@ from documents import index from documents.admin import DocumentAdmin from documents.models import Document from documents.tests.utils import DirectoriesMixin +from paperless.admin import PaperlessUserAdmin class TestDocumentAdmin(DirectoriesMixin, TestCase): @@ -64,3 +68,22 @@ class TestDocumentAdmin(DirectoriesMixin, TestCase): created=timezone.make_aware(timezone.datetime(2020, 4, 12)), ) self.assertEqual(self.doc_admin.created_(doc), "2020-04-12") + + +class TestPaperlessAdmin(DirectoriesMixin, TestCase): + def setUp(self) -> None: + super().setUp() + self.user_admin = PaperlessUserAdmin(model=User, admin_site=AdminSite()) + + def test_only_superuser_can_change_superuser(self): + non_superuser = User.objects.create(username="requestuser") + user = User.objects.create(username="test", is_superuser=False) + + data = {"is_superuser": True} + form = self.user_admin.form(data, instance=user) + form.request = types.SimpleNamespace(user=non_superuser) + self.assertFalse(form.is_valid()) + self.assertEqual( + form.errors.get("__all__"), + ["Superuser status can only be changed by a superuser"], + ) diff --git a/src/documents/tests/test_api_permissions.py b/src/documents/tests/test_api_permissions.py index ef50c55f7..5de1887b2 100644 --- a/src/documents/tests/test_api_permissions.py +++ b/src/documents/tests/test_api_permissions.py @@ -681,6 +681,80 @@ class TestApiUser(DirectoriesMixin, APITestCase): ) self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) + def test_only_superusers_can_create_or_alter_superuser_status(self): + """ + GIVEN: + - Existing user account + WHEN: + - API request is made to add a user account with superuser status + - API request is made to change superuser status + THEN: + - Only superusers can change superuser status + """ + + user1 = User.objects.create_user(username="user1") + user1.user_permissions.add(*Permission.objects.all()) + user2 = User.objects.create_superuser(username="user2") + + self.client.force_authenticate(user1) + + response = self.client.patch( + f"{self.ENDPOINT}{user1.pk}/", + json.dumps( + { + "is_superuser": True, + }, + ), + content_type="application/json", + ) + + self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) + + response = self.client.post( + f"{self.ENDPOINT}", + json.dumps( + { + "username": "user3", + "is_superuser": True, + }, + ), + content_type="application/json", + ) + + self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) + + self.client.force_authenticate(user2) + + response = self.client.patch( + f"{self.ENDPOINT}{user1.pk}/", + json.dumps( + { + "is_superuser": True, + }, + ), + content_type="application/json", + ) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + + returned_user1 = User.objects.get(pk=user1.pk) + self.assertEqual(returned_user1.is_superuser, True) + + response = self.client.patch( + f"{self.ENDPOINT}{user1.pk}/", + json.dumps( + { + "is_superuser": False, + }, + ), + content_type="application/json", + ) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + + returned_user1 = User.objects.get(pk=user1.pk) + self.assertEqual(returned_user1.is_superuser, False) + class TestApiGroup(DirectoriesMixin, APITestCase): ENDPOINT = "/api/groups/" diff --git a/src/paperless/admin.py b/src/paperless/admin.py new file mode 100644 index 000000000..89575fe2e --- /dev/null +++ b/src/paperless/admin.py @@ -0,0 +1,53 @@ +from django import forms +from django.contrib import admin +from django.contrib.auth.admin import UserAdmin +from django.contrib.auth.models import User + + +class PaperlessUserForm(forms.ModelForm): + """ + Custom form for the User model that adds validation to prevent non-superusers + from changing the superuser status of a user. + """ + + class Meta: + model = User + fields = [ + "username", + "first_name", + "last_name", + "email", + "is_staff", + "is_active", + "is_superuser", + "groups", + "user_permissions", + ] + + def clean(self): + cleaned_data = super().clean() + user_being_edited = self.instance + is_superuser = cleaned_data.get("is_superuser") + + if ( + not self.request.user.is_superuser + and is_superuser != user_being_edited.is_superuser + ): + raise forms.ValidationError( + "Superuser status can only be changed by a superuser", + ) + + return cleaned_data + + +class PaperlessUserAdmin(UserAdmin): + form = PaperlessUserForm + + def get_form(self, request, obj=None, **kwargs): + form = super().get_form(request, obj, **kwargs) + form.request = request + return form + + +admin.site.unregister(User) +admin.site.register(User, PaperlessUserAdmin) diff --git a/src/paperless/views.py b/src/paperless/views.py index b5142ed62..03721adf2 100644 --- a/src/paperless/views.py +++ b/src/paperless/views.py @@ -109,6 +109,25 @@ class UserViewSet(ModelViewSet): filterset_class = UserFilterSet ordering_fields = ("username",) + def create(self, request, *args, **kwargs): + if not request.user.is_superuser and request.data.get("is_superuser") is True: + return HttpResponseForbidden( + "Superuser status can only be granted by a superuser", + ) + return super().create(request, *args, **kwargs) + + def update(self, request, *args, **kwargs): + user_to_update: User = self.get_object() + if ( + not request.user.is_superuser + and request.data.get("is_superuser") is not None + and request.data.get("is_superuser") != user_to_update.is_superuser + ): + return HttpResponseForbidden( + "Superuser status can only be changed by a superuser", + ) + return super().update(request, *args, **kwargs) + @action(detail=True, methods=["post"]) def deactivate_totp(self, request, pk=None): request_user = request.user