From 9ad8dd805272d689cdff6d48dc16da77936f256a Mon Sep 17 00:00:00 2001 From: Rowan Seymour Date: Thu, 15 Aug 2024 11:06:03 -0500 Subject: [PATCH 1/2] API tokens should use the current role of the user --- temba/api/models.py | 16 ++-------------- temba/api/tests.py | 11 ----------- temba/api/v2/tests.py | 12 ++---------- 3 files changed, 4 insertions(+), 35 deletions(-) diff --git a/temba/api/models.py b/temba/api/models.py index a83cfb1297c..ad023636302 100644 --- a/temba/api/models.py +++ b/temba/api/models.py @@ -74,13 +74,9 @@ def has_permission(self, request, view): org = request.org if request.auth: - # check that user is still allowed to use the token's role - if not request.auth.is_valid(): - return False - - role = OrgRole.from_group(request.auth.role) + # auth token was used + role = org.get_user_role(request.auth.user) elif org: - # user may not have used token authentication role = org.get_user_role(request.user) else: return False @@ -260,14 +256,6 @@ def get_default_role(cls, org, user): return role - def is_valid(self) -> bool: - """ - A user's role in an org can change so this return whether this token is still valid. - """ - role = self.org.get_user_role(self.user) - roles_allowed_this_perm_group = self.GROUP_GRANTED_TO.get(self.role.name, ()) - return role and role in roles_allowed_this_perm_group - def record_used(self): r = get_redis_connection() r.sadd("api_tokens_used", self.key) diff --git a/temba/api/tests.py b/temba/api/tests.py index 46b3167acb2..45bb62b6667 100644 --- a/temba/api/tests.py +++ b/temba/api/tests.py @@ -48,17 +48,6 @@ def test_get_or_create(self): self.assertRaises(ValueError, APIToken.get_or_create, self.org, self.admin, role=OrgRole.VIEWER) self.assertRaises(ValueError, APIToken.get_or_create, self.org, self.user) - def test_is_valid(self): - token1 = APIToken.get_or_create(self.org, self.admin, role=OrgRole.ADMINISTRATOR) - token2 = APIToken.get_or_create(self.org, self.admin, role=OrgRole.EDITOR) - - # demote admin to an editor - self.org.add_user(self.admin, OrgRole.EDITOR) - self.admin.refresh_from_db() - - self.assertFalse(token1.is_valid()) - self.assertTrue(token2.is_valid()) - def test_get_default_role(self): self.assertEqual(APIToken.get_default_role(self.org, self.admin), OrgRole.ADMINISTRATOR) self.assertEqual(APIToken.get_default_role(self.org, self.editor), OrgRole.EDITOR) diff --git a/temba/api/v2/tests.py b/temba/api/v2/tests.py index 844dce82855..01520c3f1a3 100644 --- a/temba/api/v2/tests.py +++ b/temba/api/v2/tests.py @@ -40,7 +40,7 @@ from .serializers import format_datetime, normalize_extra NUM_BASE_SESSION_QUERIES = 4 # number of queries required for any request using session auth -NUM_BASE_TOKEN_QUERIES = 3 # number of queries required for any request using token auth +NUM_BASE_TOKEN_QUERIES = 2 # number of queries required for any request using token auth class APITest(APITestMixin, TembaTest): @@ -536,15 +536,7 @@ def request_by_session(endpoint, user): response = request_by_token(fields_url, token1.key) self.assertEqual(response.status_code, 429) - # if user loses access to the token's role, don't allow the request - self.org.add_user(self.admin, OrgRole.EDITOR) - - self.assertEqual(request_by_token(campaigns_url, token1.key).status_code, 403) - self.assertEqual(request_by_basic_auth(campaigns_url, self.admin.username, token1.key).status_code, 403) - self.assertEqual(request_by_token(contacts_url, token2.key).status_code, 200) # other token unaffected - self.assertEqual(request_by_basic_auth(contacts_url, self.editor.username, token2.key).status_code, 200) - - # and if user is inactive, disallow the request + # if user is inactive, disallow the request self.admin.is_active = False self.admin.save() From f2bd02beec53ccdc0a635ced901f8887986e0f58 Mon Sep 17 00:00:00 2001 From: Rowan Seymour Date: Thu, 15 Aug 2024 16:30:46 +0000 Subject: [PATCH 2/2] API tokens should only work if user is in role that allows them --- temba/api/models.py | 4 ++++ temba/api/v2/tests.py | 9 ++++++++- temba/orgs/tests.py | 13 ------------- temba/orgs/views.py | 5 ----- 4 files changed, 12 insertions(+), 19 deletions(-) diff --git a/temba/api/models.py b/temba/api/models.py index ad023636302..7ee9d220993 100644 --- a/temba/api/models.py +++ b/temba/api/models.py @@ -76,6 +76,10 @@ def has_permission(self, request, view): if request.auth: # auth token was used role = org.get_user_role(request.auth.user) + + # only editors and administrators can use API tokens + if role not in (OrgRole.EDITOR, OrgRole.ADMINISTRATOR): + return False elif org: role = org.get_user_role(request.user) else: diff --git a/temba/api/v2/tests.py b/temba/api/v2/tests.py index 01520c3f1a3..ddb3d81441b 100644 --- a/temba/api/v2/tests.py +++ b/temba/api/v2/tests.py @@ -536,7 +536,14 @@ def request_by_session(endpoint, user): response = request_by_token(fields_url, token1.key) self.assertEqual(response.status_code, 429) - # if user is inactive, disallow the request + # if user is demoted to a role that can't use tokens, tokens shouldn't work for them + self.org.add_user(self.admin, OrgRole.VIEWER) + + self.assertEqual(request_by_token(campaigns_url, token1.key).status_code, 403) + self.assertEqual(request_by_basic_auth(campaigns_url, self.admin.username, token1.key).status_code, 403) + + # and if user is inactive, disallow the request + self.org.add_user(self.admin, OrgRole.ADMINISTRATOR) self.admin.is_active = False self.admin.save() diff --git a/temba/orgs/tests.py b/temba/orgs/tests.py index 2184342dc0c..0a91ba6bf2d 100644 --- a/temba/orgs/tests.py +++ b/temba/orgs/tests.py @@ -1656,11 +1656,6 @@ def test_manage_accounts(self): self.assertEqual(set(), set(self.org.get_users(roles=[OrgRole.VIEWER]))) self.assertEqual({self.agent}, set(self.org.get_users(roles=[OrgRole.AGENT]))) - # our second editors API token should be deleted - self.assertEqual(self.admin.api_tokens.filter(is_active=True).count(), 1) - self.assertEqual(self.editor.api_tokens.filter(is_active=True).count(), 1) - self.assertEqual(editor2.api_tokens.filter(is_active=True).count(), 0) - # pretend our first invite was acted on invitation1.release() @@ -1724,14 +1719,6 @@ def test_manage_accounts(self): self.assertEqual(set(), set(self.org.get_users(roles=[OrgRole.VIEWER]))) self.assertEqual({self.editor}, set(self.org.get_users(roles=[OrgRole.AGENT]))) - # editor will have lost their API tokens - self.editor.refresh_from_db() - self.assertEqual(0, self.editor.api_tokens.filter(is_active=True).count()) - - # and all our API tokens for the admin are deleted - self.admin.refresh_from_db() - self.assertEqual(self.admin.api_tokens.filter(is_active=True).count(), 0) - def test_manage_children(self): children_url = reverse("orgs.org_sub_orgs") diff --git a/temba/orgs/views.py b/temba/orgs/views.py index 8954155fc14..7bb155345b9 100644 --- a/temba/orgs/views.py +++ b/temba/orgs/views.py @@ -2134,11 +2134,6 @@ def post_save(self, obj): elif org.get_user_role(user) != new_role: org.add_user(user, new_role) - # when a user's role changes, delete any API tokens they're no longer allowed to have - for token in APIToken.objects.filter(org=org, user=user): - if not token.is_valid(): - token.release() - return obj def get_context_data(self, **kwargs):