From 05b7617d03adb66cca78e83c2d609259e1b418c7 Mon Sep 17 00:00:00 2001 From: amandazhu Date: Wed, 12 Nov 2025 11:53:40 +1100 Subject: [PATCH] fix: revoke platform access properly --- db/models.py | 40 +++++++++++++++++++++++++++++- routers/admin.py | 23 +++++++++++------ tests/db/test_models.py | 55 +++++++++++++++++++++++++++++++++++++++++ tests/test_admin.py | 20 ++++++++++++++- 4 files changed, 128 insertions(+), 10 deletions(-) diff --git a/db/models.py b/db/models.py index e8044599..20813e60 100644 --- a/db/models.py +++ b/db/models.py @@ -389,7 +389,7 @@ def save_history(self, session: Session, commit: bool = False) -> "PlatformMembe history = PlatformMembershipHistory( platform_id=self.platform_id, - user=self.user, + user_id=self.user_id, approval_status=self.approval_status, updated_at=self.updated_at, updated_by=self.updated_by, @@ -400,6 +400,44 @@ def save_history(self, session: Session, commit: bool = False) -> "PlatformMembe session.commit() return history + def revoke_auth0_role(self, auth0_client: Auth0Client) -> bool: + """ + Remove the Auth0 role associated with this platform membership. + """ + if self.approval_status != ApprovalStatusEnum.APPROVED: + return False + platform_value = self.platform_id.value if isinstance(self.platform_id, PlatformEnum) else self.platform_id + role_name = f"biocommons/platform/{platform_value}" + role = auth0_client.get_role_by_name(role_name) + auth0_client.remove_roles_from_user(user_id=self.user_id, role_id=role.id) + return True + + def revoke( + self, + *, + auth0_client: Auth0Client, + reason: str | None, + updated_by: Optional["BiocommonsUser"], + session: Session, + commit: bool = True, + ) -> bool: + """ + Revoke the membership by removing the Auth0 role (if present) and + persisting the revoked status in the database. + + :return: True when an Auth0 role removal call was performed. + """ + role_revoked = self.revoke_auth0_role(auth0_client) + self.approval_status = ApprovalStatusEnum.REVOKED + self.revocation_reason = reason + self.updated_at = datetime.now(timezone.utc) + self.updated_by = updated_by + session.add(self) + self.save_history(session, commit=False) + if commit: + session.commit() + return role_revoked + def get_data(self) -> PlatformMembershipData: """ Get a data model for this membership, suitable for returning to the frontend. diff --git a/routers/admin.py b/routers/admin.py index 43def782..7c3a8bad 100644 --- a/routers/admin.py +++ b/routers/admin.py @@ -156,21 +156,26 @@ def _revoke_platform_membership( reason: str | None, admin_record: BiocommonsUser, db_session: Session, + client: Auth0Client, ) -> None: membership = PlatformMembership.get_by_user_id_and_platform_id_or_404( user_id=user_id, platform_id=platform, session=db_session, ) - membership.approval_status = ApprovalStatusEnum.REVOKED - membership.revocation_reason = reason - membership.updated_at = datetime.now(timezone.utc) - membership.updated_by = admin_record - db_session.add(membership) - membership.save_history(db_session) - db_session.commit() + role_revoked = membership.revoke( + auth0_client=client, + reason=reason, + updated_by=admin_record, + session=db_session, + ) db_session.refresh(membership) - logger.info("Revoked platform %s for user %s", platform.value, user_id) + logger.info( + "Revoked platform %s for user %s%s", + platform.value, + user_id, + "" if role_revoked else " (no Auth0 role assigned)", + ) def _approve_group_membership( @@ -592,6 +597,7 @@ def approve_platform_membership(user_id: Annotated[str, UserIdParam], def revoke_platform_membership(user_id: Annotated[str, UserIdParam], platform_id: Annotated[str, ServiceIdParam], payload: RevokeServiceRequest, + client: Annotated[Auth0Client, Depends(get_auth0_client)], admin_record: Annotated[BiocommonsUser, Depends(get_db_user)], db_session: Annotated[Session, Depends(get_db_session)]): platform_record = Platform.get_by_id_or_404(platform_id, db_session) @@ -601,6 +607,7 @@ def revoke_platform_membership(user_id: Annotated[str, UserIdParam], reason=payload.reason, admin_record=admin_record, db_session=db_session, + client=client, ) return _membership_response() diff --git a/tests/db/test_models.py b/tests/db/test_models.py index 2c43a371..695e579a 100644 --- a/tests/db/test_models.py +++ b/tests/db/test_models.py @@ -436,6 +436,61 @@ def test_platform_membership_save_history_stores_reason(test_db_session, persist assert history.reason == "Policy violation" +def test_platform_membership_revoke_auth0_role(mock_auth0_client, persistent_factories): + membership = PlatformMembershipFactory.create_sync( + platform_id=PlatformEnum.GALAXY, + approval_status=ApprovalStatusEnum.APPROVED.value, + ) + role = RoleDataFactory.build(name="biocommons/platform/galaxy") + mock_auth0_client.get_role_by_name.return_value = role + + assert membership.revoke_auth0_role(mock_auth0_client) is True + mock_auth0_client.get_role_by_name.assert_called_once_with("biocommons/platform/galaxy") + mock_auth0_client.remove_roles_from_user.assert_called_once_with( + user_id=membership.user_id, + role_id=role.id, + ) + + +@pytest.mark.parametrize("status", [ApprovalStatusEnum.PENDING, ApprovalStatusEnum.REVOKED]) +def test_platform_membership_revoke_auth0_role_not_approved(status, mock_auth0_client, persistent_factories): + membership = PlatformMembershipFactory.create_sync( + platform_id=PlatformEnum.GALAXY, + approval_status=status, + ) + + assert membership.revoke_auth0_role(mock_auth0_client) is False + mock_auth0_client.get_role_by_name.assert_not_called() + mock_auth0_client.remove_roles_from_user.assert_not_called() + + +def test_platform_membership_revoke_updates_state(test_db_session, mock_auth0_client, persistent_factories): + admin = BiocommonsUserFactory.create_sync() + membership = PlatformMembershipFactory.create_sync( + platform_id=PlatformEnum.GALAXY, + approval_status=ApprovalStatusEnum.APPROVED.value, + ) + role = RoleDataFactory.build(name="biocommons/platform/galaxy") + mock_auth0_client.get_role_by_name.return_value = role + + result = membership.revoke( + auth0_client=mock_auth0_client, + reason="No longer required", + updated_by=admin, + session=test_db_session, + ) + + assert result is True + mock_auth0_client.remove_roles_from_user.assert_called_once_with( + user_id=membership.user_id, + role_id=role.id, + ) + test_db_session.refresh(membership) + assert membership.approval_status == ApprovalStatusEnum.REVOKED + assert membership.revocation_reason == "No longer required" + assert membership.updated_by_id == admin.id + + def test_create_group_membership_no_updater(test_db_session, persistent_factories): """ Test creating a group membership without an updated_by (for automatic approvals) diff --git a/tests/test_admin.py b/tests/test_admin.py index 75ebff04..faf8f630 100644 --- a/tests/test_admin.py +++ b/tests/test_admin.py @@ -50,10 +50,13 @@ def galaxy_platform(persistent_factories): Set up a Galaxy platform with the admin role set to the Galaxy admin scope. """ admin_role = Auth0RoleFactory.create_sync(name="biocommons/role/galaxy/admin") + platform_role = Auth0RoleFactory.create_sync(name="biocommons/platform/galaxy") return PlatformFactory.create_sync( id=PlatformEnum.GALAXY, name="Galaxy Australia", admin_roles=[admin_role], + role_id=platform_role.id, + platform_role=platform_role, ) @@ -72,10 +75,13 @@ def bpa_platform(persistent_factories): """ Set up a BPA platform in the DB (no admin roles configured) """ + platform_role = Auth0RoleFactory.create_sync(name="biocommons/platform/bpa_data_portal") return PlatformFactory.create_sync( id=PlatformEnum.BPA_DATA_PORTAL, name="BPA Data Portal", - admin_roles=[] + admin_roles=[], + role_id=platform_role.id, + platform_role=platform_role, ) @@ -602,6 +608,7 @@ def test_revoke_platform_membership_records_reason( admin_user, galaxy_platform, persistent_factories, + mock_auth0_client, ): user = BiocommonsUserFactory.create_sync(platform_memberships=[]) membership = PlatformMembershipFactory.create_sync( @@ -616,6 +623,9 @@ def test_revoke_platform_membership_records_reason( ) test_db_session.commit() + mock_role = RoleDataFactory.build(name="biocommons/platform/galaxy") + mock_auth0_client.get_role_by_name.return_value = mock_role + reason = " No longer meets access requirements " resp = test_client.post( f"/admin/users/{user.id}/platforms/galaxy/revoke", @@ -640,6 +650,11 @@ def test_revoke_platform_membership_records_reason( ).all() assert history_entries[-1].approval_status == ApprovalStatusEnum.REVOKED assert history_entries[-1].reason == reason.strip() + mock_auth0_client.get_role_by_name.assert_called_once_with("biocommons/platform/galaxy") + mock_auth0_client.remove_roles_from_user.assert_called_once_with( + user_id=user.id, + role_id=mock_role.id, + ) def test_revoke_platform_membership_forbidden_without_platform_role( @@ -647,6 +662,7 @@ def test_revoke_platform_membership_forbidden_without_platform_role( test_db_session, galaxy_platform, persistent_factories, + mock_auth0_client, ): user = BiocommonsUserFactory.create_sync(platform_memberships=[]) membership = PlatformMembershipFactory.create_sync( @@ -682,6 +698,8 @@ def test_revoke_platform_membership_forbidden_without_platform_role( test_db_session.refresh(membership) assert membership.approval_status == original_status + mock_auth0_client.get_role_by_name.assert_not_called() + mock_auth0_client.remove_roles_from_user.assert_not_called() assert membership.revocation_reason == original_reason