From 64c792b11ffb67387f0d402728d3b086bb1cfe3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20P=C3=A9pin?= Date: Sat, 21 Dec 2019 16:26:59 +0100 Subject: [PATCH 1/2] Disambiguation in kfet's permission handling In some places we used to refer to permissions based on their codename only (the part after the dot "." in the following examples) which can be ambiguous. Typically, we might define permissions like "bds.is_team" or "cof.is_team" in the near future ;) --- kfet/management/commands/loadkfetdevdata.py | 8 +++----- kfet/open/tests.py | 14 ++++++++++---- kfet/tests/test_statistic.py | 2 +- kfet/tests/test_views.py | 17 ++++++++++------- kfet/views.py | 21 +++++++++++++-------- 5 files changed, 37 insertions(+), 25 deletions(-) diff --git a/kfet/management/commands/loadkfetdevdata.py b/kfet/management/commands/loadkfetdevdata.py index 0543be80..43154d6e 100644 --- a/kfet/management/commands/loadkfetdevdata.py +++ b/kfet/management/commands/loadkfetdevdata.py @@ -6,7 +6,7 @@ import os import random from datetime import timedelta -from django.contrib.auth.models import ContentType, Group, Permission, User +from django.contrib.auth.models import Group, Permission, User from django.core.management import call_command from django.utils import timezone @@ -41,11 +41,9 @@ class Command(MyBaseCommand): group_chef.save() group_boy.save() - permissions_chef = Permission.objects.filter( - content_type__in=ContentType.objects.filter(app_label="kfet") - ) + permissions_chef = Permission.objects.filter(content_type__app_label="kfet",) permissions_boy = Permission.objects.filter( - codename__in=["is_team", "perform_deposit"] + content_type__app_label="kfet", codename__in=["is_team", "perform_deposit"] ) group_chef.permissions.add(*permissions_chef) diff --git a/kfet/open/tests.py b/kfet/open/tests.py index 4e652cb6..ae9bfa4b 100644 --- a/kfet/open/tests.py +++ b/kfet/open/tests.py @@ -84,7 +84,8 @@ class OpenKfetTest(ChannelTestCase): def test_export_team(self): """Export all values for a team member.""" user = User.objects.create_user("team", "", "team") - user.user_permissions.add(Permission.objects.get(codename="is_team")) + is_team = Permission.objects.get_by_natural_key("is_team", "kfet", "account") + user.user_permissions.add(is_team) export = self.kfet_open.export(user) self.assertSetEqual(set(["status", "admin_status", "force_close"]), set(export)) @@ -114,8 +115,12 @@ class OpenKfetViewsTest(ChannelTestCase): # get some permissions perms = { - "kfet.is_team": Permission.objects.get(codename="is_team"), - "kfet.can_force_close": Permission.objects.get(codename="can_force_close"), + "kfet.is_team": Permission.objects.get_by_natural_key( + "is_team", "kfet", "account" + ), + "kfet.can_force_close": Permission.objects.get_by_natural_key( + "can_force_close", "kfet", "account" + ), } # authenticated user and its client @@ -199,7 +204,8 @@ class OpenKfetConsumerTest(ChannelTestCase): """Team user is added to kfet.open.team group.""" # setup team user and its client t = User.objects.create_user("team", "", "team") - t.user_permissions.add(Permission.objects.get(codename="is_team")) + is_team = Permission.objects.get_by_natural_key("is_team", "kfet", "account") + t.user_permissions.add(is_team) c = WSClient() c.force_login(t) diff --git a/kfet/tests/test_statistic.py b/kfet/tests/test_statistic.py index eda386b7..9fafada4 100644 --- a/kfet/tests/test_statistic.py +++ b/kfet/tests/test_statistic.py @@ -18,7 +18,7 @@ class TestStats(TestCase): user.set_password("foobar") user.save() Account.objects.create(trigramme="FOO", cofprofile=user.profile) - perm = Permission.objects.get(codename="is_team") + perm = Permission.objects.get_by_natural_key("is_team", "kfet", "account") user.user_permissions.add(perm) user2 = User.objects.create(username="Barfoo") diff --git a/kfet/tests/test_views.py b/kfet/tests/test_views.py index decc8915..0a5c4e49 100644 --- a/kfet/tests/test_views.py +++ b/kfet/tests/test_views.py @@ -1855,7 +1855,7 @@ class KPsulPerformOperationsViewTests(ViewTestCaseMixin, TestCase): json_data = json.loads(resp.content.decode("utf-8")) self.assertEqual( json_data["errors"]["missing_perms"], - ["Enregistrer des commandes avec commentaires"], + ["[kfet] Enregistrer des commandes avec commentaires"], ) def test_group_on_acc_frozen(self): @@ -1898,7 +1898,7 @@ class KPsulPerformOperationsViewTests(ViewTestCaseMixin, TestCase): self.assertEqual(resp.status_code, 403) json_data = json.loads(resp.content.decode("utf-8")) self.assertEqual( - json_data["errors"]["missing_perms"], ["Forcer le gel d'un compte"] + json_data["errors"]["missing_perms"], ["[kfet] Forcer le gel d'un compte"] ) def test_invalid_group_checkout(self): @@ -2373,7 +2373,9 @@ class KPsulPerformOperationsViewTests(ViewTestCaseMixin, TestCase): self.assertEqual(resp.status_code, 403) json_data = json.loads(resp.content.decode("utf-8")) - self.assertEqual(json_data["errors"]["missing_perms"], ["Effectuer une charge"]) + self.assertEqual( + json_data["errors"]["missing_perms"], ["[kfet] Effectuer une charge"] + ) def test_withdraw(self): data = dict( @@ -2648,7 +2650,8 @@ class KPsulPerformOperationsViewTests(ViewTestCaseMixin, TestCase): self.assertEqual(resp.status_code, 403) json_data = json.loads(resp.content.decode("utf-8")) self.assertEqual( - json_data["errors"]["missing_perms"], ["Modifier la balance d'un compte"] + json_data["errors"]["missing_perms"], + ["[kfet] Modifier la balance d'un compte"], ) def test_invalid_edit_expects_comment(self): @@ -2956,7 +2959,7 @@ class KPsulPerformOperationsViewTests(ViewTestCaseMixin, TestCase): json_data = json.loads(resp.content.decode("utf-8")) self.assertEqual( json_data["errors"], - {"missing_perms": ["Enregistrer des commandes en négatif"]}, + {"missing_perms": ["[kfet] Enregistrer des commandes en négatif"]}, ) def test_invalid_negative_exceeds_allowed_duration_from_config(self): @@ -3780,7 +3783,7 @@ class KPsulCancelOperationsViewTests(ViewTestCaseMixin, TestCase): json_data = json.loads(resp.content.decode("utf-8")) self.assertEqual( json_data["errors"], - {"missing_perms": ["Annuler des commandes non récentes"]}, + {"missing_perms": ["[kfet] Annuler des commandes non récentes"]}, ) def test_already_canceled(self): @@ -3926,7 +3929,7 @@ class KPsulCancelOperationsViewTests(ViewTestCaseMixin, TestCase): json_data = json.loads(resp.content.decode("utf-8")) self.assertEqual( json_data["errors"], - {"missing_perms": ["Enregistrer des commandes en négatif"]}, + {"missing_perms": ["[kfet] Enregistrer des commandes en négatif"]}, ) def test_partial_0(self): diff --git a/kfet/views.py b/kfet/views.py index a5babe52..655e856d 100644 --- a/kfet/views.py +++ b/kfet/views.py @@ -3,6 +3,7 @@ import heapq import statistics from collections import defaultdict from decimal import Decimal +from typing import List from urllib.parse import urlencode from django.contrib import messages @@ -993,15 +994,19 @@ def kpsul_update_addcost(request): return JsonResponse(data) -def get_missing_perms(required_perms, user): - missing_perms_codenames = [ - (perm.split("."))[1] for perm in required_perms if not user.has_perm(perm) - ] - missing_perms = list( - Permission.objects.filter(codename__in=missing_perms_codenames).values_list( - "name", flat=True +def get_missing_perms(required_perms: List[str], user: User) -> List[str]: + def get_perm_description(app_label: str, codename: str) -> str: + name = Permission.objects.values_list("name", flat=True).get( + codename=codename, content_type__app_label=app_label ) - ) + return "[{}] {}".format(app_label, name) + + missing_perms = [ + get_perm_description(*perm.split(".")) + for perm in required_perms + if not user.has_perm(perm) + ] + return missing_perms From 1f945d1af3a2d8a72d9e1a9caf465e4dfe1346d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20P=C3=A9pin?= Date: Tue, 24 Dec 2019 17:13:27 +0100 Subject: [PATCH 2/2] Avoid using `get_by_natural_key` --- events/tests/test_views.py | 4 ++-- kfet/open/tests.py | 16 ++++++++++------ kfet/tests/test_statistic.py | 4 +++- 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/events/tests/test_views.py b/events/tests/test_views.py index 8dd81df7..5dc01fbb 100644 --- a/events/tests/test_views.py +++ b/events/tests/test_views.py @@ -15,8 +15,8 @@ def make_user(name): def make_staff_user(name): - view_event_perm = Permission.objects.get_by_natural_key( - codename="view_event", app_label="events", model="event" + view_event_perm = Permission.objects.get( + codename="view_event", content_type__app_label="events", ) user = make_user(name) user.user_permissions.add(view_event_perm) diff --git a/kfet/open/tests.py b/kfet/open/tests.py index ae9bfa4b..0d527644 100644 --- a/kfet/open/tests.py +++ b/kfet/open/tests.py @@ -84,7 +84,9 @@ class OpenKfetTest(ChannelTestCase): def test_export_team(self): """Export all values for a team member.""" user = User.objects.create_user("team", "", "team") - is_team = Permission.objects.get_by_natural_key("is_team", "kfet", "account") + is_team = Permission.objects.get( + codename="is_team", content_type__app_label="kfet" + ) user.user_permissions.add(is_team) export = self.kfet_open.export(user) self.assertSetEqual(set(["status", "admin_status", "force_close"]), set(export)) @@ -115,11 +117,11 @@ class OpenKfetViewsTest(ChannelTestCase): # get some permissions perms = { - "kfet.is_team": Permission.objects.get_by_natural_key( - "is_team", "kfet", "account" + "kfet.is_team": Permission.objects.get( + codename="is_team", content_type__app_label="kfet" ), - "kfet.can_force_close": Permission.objects.get_by_natural_key( - "can_force_close", "kfet", "account" + "kfet.can_force_close": Permission.objects.get( + codename="can_force_close", content_type__app_label="kfet" ), } @@ -204,7 +206,9 @@ class OpenKfetConsumerTest(ChannelTestCase): """Team user is added to kfet.open.team group.""" # setup team user and its client t = User.objects.create_user("team", "", "team") - is_team = Permission.objects.get_by_natural_key("is_team", "kfet", "account") + is_team = Permission.objects.get( + codename="is_team", content_type__app_label="kfet" + ) t.user_permissions.add(is_team) c = WSClient() c.force_login(t) diff --git a/kfet/tests/test_statistic.py b/kfet/tests/test_statistic.py index 9fafada4..a5e3192c 100644 --- a/kfet/tests/test_statistic.py +++ b/kfet/tests/test_statistic.py @@ -18,7 +18,9 @@ class TestStats(TestCase): user.set_password("foobar") user.save() Account.objects.create(trigramme="FOO", cofprofile=user.profile) - perm = Permission.objects.get_by_natural_key("is_team", "kfet", "account") + perm = Permission.objects.get( + codename="is_team", content_type__app_label="kfet" + ) user.user_permissions.add(perm) user2 = User.objects.create(username="Barfoo")