diff --git a/src/mavedb/lib/permissions/models.py b/src/mavedb/lib/permissions/models.py index 0145fc08..8f02535f 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 b61edead..8475322d 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 77861dc2..b3df6353 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"] == []