From dd68ce275e624a7b175d9cd4986dd766a41d5a92 Mon Sep 17 00:00:00 2001 From: Tasko Olevski Date: Wed, 26 Jun 2024 10:47:01 +0200 Subject: [PATCH] fix: properly cascade group permissions to all projects in the group (#277) --- .../background_jobs/config.py | 11 ++- .../background_jobs/core.py | 62 ++++++++++++- .../background_jobs/main.py | 3 +- components/renku_data_services/authz/authz.py | 13 ++- components/renku_data_services/git/gitlab.py | 4 +- .../migrations/alembic.ini | 2 +- .../namespace/blueprints.py | 4 +- .../renku_data_services/namespace/db.py | 47 +++------- .../renku_data_services/namespace/models.py | 6 +- .../renku_data_services/namespace/orm.py | 58 +++++++++--- .../background_jobs/test_sync.py | 90 ++++++++++++++++++- .../renku_data_services/data_api/conftest.py | 20 ++++- .../data_api/test_groups.py | 85 ++++++++++++++++-- .../data_api/test_storage.py | 34 +++---- .../authz/test_authorization.py | 40 +++++++-- .../message_queue/test_queue.py | 8 +- 16 files changed, 388 insertions(+), 99 deletions(-) diff --git a/bases/renku_data_services/background_jobs/config.py b/bases/renku_data_services/background_jobs/config.py index 4a3f63f4a..706729871 100644 --- a/bases/renku_data_services/background_jobs/config.py +++ b/bases/renku_data_services/background_jobs/config.py @@ -14,6 +14,7 @@ from renku_data_services.message_queue.db import EventRepository from renku_data_services.message_queue.redis_queue import RedisQueue from renku_data_services.namespace.db import GroupRepository +from renku_data_services.project.db import ProjectRepository from renku_data_services.users.db import UsersSync from renku_data_services.users.kc_api import IKeycloakAPI, KeycloakAPI @@ -27,6 +28,7 @@ class SyncConfig: authz_config: AuthzConfig group_repo: GroupRepository event_repo: EventRepository + project_repo: ProjectRepository session_maker: Callable[..., AsyncSession] @classmethod @@ -58,6 +60,13 @@ def from_env(cls, prefix: str = "") -> "SyncConfig": group_authz=Authz(authz_config), message_queue=message_queue, ) + project_repo = ProjectRepository( + session_maker=session_maker, + message_queue=message_queue, + event_repo=event_repo, + group_repo=group_repo, + authz=Authz(authz_config), + ) syncer = UsersSync( session_maker, message_queue=message_queue, @@ -70,4 +79,4 @@ def from_env(cls, prefix: str = "") -> "SyncConfig": client_secret = os.environ[f"{prefix}KEYCLOAK_CLIENT_SECRET"] realm = os.environ.get(f"{prefix}KEYCLOAK_REALM", "Renku") kc_api = KeycloakAPI(keycloak_url=keycloak_url, client_id=client_id, client_secret=client_secret, realm=realm) - return cls(syncer, kc_api, authz_config, group_repo, event_repo, session_maker) + return cls(syncer, kc_api, authz_config, group_repo, event_repo, project_repo, session_maker) diff --git a/bases/renku_data_services/background_jobs/core.py b/bases/renku_data_services/background_jobs/core.py index 0284e4d4a..f4bda5fe7 100644 --- a/bases/renku_data_services/background_jobs/core.py +++ b/bases/renku_data_services/background_jobs/core.py @@ -2,12 +2,21 @@ import logging -from authzed.api.v1.permission_service_pb2 import ReadRelationshipsRequest, RelationshipFilter +from authzed.api.v1.core_pb2 import ObjectReference, Relationship, RelationshipUpdate, SubjectReference +from authzed.api.v1.permission_service_pb2 import ( + Consistency, + ReadRelationshipsRequest, + RelationshipFilter, + SubjectFilter, + WriteRelationshipsRequest, +) from renku_data_services.authz.authz import Authz, ResourceType, _Relation from renku_data_services.background_jobs.config import SyncConfig +from renku_data_services.base_models.core import InternalServiceAdmin, ServiceAdminId from renku_data_services.message_queue.avro_models.io.renku.events import v2 from renku_data_services.message_queue.converters import EventConverter +from renku_data_services.namespace.models import NamespaceKind async def sync_user_namespaces(config: SyncConfig) -> None: @@ -67,3 +76,54 @@ async def bootstrap_user_namespaces(config: SyncConfig) -> None: ) return await sync_user_namespaces(config) + + +async def fix_mismatched_project_namespace_ids(config: SyncConfig) -> None: + """Fixes a problem where the project namespace relationship for projects has the wrong group ID.""" + api_user = InternalServiceAdmin(id=ServiceAdminId.migrations) + authz = Authz(config.authz_config) + res = authz.client.ReadRelationships( + ReadRelationshipsRequest( + consistency=Consistency(fully_consistent=True), + relationship_filter=RelationshipFilter( + resource_type=ResourceType.project, + optional_relation=_Relation.project_namespace.value, + optional_subject_filter=SubjectFilter(subject_type=ResourceType.group.value), + ), + ) + ) + async for rel in res: + logging.info(f"Checking project namespace - group relation {rel} for correct group ID") + project_id = rel.relationship.resource.object_id + project = await config.project_repo.get_project(api_user, project_id) + if project.namespace.kind != NamespaceKind.group: + continue + correct_group_id = project.namespace.underlying_resource_id + authzed_group_id = rel.relationship.subject.object.object_id + if authzed_group_id != correct_group_id: + logging.info( + f"The project namespace ID in Authzed {authzed_group_id} " + f"does not match the expected group ID {correct_group_id}, correcting it..." + ) + authz.client.WriteRelationships( + WriteRelationshipsRequest( + updates=[ + RelationshipUpdate( + operation=RelationshipUpdate.OPERATION_TOUCH, + relationship=Relationship( + resource=rel.relationship.resource, + relation=rel.relationship.relation, + subject=SubjectReference( + object=ObjectReference( + object_type=ResourceType.group.value, object_id=correct_group_id + ) + ), + ), + ), + RelationshipUpdate( + operation=RelationshipUpdate.OPERATION_DELETE, + relationship=rel.relationship, + ), + ] + ) + ) diff --git a/bases/renku_data_services/background_jobs/main.py b/bases/renku_data_services/background_jobs/main.py index 88989f749..32937f992 100644 --- a/bases/renku_data_services/background_jobs/main.py +++ b/bases/renku_data_services/background_jobs/main.py @@ -7,7 +7,7 @@ from renku_data_services.authz.admin_sync import sync_admins_from_keycloak from renku_data_services.authz.authz import Authz from renku_data_services.background_jobs.config import SyncConfig -from renku_data_services.background_jobs.core import bootstrap_user_namespaces +from renku_data_services.background_jobs.core import bootstrap_user_namespaces, fix_mismatched_project_namespace_ids from renku_data_services.migrations.core import run_migrations_for_app logging.basicConfig(level=logging.INFO) @@ -20,6 +20,7 @@ async def short_period_sync() -> None: await bootstrap_user_namespaces(config) await config.syncer.events_sync(config.kc_api) await sync_admins_from_keycloak(config.kc_api, Authz(config.authz_config)) + await fix_mismatched_project_namespace_ids(config) async def long_period_sync() -> None: diff --git a/components/renku_data_services/authz/authz.py b/components/renku_data_services/authz/authz.py index 12ef69cae..595595398 100644 --- a/components/renku_data_services/authz/authz.py +++ b/components/renku_data_services/authz/authz.py @@ -30,6 +30,7 @@ from renku_data_services import base_models from renku_data_services.authz.config import AuthzConfig from renku_data_services.authz.models import Change, Member, MembershipChange, Role, Scope, Visibility +from renku_data_services.base_models.core import InternalServiceAdmin from renku_data_services.errors import errors from renku_data_services.namespace.models import Group, GroupUpdate, Namespace, NamespaceKind, NamespaceUpdate from renku_data_services.project.models import Project, ProjectUpdate @@ -232,7 +233,8 @@ async def decorated_function( message=f"The user with ID {user.id} cannot perform operation {operation} " f"on resource {resource_type} with ID {resource.id} or the resource does not exist." ) - return await f(self, user, *args, **kwargs, zed_token=zed_token) + kwargs["zed_token"] = zed_token + return await f(self, user, *args, **kwargs) return decorated_function @@ -267,7 +269,8 @@ async def decorated_function( message=f"The user with ID {user.id} cannot perform operation {operation} on {resource_type.value} " f"with ID {resource_id} or the resource does not exist." ) - return await f(self, user, resource_type, resource_id, *args, **kwargs, zed_token=zed_token) + kwargs["zed_token"] = zed_token + return await f(self, user, resource_type, resource_id, *args, **kwargs) return decorated_function @@ -291,12 +294,14 @@ def client(self) -> AsyncClient: async def _has_permission( self, user: base_models.APIUser, resource_type: ResourceType, resource_id: str | None, scope: Scope - ) -> tuple[bool, ZedToken]: + ) -> tuple[bool, ZedToken | None]: """Checks whether the provided user has a specific permission on the specific resource.""" if not resource_id: raise errors.ProgrammingError( message=f"Cannot check permissions on a resource of type {resource_type} with missing resource ID." ) + if isinstance(user, InternalServiceAdmin): + return True, None res = _AuthzConverter.to_object(resource_type, resource_id) sub = SubjectReference( object=_AuthzConverter.to_object(ResourceType.user, user.id) @@ -565,7 +570,7 @@ def _add_project(self, project: Project) -> _AuthzChange: project_namespace = SubjectReference( object=_AuthzConverter.user_namespace(project.namespace.id) if project.namespace.kind == NamespaceKind.user - else _AuthzConverter.group(project.namespace.id) + else _AuthzConverter.group(project.namespace.underlying_resource_id) ) project_in_platform = Relationship( resource=project_res, diff --git a/components/renku_data_services/git/gitlab.py b/components/renku_data_services/git/gitlab.py index 426a2b4da..4c5406d51 100644 --- a/components/renku_data_services/git/gitlab.py +++ b/components/renku_data_services/git/gitlab.py @@ -119,11 +119,11 @@ def _process_projects( class DummyGitlabAPI: """Dummy gitlab API. - The user with name John Doe has admin access to project 123456 and member access to 999999. + The user with name Admin Doe has admin access to project 123456 and member access to 999999. """ _store = { - "John Doe": { + "Admin Doe": { GitlabAccessLevel.MEMBER: ["999999", "123456"], GitlabAccessLevel.ADMIN: ["123456"], }, diff --git a/components/renku_data_services/migrations/alembic.ini b/components/renku_data_services/migrations/alembic.ini index bcbc03dff..d149162b2 100644 --- a/components/renku_data_services/migrations/alembic.ini +++ b/components/renku_data_services/migrations/alembic.ini @@ -87,7 +87,7 @@ keys = console keys = generic [logger_root] -level = WARN +level = INFO handlers = console qualname = diff --git a/components/renku_data_services/namespace/blueprints.py b/components/renku_data_services/namespace/blueprints.py index 81f237cbc..b1ef966eb 100644 --- a/components/renku_data_services/namespace/blueprints.py +++ b/components/renku_data_services/namespace/blueprints.py @@ -163,7 +163,7 @@ async def _get_namespaces( name=ns.name, slug=ns.latest_slug if ns.latest_slug else ns.slug, created_by=ns.created_by, - creation_date=ns.creation_date, + creation_date=None, # NOTE: we do not save creation date in the DB namespace_kind=apispec.NamespaceKind(ns.kind.value), ).model_dump(exclude_none=True, mode="json") for ns in nss @@ -186,7 +186,7 @@ async def _get_namespace(_: Request, user: base_models.APIUser, slug: str) -> JS name=ns.name, slug=ns.latest_slug if ns.latest_slug else ns.slug, created_by=ns.created_by, - creation_date=ns.creation_date, + creation_date=None, # NOTE: we do not save creation date in the DB namespace_kind=apispec.NamespaceKind(ns.kind.value), ).model_dump(exclude_none=True, mode="json") ) diff --git a/components/renku_data_services/namespace/db.py b/components/renku_data_services/namespace/db.py index 30881ef72..4100e0a3d 100644 --- a/components/renku_data_services/namespace/db.py +++ b/components/renku_data_services/namespace/db.py @@ -283,6 +283,7 @@ async def insert_group( message="The slug for the group should be unique but it already exists in the database", detail="Please modify the slug field and then retry", ) + raise err # NOTE: This is needed to populate the relationship fields in the group after inserting the ID above await session.refresh(group) return group.dump() @@ -344,15 +345,7 @@ async def get_namespace_by_slug(self, user: base_models.APIUser, slug: str) -> m raise errors.MissingResourceError( message=f"The group with slug {slug} does not exist or you do not have permissions to view it" ) - return models.Namespace( - id=ns.id, - name=ns.group.name, - created_by=ns.group.created_by, - creation_date=ns.group.creation_date, - kind=models.NamespaceKind.group, - slug=old_ns.slug if old_ns else ns.slug, - latest_slug=ns.slug, - ) + return ns.dump() if not ns.user or not ns.user_id: raise errors.ProgrammingError(message="Found a namespace that has no group or user associated with it.") is_allowed = await self.authz.has_permission(user, ResourceType.user_namespace, ns.id, Scope.READ) @@ -360,19 +353,7 @@ async def get_namespace_by_slug(self, user: base_models.APIUser, slug: str) -> m raise errors.MissingResourceError( message=f"The namespace with slug {slug} does not exist or you do not have permissions to view it" ) - name: str | None - if ns.user.first_name and ns.user.last_name: - name = f"{ns.user.first_name} {ns.user.last_name}" - else: - name = ns.user.first_name or ns.user.last_name - return models.Namespace( - id=ns.id, - name=name, - created_by=ns.user.keycloak_id, - kind=models.NamespaceKind.user, - slug=old_ns.slug if old_ns else ns.slug, - latest_slug=ns.slug, - ) + return ns.dump() async def _get_user_namespace(self, user_id: str) -> models.Namespace | None: """Get the namespace for a slug.""" @@ -382,19 +363,7 @@ async def _get_user_namespace(self, user_id: str) -> models.Namespace | None: return None if not ns.user or not ns.user_id: raise errors.ProgrammingError(message="Found a namespace that has no user associated with it.") - name: str | None - if ns.user.first_name and ns.user.last_name: - name = f"{ns.user.first_name} {ns.user.last_name}" - else: - name = ns.user.first_name or ns.user.last_name - return models.Namespace( - id=ns.id, - name=name, - created_by=ns.user.keycloak_id, - kind=models.NamespaceKind.user, - slug=ns.slug, - latest_slug=ns.slug, - ) + return ns.dump() async def _insert_user_namespace( self, session: AsyncSession, user: schemas.UserORM, retry_enumerate: int = 0, retry_random: bool = False @@ -411,12 +380,14 @@ async def _insert_user_namespace( try: async with session.begin_nested(): session.add(ns) + await session.flush() except IntegrityError: if retry_enumerate == 0: raise errors.ValidationError(message=f"The user namespace slug {slug.value} already exists") continue else: - return models.Namespace(ns.id, slug=ns.slug, created_by=ns.user_id, kind=models.NamespaceKind.user) + await session.refresh(ns) + return ns.dump() if not retry_random: raise errors.ValidationError( message=f"Cannot create generate a unique namespace slug for the user with ID {user.keycloak_id}" @@ -427,4 +398,6 @@ async def _insert_user_namespace( slug = base_models.Slug.from_name(original_slug.value.lower() + suffix) ns = schemas.NamespaceORM(slug.value, user_id=user.keycloak_id) session.add(ns) - return models.Namespace(ns.id, slug=ns.slug, created_by=ns.user_id, kind=models.NamespaceKind.user) + await session.flush() + await session.refresh(ns) + return ns.dump() diff --git a/components/renku_data_services/namespace/models.py b/components/renku_data_services/namespace/models.py index 1a748ca39..9cb93f337 100644 --- a/components/renku_data_services/namespace/models.py +++ b/components/renku_data_services/namespace/models.py @@ -53,10 +53,10 @@ class Namespace: id: str slug: str kind: NamespaceKind - name: str | None = None - creation_date: datetime | None = None - created_by: str | None = None + created_by: str + underlying_resource_id: str # The user or group ID depending on the Namespace kind latest_slug: str | None = None + name: str | None = None @dataclass diff --git a/components/renku_data_services/namespace/orm.py b/components/renku_data_services/namespace/orm.py index 419a916a3..4e768a489 100644 --- a/components/renku_data_services/namespace/orm.py +++ b/components/renku_data_services/namespace/orm.py @@ -71,15 +71,33 @@ class NamespaceORM(BaseORM): def dump(self) -> models.Namespace: """Create a namespace model from the ORM.""" - created_by = self.user_id - if self.group: - created_by = self.group.created_by + if self.group_id and self.group: + return models.Namespace( + id=self.id, + slug=self.slug, + kind=models.NamespaceKind.group, + created_by=self.group.created_by, + underlying_resource_id=self.group_id, + latest_slug=self.slug, + name=self.group.name, + ) + + if not self.user or not self.user_id: + raise errors.ProgrammingError(message="Found a namespace that has no group or user associated with it.") + + name = ( + f"{self.user.first_name} {self.user.last_name}" + if self.user.first_name and self.user.last_name + else self.user.first_name or self.user.last_name + ) return models.Namespace( id=self.id, slug=self.slug, + kind=models.NamespaceKind.user, + created_by=self.user_id, + underlying_resource_id=self.user_id, latest_slug=self.slug, - created_by=created_by, - kind=models.NamespaceKind.user if self.user_id else models.NamespaceKind.group, + name=name, ) def dump_user(self) -> UserWithNamespace: @@ -109,13 +127,33 @@ class NamespaceOldORM(BaseORM): def dump(self) -> models.Namespace: """Create an namespace model from the ORM.""" - created_by = self.latest_slug.user_id - if self.latest_slug.group: - created_by = self.latest_slug.group.created_by + if self.latest_slug.group_id and self.latest_slug.group: + return models.Namespace( + id=self.id, + slug=self.slug, + latest_slug=self.slug, + created_by=self.latest_slug.group.created_by, + kind=models.NamespaceKind.group, + underlying_resource_id=self.latest_slug.group_id, + name=self.latest_slug.group.name, + ) + + if not self.latest_slug.user or not self.latest_slug.user_id: + raise errors.ProgrammingError( + message="Found an old namespace that has no group or user associated with it in its latest slug." + ) + + name = ( + f"{self.latest_slug.user.first_name} {self.latest_slug.user.last_name}" + if self.latest_slug.user.first_name and self.latest_slug.user.last_name + else self.latest_slug.user.first_name or self.latest_slug.user.last_name + ) return models.Namespace( id=self.id, slug=self.slug, latest_slug=self.latest_slug.slug, - created_by=created_by, - kind=models.NamespaceKind.user if self.latest_slug.user_id else models.NamespaceKind.group, + created_by=self.latest_slug.user_id, + kind=models.NamespaceKind.user, + underlying_resource_id=self.latest_slug.user_id, + name=name, ) diff --git a/test/bases/renku_data_services/background_jobs/test_sync.py b/test/bases/renku_data_services/background_jobs/test_sync.py index 5859fac5a..0e67b1a45 100644 --- a/test/bases/renku_data_services/background_jobs/test_sync.py +++ b/test/bases/renku_data_services/background_jobs/test_sync.py @@ -8,22 +8,36 @@ from uuid import uuid4 import pytest -from authzed.api.v1.permission_service_pb2 import ReadRelationshipsRequest, RelationshipFilter +from authzed.api.v1.core_pb2 import Relationship, RelationshipUpdate, SubjectReference +from authzed.api.v1.permission_service_pb2 import ( + ReadRelationshipsRequest, + RelationshipFilter, + WriteRelationshipsRequest, +) from bases.renku_data_services.background_jobs.config import SyncConfig from renku_data_services.authz.admin_sync import sync_admins_from_keycloak -from renku_data_services.authz.authz import Authz, ResourceType, _Relation +from renku_data_services.authz.authz import Authz, ResourceType, _AuthzConverter, _Relation from renku_data_services.authz.config import AuthzConfig -from renku_data_services.background_jobs.core import bootstrap_user_namespaces +from renku_data_services.background_jobs.core import bootstrap_user_namespaces, fix_mismatched_project_namespace_ids from renku_data_services.base_api.pagination import PaginationRequest from renku_data_services.base_models import APIUser from renku_data_services.db_config import DBConfig +from renku_data_services.errors import errors from renku_data_services.message_queue.config import RedisConfig from renku_data_services.message_queue.db import EventRepository from renku_data_services.message_queue.redis_queue import RedisQueue from renku_data_services.migrations.core import run_migrations_for_app +from renku_data_services.namespace.apispec import ( + GroupMemberPatchRequest, + GroupMemberPatchRequestList, + GroupPostRequest, + GroupRole, +) from renku_data_services.namespace.db import GroupRepository from renku_data_services.namespace.orm import NamespaceORM +from renku_data_services.project.db import ProjectRepository +from renku_data_services.project.models import UnsavedProject from renku_data_services.users.db import UserRepo, UsersSync from renku_data_services.users.dummy_kc_api import DummyKeycloakAPI from renku_data_services.users.models import KeycloakAdminEvent, UserInfo, UserInfoUpdate @@ -49,6 +63,13 @@ def _get_app_configs(kc_api: DummyKeycloakAPI, total_user_sync: bool = False) -> group_repo=group_repo, authz=Authz(authz_config), ) + project_repo = ProjectRepository( + session_maker=db_config.async_session_maker, + message_queue=message_queue, + event_repo=event_repo, + group_repo=group_repo, + authz=Authz(authz_config), + ) config = SyncConfig( syncer=users_sync, kc_api=kc_api, @@ -56,6 +77,7 @@ def _get_app_configs(kc_api: DummyKeycloakAPI, total_user_sync: bool = False) -> group_repo=group_repo, event_repo=event_repo, session_maker=db_config.async_session_maker, + project_repo=project_repo, ) user_repo = UserRepo( db_config.async_session_maker, @@ -506,3 +528,65 @@ async def test_bootstraping_user_namespaces(get_app_configs, admin_user: APIUser await bootstrap_user_namespaces(sync_config) authz_user_namespace_ids = await get_user_namespace_ids_in_authz(authz) assert db_user_namespace_ids == authz_user_namespace_ids + + +@pytest.mark.asyncio +async def test_fixing_project_group_namespace_relations( + get_app_configs: Callable[..., tuple[SyncConfig, UserRepo]], admin_user: APIUser +): + admin_user_info = UserInfo( + id=admin_user.id, + first_name=admin_user.first_name, + last_name=admin_user.last_name, + email=admin_user.email, + ) + user1 = UserInfo("user-1-id", "John", "Doe", "john.doe@gmail.com") + user2 = UserInfo("user-2-id", "Jane", "Doe", "jane.doe@gmail.com") + user1_api = APIUser(is_admin=False, id=user1.id, access_token="access_token") + user2_api = APIUser(is_admin=False, id=user2.id, access_token="access_token") + user_roles = {admin_user.id: get_kc_roles(["renku-admin"])} + kc_api = DummyKeycloakAPI(users=get_kc_users([admin_user_info, user1, user2]), user_roles=user_roles) + sync_config, user_repo = get_app_configs(kc_api) + # Sync users + await sync_config.syncer.users_sync(kc_api) + authz = Authz(sync_config.authz_config) + # Create group + group_payload = GroupPostRequest(name="group1", slug="group1", description=None) + group = await sync_config.group_repo.insert_group(user1_api, group_payload) + # Create project + project_payload = UnsavedProject( + name="project1", slug="project1", namespace="group1", created_by=user1.id, visibility="private" + ) + project = await sync_config.project_repo.insert_project(user1_api, project_payload) + # Write the wrong group ID + await authz.client.WriteRelationships( + WriteRelationshipsRequest( + updates=[ + RelationshipUpdate( + operation=RelationshipUpdate.OPERATION_DELETE, + relationship=Relationship( + resource=_AuthzConverter.project(project.id), + relation=_Relation.project_namespace.value, + subject=SubjectReference(object=_AuthzConverter.group(group.id)), + ), + ), + RelationshipUpdate( + operation=RelationshipUpdate.OPERATION_TOUCH, + relationship=Relationship( + resource=_AuthzConverter.project(project.id), + relation=_Relation.project_namespace.value, + subject=SubjectReference(object=_AuthzConverter.group("random")), + ), + ), + ] + ) + ) + # Add group member + await sync_config.group_repo.update_group_members( + user1_api, "group1", GroupMemberPatchRequestList([GroupMemberPatchRequest(id=user2.id, role=GroupRole.viewer)]) + ) + with pytest.raises(errors.MissingResourceError): + await sync_config.project_repo.get_project(user2_api, project.id) + await fix_mismatched_project_namespace_ids(sync_config) + # After the fix you can read the project + await sync_config.project_repo.get_project(user2_api, project.id) diff --git a/test/bases/renku_data_services/data_api/conftest.py b/test/bases/renku_data_services/data_api/conftest.py index 32047f06e..31fe9ed56 100644 --- a/test/bases/renku_data_services/data_api/conftest.py +++ b/test/bases/renku_data_services/data_api/conftest.py @@ -55,7 +55,15 @@ def users(admin_user, regular_user, member_1_user, member_2_user) -> list[UserIn def admin_headers(admin_user: UserInfo) -> dict[str, str]: """Authentication headers for an admin user.""" access_token = json.dumps( - {"is_admin": True, "id": admin_user.id, "name": f"{admin_user.first_name} {admin_user.last_name}"} + { + "is_admin": True, + "id": admin_user.id, + "name": f"{admin_user.first_name} {admin_user.last_name}", + "first_name": admin_user.first_name, + "last_name": admin_user.last_name, + "email": admin_user.email, + "full_name": f"{admin_user.first_name} {admin_user.last_name}", + } ) return {"Authorization": f"Bearer {access_token}"} @@ -64,7 +72,15 @@ def admin_headers(admin_user: UserInfo) -> dict[str, str]: def user_headers(regular_user: UserInfo) -> dict[str, str]: """Authentication headers for a normal user.""" access_token = json.dumps( - {"is_admin": False, "id": regular_user.id, "name": f"{regular_user.first_name} {regular_user.last_name}"} + { + "is_admin": False, + "id": regular_user.id, + "name": f"{regular_user.first_name} {regular_user.last_name}", + "first_name": regular_user.first_name, + "last_name": regular_user.last_name, + "email": regular_user.email, + "full_name": f"{regular_user.first_name} {regular_user.last_name}", + } ) return {"Authorization": f"Bearer {access_token}"} diff --git a/test/bases/renku_data_services/data_api/test_groups.py b/test/bases/renku_data_services/data_api/test_groups.py index 6fde7d132..5eb6b7182 100644 --- a/test/bases/renku_data_services/data_api/test_groups.py +++ b/test/bases/renku_data_services/data_api/test_groups.py @@ -2,7 +2,10 @@ from datetime import datetime import pytest +from sanic_testing.testing import SanicASGITestClient +from renku_data_services.app_config.config import Config +from renku_data_services.authz.models import Role, Visibility from renku_data_services.message_queue.avro_models.io.renku.events.v2 import ( GroupAdded, GroupMemberAdded, @@ -17,7 +20,9 @@ @pytest.mark.asyncio -async def test_group_creation_basic(sanic_client, user_headers, app_config): +async def test_group_creation_basic( + sanic_client: SanicASGITestClient, user_headers: dict[str, str], app_config: Config +) -> None: payload = { "name": "Group1", "slug": "group-1", @@ -67,7 +72,9 @@ async def test_group_creation_basic(sanic_client, user_headers, app_config): @pytest.mark.asyncio -async def test_group_pagination(sanic_client, user_headers, admin_headers): +async def test_group_pagination( + sanic_client: SanicASGITestClient, user_headers: dict[str, str], admin_headers: dict[str, str] +) -> None: for i in range(5): payload = {"name": f"group{i}", "slug": f"group{i}"} _, response = await sanic_client.post("/api/data/groups", headers=admin_headers, json=payload) @@ -96,7 +103,9 @@ async def test_group_pagination(sanic_client, user_headers, admin_headers): @pytest.mark.asyncio -async def test_group_patch_delete(sanic_client, user_headers, app_config): +async def test_group_patch_delete( + sanic_client: SanicASGITestClient, user_headers: dict[str, str], app_config: Config +) -> None: payload = { "name": "GroupOther", "slug": "group-other", @@ -157,7 +166,9 @@ async def test_group_patch_delete(sanic_client, user_headers, app_config): @pytest.mark.asyncio -async def test_group_members(sanic_client, user_headers, app_config): +async def test_group_members( + sanic_client: SanicASGITestClient, user_headers: dict[str, str], app_config: Config +) -> None: payload = { "name": "Group1", "slug": "group-1", @@ -194,7 +205,12 @@ async def test_group_members(sanic_client, user_headers, app_config): @pytest.mark.asyncio -async def test_removing_single_group_owner_not_allowed(sanic_client, user_headers, member_1_headers, app_config): +async def test_removing_single_group_owner_not_allowed( + sanic_client: SanicASGITestClient, + user_headers: dict[str, str], + member_1_headers: dict[str, str], + app_config: Config, +) -> None: payload = { "name": "Group1", "slug": "group-1", @@ -256,8 +272,12 @@ async def test_removing_single_group_owner_not_allowed(sanic_client, user_header @pytest.mark.asyncio async def test_cannot_change_role_for_last_group_owner( - sanic_client, user_headers, regular_user, app_config, member_1_headers -): + sanic_client: SanicASGITestClient, + user_headers: dict[str, str], + regular_user: UserInfo, + app_config: Config, + member_1_headers: dict[str, str], +) -> None: payload = { "name": "Group1", "slug": "group-1", @@ -291,7 +311,9 @@ async def test_cannot_change_role_for_last_group_owner( @pytest.mark.asyncio -async def test_moving_project_across_groups(sanic_client, user_headers, regular_user: UserInfo): +async def test_moving_project_across_groups( + sanic_client: SanicASGITestClient, user_headers: dict[str, str], regular_user: UserInfo +) -> None: payload = { "name": "Group1", "slug": "group-1", @@ -321,7 +343,9 @@ async def test_moving_project_across_groups(sanic_client, user_headers, regular_ @pytest.mark.asyncio -async def test_removing_group_removes_projects(sanic_client, user_headers, regular_user: UserInfo): +async def test_removing_group_removes_projects( + sanic_client: SanicASGITestClient, user_headers: dict[str, str], regular_user: UserInfo +) -> None: payload = { "name": "Group1", "slug": "group-1", @@ -358,3 +382,46 @@ async def test_removing_group_removes_projects(sanic_client, user_headers, regul # The group should not exist _, response = await sanic_client.get("/api/data/groups/group-1", headers=user_headers) assert response.status_code == 404 + + +@pytest.mark.asyncio +async def test_group_members_get_project_access( + sanic_client: SanicASGITestClient, + user_headers: dict[str, str], + regular_user: UserInfo, + member_1_user: UserInfo, + member_1_headers: dict[str, str], +) -> None: + group_slug = "group-1" + payload = { + "name": "Group1", + "slug": group_slug, + "description": "Group 1 Description", + } + # Create a group + _, response = await sanic_client.post("/api/data/groups", headers=member_1_headers, json=payload) + assert response.status_code == 201, response.text + assert regular_user.email + # Create a project in the group + project1_payload = { + "name": "project-1", + "slug": "project-1", + "namespace": "group-1", + "visibility": Visibility.PRIVATE.value, + } + _, response = await sanic_client.post("/api/data/projects", headers=member_1_headers, json=project1_payload) + assert response.status_code == 201, response.text + project_id = response.json["id"] + # Other user cannot see the private project + _, response = await sanic_client.get(f"/api/data/projects/{project_id}", headers=user_headers) + assert response.status_code == 404 + # Add other user to group + _, response = await sanic_client.patch( + f"/api/data/groups/{group_slug}/members", + json=[{"id": regular_user.id, "role": Role.VIEWER.value}], + headers=member_1_headers, + ) + assert response.status_code == 200 + # Now other user can see the private project in the group + _, response = await sanic_client.get(f"/api/data/projects/{project_id}", headers=user_headers) + assert response.status_code == 200 diff --git a/test/bases/renku_data_services/data_api/test_storage.py b/test/bases/renku_data_services/data_api/test_storage.py index 86a82afb3..080104e6e 100644 --- a/test/bases/renku_data_services/data_api/test_storage.py +++ b/test/bases/renku_data_services/data_api/test_storage.py @@ -306,11 +306,11 @@ async def test_get_storage(storage_test_client, valid_storage_payload, admin_hea @pytest.mark.asyncio -async def test_get_storage_unauthorized(storage_test_client, valid_storage_payload) -> None: +async def test_get_storage_unauthorized(storage_test_client, valid_storage_payload, admin_headers) -> None: storage_test_client, gl_auth = storage_test_client _, res = await storage_test_client.post( "/api/data/storage", - headers={"Authorization": '{"is_admin": false}'}, + headers=admin_headers, data=json.dumps(valid_storage_payload), ) assert res.status_code == 201 @@ -326,11 +326,11 @@ async def test_get_storage_unauthorized(storage_test_client, valid_storage_paylo @pytest.mark.asyncio -async def test_storage_deletion(storage_test_client, valid_storage_payload) -> None: +async def test_storage_deletion(storage_test_client, valid_storage_payload, admin_headers) -> None: storage_test_client, _ = storage_test_client _, res = await storage_test_client.post( "/api/data/storage", - headers={"Authorization": '{"is_admin": false}'}, + headers=admin_headers, data=json.dumps(valid_storage_payload), ) assert res.status_code == 201 @@ -339,24 +339,24 @@ async def test_storage_deletion(storage_test_client, valid_storage_payload) -> N _, res = await storage_test_client.delete( f"/api/data/storage/{storage_id}", - headers={"Authorization": '{"is_admin": false}'}, + headers=admin_headers, ) assert res.status_code == 204 _, res = await storage_test_client.get( f"/api/data/storage/{storage_id}", - headers={"Authorization": '{"is_admin": false}'}, + headers=admin_headers, ) assert res.status_code == 404 @pytest.mark.asyncio -async def test_storage_deletion_unauthorized(storage_test_client, valid_storage_payload) -> None: +async def test_storage_deletion_unauthorized(storage_test_client, valid_storage_payload, admin_headers) -> None: storage_test_client, gl_auth = storage_test_client _, res = await storage_test_client.post( "/api/data/storage", - headers={"Authorization": '{"is_admin": false}'}, + headers=admin_headers, data=json.dumps(valid_storage_payload), ) assert res.status_code == 201 @@ -370,11 +370,11 @@ async def test_storage_deletion_unauthorized(storage_test_client, valid_storage_ @pytest.mark.asyncio -async def test_storage_put(storage_test_client, valid_storage_payload) -> None: +async def test_storage_put(storage_test_client, valid_storage_payload, admin_headers) -> None: storage_test_client, _ = storage_test_client _, res = await storage_test_client.post( "/api/data/storage", - headers={"Authorization": '{"is_admin": false}'}, + headers=admin_headers, data=json.dumps(valid_storage_payload), ) assert res.status_code == 201 @@ -383,7 +383,7 @@ async def test_storage_put(storage_test_client, valid_storage_payload) -> None: _, res = await storage_test_client.put( f"/api/data/storage/{storage_id}", - headers={"Authorization": '{"is_admin": false}'}, + headers=admin_headers, data=json.dumps( { "project_id": valid_storage_payload["project_id"], @@ -399,11 +399,11 @@ async def test_storage_put(storage_test_client, valid_storage_payload) -> None: @pytest.mark.asyncio -async def test_storage_put_unauthorized(storage_test_client, valid_storage_payload) -> None: +async def test_storage_put_unauthorized(storage_test_client, valid_storage_payload, admin_headers) -> None: storage_test_client, gl_auth = storage_test_client _, res = await storage_test_client.post( "/api/data/storage", - headers={"Authorization": '{"is_admin": false}'}, + headers=admin_headers, data=json.dumps(valid_storage_payload), ) assert res.status_code == 201 @@ -429,8 +429,8 @@ async def test_storage_put_unauthorized(storage_test_client, valid_storage_paylo async def test_storage_patch(storage_test_client, valid_storage_payload) -> None: storage_test_client, _ = storage_test_client # NOTE: The keycloak dummy client used to authorize the storage patch requests only has info - # on a user with name John Doe, using a different user will fail with a 401 error. - access_token = json.dumps({"is_admin": False, "id": "some-id", "name": "John Doe"}) + # on a user with name Admin Doe, using a different user will fail with a 401 error. + access_token = json.dumps({"is_admin": False, "id": "some-id", "full_name": "Admin Doe"}) _, res = await storage_test_client.post( "/api/data/storage", headers={"Authorization": f"bearer {access_token}"}, @@ -470,11 +470,11 @@ async def test_storage_patch(storage_test_client, valid_storage_payload) -> None @pytest.mark.asyncio -async def test_storage_patch_unauthorized(storage_test_client, valid_storage_payload) -> None: +async def test_storage_patch_unauthorized(storage_test_client, valid_storage_payload, admin_headers) -> None: storage_test_client, gl_auth = storage_test_client _, res = await storage_test_client.post( "/api/data/storage", - headers={"Authorization": '{"is_admin": false}'}, + headers=admin_headers, data=json.dumps(valid_storage_payload), ) assert res.status_code == 201 diff --git a/test/components/renku_data_services/authz/test_authorization.py b/test/components/renku_data_services/authz/test_authorization.py index 4450c4b17..656299767 100644 --- a/test/components/renku_data_services/authz/test_authorization.py +++ b/test/components/renku_data_services/authz/test_authorization.py @@ -52,7 +52,13 @@ async def test_adding_deleting_project(app_config: Config, bootstrap_admins, pub id=project_id, name=project_id, slug="slug", - namespace=Namespace("namespace", "namespace", NamespaceKind.user), + namespace=Namespace( + "namespace", + "namespace", + NamespaceKind.user, + created_by=project_owner.id, + underlying_resource_id=project_owner.id, + ), visibility=Visibility.PUBLIC if public_project else Visibility.PRIVATE, created_by=project_owner.id, ) @@ -96,7 +102,13 @@ async def test_granting_access(app_config: Config, bootstrap_admins, public_proj id=project_id, name=project_id, slug="slug", - namespace=Namespace("namespace", "namespace", NamespaceKind.user), + namespace=Namespace( + "namespace", + "namespace", + NamespaceKind.user, + created_by=project_owner.id, + underlying_resource_id=project_owner.id, + ), visibility=Visibility.PUBLIC if public_project else Visibility.PRIVATE, created_by=project_owner.id, ) @@ -135,7 +147,13 @@ async def test_listing_users_with_access(app_config: Config, public_project: boo id=project1_id, name=project1_id, slug=project1_id, - namespace=Namespace(project_owner.id, project_owner.id, NamespaceKind.user), + namespace=Namespace( + project_owner.id, + project_owner.id, + NamespaceKind.user, + created_by=project_owner.id, + underlying_resource_id=project_owner.id, + ), visibility=Visibility.PUBLIC if public_project else Visibility.PRIVATE, created_by=project_owner.id, ) @@ -144,7 +162,13 @@ async def test_listing_users_with_access(app_config: Config, public_project: boo id=project2_id, name=project2_id, slug=project2_id, - namespace=Namespace(regular_user2.id, regular_user2.id, NamespaceKind.user), + namespace=Namespace( + regular_user2.id, + regular_user2.id, + NamespaceKind.user, + created_by=regular_user2.id, + underlying_resource_id=regular_user2.id, + ), visibility=Visibility.PRIVATE, created_by=regular_user2.id, ) @@ -167,7 +191,13 @@ async def test_listing_projects_with_access(app_config: Config, bootstrap_admins private_project_id1 = str(ULID()) private_project_id2 = str(ULID()) project_owner = regular_user1 - namespace = Namespace(project_owner.id, project_owner.id, NamespaceKind.user) + namespace = Namespace( + project_owner.id, + project_owner.id, + NamespaceKind.user, + created_by=project_owner.id, + underlying_resource_id=project_owner.id, + ) assert project_owner.id assert regular_user2.id public_project = Project( diff --git a/test/components/renku_data_services/message_queue/test_queue.py b/test/components/renku_data_services/message_queue/test_queue.py index d4ca4caba..225910f18 100644 --- a/test/components/renku_data_services/message_queue/test_queue.py +++ b/test/components/renku_data_services/message_queue/test_queue.py @@ -28,7 +28,13 @@ async def fake_db_method(self, *, session, some_arg): id="sample-id-1", name="name", slug="slug", - namespace=Namespace("namespace", "namespace", NamespaceKind.user), + namespace=Namespace( + "namespace", + "namespace", + NamespaceKind.user, + created_by="some-user", + underlying_resource_id="some-user", + ), visibility=Visibility.PRIVATE, created_by="some-user", )