Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions ami/users/api/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
UserProjectMembershipListSerializer,
UserProjectMembershipSerializer,
)
from ami.users.roles import Role
from ami.users.roles import BasicMember, Role
from ami.users.signals import manage_project_membership

logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -50,7 +50,6 @@ def get_serializer_context(self):

def perform_create(self, serializer):
project = self.get_active_project()
# user = serializer._validated_user
role_cls = serializer._validated_role_cls
with transaction.atomic():
membership = serializer.save(project=project)
Expand All @@ -60,12 +59,11 @@ def perform_create(self, serializer):
# The membership is already created above, so we don't need the signal to modify it
m2m_changed.disconnect(manage_project_membership, sender=Group.user_set.through)
try:
# unassign all existing roles for this project
# Unassign all roles, assign the chosen role, then BasicMember
for r in Role.__subclasses__():
r.unassign_user(user, project)

# assign new role
role_cls.assign_user(user, project)
BasicMember.assign_user(user, project)
finally:
# Reconnect signal
m2m_changed.connect(manage_project_membership, sender=Group.user_set.through)
Expand All @@ -86,10 +84,11 @@ def perform_update(self, serializer):
membership.user = user
membership.save()

# Unassign all roles, assign the chosen role, then BasicMember
for r in Role.__subclasses__():
r.unassign_user(user, project)

role_cls.assign_user(user, project)
BasicMember.assign_user(user, project)
finally:
# Reconnect signal
m2m_changed.connect(manage_project_membership, sender=Group.user_set.through)
Expand All @@ -103,7 +102,7 @@ def perform_destroy(self, instance):
m2m_changed.disconnect(manage_project_membership, sender=Group.user_set.through)
try:
with transaction.atomic():
# remove roles for this project
# Revoke all roles (including BasicMember) before deleting membership
for r in Role.__subclasses__():
r.unassign_user(user, project)

Expand Down
63 changes: 62 additions & 1 deletion ami/users/tests/test_membership_management_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
from ami.main.models import Project, UserProjectMembership
from ami.tests.fixtures.main import setup_test_project
from ami.users.models import User
from ami.users.roles import BasicMember, ProjectManager, Role
from ami.users.roles import BasicMember, Identifier, ProjectManager, Researcher, Role, create_roles_for_project


class TestUserProjectMembershipAPI(APITestCase):
Expand Down Expand Up @@ -243,3 +243,64 @@ def test_create_membership_missing_role_id(self):
payload = {"email": self.user2.email}
resp = self.client.post(self.members_url, payload, format="json")
self.assertEqual(resp.status_code, 400)


class TestMembersApiDraftProjectAccess(APITestCase):
"""
Verify that members added via the Members API can access draft project details.
Regression tests for the BasicMember manual-assign fix.
"""

def setUp(self):
self.project, _ = setup_test_project()
self.project.draft = True
self.project.save()
create_roles_for_project(self.project)

self.superuser = User.objects.create_superuser(email="[email protected]", password="x")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Static analysis flag S106 (hardcoded test password) — consistent with existing pattern

Ruff flags password="x" at this line. The identical credential is already present on line 16 in the same file (TestUserProjectMembershipAPI.setUp), so this is consistent. No production risk; flagging for awareness since the linter will surface it in CI.

🧰 Tools
🪛 Ruff (0.15.1)

[error] 260-260: Possible hardcoded password assigned to argument: "password"

(S106)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@ami/users/tests/test_membership_management_api.py` at line 260, Ruff flags
the hardcoded password literal used when creating the superuser
(User.objects.create_superuser(password="x")), so replace the literal with a
single shared test password constant (e.g., TEST_PASSWORD = "x") defined at the
top of the test module and use that constant in
TestUserProjectMembershipAPI.setUp and here, or alternatively add a local noqa
comment to silence S106 for this line; update usages to reference TEST_PASSWORD
and ensure create_superuser(...) uses that symbol instead of the raw string.

self.user_basic = User.objects.create_user(email="[email protected]")
self.user_identifier = User.objects.create_user(email="[email protected]")
self.user_researcher = User.objects.create_user(email="[email protected]")
self.user_project_manager = User.objects.create_user(email="[email protected]")
self.outsider = User.objects.create_user(email="[email protected]")

self.members_url = f"/api/v2/projects/{self.project.pk}/members/"
self.detail_url = f"/api/v2/projects/{self.project.pk}/"

def _add_member_and_assert_can_access_draft(self, user, role_id: str) -> None:
"""Add user as role via API, then assert they can GET draft project details."""
self.client.force_authenticate(self.superuser)
resp = self.client.post(
self.members_url,
{"email": user.email, "role_id": role_id},
format="json",
)
self.assertEqual(resp.status_code, 201, f"Failed to add {role_id}: {resp.json()}")
self.client.force_authenticate(user)
detail_resp = self.client.get(self.detail_url)
self.assertEqual(
detail_resp.status_code,
200,
f"{role_id} member should access draft project, got {detail_resp.status_code}",
)

def test_member_added_via_api_can_access_draft_project_basic_member(self):
self._add_member_and_assert_can_access_draft(self.user_basic, BasicMember.__name__)

def test_member_added_via_api_can_access_draft_project_identifier(self):
self._add_member_and_assert_can_access_draft(self.user_identifier, Identifier.__name__)

def test_member_added_via_api_can_access_draft_project_researcher(self):
self._add_member_and_assert_can_access_draft(self.user_researcher, Researcher.__name__)

def test_member_added_via_api_can_access_draft_project_manager(self):
self._add_member_and_assert_can_access_draft(self.user_project_manager, ProjectManager.__name__)

def test_non_member_cannot_access_draft_project(self):
self.client.force_authenticate(self.outsider)
detail_resp = self.client.get(self.detail_url)
self.assertIn(
detail_resp.status_code,
(403, 404),
"Non-member should not access draft project",
)