Skip to content

feat: allow wildcard tokens to query/assign any group without membership#138

Closed
pmelab wants to merge 2 commits into
devfrom
feature/service-account-group-query-bypass
Closed

feat: allow wildcard tokens to query/assign any group without membership#138
pmelab wants to merge 2 commits into
devfrom
feature/service-account-group-query-bypass

Conversation

@pmelab
Copy link
Copy Markdown
Contributor

@pmelab pmelab commented May 13, 2026

Problem

GET /instances?group_id=X and PATCH /instance/{uuid}/group both gate access by checking whether the token's user is a member of the target group:

if ($targetGroup !== null && ! $tokenUser->groups()->whereKey($targetGroup->id)->exists()) {
    abort(403, 'You do not have access to the selected group.');
}

This check is correct for end-user tokens — a regular user should only see their own groups. However, it makes both endpoints unusable for service-account tokens (e.g. the POLYDOCK_API_KEY held by moad), because the service account user is never enrolled as a member of any workspace group.

Real-world impact

moad's appInstances GraphQL resolver queries instances for a workspace by calling:

GET /instances?group_id={polydockGroupId}

Every call gets a 403, so the app-instances list is always empty for every workspace in production.

Fix

Tokens with the * (wildcard) ability bypass the group-membership check on both endpoints. Regular user tokens (instances.read / instances.write without *) are unchanged and still require membership.

The * ability is the standard marker for service-account / administrative tokens in Sanctum — existing end-user tokens are scoped to specific abilities and are unaffected.

Changes

  • AuthenticatedApiController::getInstances — skip membership check when token has * ability
  • AuthenticatedApiController::assignInstanceToGroup — same bypass for consistency
  • Four new test cases covering both the bypass (wildcard token) and the preservation of existing behaviour (read/write-only tokens still 403 for non-member groups)

Service-account tokens (ability '*') can now call GET /instances?group_id=X
and PATCH /instance/{uuid}/group without being enrolled as members of the
target group.

Previously both endpoints checked whether the token's user was a member of
the requested group, regardless of token scope. This made them unusable for
upstream orchestrators (e.g. moad) that hold a single service-account token
and need to list or assign instances across all workspace groups.

Regular user tokens (instances.read / instances.write without '*') are
unchanged — they still require group membership.

Adds four new test cases that cover:
- wildcard token can query a group it is not a member of
- read-only token is still forbidden for non-member groups
- wildcard token can assign an instance to a group it is not a member of
Copilot AI review requested due to automatic review settings May 13, 2026 12:27
@pmelab pmelab requested a review from dan2k3k4 May 13, 2026 12:31
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the authenticated API’s group-authorization logic so that Sanctum tokens with the wildcard (*) ability (service-account/admin tokens) can query instances for, and assign instances to, any workspace group even when the token’s user is not enrolled in that group—while preserving the existing membership restrictions for regular end-user tokens.

Changes:

  • Bypass the group-membership check in GET /api/instances when the current token has * ability.
  • Bypass the group-membership check in PATCH /api/instance/{uuid}/group when the current token has * ability.
  • Add new feature tests covering wildcard-token bypass and preserving forbidden behavior for non-wildcard tokens.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
app/Http/Controllers/Api/AuthenticatedApiController.php Allows *-ability tokens to bypass group membership checks for instance listing and group reassignment.
tests/Feature/Api/AuthenticatedApiTest.php Adds tests for wildcard-token behavior and verifies non-wildcard tokens remain forbidden for non-member groups.
Comments suppressed due to low confidence (3)

app/Http/Controllers/Api/AuthenticatedApiController.php:510

  • Same as above: $isServiceAccountToken may be null and is used as a boolean. Coalesce to false (and reuse the existing $user variable) to keep the authorization check strictly boolean and avoid repeating $request->user() calls.
        // Tokens with wildcard (*) ability act as service accounts and are permitted
        // to assign instances to any group without being a member. Regular user
        // tokens (instances.write only) still require group membership.
        $isServiceAccountToken = $request->user()->currentAccessToken()?->can('*');

        if (! $isServiceAccountToken && ! $user->groups()->whereKey($group->id)->exists()) {

tests/Feature/Api/AuthenticatedApiTest.php:741

  • This test appears to duplicate the existing test_get_instances_forbidden_when_not_member_of_group (same ability, same request, same expectation). Consider removing it or changing it to cover a distinct path introduced/affected by this PR (e.g., group_slug with a non-wildcard token).
    public function test_get_instances_read_only_token_still_forbidden_for_non_member_group(): void
    {
        // Regular user tokens (instances.read, not *) still enforce group membership.
        Sanctum::actingAs($this->user, ['instances.read']);

        $inaccessibleGroup = UserGroup::create(['name' => 'Still Inaccessible Group']);

        $response = $this->getJson('/api/instances?group_id='.$inaccessibleGroup->id);

        $response->assertForbidden();
    }

tests/Feature/Api/AuthenticatedApiTest.php:729

  • This test only asserts that exactly one item is returned. It would be more robust to also assert that the returned instance is the one created for the target group (e.g., by UUID or name), so the test can’t pass if unrelated fixtures happen to match the count.
        $response = $this->getJson('/api/instances?group_id='.$group->id);

        $response->assertOk();
        $this->assertCount(1, $response->json('data'));
    }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

// This allows upstream orchestrators (e.g. moad) to list instances for any
// workspace group using the group_id or group_slug query parameters without
// needing the service-account user to be enrolled in every group.
$isServiceAccountToken = $request->user()->currentAccessToken()?->can('*');
Comment on lines +711 to +715
$otherUser = \App\Models\User::factory()->create();
$group = UserGroup::create(['name' => 'Service Account Test Group']);
$otherUser->groups()->syncWithoutDetaching([
$group->id => ['role' => UserGroupRoleEnum::MEMBER->value],
]);
Comment on lines +705 to +710
public function test_get_instances_wildcard_token_can_query_any_group_without_membership(): void
{
// Tokens with '*' ability (service accounts) bypass group membership checks
// so orchestrators like moad can list instances for any workspace group.
Sanctum::actingAs($this->user, ['*']);

- Coalesce currentAccessToken()?->can('*') to false to avoid nullable bool
- Reuse existing $tokenUser/$user variables instead of calling $request->user() again
- Remove duplicate test_get_instances_read_only_token_still_forbidden_for_non_member_group
  (already covered by test_get_instances_forbidden_when_not_member_of_group)
- Assert instance name in wildcard-token query test for stronger assertion
@dan2k3k4
Copy link
Copy Markdown
Member

Different approach in PR #139

@pmelab pmelab closed this May 13, 2026
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.

3 participants