From f7713170071a33518d8a3334fa59189f9b308b8d Mon Sep 17 00:00:00 2001 From: Vincent ROBERT Date: Wed, 29 Jul 2026 18:35:48 +0200 Subject: [PATCH 1/2] fix(org:user): report missing organization permissions plainly 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) --- integration-tests/org_user_permission_test.go | 170 ++++++++++++++++++ .../User/OrganizationUserAddCommand.php | 14 ++ .../User/OrganizationUserDeleteCommand.php | 9 + 3 files changed, 193 insertions(+) create mode 100644 integration-tests/org_user_permission_test.go diff --git a/integration-tests/org_user_permission_test.go b/integration-tests/org_user_permission_test.go new file mode 100644 index 000000000..ae02bc47e --- /dev/null +++ b/integration-tests/org_user_permission_test.go @@ -0,0 +1,170 @@ +package tests + +import ( + "net/http" + "net/http/httptest" + "net/url" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/upsun/cli/pkg/mockapi" +) + +// TestOrgUserNoPermission covers the organization user commands when the user +// has no permission on the organization itself, as happens when they are only +// an admin on one of its projects. The API then returns an organization without +// the "members" and "invitations" links, and the commands must say so plainly +// instead of failing on an API error or an unresolved HAL link. +func TestOrgUserNoPermission(t *testing.T) { + authServer := mockapi.NewAuthServer(t) + defer authServer.Close() + + myUserID := "my-user-id" + orgID := "org-id-no-perms" + orgName := "no-perms-org" + + apiHandler := mockapi.NewHandler(t) + apiHandler.SetMyUser(&mockapi.User{ID: myUserID}) + + // An organization the user cannot administer: no "members" link, and no + // "invitations" link. Only the owner-less "self" link is present. + apiHandler.SetOrgs([]*mockapi.Org{makeOrg(orgID, orgName, "No Perms Org", "another-user-id", "flexible")}) + + // The members endpoint is what the commands used to reach anyway, bypassing + // the links: it must not be requested at all. + membersRequested := false + apiHandler.Get("/organizations/"+orgID+"/members", func(w http.ResponseWriter, _ *http.Request) { + membersRequested = true + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte(`{"status":403,"title":"Forbidden",` + + `"detail":"You do not have access to view this organization's members."}`)) + }) + + apiServer := httptest.NewServer(apiHandler) + defer apiServer.Close() + + f := newCommandFactory(t, apiServer.URL, authServer.URL) + + cases := []struct { + name string + args []string + expected string + }{ + { + name: "org:user:add", + args: []string{"org:user:add", "-o", orgName, "test@example.com"}, + expected: "You do not have permission to add users to the organization", + }, + { + name: "org:user:update", + args: []string{"org:user:update", "-o", orgName, "test@example.com", "--permission", "billing"}, + expected: "You do not have permission to update users in the organization", + }, + { + name: "org:user:delete", + args: []string{"org:user:delete", "-o", orgName, "test@example.com"}, + expected: "You do not have permission to delete users from the organization", + }, + { + // The read-only commands already behaved this way; keep them covered + // so the wording of the whole family stays consistent. + name: "org:user:list", + args: []string{"org:user:list", "-o", orgName}, + expected: "You do not have permission to view users in the organization", + }, + { + name: "org:user:get", + args: []string{"org:user:get", "-o", orgName, "test@example.com"}, + expected: "You do not have permission to view users in the organization", + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + stdout, stderr, err := f.RunCombinedOutput(c.args...) + require.Error(t, err, "the command must fail\nstdout: %s\nstderr: %s", stdout, stderr) + assert.Contains(t, stderr, c.expected) + assert.Contains(t, stderr, orgName) + + // None of the raw failures should surface. + assert.NotContains(t, stderr, "Link not found") + assert.NotContains(t, stderr, "ApiResponseException") + assert.NotContains(t, stderr, "403 Forbidden") + assert.NotContains(t, stderr, "Permission denied. Check your permissions") + // Nor should the usage summary, which Symfony prints for exceptions. + assert.NotContains(t, stderr, "[-o|--org ORG]") + }) + } + + // Interactively, with no email given, org:user:update used to pick the user + // from a members listing built from the organization URL, bypassing the links + // entirely — which produced a raw 403 on top of the permission error. + t.Run("org:user:update interactive", func(t *testing.T) { + stdout, stderr, err := f.RunInteractive("", "org:user:update", "-o", orgName) + require.Error(t, err, "stdout: %s\nstderr: %s", stdout, stderr) + assert.Contains(t, stderr, "You do not have permission to update users in the organization") + assert.NotContains(t, stderr, "403 Forbidden") + assert.NotContains(t, stderr, "ApiResponseException") + assert.NotContains(t, stderr, "Permission denied. Check your permissions") + }) + + assert.False(t, membersRequested, "the members endpoint must not be requested without the link") +} + +// TestOrgUserWithPermission is the counterpart: with the links present, the +// commands must get past the permission check and reach their normal behavior. +func TestOrgUserWithPermission(t *testing.T) { + authServer := mockapi.NewAuthServer(t) + defer authServer.Close() + + myUserID := "my-user-id" + orgID := "org-id-with-perms" + orgName := "with-perms-org" + + apiHandler := mockapi.NewHandler(t) + apiHandler.SetMyUser(&mockapi.User{ID: myUserID}) + + org := makeOrg(orgID, orgName, "With Perms Org", myUserID, "flexible") + org.Links["members"] = mockapi.HALLink{HREF: "/organizations/" + url.PathEscape(orgID) + "/members"} + org.Links["invitations"] = mockapi.HALLink{HREF: "/organizations/" + url.PathEscape(orgID) + "/invitations"} + apiHandler.SetOrgs([]*mockapi.Org{org}) + + // An empty members list: the commands get past the permission check and then + // report that the user was not found. + apiHandler.Get("/organizations/"+orgID+"/members", func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"items":[],"_links":{"self":{"href":"/organizations/` + orgID + `/members"}}}`)) + }) + + apiServer := httptest.NewServer(apiHandler) + defer apiServer.Close() + + f := newCommandFactory(t, apiServer.URL, authServer.URL) + + cases := []struct { + name string + args []string + expected string + }{ + { + name: "org:user:update", + args: []string{"org:user:update", "-o", orgName, "test@example.com", "--permission", "billing"}, + expected: "was not found in the organization", + }, + { + name: "org:user:delete", + args: []string{"org:user:delete", "-o", orgName, "test@example.com"}, + expected: "User not found", + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + stdout, stderr, err := f.RunCombinedOutput(c.args...) + require.Error(t, err, "stdout: %s\nstderr: %s", stdout, stderr) + assert.Contains(t, stderr, c.expected) + assert.NotContains(t, stderr, "You do not have permission") + }) + } +} diff --git a/legacy/src/Command/Organization/User/OrganizationUserAddCommand.php b/legacy/src/Command/Organization/User/OrganizationUserAddCommand.php index 262f0a107..b936580c5 100644 --- a/legacy/src/Command/Organization/User/OrganizationUserAddCommand.php +++ b/legacy/src/Command/Organization/User/OrganizationUserAddCommand.php @@ -36,6 +36,20 @@ protected function execute(InputInterface $input, OutputInterface $output): int $organization = $this->selector->selectOrganization($input, 'create-member'); $update = get_called_class() === OrganizationUserUpdateCommand::class; + + // The selector only checks the link when it picks an organization + // itself: with an explicit --org it returns the organization as is, so + // the check has to happen here, or the command fails later on a raw API + // error. Members are listed in both cases; adding a user also needs the + // 'invitations' link. + if (!$organization->hasLink('members') || (!$update && !$organization->hasLink('invitations'))) { + $orgLabel = $this->api->getOrganizationLabel($organization, 'comment'); + $this->stdErr->writeln($update + ? 'You do not have permission to update users in the organization ' . $orgLabel . '.' + : 'You do not have permission to add users to the organization ' . $orgLabel . '.'); + return 1; + } + if ($update) { $email = $input->getArgument('email'); if (!empty($email)) { diff --git a/legacy/src/Command/Organization/User/OrganizationUserDeleteCommand.php b/legacy/src/Command/Organization/User/OrganizationUserDeleteCommand.php index c4c0a51ea..6de330366 100644 --- a/legacy/src/Command/Organization/User/OrganizationUserDeleteCommand.php +++ b/legacy/src/Command/Organization/User/OrganizationUserDeleteCommand.php @@ -32,6 +32,15 @@ protected function execute(InputInterface $input, OutputInterface $output): int // The 'create-member' link shows the user has the ability to read/write members. $organization = $this->selector->selectOrganization($input, 'create-member'); + // The selector only checks the link when it picks an organization + // itself: with an explicit --org it returns the organization as is, so + // the check has to happen here, or the command fails later on a raw API + // error. + if (!$organization->hasLink('members')) { + $this->stdErr->writeln('You do not have permission to delete users from the organization ' . $this->api->getOrganizationLabel($organization, 'comment') . '.'); + return 1; + } + $email = $input->getArgument('email'); $member = $this->api->loadMemberByEmail($organization, $email); From c8232835bcbd75df0a7042125b17897308d23e95 Mon Sep 17 00:00:00 2001 From: Vincent ROBERT Date: Wed, 29 Jul 2026 18:45:33 +0200 Subject: [PATCH 2/2] test(org:user): make the members-endpoint flag atomic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- integration-tests/org_user_permission_test.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/integration-tests/org_user_permission_test.go b/integration-tests/org_user_permission_test.go index ae02bc47e..adf102f52 100644 --- a/integration-tests/org_user_permission_test.go +++ b/integration-tests/org_user_permission_test.go @@ -4,6 +4,7 @@ import ( "net/http" "net/http/httptest" "net/url" + "sync/atomic" "testing" "github.com/stretchr/testify/assert" @@ -33,10 +34,11 @@ func TestOrgUserNoPermission(t *testing.T) { apiHandler.SetOrgs([]*mockapi.Org{makeOrg(orgID, orgName, "No Perms Org", "another-user-id", "flexible")}) // The members endpoint is what the commands used to reach anyway, bypassing - // the links: it must not be requested at all. - membersRequested := false + // the links: it must not be requested at all. The flag is atomic because the + // handler runs on the test server's goroutine. + var membersRequested atomic.Bool apiHandler.Get("/organizations/"+orgID+"/members", func(w http.ResponseWriter, _ *http.Request) { - membersRequested = true + membersRequested.Store(true) w.WriteHeader(http.StatusForbidden) _, _ = w.Write([]byte(`{"status":403,"title":"Forbidden",` + `"detail":"You do not have access to view this organization's members."}`)) @@ -110,7 +112,7 @@ func TestOrgUserNoPermission(t *testing.T) { assert.NotContains(t, stderr, "Permission denied. Check your permissions") }) - assert.False(t, membersRequested, "the members endpoint must not be requested without the link") + assert.False(t, membersRequested.Load(), "the members endpoint must not be requested without the link") } // TestOrgUserWithPermission is the counterpart: with the links present, the