diff --git a/backend/lambdas/api/authorizer/authorizer/handlers.py b/backend/lambdas/api/authorizer/authorizer/handlers.py index 8c0aa0dd..8cd41aad 100644 --- a/backend/lambdas/api/authorizer/authorizer/handlers.py +++ b/backend/lambdas/api/authorizer/authorizer/handlers.py @@ -3,11 +3,10 @@ import re import time import urllib.request - from datetime import datetime, timezone -from django.db import transaction from typing import Tuple +from django.db import transaction from jose import jwk, jwt from jose.utils import base64url_decode @@ -181,15 +180,13 @@ def _get_update_or_create_user(email: str) -> User: """ user = _get_user(email) - # If user is found, return the user (unless the user is soft-deleted) - if user: - # If user is soft-deleted, return None so that auth fails - if user.deleted: - return None - else: - return user + # If user is soft-deleted, return None so that auth fails + # this should never occur in practice because user email is modified with a suffix of "_DELETED_{timestamp}" at deletion, but it is ok to retain this check + if user and user.deleted: + return None - if EMAIL_DOMAIN_ALIASES: + # if user is not found by email, check for aliases + if not user and EMAIL_DOMAIN_ALIASES: email_local_part = email.split("@")[0] email_domain = email.split("@")[1] @@ -213,7 +210,14 @@ def _get_update_or_create_user(email: str) -> User: if user and not user.deleted: user.email = email user.save() - return user + + # if user is found directly or via alias (and was not soft-deleted), ensure that user's self group is named correctly, then return user + if user and not user.deleted: + if user.self_group.name != user.email: + user.self_group.name = user.email + user.self_group.save() + + return user # Create the user since no match has been found at this point return _create_user(email) diff --git a/backend/lambdas/api/authorizer/tests/test_get_user.py b/backend/lambdas/api/authorizer/tests/test_get_user.py index 1a1f7dce..8daea677 100644 --- a/backend/lambdas/api/authorizer/tests/test_get_user.py +++ b/backend/lambdas/api/authorizer/tests/test_get_user.py @@ -1,9 +1,9 @@ -import authorizer.handlers import copy import unittest - from unittest.mock import patch +import authorizer.handlers + EMAIL_DOMAIN_ALIASES = [ { "new_domain": "company1.com", @@ -30,28 +30,51 @@ "email": "first.last@company.com", "deleted": False, "last_login": "2023-01-01 00:00:00.000000+00:00", + "self_group": {"name": "first.last@company.com"}, }, { "id": 2, "email": "first_last@company1.com", "deleted": False, "last_login": "2023-01-01 00:00:00.000000+00:00", + "self_group": {"name": "first_last@company1.com"}, }, { "id": 3, "email": "first.last.1@company.com", "deleted": True, "last_login": "2023-01-01 00:00:00.000000+00:00", + "self_group": {"name": "first.last.1@company.com"}, }, { "id": 4, "email": "last.first@company2.com", "deleted": False, "last_login": "2023-01-01 00:00:00.000000+00:00", + "self_group": {"name": "last.first@company2.com"}, + }, + { + "id": 5, + "email": "first.last@company4.com", + "deleted": False, + "last_login": "2023-01-01 00:00:00.000000+00:00", + "self_group": {"name": "first_last@company3.com"}, }, ] +class MockGroup(object): + def __init__(self, **kwargs): + self.name = kwargs.get("name") or "" + + def save(self): + pass + + @classmethod + def create_self_group(cls, user): + user.self_group = MockGroup(name=user.email) + + class MockUser(object): users = [] @@ -62,6 +85,9 @@ def __init__(self, **kwargs): self.email = kwargs.get("email") self.deleted = kwargs.get("deleted") or False self.last_login = kwargs.get("last_login") or "" + self_group = kwargs.get("self_group") or None + if self_group: + self.self_group = MockGroup(name=self_group.get("name")) def save(self): for user in MockUser.users: @@ -93,8 +119,8 @@ def get(email: str, **kwargs): @patch("authorizer.handlers.EMAIL_DOMAIN_ALIASES", EMAIL_DOMAIN_ALIASES) -@patch("authorizer.handlers.Group.create_self_group", lambda *x, **y: None) @patch("authorizer.handlers.User", MockUser) +@patch("authorizer.handlers.Group", MockGroup) class TestGetUser(unittest.TestCase): def test_get_existing_user(self): """ @@ -103,7 +129,11 @@ def test_get_existing_user(self): MockUser.users = copy.deepcopy(USERS) email = "first.last@company.com" user = _get_update_or_create_user(email=email) - self.assertTrue(user.__dict__.get("id") == 1 and user.__dict__.get("email") == email) + self.assertTrue( + user.__dict__.get("id") == 1 + and user.__dict__.get("email") == email + and user.__dict__.get("self_group").name == email + ) def test_get_deleted_user(self): """ @@ -120,48 +150,83 @@ def test_get_nonexistent_user(self): """ MockUser.users = copy.deepcopy(USERS) email = "first.last@doesnotexist.com" + expected_userid = MockUser.users[-1].get("id") + 1 user = _get_update_or_create_user(email=email) - self.assertTrue(user.__dict__.get("id") == 5 and user.__dict__.get("email") == email) + self.assertTrue( + user.__dict__.get("id") == expected_userid + and user.__dict__.get("email") == email + and user.__dict__.get("self_group").name == email + ) def test_get_user_with_new_email(self): """ User logs in with email "first.last@newcompany.com" and has an existing account with email "first.last@company.com" - Existing account is found, and email is updated to the new email "first.last@newcompany.com" + Existing account is found, and email and self group name are updated to the new email "first.last@newcompany.com" """ MockUser.users = copy.deepcopy(USERS) email = "first.last@newcompany.com" user = _get_update_or_create_user(email=email) - self.assertTrue(user.__dict__.get("id") == 1 and user.__dict__.get("email") == email) + self.assertTrue( + user.__dict__.get("id") == 1 + and user.__dict__.get("email") == email + and user.__dict__.get("self_group").name == email + ) def test_get_user_with_new_email_with_transformation(self): """ User logs in with email "first.last@company1.com" and has an existing account with email "first_last@company1.com" - Existing account is found, and email is updated to the new email "first.last@company1.com" + Existing account is found, and email and self group name are updated to the new email "first.last@company1.com" """ MockUser.users = copy.deepcopy(USERS) email = "first.last@company1.com" user = _get_update_or_create_user(email=email) - self.assertTrue(user.__dict__.get("id") == 2 and user.__dict__.get("email") == email) + self.assertTrue( + user.__dict__.get("id") == 2 + and user.__dict__.get("email") == email + and user.__dict__.get("self_group").name == email + ) def test_get_user_with_new_email_with_transformation2(self): """ User logs in with email "first.last@company2.com" and has an existing account with email "last.first@company2.com" - Existing account is found, and email is updated to the new email "first.last@company2.com" + Existing account is found, and email and self group name are updated to the new email "first.last@company2.com" """ MockUser.users = copy.deepcopy(USERS) email = "first.last@company2.com" user = _get_update_or_create_user(email=email) - self.assertTrue(user.__dict__.get("id") == 4 and user.__dict__.get("email") == email) + self.assertTrue( + user.__dict__.get("id") == 4 + and user.__dict__.get("email") == email + and user.__dict__.get("self_group").name == email + ) def test_get_user_with_new_email_with_transformation3(self): """ User logs in with email "first_last@company3.com" and has an existing account with email "first.last@company.com" - Existing account is found, and email is updated to the new email "first_last@company3.com" + Existing account is found, and email and self group name are updated to the new email "first_last@company3.com" """ MockUser.users = copy.deepcopy(USERS) email = "first_last@company3.com" user = _get_update_or_create_user(email=email) - self.assertTrue(user.__dict__.get("id") == 1 and user.__dict__.get("email") == email) + self.assertTrue( + user.__dict__.get("id") == 1 + and user.__dict__.get("email") == email + and user.__dict__.get("self_group").name == email + ) + + def test_get_user_with_email_and_self_group_mismatch(self): + """ + User logs in with email "first.last@company4.com" and has an existing account with email "first.last@company4.com", but self-group name does not match + Existing account is found, and self group name is updated to the new email "first.last@company4.com" + """ + MockUser.users = copy.deepcopy(USERS) + email = "first.last@company4.com" + user = _get_update_or_create_user(email=email) + self.assertTrue( + user.__dict__.get("id") == 5 + and user.__dict__.get("email") == email + and user.__dict__.get("self_group").name == email + ) def test_get_user_with_new_email_and_deleted_old_user(self): """ @@ -170,5 +235,10 @@ def test_get_user_with_new_email_and_deleted_old_user(self): """ MockUser.users = copy.deepcopy(USERS) email = "first.last.1@newcompany.com" + expected_userid = MockUser.users[-1].get("id") + 1 user = _get_update_or_create_user(email=email) - self.assertTrue(user.__dict__.get("id") == 5 and user.__dict__.get("email") == email) + self.assertTrue( + user.__dict__.get("id") == expected_userid + and user.__dict__.get("email") == email + and user.__dict__.get("self_group").name == email + ) diff --git a/backend/lambdas/api/repo/repo/util/authorize.py b/backend/lambdas/api/repo/repo/util/authorize.py index fed6ddb8..388fd623 100644 --- a/backend/lambdas/api/repo/repo/util/authorize.py +++ b/backend/lambdas/api/repo/repo/util/authorize.py @@ -3,10 +3,13 @@ from artemisapi.response import response from artemisdb.artemisdb.models import UserService +from artemislib.logging import Logger from repo.util.parse_event import EventParser from repo.util.scope import update_scope_cache, validate_scope_with_github from repo.util.utils import auth +log = Logger(__name__) + def authorize(event_parser: EventParser) -> dict: """ @@ -24,17 +27,25 @@ def authorize(event_parser: EventParser) -> dict: if not allowed and service_id == "github" and identity.principal_type != "group_api_key": # If not allowed and not a group API key, check permission with GitHub + log.debug( + f"{identity.principal_id} does not have static or cached scope to {repo_id}, checking for Github access" + ) try: github_account = UserService.objects.get( user__email=identity.principal_id, user__deleted=False, service="github" ) github_username = github_account.username except UserService.DoesNotExist: + log.debug(f"Linked Github account not found for {identity.principal_id}") return response({"message": f"Not authorized for {repo_id}"}, code=HTTPStatus.FORBIDDEN) + log.debug(f"Linked Github account found for {identity.principal_id}, validating Github access to {repo_id}") validated = validate_scope_with_github(repo_id, service_id, github_username) if validated: + log.debug( + f"Linked Github account found for {identity.principal_id} has access to {repo_id}, updating scope cache" + ) scope_cache += [f"github/{repo_id}"] update_scope_cache(github_username, scope_cache) @@ -58,8 +69,14 @@ def authorize(event_parser: EventParser) -> dict: # the scopes from the self group so that they are given equal consideration # and the group scope does not limit what was verified with GitHub but is # still restricted by the key's own scope. + + log.debug(f"Checking if {repo_id} in API key scope {identity.scope} for {identity.principal_id}") identity.scope[0][1] += [f"github/{repo_id}"] allowed = auth(repo_id, service_id, identity.scope) + if allowed: + log.debug(f"Repo {repo_id} in key scope") + else: + log.debug(f"Repo {repo_id} not in key scope") else: # Users have this scope structure: # [ @@ -88,6 +105,8 @@ def authorize(event_parser: EventParser) -> dict: if not allowed: return response({"message": f"Not authorized for {repo_id}"}, code=HTTPStatus.FORBIDDEN) + log.debug(f"{identity.principal_id} authorized to {repo_id}") + def allowlist_is_denied(method: str, resource: str, service: str, repo: str, allowlist_denied: list): if method in ["POST", "PUT", "DELETE"] and resource == "whitelist": diff --git a/backend/lambdas/api/repo/repo/util/scope.py b/backend/lambdas/api/repo/repo/util/scope.py index 07cd579c..0f305c5f 100644 --- a/backend/lambdas/api/repo/repo/util/scope.py +++ b/backend/lambdas/api/repo/repo/util/scope.py @@ -1,7 +1,6 @@ import json import requests - from artemislib.datetime import get_utc_datetime from artemislib.db_cache import DBLookupCache from artemislib.logging import Logger @@ -35,11 +34,15 @@ def validate_scope_with_github(repo_id: str, service_id: str, service_user: str) headers = {"accept": "application/vnd.github.v3+json", "authorization": authorization} + log.debug(f"Attempting to validate permissions for github user {service_user} to repo {repo_id}") + r = requests.get( f"https://api.github.com/repos/{repo_id}/collaborators/{service_user}/permission", headers=headers, ) + log.debug(f"Github request returned status code: {r.status_code}") + permission = r.json().get("permission") if not permission: @@ -47,4 +50,6 @@ def validate_scope_with_github(repo_id: str, service_id: str, service_user: str) log.error(f"Status: {r.status_code}") log.error(f"Body: {r.text}") + log.debug(f"Permission returned for user {service_user} to {repo_id}: {permission}") + return permission in ["admin", "write", "read"]