Whitelist ownership grants via allow policies

Replace the deny-based restriction on granting OWNER with role-scoped
allow policies so authorization fails closed: admins may create and
update memberships only when the assigned role is not OWNER, and the
absence of a target role no longer implies permission.

To keep console UI gating accurate without loosening the base grants,
the permission field gains an optional typed options argument
(PermissionOptionsInput) that forwards target_role into the dry-run
authorization. Only the two role-related console calls (create user,
update membership) pass it; the OWNER option stays hidden for admins via
the existing assignable-roles helper.

Add a non-regression test that an admin cannot promote a member to OWNER
while still being able to change members between non-owner roles.
This commit is contained in:
Sacha Al Himdani
2026-07-07 10:56:33 +02:00
parent ff9cb881e8
commit 86c45875a4
12 changed files with 133 additions and 27 deletions

View File

@@ -29,7 +29,7 @@ export const peoplePageQuery = graphql`
organization: node(id: $organizationId) @required(action: THROW) {
__typename
... on Organization {
canCreateUser: permission(action: "iam:membership-profile:create")
canCreateUser: permission(action: "iam:membership-profile:create", attributes: { target_role: "VIEWER" })
...PeopleListFragment
@arguments(first: 20, order: { direction: ASC, field: FULL_NAME })
}

View File

@@ -48,7 +48,7 @@ const fragment = graphql`
membership @required(action: THROW) {
id
role
canUpdate: permission(action: "iam:membership:update")
canUpdate: permission(action: "iam:membership:update", attributes: { target_role: "VIEWER" })
}
lastInvitation: pendingInvitations(first: 1, orderBy: { field: CREATED_AT, direction: DESC })
@required(action: THROW)
@@ -134,6 +134,10 @@ export function PeopleListItem(props: {
const profile = useFragment<PeopleListItemFragment$key>(fragment, fKey);
const lastInvitation = profile.lastInvitation.edges[0]?.node;
const roleOptions = availableRoles.includes(profile.membership.role)
? availableRoles
: [...availableRoles, profile.membership.role];
const isInactive = profile.state === "INACTIVE";
const canSendActivationMail = isInactive && profile.source !== "SCIM" && profile.canInvite;
@@ -286,19 +290,19 @@ export function PeopleListItem(props: {
value={profile.membership.role}
onValueChange={role => void handleUpdateRole(role)}
>
{availableRoles.includes("OWNER") && (
{roleOptions.includes("OWNER") && (
<Option value="OWNER">{__("Owner")}</Option>
)}
{availableRoles.includes("ADMIN") && (
{roleOptions.includes("ADMIN") && (
<Option value="ADMIN">{__("Admin")}</Option>
)}
{availableRoles.includes("VIEWER") && (
{roleOptions.includes("VIEWER") && (
<Option value="VIEWER">{__("Viewer")}</Option>
)}
{availableRoles.includes("AUDITOR") && (
{roleOptions.includes("AUDITOR") && (
<Option value="AUDITOR">{__("Auditor")}</Option>
)}
{availableRoles.includes("EMPLOYEE") && (
{roleOptions.includes("EMPLOYEE") && (
<Option value="EMPLOYEE">{__("Employee")}</Option>
)}
</Select>

View File

@@ -10,7 +10,7 @@ All notable changes to `probod` (the server, including the bundled `@probo/conso
### Changed
- Consolidated ownership-grant authorization into policy: granting OWNER (via `createUser` or `updateMembership`) is now restricted to organization owners through role-scoped allow policies conditioned on the assigned role, replacing the per-resolver custom checks and the now-removed `iam:membership-role:set-owner` action. The `permission` field gained an optional typed `attributes` argument so the console can refine dry-run checks (e.g. by target role) without loosening the base grants
- Consolidated ownership-grant authorization into policy: granting OWNER (via `createUser` or `updateMembership`) is now restricted to organization owners through role-scoped allow policies conditioned on the assigned role, replacing the per-resolver custom checks and the now-removed `iam:membership-role:set-owner` action. The `permission` field gained an optional generic `attributes` key/value argument so the console can refine dry-run checks (e.g. by target role) without loosening the base grants
## [0.223.3] - 2026-07-06

View File

@@ -97,6 +97,92 @@ func TestUser_AdminCannotCreateOwner(t *testing.T) {
assert.Equal(t, "OWNER", ownerCreate.CreateUser.ProfileEdge.Node.Membership.Role)
}
// TestUser_AdminCannotPromoteToOwner is a non-regression test that an ADMIN
// cannot grant ownership by promoting an existing member to OWNER via
// updateMembership. Granting ownership is owner-only, enforced by policy on the
// assigned (target) role; an ADMIN may still change members between non-owner
// roles.
func TestUser_AdminCannotPromoteToOwner(t *testing.T) {
t.Parallel()
owner := testutil.NewClient(t, testutil.RoleOwner)
admin := testutil.NewClientInOrg(t, testutil.RoleAdmin, owner)
viewer := testutil.NewClientInOrg(t, testutil.RoleViewer, owner)
const membershipQuery = `
query($id: ID!) {
node(id: $id) {
... on Profile {
membership { id }
}
}
}
`
var viewerMembership struct {
Node struct {
Membership struct {
ID string `json:"id"`
} `json:"membership"`
} `json:"node"`
}
err := owner.ExecuteConnect(membershipQuery, map[string]any{
"id": viewer.GetProfileID().String(),
}, &viewerMembership)
require.NoError(t, err)
require.NotEmpty(t, viewerMembership.Node.Membership.ID)
membershipID := viewerMembership.Node.Membership.ID
const updateMembershipMutation = `
mutation($input: UpdateMembershipInput!) {
updateMembership(input: $input) {
membership { role }
}
}
`
updateInput := func(role string) map[string]any {
return map[string]any{
"input": map[string]any{
"organizationId": owner.GetOrganizationID().String(),
"membershipId": membershipID,
"role": role,
},
}
}
// An ADMIN must not be able to promote a member to OWNER.
_, err = admin.DoConnect(updateMembershipMutation, updateInput("OWNER"))
testutil.RequireForbiddenError(t, err)
// The same ADMIN can still change a member between non-owner roles.
var adminUpdate struct {
UpdateMembership struct {
Membership struct {
Role string `json:"role"`
} `json:"membership"`
} `json:"updateMembership"`
}
err = admin.ExecuteConnect(updateMembershipMutation, updateInput("ADMIN"), &adminUpdate)
require.NoError(t, err)
assert.Equal(t, "ADMIN", adminUpdate.UpdateMembership.Membership.Role)
// An OWNER remains able to promote a member to OWNER.
var ownerUpdate struct {
UpdateMembership struct {
Membership struct {
Role string `json:"role"`
} `json:"membership"`
} `json:"updateMembership"`
}
err = owner.ExecuteConnect(updateMembershipMutation, updateInput("OWNER"), &ownerUpdate)
require.NoError(t, err)
assert.Equal(t, "OWNER", ownerUpdate.UpdateMembership.Membership.Role)
}
// TestUser_AdminCannotRemoveMembers is a non-regression test for the broken
// access control where an ADMIN could hard-remove members (including OWNERs)
// via removeUser because the mutation only checked the weaker

View File

@@ -265,6 +265,9 @@ var IAMAdminPolicy = policy.NewPolicy(
WithSID("membership-admin-access").
When(policy.Equals("principal.organization_id", "resource.organization_id")),
// Can update memberships, but neither of an existing owner (resource.role)
// nor to grant ownership (resource.target_role); only owner can grant
// ownership.
policy.Allow(
ActionMembershipUpdate,
).
@@ -272,13 +275,13 @@ var IAMAdminPolicy = policy.NewPolicy(
When(
policy.Equals("principal.organization_id", "resource.organization_id"),
policy.NotEquals("resource.role", "OWNER"),
policy.NotEquals("resource.target_role", "OWNER"),
),
// Can view membership profiles (scoped to own organization)
// Can view and manage membership profiles (scoped to own organization)
policy.Allow(
ActionMembershipProfileGet,
ActionMembershipProfileList,
ActionMembershipProfileCreate,
ActionMembershipProfileUpdate,
ActionMembershipProfileDelete,
ActionMembershipProfileActivate,
@@ -287,6 +290,15 @@ var IAMAdminPolicy = policy.NewPolicy(
WithSID("membership-profile-admin-access").
When(policy.Equals("principal.organization_id", "resource.organization_id")),
// Can create members, but not with the OWNER role (resource.target_role);
// only owner can grant ownership.
policy.Allow(ActionMembershipProfileCreate).
WithSID("membership-profile-admin-create").
When(
policy.Equals("principal.organization_id", "resource.organization_id"),
policy.NotEquals("resource.target_role", "OWNER"),
),
// Can view identities of members in the same organization
policy.Allow(ActionIdentityGet).
WithSID("view-member-identity").
@@ -313,15 +325,6 @@ var IAMAdminPolicy = policy.NewPolicy(
policy.Deny(ActionMembershipDelete).
WithSID("deny-remove-member"),
// Cannot grant ownership, whether by creating an OWNER member or promoting an
// existing member to OWNER (only owner can grant ownership)
policy.Deny(ActionMembershipProfileCreate).
WithSID("deny-create-owner").
When(policy.Equals("resource.target_role", "OWNER")),
policy.Deny(ActionMembershipUpdate).
WithSID("deny-promote-owner").
When(policy.Equals("resource.target_role", "OWNER")),
// Cannot manage SAML configurations (only owner can)
policy.Deny(
ActionSAMLConfigurationCreate,

View File

@@ -17,6 +17,7 @@ scalar Datetime
scalar Upload
scalar EmailAddr
scalar OAuth2Scope
scalar Map
interface Node {
id: ID!

View File

@@ -16,7 +16,7 @@ type Membership implements Node {
lastSession: Session @goField(forceResolver: true)
permission(action: String!): Boolean!
permission(action: String!, attributes: Map): Boolean!
@goField(forceResolver: true)
@authentication(required: PRESENT)
}

View File

@@ -39,7 +39,7 @@ type Organization implements Node {
viewer: Profile @goField(forceResolver: true)
permission(action: String!): Boolean!
permission(action: String!, attributes: Map): Boolean!
@goField(forceResolver: true)
@authentication(required: PRESENT)
}

View File

@@ -44,8 +44,8 @@ func (r *membershipResolver) LastSession(ctx context.Context, obj *types.Members
}
// Permission is the resolver for the permission field.
func (r *membershipResolver) Permission(ctx context.Context, obj *types.Membership, action string) (bool, error) {
return r.Resolver.Permission(ctx, obj, action)
func (r *membershipResolver) Permission(ctx context.Context, obj *types.Membership, action string, attributes map[string]any) (bool, error) {
return r.Resolver.permission(ctx, obj, action, attributes)
}
// UpdateMembership is the resolver for the updateMembership field.

View File

@@ -358,8 +358,8 @@ func (r *organizationResolver) Viewer(ctx context.Context, obj *types.Organizati
}
// Permission is the resolver for the permission field.
func (r *organizationResolver) Permission(ctx context.Context, obj *types.Organization, action string) (bool, error) {
return r.Resolver.Permission(ctx, obj, action)
func (r *organizationResolver) Permission(ctx context.Context, obj *types.Organization, action string, attributes map[string]any) (bool, error) {
return r.Resolver.permission(ctx, obj, action, attributes)
}
// Organization returns schema.OrganizationResolver implementation.

View File

@@ -129,7 +129,18 @@ func NewMux(
}
func (r *Resolver) Permission(ctx context.Context, obj types.Node, action string) (bool, error) {
_, err := r.authorize(ctx, obj.GetID(), action, authz.WithDryRun())
return r.permission(ctx, obj, action, nil)
}
func (r *Resolver) permission(ctx context.Context, obj types.Node, action string, attributes map[string]any) (bool, error) {
opts := []authz.AuthorizeFuncOption{authz.WithDryRun()}
for key, value := range attributes {
if s, ok := value.(string); ok {
opts = append(opts, authz.WithAttr(key, s))
}
}
_, err := r.authorize(ctx, obj.GetID(), action, opts...)
return err == nil, nil
}

View File

@@ -39,7 +39,8 @@
"Duration": "string",
"BigInt": "number",
"EmailAddr": "string",
"OAuth2Scope": "string"
"OAuth2Scope": "string",
"Map": "Record<string, string>"
}
},
"trust": {