Skip to content

fix: use membership role as source-of-truth for organization roles #1988

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rafaumeu wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: use membership role as source-of-truth for organization roles #1988

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ff88668
fix: use membership role as source-of-truth for organization roles (#…
rafaumeu Jul 8, 2026
f7d0dfd
fix: address review feedback — restrict ownerId fallback to missing m…
rafaumeu Jul 8, 2026
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
3 changes: 2 additions & 1 deletion apps/web/__tests__/unit/desktop-organization-branding.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,12 +144,13 @@ describe("desktop organization branding", () => {
expect(
canEditOrganizationBranding(row({ tombstoneAt: new Date() }), "user-1"),
).toBe(false);
// memberRole "owner" takes precedence — user CAN edit
expect(
canEditOrganizationBranding(
row({ ownerId: "user-2", role: "owner" }),
"user-1",
),
).toBe(false);
).toBe(true);
});

it("validates and normalizes branding patch payloads", () => {
Expand Down
15 changes: 12 additions & 3 deletions apps/web/__tests__/unit/roles-permissions.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,15 @@ describe("organization role permissions", () => {
expect(normalizeAssignableOrganizationRole("owner")).toBeNull();
});

it("derives owner from organization ownership even when membership role differs", () => {
it("uses membership role as source-of-truth for organization roles (#1641)", () => {
// memberRole takes precedence over ownerId
expect(
getEffectiveOrganizationRole({
userId: "user-1",
ownerId: "user-1",
memberRole: "member",
}),
).toBe("owner");
).toBe("member");
expect(
getEffectiveOrganizationRole({
userId: "user-2",
Expand All @@ -48,7 +49,15 @@ describe("organization role permissions", () => {
ownerId: "real-owner",
memberRole: "owner",
}),
).toBe("member");
).toBe("owner");
// ownerId fallback when membership record is missing
expect(
getEffectiveOrganizationRole({
userId: "user-1",
ownerId: "user-1",
memberRole: null,
}),
).toBe("owner");
});

it("allows only owners and admins to view and manage organization members", () => {
Expand Down
3 changes: 2 additions & 1 deletion apps/web/app/(org)/dashboard/settings/organization/components/MembersCard.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,8 @@ export const MembersCard = ({ setIsInviteDialogOpen }: MembersCardProps) => {
};

const isMemberOwner = (id: string) => {

@tembo tembo Bot Jul 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If activeOrganization.members can ever be missing the owner membership row (legacy/migration state), this drops the ownerId fallback and can show non-owner UI for the owner. If you still want a legacy fallback without reintroducing the stale-ownerId override, only fall back when no membership record exists.

Suggested change
const isMemberOwner = (id: string) => {
return member ? member.role === "owner" : id === activeOrganization?.organization.ownerId;

@rafaumeu rafaumeu Jul 8, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Applied your suggestion in f7d0dfd. isMemberOwner now falls back to ownerId only when no membership record is found: member ? member.role === 'owner' : id === activeOrganization?.organization.ownerId

return id === activeOrganization?.organization.ownerId;
const member = activeOrganization?.members.find((m) => m.userId === id);
return member ? member.role === "owner" : id === activeOrganization?.organization.ownerId;
};

return (
Expand Down
16 changes: 13 additions & 3 deletions apps/web/lib/permissions/roles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,15 @@ export function getEffectiveOrganizationRole({
ownerId: string | null | undefined;
memberRole: string | null | undefined;
}): OrganizationRole | null {
// Prefer organization_members.role as source-of-truth over organization.ownerId
// to avoid drift between the two fields (see #1641).
const normalizedRole = normalizeOrganizationRole(memberRole);
if (normalizedRole === "owner") return "owner";
if (normalizedRole) return normalizedRole;
// Fallback: if membership record is missing (e.g. legacy data),
// derive owner from the organization.ownerId field.
if (userId && ownerId && userId === ownerId) return "owner";
const role = normalizeOrganizationRole(memberRole);
return role === "owner" ? "member" : role;
return null;
}

export function normalizeSpaceRole(
Expand Down Expand Up @@ -131,7 +137,11 @@ export function isOrganizationOwnerTarget({
ownerId: string | null | undefined;
targetRole: OrganizationRole | null | undefined;
}) {
return targetRole === "owner" || (!!targetUserId && targetUserId === ownerId);
// Prefer membership role over ownerId (see #1641).
if (targetRole === "owner") return true;
if (targetRole) return false;
// Fallback for legacy/missing membership records.
return !!targetUserId && !!ownerId && targetUserId === ownerId;

@greptile-apps greptile-apps Bot Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 security Stale OwnerId Still Wins

When targetRole is already known to be "member" or "admin", this fallback can still classify the target as owner because their id matches stale ownerId. In that drift state, canChangeOrganizationMemberRole and canRemoveOrganizationMember reject changes to a non-owner member, so the member can become stuck even though membership role is now the source of truth.

Context Used: AGENTS.md (source)

Prompt To Fix With AI 
This is a comment left during a code review.
Path: apps/web/lib/permissions/roles.ts
Line: 143

Comment:
**Stale OwnerId Still Wins**

When `targetRole` is already known to be `"member"` or `"admin"`, this fallback can still classify the target as owner because their id matches stale `ownerId`. In that drift state, `canChangeOrganizationMemberRole` and `canRemoveOrganizationMember` reject changes to a non-owner member, so the member can become stuck even though membership role is now the source of truth.

**Context Used:** AGENTS.md ([source](https://app.greptile.com/cap/github/capsoftware/cap/-/custom-context?memory=27801409-c24c-4476-9c6c-180f1ef0a7f2))

How can I resolve this? If you propose a fix, please make it concise.

@rafaumeu rafaumeu Jul 8, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed in f7d0dfd — isOrganizationOwnerTarget now only falls back to ownerId when targetRole is null/undefined. When targetRole is a known non-owner role ("member"/"admin"), it returns false immediately. Same logic applied to MembersCard.isMemberOwner — falls back to ownerId only when no membership record exists.

Comment on lines +140 to +144

@tembo tembo Bot Jul 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Minor edge case: if targetRole is already known to be "member"/"admin", the current ownerId fallback can still classify the target as owner if ownerId is stale. You can restrict the fallback to only when targetRole is missing.

Suggested change
// Prefer membership role over ownerId (see #1641).
if (targetRole === "owner") return true;
// Fallback for legacy/missing membership records.
return !!targetUserId && !!ownerId && targetUserId === ownerId;
// Prefer membership role over ownerId (see #1641).
if (targetRole === "owner") return true;
if (targetRole) return false;
// Fallback for legacy/missing membership records.
return !!targetUserId && !!ownerId && targetUserId === ownerId;

@rafaumeu rafaumeu Jul 8, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Applied your suggestion in f7d0dfd. if (targetRole) return false; now short-circuits when targetRole is a known non-owner role before reaching the ownerId fallback.

}

export function canChangeOrganizationMemberRole({
Expand Down