Skip to content

[codex] enforce invitation role hierarchy#673

Merged
PierreLeGuen merged 4 commits into
mainfrom
codex/enforce-invitation-role-hierarchy
May 27, 2026
Merged

[codex] enforce invitation role hierarchy#673
PierreLeGuen merged 4 commits into
mainfrom
codex/enforce-invitation-role-hierarchy

Conversation

@PierreLeGuen
Copy link
Copy Markdown
Contributor

Summary

Fixes #438.

This enforces the organization role hierarchy when creating email invitations. The backend now resolves the requester's effective organization role once and rejects invitation roles above that role before writing invitation records.

Root Cause

create_invitations_impl only checked whether a requester could manage members. Since admins satisfy that check, an admin could invite another user as owner by calling the API directly, bypassing the frontend role dropdown filtering.

Impact

Owners can invite owners, admins, and members. Admins can invite admins and members. Members remain unable to invite.

Validation

  • cargo fmt
  • cargo test -p services create_invitations
  • git diff --check

@PierreLeGuen PierreLeGuen marked this pull request as ready for review May 26, 2026 11:51
@PierreLeGuen PierreLeGuen requested a review from Evrard-Nil May 26, 2026 11:51
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the organization invitation logic to enforce role-based restrictions when inviting new members. Specifically, it introduces a can_invite_as check to ensure that users cannot invite others with a role higher than their own (e.g., an Admin cannot invite an Owner). It also refactors permission checks into a helper method and adds corresponding unit tests. The review feedback suggests using the existing Self::map_repository_error helper function to map repository errors consistently instead of manually formatting them as internal errors.

Comment thread crates/services/src/organization/mod.rs Outdated
Copy link
Copy Markdown
Collaborator

@Evrard-Nil Evrard-Nil left a comment

Choose a reason for hiding this comment

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

Pulled the branch and reviewed end-to-end. Build clean (cargo check -p services); the new create_invitations_rejects_roles_above_requester_role test plus the 4 pre-existing invitation tests all pass locally.

Fix is correctly scoped. The privilege-escalation gap was specifically the invitation path. Adjacent role-management code is already locked down:

  • update_member_role_impl (line 337) — owner-only, blocks setting role = Owner. Admins can't promote via this path.
  • add_member_impl (line 263) — blocks role = Owner unconditionally and routes Owner via transfer_ownership.
  • remove_member_impl already prevents removing the owner.

So invitation was the only remaining hole, and can_invite_as plugs it.

can_invite_as matrix is sensible.

Requester Owner Admin Member
Owner
Admin
Member

Member→any is unreachable in practice because get_invitation_requester_role short-circuits on can_manage_members() before the per-row check runs — but it's defensive defense-in-depth and matches the MemberRole predicate vocabulary.

Worth flagging (non-blocking):

  1. Admin→Admin is a policy choice with a sharp edge. An admin can invite another admin; once both exist, remove_member allows either admin to revoke the other. A compromised admin could swamp the org with peer admins or churn the admin set faster than the owner notices. Description says this is intended (matches frontend dropdown), so I'm not pushing back, just naming the tradeoff. If you'd rather "admins can only invite at-or-below-strict" you'd change the Admin arm to matches!(role, MemberRole::Member).

  2. Batch semantics — partial success on authz failure. When an admin sends [Owner, Admin, Member], the Owner row fails and the Admin+Member rows succeed. Consistent with how "already a member" and email-send failures are handled in the same loop, but it does mean a malicious batch isn't a hard stop. Probably fine because the unauthorized rows are still rejected, just thought I'd note the choice.

  3. Adjacent swallowed-error pattern. add_member_impl (line 245) and remove_member_impl (line 302) still use if let Ok(Some(member)) = … which masks DB errors as "User is not a member of this organization" — the same antipattern gemini flagged here that you fixed in get_invitation_requester_role via map_repository_error. Out of scope for this PR, but worth a follow-up sweep so DB blips don't surface as Unauthorized for adds/removes.

  4. Test gap (tiny). make_service_with_requester_role is only exercised for MemberRole::Admin. An explicit Member test (calling create_invitations and asserting Unauthorized) would lock in the helper's can_manage_members gate, but the existing can_manage_members test elsewhere implicitly covers it.

LGTM. Approving — the security fix is solid, the iteration after gemini's review properly switched to map_repository_error, and the test exercises the exact reported attack (admin self-elevating via invite-as-owner).

@PierreLeGuen PierreLeGuen temporarily deployed to Cloud API test env May 27, 2026 17:07 — with GitHub Actions Inactive
@PierreLeGuen PierreLeGuen temporarily deployed to Cloud API test env May 27, 2026 17:22 — with GitHub Actions Inactive
@PierreLeGuen PierreLeGuen merged commit 9575972 into main May 27, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enforce role hierarchy in invitation creation

2 participants