From 1dd0479777f4c5cbf1d9bdb42645125af1685415 Mon Sep 17 00:00:00 2001 From: Jay Zeng Date: Thu, 23 Jan 2025 09:52:30 -0500 Subject: [PATCH 1/3] Get user id if not set --- rbac/management/tenant_service/__init__.py | 14 ++++++-- rbac/management/tenant_service/v2.py | 34 ++++++++++-------- rbac/rbac/middleware.py | 19 +++++++++- tests/management/tenant/test_model.py | 41 ++++++++++++++++++++++ 4 files changed, 91 insertions(+), 17 deletions(-) diff --git a/rbac/management/tenant_service/__init__.py b/rbac/management/tenant_service/__init__.py index 9020b48e..3fb1d0a6 100644 --- a/rbac/management/tenant_service/__init__.py +++ b/rbac/management/tenant_service/__init__.py @@ -1,12 +1,22 @@ """Tenant management business logic.""" +from typing import Callable, Optional + from django.conf import settings from management.relation_replicator.relation_replicator import RelationReplicator from management.tenant_service.tenant_service import TenantBootstrapService from management.tenant_service.v1 import V1TenantBootstrapService from management.tenant_service.v2 import V2TenantBootstrapService +from api.models import User + -def get_tenant_bootstrap_service(replicator: RelationReplicator) -> "TenantBootstrapService": +def get_tenant_bootstrap_service( + replicator: RelationReplicator, get_user_id: Optional[Callable[[User], str]] = None +) -> "TenantBootstrapService": """Get a TenantBootstrapService instance based on settings.""" - return V2TenantBootstrapService(replicator) if settings.V2_BOOTSTRAP_TENANT else V1TenantBootstrapService() + return ( + V2TenantBootstrapService(replicator, get_user_id=get_user_id) + if settings.V2_BOOTSTRAP_TENANT + else V1TenantBootstrapService() + ) diff --git a/rbac/management/tenant_service/v2.py b/rbac/management/tenant_service/v2.py index 4f714289..96394066 100644 --- a/rbac/management/tenant_service/v2.py +++ b/rbac/management/tenant_service/v2.py @@ -1,6 +1,6 @@ """V2 implementation of Tenant bootstrapping.""" -from typing import List, Optional +from typing import Callable, List, Optional from uuid import UUID from django.conf import settings @@ -24,6 +24,13 @@ from api.models import Tenant, User +def get_user_id(user: User): + """Get user ID.""" + if user.user_id is None: + raise ValueError(f"Cannot update user without user_id. username={user.username}") + return user.user_id + + class V2TenantBootstrapService: """Service for bootstrapping tenants with built-in relationships.""" @@ -32,11 +39,19 @@ class V2TenantBootstrapService: _public_tenant: Optional[Tenant] _platform_default_policy_uuid: Optional[str] = None _admin_default_policy_uuid: Optional[str] = None + _get_user_id: Callable[[User], str] = get_user_id - def __init__(self, replicator: RelationReplicator, public_tenant: Optional[Tenant] = None): + def __init__( + self, + replicator: RelationReplicator, + public_tenant: Optional[Tenant] = None, + get_user_id: Optional[Callable[[User], str]] = None, + ): """Initialize the TenantBootstrapService with a RelationReplicator.""" self._replicator = replicator self._public_tenant = public_tenant + if get_user_id: + self._get_user_id = get_user_id def new_bootstrapped_tenant(self, org_id: str, account_number: Optional[str] = None) -> BootstrappedTenant: """Create a new tenant.""" @@ -90,10 +105,7 @@ def update_user( if mapping is None: raise ValueError(f"Expected TenantMapping but got None. org_id: {bootstrapped_tenant.tenant.org_id}") - user_id = user.user_id - - if user_id is None: - raise ValueError(f"Cannot update user without user_id. username={user.username}") + user_id = self._get_user_id(user) tuples_to_add = [] tuples_to_remove = [] @@ -205,13 +217,10 @@ def _disable_user_in_tenant(self, user: User): # Get tenant mapping if present but no need to create if not tuples_to_remove = [] - user_id = user.user_id + user_id = self._get_user_id(user) mapping: Optional[TenantMapping] = None principal_uuid = "" - if user_id is None: - raise ValueError(f"User {user.username} has no user_id.") - logger.info( f"Removing Principal and group membership from RBAC and Relations. user_id={user_id} org_id={user.org_id}" ) @@ -409,10 +418,7 @@ def _default_group_tuple_edits(self, user: User, mapping) -> tuple[list[Relation """Get the tuples to add and remove for a user.""" tuples_to_add = [] tuples_to_remove = [] - user_id = user.user_id - - if user_id is None: - raise ValueError(f"User {user.username} has no user_id.") + user_id = self._get_user_id(user) tuples_to_add.append(Group.relationship_to_user_id_for_group(str(mapping.default_group_uuid), user_id)) diff --git a/rbac/rbac/middleware.py b/rbac/rbac/middleware.py index 748fe833..ecc6392e 100644 --- a/rbac/rbac/middleware.py +++ b/rbac/rbac/middleware.py @@ -21,6 +21,7 @@ import logging from json.decoder import JSONDecodeError +import sentry_sdk from django.conf import settings from django.core.handlers.wsgi import WSGIRequest from django.db import IntegrityError, transaction @@ -29,6 +30,7 @@ from django.utils.deprecation import MiddlewareMixin from management.cache import TenantCache from management.models import Principal +from management.principal.proxy import PrincipalProxy from management.relation_replicator.outbox_replicator import OutboxReplicator from management.tenant_service import get_tenant_bootstrap_service from management.tenant_service.tenant_service import TenantBootstrapService @@ -90,6 +92,21 @@ class HttpResponseUnauthorizedRequest(HttpResponse): status_code = 401 +PROXY = PrincipalProxy() + + +def get_user_id(user: User): + """Return the user ID for the given user.""" + user_id = user.user_id + if not user_id: + resp = PROXY.request_filtered_principals([user.username], org_id=user.org_id, options={"return_id": True}) + if isinstance(resp, dict) and "errors" in resp: + sentry_sdk.capture_message(resp.get("errors")) + return + return resp["data"][0]["user_id"] + return user.user_id + + class IdentityHeaderMiddleware(MiddlewareMixin): """A subclass of RemoteUserMiddleware. @@ -106,7 +123,7 @@ def __init__(self, get_response): # In this case the replicator needs to include a precondition # which does not add the tuples if any others already exist for the tenant # (the tx will be rolled back in that case) - self.bootstrap_service = get_tenant_bootstrap_service(OutboxReplicator()) + self.bootstrap_service = get_tenant_bootstrap_service(OutboxReplicator(), get_user_id) def get_tenant(self, model, hostname, request): """Override the tenant selection logic.""" diff --git a/tests/management/tenant/test_model.py b/tests/management/tenant/test_model.py index 95f0a6d2..a7dc5950 100644 --- a/tests/management/tenant/test_model.py +++ b/tests/management/tenant/test_model.py @@ -17,6 +17,7 @@ """Test cases for Tenant bootstrapping logic.""" from typing import Optional, Tuple +from unittest.mock import patch from django.test import TestCase from management.group.definer import seed_group from management.group.model import Group @@ -33,6 +34,7 @@ resource, subject, ) +from rbac.middleware import get_user_id from tests.management.role.test_dual_write import RbacFixture from api.models import Tenant, User @@ -427,6 +429,45 @@ def test_force_bootstrap_replicates_already_bootstrapped_ready_tenants(self): self.assertEqual(original_mapping, new_mapping) self.assertCountEqual(original_workspaces, new_workspaces) + @patch( + "management.principal.proxy.PrincipalProxy.request_filtered_principals", + return_value={ + "status_code": 200, + "data": [ + { + "username": "user_1", + "email": "test_user@email.com", + "first_name": "user", + "last_name": "test", + "user_id": "u1", + } + ], + }, + ) + def test_inject_get_user_id_method(self, _): + bootstrapped = self.fixture.new_tenant(org_id="o1") + + user = User() + user.org_id = "o1" + user.admin = False + user.is_active = True + user.user_name = "user_1" + + service = V2TenantBootstrapService(InMemoryRelationReplicator(self.tuples), get_user_id=get_user_id) + bootstrapped = service.update_user(user) + + # But is a regular user + self.assertEqual( + 1, + self.tuples.count_tuples( + all_of( + resource("rbac", "group", bootstrapped.mapping.default_group_uuid), + relation("member"), + subject("rbac", "principal", "localhost/u1"), + ) + ), + ) + def assertAddedToDefaultGroup(self, user_id: str, tenant_mapping: TenantMapping, and_admin_group: bool = False): self.assertEqual( 1, From 9d7c9261d4bf7e32178fd8cafa74119f5ad4edb3 Mon Sep 17 00:00:00 2001 From: Jay Zeng Date: Thu, 23 Jan 2025 10:48:07 -0500 Subject: [PATCH 2/3] Address feedback --- rbac/management/tenant_service/v2.py | 6 ++--- rbac/rbac/middleware.py | 3 +++ tests/management/tenant/test_model.py | 20 ++------------ tests/rbac/test_middleware.py | 39 +++++++++++++++++++++++++++ 4 files changed, 46 insertions(+), 22 deletions(-) diff --git a/rbac/management/tenant_service/v2.py b/rbac/management/tenant_service/v2.py index 96394066..6d345d85 100644 --- a/rbac/management/tenant_service/v2.py +++ b/rbac/management/tenant_service/v2.py @@ -24,7 +24,7 @@ from api.models import Tenant, User -def get_user_id(user: User): +def default_get_user_id(user: User): """Get user ID.""" if user.user_id is None: raise ValueError(f"Cannot update user without user_id. username={user.username}") @@ -39,7 +39,6 @@ class V2TenantBootstrapService: _public_tenant: Optional[Tenant] _platform_default_policy_uuid: Optional[str] = None _admin_default_policy_uuid: Optional[str] = None - _get_user_id: Callable[[User], str] = get_user_id def __init__( self, @@ -50,8 +49,7 @@ def __init__( """Initialize the TenantBootstrapService with a RelationReplicator.""" self._replicator = replicator self._public_tenant = public_tenant - if get_user_id: - self._get_user_id = get_user_id + self._get_user_id = get_user_id if get_user_id else default_get_user_id def new_bootstrapped_tenant(self, org_id: str, account_number: Optional[str] = None) -> BootstrappedTenant: """Create a new tenant.""" diff --git a/rbac/rbac/middleware.py b/rbac/rbac/middleware.py index ecc6392e..2168c39e 100644 --- a/rbac/rbac/middleware.py +++ b/rbac/rbac/middleware.py @@ -103,6 +103,9 @@ def get_user_id(user: User): if isinstance(resp, dict) and "errors" in resp: sentry_sdk.capture_message(resp.get("errors")) return + if not resp.get("data"): + sentry_sdk.capture_message(f"No user found of user name {user.username}.") + raise Http404() return resp["data"][0]["user_id"] return user.user_id diff --git a/tests/management/tenant/test_model.py b/tests/management/tenant/test_model.py index a7dc5950..150de558 100644 --- a/tests/management/tenant/test_model.py +++ b/tests/management/tenant/test_model.py @@ -17,7 +17,6 @@ """Test cases for Tenant bootstrapping logic.""" from typing import Optional, Tuple -from unittest.mock import patch from django.test import TestCase from management.group.definer import seed_group from management.group.model import Group @@ -34,7 +33,6 @@ resource, subject, ) -from rbac.middleware import get_user_id from tests.management.role.test_dual_write import RbacFixture from api.models import Tenant, User @@ -429,22 +427,7 @@ def test_force_bootstrap_replicates_already_bootstrapped_ready_tenants(self): self.assertEqual(original_mapping, new_mapping) self.assertCountEqual(original_workspaces, new_workspaces) - @patch( - "management.principal.proxy.PrincipalProxy.request_filtered_principals", - return_value={ - "status_code": 200, - "data": [ - { - "username": "user_1", - "email": "test_user@email.com", - "first_name": "user", - "last_name": "test", - "user_id": "u1", - } - ], - }, - ) - def test_inject_get_user_id_method(self, _): + def test_inject_get_user_id_method(self): bootstrapped = self.fixture.new_tenant(org_id="o1") user = User() @@ -453,6 +436,7 @@ def test_inject_get_user_id_method(self, _): user.is_active = True user.user_name = "user_1" + get_user_id = lambda user: "u1" service = V2TenantBootstrapService(InMemoryRelationReplicator(self.tuples), get_user_id=get_user_id) bootstrapped = service.update_user(user) diff --git a/tests/rbac/test_middleware.py b/tests/rbac/test_middleware.py index 3a1e2d63..acfa39b9 100644 --- a/tests/rbac/test_middleware.py +++ b/tests/rbac/test_middleware.py @@ -776,6 +776,45 @@ def test_bootstraps_tenants_if_not_existing(self): ), ) + @patch( + "management.principal.proxy.PrincipalProxy.request_filtered_principals", + return_value={ + "status_code": 200, + "data": [ + { + "username": "user_1", + "email": "test_user@email.com", + "first_name": "user", + "last_name": "test", + "user_id": "u1", + "org_id": "12345", + } + ], + }, + ) + def test_bootstraps_tenants_if_user_id_is_missing(self, _): + with patch("rbac.middleware.OutboxReplicator", new=partial(InMemoryRelationReplicator, self._tuples)): + # Change the user's org so we create a new tenant + self.request.user.org_id = "12345" + self.org_id = "12345" + self.request.user.id = None + mock_request = self.request + tenant_cache = TenantCache() + tenant_cache.delete_tenant(self.org_id) + middleware = IdentityHeaderMiddleware(get_response=IdentityHeaderMiddleware.get_tenant) + result = middleware.get_tenant(Tenant, "localhost", mock_request) + self.assertEqual(result.org_id, mock_request.user.org_id) + tenant = Tenant.objects.get(org_id=self.org_id) + self.assertIsNotNone(tenant) + mapping = TenantMapping.objects.get(tenant=tenant) + self.assertIsNotNone(mapping) + workspaces = list(Workspace.objects.filter(tenant=tenant)) + self.assertEqual(len(workspaces), 2) + default = Workspace.objects.get(type=Workspace.Types.DEFAULT, tenant=tenant) + self.assertIsNotNone(default) + root = Workspace.objects.get(type=Workspace.Types.ROOT, tenant=tenant) + self.assertIsNotNone(root) + @override_settings(V2_BOOTSTRAP_TENANT=True) class V2IdentityHeaderMiddlewareTest(IdentityHeaderMiddlewareTest): From 0a8b8c3ac8be83cb0202c3ca1c66c545f32a8029 Mon Sep 17 00:00:00 2001 From: Jay Zeng Date: Thu, 23 Jan 2025 13:29:29 -0500 Subject: [PATCH 3/3] User logging instead of sentry capture --- rbac/rbac/middleware.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/rbac/rbac/middleware.py b/rbac/rbac/middleware.py index 2168c39e..993dd168 100644 --- a/rbac/rbac/middleware.py +++ b/rbac/rbac/middleware.py @@ -21,7 +21,6 @@ import logging from json.decoder import JSONDecodeError -import sentry_sdk from django.conf import settings from django.core.handlers.wsgi import WSGIRequest from django.db import IntegrityError, transaction @@ -101,10 +100,10 @@ def get_user_id(user: User): if not user_id: resp = PROXY.request_filtered_principals([user.username], org_id=user.org_id, options={"return_id": True}) if isinstance(resp, dict) and "errors" in resp: - sentry_sdk.capture_message(resp.get("errors")) + logging.warning(resp.get("errors")) return if not resp.get("data"): - sentry_sdk.capture_message(f"No user found of user name {user.username}.") + logging.warning(f"No user found of user name {user.username}.") raise Http404() return resp["data"][0]["user_id"] return user.user_id