fix(org:user): report missing organization permissions plainly - #136
Open
vrobert78 wants to merge 2 commits into
Open
fix(org:user): report missing organization permissions plainly#136vrobert78 wants to merge 2 commits into
vrobert78 wants to merge 2 commits into
Conversation
A user with no permission on an organization, but with an admin role on one
of its projects, got a raw API error from org:user:add, org:user:update and
org:user:delete:
$ upsun org:user:add -o an-org
In ApiResourceBase.php line 440:
Link not found: members
$ upsun org:user:update -o an-org
In EventSubscriber.php line 80:
Permission denied. Check your permissions on the organization.
In ApiResponseException.php line 78:
Client error: `GET .../members?page%5Bsize%5D=100` resulted in a `403 Forbidden` ...
org:user:list, org:user:get and org:user:projects handle this: they check the
organization's 'members' link after selecting it and report the problem in one
line. The three write commands never got that check.
The link the selector is given ('create-member') is only applied when the
selector picks an organization itself, either automatically or interactively;
an explicit --org returns the organization as is. So the commands went on to
list members, either through Api::loadMemberByEmail(), which resolves the
'members' link, or through chooseMember(), which builds the members URL from
the organization URL and so bypasses the links entirely and gets a bare 403.
Check the links these commands actually dereference right after selecting the
organization, wording the message like the read-only commands:
You do not have permission to add users to the organization An Org (an-org).
You do not have permission to update users in the organization An Org (an-org).
You do not have permission to delete users from the organization An Org (an-org).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the legacy (PHP) organization user write commands by detecting missing organization-level permissions earlier and emitting a clear, user-facing message (matching the existing read-only command wording) instead of surfacing raw API/HAL-link failures. It also adds integration coverage to ensure these commands no longer hit the members endpoint when the organization’s links indicate the user lacks access.
Changes:
- Add explicit post-selection link guards to
org:user:add,org:user:update, andorg:user:deleteto fail fast with plain permission messages when required organization links are missing. - Add integration tests that reproduce the prior failures and assert the new behavior (including the interactive update path).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| legacy/src/Command/Organization/User/OrganizationUserDeleteCommand.php | Adds a members-link guard after organization selection to avoid raw API errors and print a clear “no permission to delete users” message. |
| legacy/src/Command/Organization/User/OrganizationUserAddCommand.php | Adds members / invitations link guards (depending on add vs update) to fail early with clear “no permission” messaging. |
| integration-tests/org_user_permission_test.go | Adds integration tests covering no-permission vs with-permission cases across org user commands, including interactive update behavior. |
Comments suppressed due to low confidence (1)
integration-tests/org_user_permission_test.go:113
- After switching to a synchronized
membersRequestedflag, the final assertion should also use a synchronized read (e.g., non-blocking receive) instead of reading a shared variable directly.
assert.False(t, membersRequested, "the members endpoint must not be requested without the link")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The flag is written from the test server's handler goroutine and read from the test, which is unsynchronized. The race detector does not report it — in the passing case the handler is never called at all, which is what the assertion checks — but an atomic makes a regression fail on the assertion rather than on goroutine timing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A user with no permission on an organization, but with an admin role on one of its projects, gets a raw API error from
org:user:add,org:user:updateandorg:user:delete:org:user:listgets this right already:Cause
org:user:list,org:user:getandorg:user:projectscheck the organization'smemberslink right after selecting it. The three write commands never got that check.The link passed to the selector (
'create-member') does not cover it:Selector::selectOrganization()only applies$filterByLinkwhen it picks an organization itself — automatically from the current project, or interactively. An explicit--orgreturns the organization as is (Selector.php:909-931). The commands then went on to list members either throughApi::loadMemberByEmail(), which resolves thememberslink and throwsLink not found: members, or throughchooseMember(), which builds the members URL from the organization URI and so bypasses the links entirely and takes a bare 403.Fix
Check the links these commands actually dereference, immediately after selecting the organization, and word the message like the read-only commands:
The guard fires only when a link the command would dereference is missing — in which case the command was already guaranteed to fail — so it cannot block an operation that used to succeed. Adding a user additionally needs the
invitationslink, whichOrganization::inviteMemberByEmail()resolves.Verification
integration-tests/org_user_permission_test.godrives the real binary against the mock API, with an organization whose_linkshold onlyself, and the members endpoint returning the 403 body above:mainthe new tests reproduce both failures verbatim (Link not found: membersplus the usage dump), while theorg:user:listandorg:user:getcases pass — a check on the existing behaviour the new messages are modelled on.org:user:update -o <org>run interactively with no email, which is the path that went throughchooseMember()and took the bare 403. The test also asserts the members endpoint is never requested.TestOrgUserWithPermissionis the counterpart: withmembersandinvitationspresent, the commands get past the check and reach their normal "user not found" behaviour.Also confirmed against the production API with a real account, on an organization where the user is only a project admin:
Full integration suite green;
php-cs-fixerand PHPStan (PHP 8.4) clean.Left alone, for a possible follow-up
Permission denied. Check your permissions on the organization.followed by the chainedApiResponseException, because Symfony renders the wholegetPrevious()chain. That affects every 4xx in the CLI, not just these commands.Selector::selectOrganization()still ignores$filterByLinkwhen--orgis explicit, so the same class of bug can reappear in a new command. Enforcing it there would prevent that, at the cost of a generic message in place of the per-command wording.🤖 Generated with Claude Code