From 4f83f8c06bb3352c50078a17edbbd5da1d8ce3ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C4=93teris=20Caune?= Date: Mon, 26 Jul 2021 12:50:43 +0300 Subject: [PATCH] Fix a 403 when transferring a project to a read-only team member --- CHANGELOG.md | 2 + hc/accounts/tests/test_project.py | 49 ++++++++++++++++++---- hc/accounts/tests/test_transfer_project.py | 17 ++++++++ hc/accounts/views.py | 18 ++++++-- templates/accounts/project.html | 2 +- 5 files changed, 76 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cfca89e3..40d77489 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,9 +7,11 @@ All notable changes to this project will be documented in this file. - Use multicolor channel icons for better appearance in the dark mode - Add SITE_LOGO_URL setting (#323) - Add admin action to log in as any user +- Add a "Manager" role (#484) ### Bug Fixes - Fix dark mode styling issues in Cron Syntax Cheatsheet +- Fix a 403 when transferring a project to a read-only team member ## v1.21.0 - 2020-07-02 diff --git a/hc/accounts/tests/test_project.py b/hc/accounts/tests/test_project.py index 2152ecac..b68a9ccf 100644 --- a/hc/accounts/tests/test_project.py +++ b/hc/accounts/tests/test_project.py @@ -49,20 +49,34 @@ class ProjectTestCase(BaseTestCase): self.assertTrue(len(api_key) > 10) self.assertFalse("b'" in api_key) + def test_it_requires_rw_access_to_create_api_key(self): + self.bobs_membership.role = "r" + self.bobs_membership.save() + + self.client.login(username="bob@example.org", password="password") + r = self.client.post(self.url, {"create_api_keys": "1"}) + self.assertEqual(r.status_code, 403) + def test_it_revokes_api_key(self): self.project.api_key_readonly = "R" * 32 self.project.save() self.client.login(username="alice@example.org", password="password") - - form = {"revoke_api_keys": "1"} - r = self.client.post(self.url, form) + r = self.client.post(self.url, {"revoke_api_keys": "1"}) self.assertEqual(r.status_code, 200) self.project.refresh_from_db() self.assertEqual(self.project.api_key, "") self.assertEqual(self.project.api_key_readonly, "") + def test_it_requires_rw_access_to_revoke_api_key(self): + self.bobs_membership.role = "r" + self.bobs_membership.save() + + self.client.login(username="bob@example.org", password="password") + r = self.client.post(self.url, {"revoke_api_keys": "1"}) + self.assertEqual(r.status_code, 403) + def test_it_adds_team_member(self): self.client.login(username="alice@example.org", password="password") @@ -160,7 +174,11 @@ class ProjectTestCase(BaseTestCase): self.client.login(username="alice@example.org", password="password") aaa = "a" * 300 - form = {"invite_team_member": "1", "email": f"frank+{aaa}@example.org", "role": "r"} + form = { + "invite_team_member": "1", + "email": f"frank+{aaa}@example.org", + "role": "r", + } r = self.client.post(self.url, form) self.assertEqual(r.status_code, 200) @@ -245,6 +263,15 @@ class ProjectTestCase(BaseTestCase): self.project.refresh_from_db() self.assertEqual(self.project.name, "Alpha Team") + def test_it_requires_rw_access_to_set_project_name(self): + self.bobs_membership.role = "r" + self.bobs_membership.save() + + self.client.login(username="bob@example.org", password="password") + form = {"set_project_name": "1", "name": "Alpha Team"} + r = self.client.post(self.url, form) + self.assertEqual(r.status_code, 403) + def test_it_shows_invite_suggestions(self): p2 = Project.objects.create(owner=self.alice) @@ -254,7 +281,7 @@ class ProjectTestCase(BaseTestCase): self.assertContains(r, "Add Users from Other Teams") self.assertContains(r, "bob@example.org") - def test_it_checks_rw_access_when_updating_project_name(self): + def test_it_requires_rw_access_to_update_project_name(self): self.bobs_membership.role = "r" self.bobs_membership.save() @@ -280,9 +307,15 @@ class ProjectTestCase(BaseTestCase): self.project.save() self.client.login(username="alice@example.org", password="password") - - form = {"show_api_keys": "1"} - r = self.client.post(self.url, form) + r = self.client.post(self.url, {"show_api_keys": "1"}) self.assertEqual(r.status_code, 200) self.assertNotContains(r, "Prometheus metrics endpoint") + + def test_it_requires_rw_access_to_show_api_key(self): + self.bobs_membership.role = "r" + self.bobs_membership.save() + + self.client.login(username="bob@example.org", password="password") + r = self.client.post(self.url, {"show_api_keys": "1"}) + self.assertEqual(r.status_code, 403) diff --git a/hc/accounts/tests/test_transfer_project.py b/hc/accounts/tests/test_transfer_project.py index 48231549..6de29d70 100644 --- a/hc/accounts/tests/test_transfer_project.py +++ b/hc/accounts/tests/test_transfer_project.py @@ -1,5 +1,6 @@ from django.core import mail from django.utils.timezone import now +from hc.accounts.models import Member from hc.api.models import Check from hc.test import BaseTestCase @@ -149,3 +150,19 @@ class TransferProjectTestCase(BaseTestCase): self.client.login(username="alice@example.org", password="password") r = self.client.post(self.url, {"reject_transfer": "1"}) self.assertEqual(r.status_code, 403) + + def test_readonly_user_can_accept(self): + self.bobs_membership.transfer_request_date = now() + self.bobs_membership.role = "r" + self.bobs_membership.save() + + self.client.login(username="bob@example.org", password="password") + self.client.post(self.url, {"accept_transfer": "1"}) + + self.project.refresh_from_db() + # Bob should now be the owner + self.assertEqual(self.project.owner, self.bob) + + # Alice, the previous owner, should now be a *regular* member + m = Member.objects.get(user=self.alice, project=self.project) + self.assertEqual(m.role, "w") diff --git a/hc/accounts/views.py b/hc/accounts/views.py index 021ac6b7..10c5c778 100644 --- a/hc/accounts/views.py +++ b/hc/accounts/views.py @@ -302,10 +302,10 @@ def project(request, code): } if request.method == "POST": - if not rw: - return HttpResponseForbidden() - if "create_api_keys" in request.POST: + if not rw: + return HttpResponseForbidden() + project.set_api_keys() project.save() @@ -313,6 +313,9 @@ def project(request, code): ctx["api_keys_created"] = True ctx["api_status"] = "success" elif "revoke_api_keys" in request.POST: + if not rw: + return HttpResponseForbidden() + project.api_key = "" project.api_key_readonly = "" project.save() @@ -320,6 +323,9 @@ def project(request, code): ctx["api_keys_revoked"] = True ctx["api_status"] = "info" elif "show_api_keys" in request.POST: + if not rw: + return HttpResponseForbidden() + ctx["show_api_keys"] = True elif "invite_team_member" in request.POST: if not is_manager: @@ -372,6 +378,9 @@ def project(request, code): ctx["team_member_removed"] = form.cleaned_data["email"] ctx["team_status"] = "info" elif "set_project_name" in request.POST: + if not rw: + return HttpResponseForbidden() + form = forms.ProjectNameForm(request.POST) if form.is_valid(): project.name = form.cleaned_data["name"] @@ -427,6 +436,9 @@ def project(request, code): # 1. Reuse the existing membership, and change its user tr.user = project.owner tr.transfer_request_date = None + # The previous owner becomes a regular member + # (not readonly, not manager): + tr.role = Member.Role.REGULAR tr.save() # 2. Change project's owner diff --git a/templates/accounts/project.html b/templates/accounts/project.html index 2f36d35a..8a2d2eaf 100644 --- a/templates/accounts/project.html +++ b/templates/accounts/project.html @@ -167,7 +167,7 @@ {% for m in project.member_set.all %} {{ m.user.email }} - {{ m.get_role_display}} + {{ m.get_role_display }} {% if is_manager and m.user != request.user %}