From 1c3a1991a17b2b696fab7fba0ad4e81ea73d41a2 Mon Sep 17 00:00:00 2001 From: Benjamin Capodanno Date: Fri, 7 Aug 2026 12:46:01 -0700 Subject: [PATCH] fix(collections): check .permitted so permission filters take effect `PermissionResponse` defined no `__bool__`, so the object was always truthy. `routers/collections.py` was the only module testing the response directly rather than reading `.permitted`, which left every permission check in it inert. - 18 association filters kept every member, disclosing the URNs of private score sets and experiments to any caller who could read the collection - 6 roster checks never took their narrowing branch, disclosing the full collection user roster, names and ORCID iDs included, to non-admins and to anonymous readers of public collections Raise `TypeError` from `PermissionResponse.__bool__` so a bare `if has_permission(...)` cannot be written again. Collapse the six duplicated roster blocks into `_narrow_user_roles_for_non_admins` and route `list_my_collections` through it as well. That endpoint held the only working version of the rule, gating on the caller's own collection role; sharing one implementation stops the two endpoints drifting apart again. The rule is `Action.ADD_ROLE`: whoever may add a user to a collection may see who is in it. Four existing tests asserted that editors and viewers see themselves in the roster, which was the bug's behaviour rather than the intended rule, and now expect the narrowed roster that `list_my_collections` has always returned. New tests cover both disclosures from the outside via response bodies, the roster rule parameterized over each contribution role, and the fact that the newly active filters must not delete the association rows they decline to show, since they assign to `delete-orphan` collections. --- src/mavedb/lib/permissions/models.py | 13 ++ src/mavedb/routers/collections.py | 180 +++++++++++-------------- tests/routers/test_collections.py | 195 ++++++++++++++++++++++----- 3 files changed, 254 insertions(+), 134 deletions(-) diff --git a/src/mavedb/lib/permissions/models.py b/src/mavedb/lib/permissions/models.py index 0145fc085..8f02535f0 100644 --- a/src/mavedb/lib/permissions/models.py +++ b/src/mavedb/lib/permissions/models.py @@ -7,6 +7,19 @@ class PermissionResponse: + """The outcome of a permission check. + + Deliberately not truthy. Callers must read `.permitted`; evaluating the response + object itself in a boolean context silently resolves any permission check to True. + `__bool__` raises to ensure this contract is enforced. + """ + + def __bool__(self) -> bool: + raise TypeError( + "PermissionResponse is not truthy; check `.permitted` instead. " + "A bare `if has_permission(...)` is always true and silently permits everything." + ) + def __init__(self, permitted: bool, http_code: int = 403, message: Optional[str] = None): self.permitted = permitted self.http_code = http_code if not permitted else None diff --git a/src/mavedb/routers/collections.py b/src/mavedb/routers/collections.py index b61edead1..8475322d7 100644 --- a/src/mavedb/routers/collections.py +++ b/src/mavedb/routers/collections.py @@ -53,6 +53,27 @@ } +def _narrow_user_roles_for_non_admins(item: Collection, user_data: Optional[UserData]) -> None: + """Reduce a collection's visible user roster to admins only, for callers who cannot add users. + + The rule is `Action.ADD_ROLE`: whoever may add a user to a collection may see who is in it. + Everyone else sees the admin list alone, so contributors know who to contact without the + full membership being disclosed to them. + """ + if has_permission(user_data, item, Action.ADD_ROLE).permitted: + return + + admins = [] + for user_assoc in item.user_associations: + if user_assoc.contribution_role == ContributionRole.admin: + admin = user_assoc.user + # role must be set in order to assign users to collection + setattr(admin, "role", ContributionRole.admin) + admins.append(admin) + + item.users = admins + + @router.get( "/users/me/collections", status_code=200, @@ -89,25 +110,14 @@ def list_my_collections( item.score_set_associations = [ assoc for assoc in item.score_set_associations - if has_permission(user_data, assoc.score_set, Action.READ) + if has_permission(user_data, assoc.score_set, Action.READ).permitted ] item.experiment_associations = [ assoc for assoc in item.experiment_associations - if has_permission(user_data, assoc.experiment, Action.READ) + if has_permission(user_data, assoc.experiment, Action.READ).permitted ] - # unless user is admin of this collection, filter users to only admins - # the rationale is that all collection contributors should be able to see admins - # to know who to contact, but only collection admins should be able to see viewers and editors - if role in (ContributionRole.viewer, ContributionRole.editor): - admins = [] - for user_assoc in item.user_associations: - if user_assoc.contribution_role == ContributionRole.admin: - admin = user_assoc.user - # role must be set in order to assign users to collection - setattr(admin, "role", ContributionRole.admin) - admins.append(admin) - item.users = admins + _narrow_user_roles_for_non_admins(item, user_data) return collection_bundle @@ -140,24 +150,17 @@ def fetch_collection( # filter score set and experiment associations based on user permissions # work with associations directly to preserve position ordering item.score_set_associations = [ - assoc for assoc in item.score_set_associations if has_permission(user_data, assoc.score_set, Action.READ) + assoc + for assoc in item.score_set_associations + if has_permission(user_data, assoc.score_set, Action.READ).permitted ] item.experiment_associations = [ - assoc for assoc in item.experiment_associations if has_permission(user_data, assoc.experiment, Action.READ) + assoc + for assoc in item.experiment_associations + if has_permission(user_data, assoc.experiment, Action.READ).permitted ] - # Only collection admins can see all user roles for the collection. Other users can only see the list of admins. - # We could create a new permission action for this. But for now, assume that any user who has the ADD_ROLE - # permission is a collection admin and should be able to see all user roles for the collection. - if not has_permission(user_data, item, Action.ADD_ROLE): - admins = [] - for user_assoc in item.user_associations: - if user_assoc.contribution_role == ContributionRole.admin: - admin = user_assoc.user - # role must be set in order to assign users to collection - setattr(admin, "role", ContributionRole.admin) - admins.append(admin) - item.users = admins + _narrow_user_roles_for_non_admins(item, user_data) return item @@ -399,24 +402,17 @@ async def update_collection( # note that this filtering occurs after saving changes to db; the filtering is only for the returned view model # work with associations directly to preserve position ordering item.score_set_associations = [ - assoc for assoc in item.score_set_associations if has_permission(user_data, assoc.score_set, Action.READ) + assoc + for assoc in item.score_set_associations + if has_permission(user_data, assoc.score_set, Action.READ).permitted ] item.experiment_associations = [ - assoc for assoc in item.experiment_associations if has_permission(user_data, assoc.experiment, Action.READ) + assoc + for assoc in item.experiment_associations + if has_permission(user_data, assoc.experiment, Action.READ).permitted ] - # Only collection admins can see all user roles for the collection. Other users can only see the list of admins. - # We could create a new permission action for this. But for now, assume that any user who has the ADD_ROLE - # permission is a collection admin and should be able to see all user roles for the collection. - if not has_permission(user_data, item, Action.ADD_ROLE): - admins = [] - for user_assoc in item.user_associations: - if user_assoc.contribution_role == ContributionRole.admin: - admin = user_assoc.user - # role must be set in order to assign users to collection - setattr(admin, "role", ContributionRole.admin) - admins.append(admin) - item.users = admins + _narrow_user_roles_for_non_admins(item, user_data) return item @@ -482,24 +478,17 @@ async def add_score_set_to_collection( # note that this filtering occurs after saving changes to db; the filtering is only for the returned view model # work with associations directly to preserve position ordering item.score_set_associations = [ - assoc for assoc in item.score_set_associations if has_permission(user_data, assoc.score_set, Action.READ) + assoc + for assoc in item.score_set_associations + if has_permission(user_data, assoc.score_set, Action.READ).permitted ] item.experiment_associations = [ - assoc for assoc in item.experiment_associations if has_permission(user_data, assoc.experiment, Action.READ) + assoc + for assoc in item.experiment_associations + if has_permission(user_data, assoc.experiment, Action.READ).permitted ] - # Only collection admins can see all user roles for the collection. Other users can only see the list of admins. - # We could create a new permission action for this. But for now, assume that any user who has the ADD_ROLE - # permission is a collection admin and should be able to see all user roles for the collection. - if not has_permission(user_data, item, Action.ADD_ROLE): - admins = [] - for user_assoc in item.user_associations: - if user_assoc.contribution_role == ContributionRole.admin: - admin = user_assoc.user - # role must be set in order to assign users to collection - setattr(admin, "role", ContributionRole.admin) - admins.append(admin) - item.users = admins + _narrow_user_roles_for_non_admins(item, user_data) return item @@ -574,24 +563,17 @@ async def delete_score_set_from_collection( # note that this filtering occurs after saving changes to db; the filtering is only for the returned view model # work with associations directly to preserve position ordering item.score_set_associations = [ - assoc for assoc in item.score_set_associations if has_permission(user_data, assoc.score_set, Action.READ) + assoc + for assoc in item.score_set_associations + if has_permission(user_data, assoc.score_set, Action.READ).permitted ] item.experiment_associations = [ - assoc for assoc in item.experiment_associations if has_permission(user_data, assoc.experiment, Action.READ) + assoc + for assoc in item.experiment_associations + if has_permission(user_data, assoc.experiment, Action.READ).permitted ] - # Only collection admins can see all user roles for the collection. Other users can only see the list of admins. - # We could create a new permission action for this. But for now, assume that any user who has the ADD_ROLE - # permission is a collection admin and should be able to see all user roles for the collection. - if not has_permission(user_data, item, Action.ADD_ROLE): - admins = [] - for user_assoc in item.user_associations: - if user_assoc.contribution_role == ContributionRole.admin: - admin = user_assoc.user - # role must be set in order to assign users to collection - setattr(admin, "role", ContributionRole.admin) - admins.append(admin) - item.users = admins + _narrow_user_roles_for_non_admins(item, user_data) return item @@ -650,24 +632,17 @@ async def add_experiment_to_collection( # note that this filtering occurs after saving changes to db; the filtering is only for the returned view model # work with associations directly to preserve position ordering item.score_set_associations = [ - assoc for assoc in item.score_set_associations if has_permission(user_data, assoc.score_set, Action.READ) + assoc + for assoc in item.score_set_associations + if has_permission(user_data, assoc.score_set, Action.READ).permitted ] item.experiment_associations = [ - assoc for assoc in item.experiment_associations if has_permission(user_data, assoc.experiment, Action.READ) + assoc + for assoc in item.experiment_associations + if has_permission(user_data, assoc.experiment, Action.READ).permitted ] - # Only collection admins can see all user roles for the collection. Other users can only see the list of admins. - # We could create a new permission action for this. But for now, assume that any user who has the ADD_ROLE - # permission is a collection admin and should be able to see all user roles for the collection. - if not has_permission(user_data, item, Action.ADD_ROLE): - admins = [] - for user_assoc in item.user_associations: - if user_assoc.contribution_role == ContributionRole.admin: - admin = user_assoc.user - # role must be set in order to assign users to collection - setattr(admin, "role", ContributionRole.admin) - admins.append(admin) - item.users = admins + _narrow_user_roles_for_non_admins(item, user_data) return item @@ -738,24 +713,17 @@ async def delete_experiment_from_collection( # note that this filtering occurs after saving changes to db; the filtering is only for the returned view model # work with associations directly to preserve position ordering item.score_set_associations = [ - assoc for assoc in item.score_set_associations if has_permission(user_data, assoc.score_set, Action.READ) + assoc + for assoc in item.score_set_associations + if has_permission(user_data, assoc.score_set, Action.READ).permitted ] item.experiment_associations = [ - assoc for assoc in item.experiment_associations if has_permission(user_data, assoc.experiment, Action.READ) + assoc + for assoc in item.experiment_associations + if has_permission(user_data, assoc.experiment, Action.READ).permitted ] - # Only collection admins can see all user roles for the collection. Other users can only see the list of admins. - # We could create a new permission action for this. But for now, assume that any user who has the ADD_ROLE - # permission is a collection admin and should be able to see all user roles for the collection. - if not has_permission(user_data, item, Action.ADD_ROLE): - admins = [] - for user_assoc in item.user_associations: - if user_assoc.contribution_role == ContributionRole.admin: - admin = user_assoc.user - # role must be set in order to assign users to collection - setattr(admin, "role", ContributionRole.admin) - admins.append(admin) - item.users = admins + _narrow_user_roles_for_non_admins(item, user_data) return item @@ -838,10 +806,14 @@ async def add_user_to_collection_role( # note that this filtering occurs after saving changes to db; the filtering is only for the returned view model # work with associations directly to preserve position ordering item.score_set_associations = [ - assoc for assoc in item.score_set_associations if has_permission(user_data, assoc.score_set, Action.READ) + assoc + for assoc in item.score_set_associations + if has_permission(user_data, assoc.score_set, Action.READ).permitted ] item.experiment_associations = [ - assoc for assoc in item.experiment_associations if has_permission(user_data, assoc.experiment, Action.READ) + assoc + for assoc in item.experiment_associations + if has_permission(user_data, assoc.experiment, Action.READ).permitted ] # Only collection admins can get to this point in the function, so here we don't need to filter the list of user @@ -926,10 +898,14 @@ async def remove_user_from_collection_role( # note that this filtering occurs after saving changes to db; the filtering is only for the returned view model # work with associations directly to preserve position ordering item.score_set_associations = [ - assoc for assoc in item.score_set_associations if has_permission(user_data, assoc.score_set, Action.READ) + assoc + for assoc in item.score_set_associations + if has_permission(user_data, assoc.score_set, Action.READ).permitted ] item.experiment_associations = [ - assoc for assoc in item.experiment_associations if has_permission(user_data, assoc.experiment, Action.READ) + assoc + for assoc in item.experiment_associations + if has_permission(user_data, assoc.experiment, Action.READ).permitted ] # Only collection admins can get to this point in the function, so here we don't need to filter the list of user diff --git a/tests/routers/test_collections.py b/tests/routers/test_collections.py index 77861dc23..b3df63538 100644 --- a/tests/routers/test_collections.py +++ b/tests/routers/test_collections.py @@ -12,12 +12,15 @@ fastapi = pytest.importorskip("fastapi") from mavedb.lib.validation.urn_re import MAVEDB_COLLECTION_URN_RE +from mavedb.models.collection_experiment_association import CollectionExperimentAssociation +from mavedb.models.collection_score_set_association import CollectionScoreSetAssociation from mavedb.models.enums.contribution_role import ContributionRole from mavedb.view_models.collection import Collection from tests.helpers.constants import ( EXTRA_USER, TEST_COLLECTION, TEST_COLLECTION_RESPONSE, + ADMIN_USER, TEST_USER, ) from tests.helpers.dependency_overrider import DependencyOverrider @@ -143,18 +146,12 @@ def test_editor_can_read_private_collection(session, client, setup_router_db, ex assert response.status_code == 200 response_data = response.json() + # Only callers who may add users see the full roster, so EXTRA_USER does not appear + # in "editors" here -- TEST_COLLECTION_RESPONSE's empty default is correct. expected_response = deepcopy(TEST_COLLECTION_RESPONSE) expected_response.update( { "urn": response_data["urn"], - "editors": [ - { - "recordType": "User", - "firstName": EXTRA_USER["first_name"], - "lastName": EXTRA_USER["last_name"], - "orcidId": EXTRA_USER["username"], - } - ], } ) assert sorted(expected_response.keys()) == sorted(response_data.keys()) @@ -171,18 +168,12 @@ def test_viewer_can_read_private_collection(session, client, setup_router_db, ex assert response.status_code == 200 response_data = response.json() + # Only callers who may add users see the full roster, so EXTRA_USER does not appear + # in "viewers" here -- TEST_COLLECTION_RESPONSE's empty default is correct. expected_response = deepcopy(TEST_COLLECTION_RESPONSE) expected_response.update( { "urn": response_data["urn"], - "viewers": [ - { - "recordType": "User", - "firstName": EXTRA_USER["first_name"], - "lastName": EXTRA_USER["last_name"], - "orcidId": EXTRA_USER["username"], - } - ], } ) assert sorted(expected_response.keys()) == sorted(response_data.keys()) @@ -307,6 +298,8 @@ def test_editor_can_add_experiment_to_collection( assert response.status_code == 200 response_data = response.json() + # Only callers who may add users see the full roster, so EXTRA_USER does not appear + # in "editors" here -- TEST_COLLECTION_RESPONSE's empty default is correct. expected_response = deepcopy(TEST_COLLECTION_RESPONSE) expected_response.update( { @@ -319,14 +312,6 @@ def test_editor_can_add_experiment_to_collection( "lastName": EXTRA_USER["last_name"], "orcidId": EXTRA_USER["username"], }, - "editors": [ - { - "recordType": "User", - "firstName": EXTRA_USER["first_name"], - "lastName": EXTRA_USER["last_name"], - "orcidId": EXTRA_USER["username"], - } - ], "experimentUrns": [score_set["experiment"]["urn"]], } ) @@ -492,6 +477,8 @@ def test_editor_can_add_score_set_to_collection( assert response.status_code == 200 response_data = response.json() + # Only callers who may add users see the full roster, so EXTRA_USER does not appear + # in "editors" here -- TEST_COLLECTION_RESPONSE's empty default is correct. expected_response = deepcopy(TEST_COLLECTION_RESPONSE) expected_response.update( { @@ -504,14 +491,6 @@ def test_editor_can_add_score_set_to_collection( "lastName": EXTRA_USER["last_name"], "orcidId": EXTRA_USER["username"], }, - "editors": [ - { - "recordType": "User", - "firstName": EXTRA_USER["first_name"], - "lastName": EXTRA_USER["last_name"], - "orcidId": EXTRA_USER["username"], - } - ], "scoreSetUrns": [score_set["urn"]], } ) @@ -985,3 +964,155 @@ def test_viewer_cannot_add_via_patch( ) assert response.status_code == 403 + + +# Regression tests for collection membership disclosure. +# +# `PermissionResponse` has no `__bool__`, so `if has_permission(...)` was always true and every +# association filter and roster check in this router was inert. These tests exercise the paths +# from the outside, via response bodies, so they stay valid regardless of how filtering is +# implemented. + + +def test_public_collection_does_not_leak_private_member_score_set( + session, client, setup_router_db, anonymous_app_overrides +): + experiment = create_experiment(client) + private_score_set = create_seq_score_set(client, experiment["urn"]) + collection = create_collection(client, update={"private": False}) + + response = client.post( + f"/api/v1/collections/{collection['urn']}/score-sets", + json={"score_set_urn": private_score_set["urn"]}, + ) + assert response.status_code == 200 + + with DependencyOverrider(anonymous_app_overrides): + response = client.get(f"/api/v1/collections/{collection['urn']}") + + assert response.status_code == 200 + assert private_score_set["urn"] not in response.text + + +def test_public_collection_does_not_leak_private_member_experiment( + session, client, setup_router_db, anonymous_app_overrides +): + private_experiment = create_experiment(client) + collection = create_collection(client, update={"private": False}) + + response = client.post( + f"/api/v1/collections/{collection['urn']}/experiments", + json={"experiment_urn": private_experiment["urn"]}, + ) + assert response.status_code == 200 + + with DependencyOverrider(anonymous_app_overrides): + response = client.get(f"/api/v1/collections/{collection['urn']}") + + assert response.status_code == 200 + assert private_experiment["urn"] not in response.text + + +def test_collection_owner_still_sees_own_private_member_score_set(session, client, setup_router_db): + """Guards against over-filtering: entitled callers must still see their own private members.""" + experiment = create_experiment(client) + private_score_set = create_seq_score_set(client, experiment["urn"]) + collection = create_collection(client, update={"private": False}) + + client.post( + f"/api/v1/collections/{collection['urn']}/score-sets", + json={"score_set_urn": private_score_set["urn"]}, + ) + response = client.get(f"/api/v1/collections/{collection['urn']}") + + assert response.status_code == 200 + assert private_score_set["urn"] in response.json()["scoreSetUrns"] + + +def test_non_member_does_not_see_collection_viewers_or_editors( + session, client, setup_router_db, anonymous_app_overrides +): + collection = create_collection(client, update={"private": False}) + assert ( + client.post( + f"/api/v1/collections/{collection['urn']}/viewers", + json={"orcid_id": EXTRA_USER["username"]}, + ).status_code + == 200 + ) + + with DependencyOverrider(anonymous_app_overrides): + response = client.get(f"/api/v1/collections/{collection['urn']}") + + assert response.status_code == 200 + body = response.json() + assert body["viewers"] == [] + assert body["editors"] == [] + # Admins remain visible so contributors can identify who to contact. + assert [admin["orcidId"] for admin in body["admins"]] == [TEST_USER["username"]] + + +def test_narrowing_associations_does_not_delete_them(session, client, setup_router_db, anonymous_app_overrides): + """The router assigns filtered lists to `delete-orphan` collections. + + Nothing commits after that assignment today, so the staged orphan deletes are never + persisted. This pins that invariant: a non-member read must not destroy the very + association rows it declines to show. + """ + experiment = create_experiment(client) + private_score_set = create_seq_score_set(client, experiment["urn"]) + collection = create_collection(client, update={"private": False}) + client.post( + f"/api/v1/collections/{collection['urn']}/score-sets", + json={"score_set_urn": private_score_set["urn"]}, + ) + + score_set_assocs_before = session.query(CollectionScoreSetAssociation).count() + experiment_assocs_before = session.query(CollectionExperimentAssociation).count() + assert score_set_assocs_before == 1 + + with DependencyOverrider(anonymous_app_overrides): + assert client.get(f"/api/v1/collections/{collection['urn']}").status_code == 200 + + session.expire_all() + assert session.query(CollectionScoreSetAssociation).count() == score_set_assocs_before + assert session.query(CollectionExperimentAssociation).count() == experiment_assocs_before + + +@pytest.mark.parametrize("role", ContributionRole._member_names_) +def test_only_users_who_can_add_users_see_the_full_roster( + role, session, client, setup_router_db, extra_user_app_overrides +): + """Whoever may add a user to a collection may see who is in it; everyone else sees admins only. + + The counterpart to `test_non_member_does_not_see_collection_viewers_or_editors`, which covers + outsiders. This covers members, and fails if the roster is opened up to editors and viewers + on the strength of their being members at all. + """ + collection = create_collection(client, update={"private": False}) + + # TEST_USER is the creator and therefore an admin. Seat EXTRA_USER in the role under test, and + # a third user as an editor, so there is a non-admin entry only admins should be able to see. + for orcid, seat in ((EXTRA_USER["username"], f"{role}s"), (ADMIN_USER["username"], "editors")): + assert ( + client.post( + f"/api/v1/collections/{collection['urn']}/{seat}", + json={"orcid_id": orcid}, + ).status_code + == 200 + ) + + with DependencyOverrider(extra_user_app_overrides): + response = client.get(f"/api/v1/collections/{collection['urn']}") + + assert response.status_code == 200 + body = response.json() + orcids = {key: [user["orcidId"] for user in body[key]] for key in ("admins", "editors", "viewers")} + + if role == ContributionRole.admin.name: + assert EXTRA_USER["username"] in orcids["admins"] + assert ADMIN_USER["username"] in orcids["editors"] + else: + assert orcids["admins"] == [TEST_USER["username"]] + assert orcids["editors"] == [] + assert orcids["viewers"] == []