[shrish-browserstack]

Change

group_helper.update_owners() now applies the same is_allowed_admin_actions_on_group(group) check that the other group-administration helpers in this module already use, and returns the existing NON_OWNER_PERMISSION_DENIED_ERROR (which the view maps to a 400) when the requester is neither a group owner nor an admin. The check runs right after the group lookup, before the request body is read or any membership is touched.

No change for owners and admins: the update path is identical.

Tests

Two new tests in Access/tests/test_group_helper.py:

• a requester without admin rights on the group gets the permission error, and no membership is read or saved and no mail is sent;
• an owner's request still updates ownership and sends the notification.

Verified locally on Python 3.12 with CI's setup (config.json.sample, access modules cloned):

new tests against current main:  1 failed (non-owner test), 1 passed
new tests on this branch:        2 passed
full suite on this branch:       140 passed, 8 skipped

The full-suite run includes browserstack/enigma-access-modules#117, which fixes the unrelated SSH-module test currently failing enigma CI on main. Until #117 merges, this PR's CI will show that same pre-existing failure.

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Central YAML (base), Organization UI (inherited), Workspace UI (inherited); Enforced overrides: Organization
• Review profile: ASSERTIVE
• Plan: Enterprise
• Run ID: 9bb5e1e8-9d9f-4678-9833-0477b4321efc

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[shrish-browserstack]

Claude Code Review (automated) — 3 inline finding(s). Full report in the PR comment below. Verdict: Failed - see PR comment.

[shrish-browserstack]

[High] This check depends on member_is_owner(), which doesn't filter by membership status

GroupV2.member_is_owner() reads is_owner from membership_group.get(user=user) with no status filter, and .get() can raise MultipleObjectsReturned when a user has more than one row in the group.

Suggestion: tighten the shared helper in this PR: return self.membership_group.filter(user=user, status="Approved", is_owner=True).exists().

Reviewer: code-review

[shrish-browserstack]

Claude Code PR Review

PR: #352 • Head: 84ebe5a • Reviewers: code-review (built-in fallback; no in-scope reviewers installed in this repo)

Summary

Adds the existing owner-or-admin check (is_allowed_admin_actions_on_group) to group_helper.update_owners(), matching the sibling group-admin helpers, plus two unit tests.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	None introduced
High	Security	Authentication/authorization checks present	Fail	Check added, but the shared ownership helper it relies on does not filter memberships by status
High	Security	Input validation and sanitization	N/A	No new input handling
High	Security	No IDOR — resource ownership validated	Fail	Same root cause as above
High	Security	No SQL injection (parameterized queries)	Pass	ORM only
High	Correctness	Logic is correct, handles edge cases	Fail	member_is_owner() uses .get() and can raise MultipleObjectsReturned when a user has more than one membership row in a group
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Uses the module's existing error shape
High	Correctness	No race conditions or concurrency issues	Pass	Check runs before the existing transaction.atomic block
Medium	Testing	New code has corresponding tests	Pass	Allowed and rejected paths covered
Medium	Testing	Error paths and edge cases tested	Fail	The ownership helper itself is stubbed; its status and multi-row behaviour are untested
Medium	Testing	Existing tests still pass (no regressions)	Pass	139 passed in CI; the single failure is the pre-existing SSH access-module test fixed by browserstack/enigma-access-modules#117
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	One extra membership lookup
Medium	Performance	Long-running tasks use background jobs	N/A
Medium	Quality	Follows existing codebase patterns	Pass	Mirrors get_user_group / add_user_to_group
Medium	Quality	Changes are focused (single concern)	Pass
Low	Quality	Meaningful names, no dead code	Pass
Low	Quality	Comments explain why, not what	Pass
Low	Quality	No unnecessary dependencies added	Pass

Findings

• File: Access/models.py:627 (relied on by Access/group_helper.py:251)

• Severity: High

• Reviewer: code-review

• Issue: GroupV2.member_is_owner() reads is_owner from membership_group.get(user=user) without filtering on status, so the check this PR adds is only as strict as that helper.

• Suggestion: Tighten the shared helper in this PR: return self.membership_group.filter(user=user, status="Approved", is_owner=True).exists(). This benefits every caller of is_allowed_admin_actions_on_group.

• File: Access/models.py:629

• Severity: Medium

• Reviewer: code-review

• Issue: member_is_owner() catches only DoesNotExist. MembershipV2 has no unique (group, user) constraint and re-adding a member creates a new row, so .get() can raise MultipleObjectsReturned, which the view turns into a 400.

• Suggestion: The .filter(...).exists() form above also removes this.

• File: Access/tests/test_group_helper.py:486

• Severity: Medium

• Reviewer: code-review

• Issue: Both tests stub is_allowed_admin_actions_on_group, so the real ownership rule is never exercised, and the allowed-path test does not assert the check was called with the group.

• Suggestion: Add direct member_is_owner tests covering an approved owner, a non-approved membership, and a user with two rows; assert the call in test_update_owners_allows_owner.

• File: Access/group_helper.py:252

• Severity: Low

• Reviewer: code-review

• Issue: The denial is logged at DEBUG without requester or group, so it is not visible at production log levels.

• Suggestion: logger.warning("update_owners denied: user=%s group=%s", request.user.username, group.name).

• File: Access/views.py:279

• Severity: Low

• Reviewer: code-review

• Issue: Permission errors are returned as HTTP 400, same as malformed input (pre-existing view behaviour).

• Suggestion: Follow-up: use 403 for permission errors across group views.

---

Verdict: FAIL — right fix in the right place; tighten member_is_owner() (status filter, .exists()) and add direct tests for it in this PR.