Skip to content

Add UI permission checks for collector pages - #27157

Open
kroepke wants to merge 22 commits into
masterfrom
fix/collector_permissions
Open

Add UI permission checks for collector pages#27157
kroepke wants to merge 22 commits into
masterfrom
fix/collector_permissions

Conversation

@kroepke

@kroepke kroepke commented Aug 28, 2026

Copy link
Copy Markdown
Member

Description

Mirror the existing backend permissions in the final version of the collector UI so that users don't run into permission errors when not using admin-level accounts.

We don't support creating grants for individual fleets yet, but the permission checks should accomodate that in the future.

Motivation and Context

The backend already checks permissions where applicable, so this check merely adds suppressing unusable UI elements from users that would end up getting a hard error.

How Has This Been Tested?

Unit tests and manual testing.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (non-breaking change)
  • Breaking change (fix or feature that would cause existing functionality to change)

kroepke added 21 commits August 27, 2026 16:40
CollectorInstancesResource.stats() previously returned totals across
all fleets regardless of the caller's grants, per an in-code TODO
admitting the missing fleet-scoping. It now sums the grouped
instance/source counts over only the fleets the caller can read,
mirroring FleetResource.bulkStats().
Compute canCreateToken(fleet.id) once into isTokenCreationPermitted instead
of calling it twice for disabled and title.
Five small, independent fixes accumulated from task reviews on the
entity-scoped collectors permissions branch, batched into one commit:

- Strengthen CollectorInstancesResourceStatsTest's hidden fleet fixture
  (InstanceCount(7,7) -> InstanceCount(7,5)) so offlineInstances also
  discriminates a regression in the fleet-read permission filter.
- Centralize the repeated `as Permission` casts for COLLECTOR_PERMISSIONS
  at their definition in Permissions.ts instead of at each call site in
  useCollectorPermissions.ts.
- Add a FleetDetail test case proving canDelete controls the Delete menu
  item independently of canEdit.
- Use ?? instead of || for the availableFleets default in
  ReassignFleetModal, per this plan's constraints.
- Add positive test cases for canEditConfig and canReadActivities in
  useCollectorPermissions.test.ts, verifying the permission-string
  mapping against the real isPermitted path.
…permissions

Final review fix wave for the collector permissions branch:

- Onboarding wizard (FirstOnboarding) called createFleet/createSource/
  createEnrollmentToken with no permission check anywhere in the file. Since
  this branch scoped total_instances to readable fleets, a Collectors Reader
  with no fleet grants now sees 0 instances and gets dropped into the wizard,
  whose first write 403s and takes down the whole page. CollectorsOverview
  now only enters the wizard when canCreateFleet is true; FleetChoice (shared
  with DeployTab) hides its "Create new fleet" button without the
  permission, and FirstOnboarding's autoChoice() no longer silently picks
  create-new for a user who can't create.
- That same FleetChoice gate also closes DeployTab's dead-end "Create new
  fleet" button, which navigated to a fleet-creation modal that
  CollectorsFleets already refuses to open without the permission.
- Removed EnrollmentTokenResourcePermissionsTest.java: its only assertion
  (token permissions != FLEET_INSTANCE_ASSIGN) is already guaranteed by the
  literal permission strings pinned in CollectorsPermissionsTest, so it
  doesn't actually cover the token/instance-assign rewiring its name claims.
- Dropped a redundant `as Permission` cast in collectors/bindings.ts now
  that the assertion lives at the definition site in Permissions.ts.
…en reassigning

The reassign modal filtered candidate fleets on two conditions -- excluding
the instance's current fleet, and requiring assign permission on the target --
but rendered a single empty state blaming permissions for both.

With one fleet in the system the instance already lives in it, so a fully
permitted user was told they lacked permission.
…OKEN_*

Disambiguates enrollment token permissions from the fleet and source
constants they sit alongside. Permission string values are unchanged, so
there is no behaviour change and no migration impact.
… token permissions

The Deployment page hosts two independently useful tabs, so gating it as a
whole was wrong: Deploy needs enrollment token create, while the token list
needs enrollment token read.

Each tab and panel is now gated on its own permission, the selected tab
defaults to whichever survives, and the nav entry and page are shown only
when at least one tab would render. A user with neither permission -- the
shipped Collectors Reader role -- no longer sees a Deployment tab, and a
deep link redirects to the Collectors overview.

The page-level guard is deliberately expressed as the union of the two tab
conditions rather than a shared helper, so it cannot drift out of step with
them. useCanAccessDeployment mirrors it for the nav entry, which cannot
destructure the hook.

Also migrates DeployTab's hand-rolled permissions stub to the shared
mockCollectorPermissions helper, which the type checker flagged once the
hook grew two members.
…tream

Collector self-logs are routed to a built-in stream, so the 'View System
Logs' links open a search scoped to it. A user without streams:read on that
stream landed on the 'Missing Stream Permissions' page -- we know the stream
id up front, so the link should not be offered at all.

Gates the three call sites: the instance row action, the instance detail
drawer's Logs row (hidden whole, since a label with no link is useless), and
the onboarding log preview's 'Open in search' action, whose searchUrl prop
is now optional. The preview itself is left alone; only the link is gated.

Note this is a STREAM permission, not a collector one -- the check lives on
useCollectorPermissions anyway so the feature has a single permission
surface, and so the shared test mock keeps existing suites green.
… fleets

The no-fleets empty state offered 'Create Fleet' and a link to the Deployment
page. For a user without create permission -- the shipped Collectors Reader
role -- both are dead ends, and the Deployment link now redirects away.

Such a user now sees the same 'No Fleets Yet' heading with a single line
pointing them at an administrator, instead of instructions for steps they
cannot perform.
@kroepke
kroepke requested review from a team August 28, 2026 12:11
@linuspahl linuspahl self-assigned this Aug 28, 2026

@linuspahl linuspahl left a comment

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.

Not sure if it fits into the scope of this PR, but when I setup a collector as a collectors manager I get a permissions error on the collector onboarding page.

The collectors manager is not allowed to call the endpoint /api/onboarding/finish.

@linuspahl linuspahl left a comment

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.

The frontend changes are solid. I tested these as a Collectors Manager and Collectors Reader. Added a few comments based on findings from my testing. Imo these can be addressed in a follow-up PR.

const useCanAccessDeployment = () => {
const { canDeployCollectors, canViewEnrollmentTokens } = useCollectorPermissions();

return canDeployCollectors || canViewEnrollmentTokens;

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.

There is a "Deploy a new Collector" button on the fleet details page, which is currently displayed for users with the role Collectors Reader. Imo it makes sense to add a permissions check for it as well.

<BulkActionsDropdown>
<DeleteMenuItem onSelect={() => setShowConfirm(true)} />
</BulkActionsDropdown>
)}

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.

Let's not display the bulk select option on the table if the user does not have the required permissions to perform bulk actions?

Image

@thll thll left a comment

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.

Backend changes look good. I didn't test it, though.

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