Self service categories endpoints + activities - #46593
Conversation
…loader, emoji literals in tests - LegacySoftwareCategoryNames doc comment matches migration branch - GetSoftwareCategoryNameToIDMap adds plain-name entries pointing to the same id, so callers can look up either form - testing_utils mysql cli uses --default-character-set=utf8mb4 to preserve emojis in schema.sql seed data - replace fleet.TranslateLegacySoftwareCategoryNames(...) wrappers in tests with literal emoji slices; inline tc.categories / tc.fmaDefaultCategories fixtures - update stale mock signatures (teamID parameter) in fleetctl gitops, vpp, maintained_apps test files - gofmt and modernize cleanups in server/service/categories.go
5 expectedPayload.Categories / titleResponse.Categories assertions in TestSoftwareInstallerUploadDownloadAndDelete, TestInHouseAppCRUD, and TestMaintainedApps need to compare to the stored emoji-prefixed form rather than the plain input. The FMA manifest-report assertion at dbAppResponse stays plain because it reports back the manifest's DefaultCategories, not the DB-stored translation.
…es' into 46391-categories-endpoints # Conflicts: # server/datastore/mysql/in_house_apps_test.go # server/datastore/mysql/schema.sql # server/datastore/mysql/software_test.go # server/service/integration_enterprise_test.go
- Request/response: team_id (canonical) with renameto:"fleet_id" on GET query, POST body, and the 3 activity structs. Matches the convention used by addAppStoreApp and ActivityTypeAddedSoftware. - ListSoftwareCategories now takes *uint and validates in the service layer so the missing-fleet_id error matches POST exactly: 422 ValidationFailed name=fleet_id reason="fleet_id is required". - Activities now emit both team_id+fleet_id and team_name+fleet_name on the wire; integration test JSON assertions updated to match.
Adding a Fleet-maintained app to the library now (re-)creates any default categories the admin previously deleted so the FMA still surfaces under its canonical (emoji) category. Custom packages, VPP apps, and in-house apps continue to reject unknown categories. - ee/server/service/maintained_apps.go: translate FMA manifest categories to canonical names, then ensure each one exists via the new createMissingCategories helper before resolving CategoryIDs. - createMissingCategories uses svc.NewSoftwareCategory so each insert emits its standard added_self_service_category activity. Case folding via strings.EqualFold mirrors the DB's utf8mb4_unicode_ci uniqueness. - Add TestAddFleetMaintainedAppCreatesMissingCategories integration test using startFMAServers; flip the existing GET-no-fleet_id assertion to 422 to match the new uniform error wording.
This reverts commit 762f556. Per updated product direction, FMA installs should NOT create categories that don't already exist on the fleet — falling back to the previous behavior of silently dropping unknown categories from the FMA's manifest. The bundled GET-no-fleet_id 400→422 test-assertion fix is kept since the 422 behavior was introduced by the earlier renameto commit and is still in place.
- TruncateTables now deletes software_categories rows where team_id != 0 so per-team defaults seeded by NewTeam don't collide with the next subtest's NewTeam (the schema-seeded team_id=0 rows are preserved). - testSoftwareCategoryCRUD counts now include the 6 seeded defaults. - Restore the original "filter out categories that don't exist" comment in AddFleetMaintainedApp.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
@claude review once |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feat/39018-self-service-categories #46593 +/- ##
=====================================================================
Coverage ? 66.81%
=====================================================================
Files ? 2807
Lines ? 223887
Branches ? 11346
=====================================================================
Hits ? 149598
Misses ? 60699
Partials ? 13590
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
No bugs found, but this is a substantial feature PR — new CRUD endpoints, authz policy changes, new activity types, schema change to scope software_categories per-team, and seeding logic on team creation — so it warrants a human review.
Extended reasoning...
Overview
This PR adds the self-service software categories feature: four new REST endpoints (GET/POST/PATCH/DELETE /api/_version_/fleet/software/self_service_categories), three new activity types, an authz object type (software_category) with corresponding rego rules + tests, and a schema change scoping software_categories per-team (previously global). It also seeds the six default category names on team creation, adds cascade-delete via teamRefs, threads team_id through GetSoftwareCategoryIDs/GetSoftwareCategoryNameToIDMap (touching VPP, FMA, software installer paths), and updates the test truncation helpers to preserve only the global (team_id=0) seed rows.
Security risks
The authz changes are the main scrutiny point: new software_category object type with global + team-role rules, plus a deliberate carve-out denying GitOps (with a TODO to revisit). The team-scoped object.team_id != 0 guards mean unassigned (team_id=0) categories require global roles, which matches the pattern for other team-scoped resources. The endpoints validate fleet_id presence, name length (255 runes), and trim whitespace; UpdateSoftwareCategory/DeleteSoftwareCategory correctly load the category before authz to scope by its team. SQL uses placeholders throughout. No obvious injection or auth bypass surface.
Level of scrutiny
High. This is production-critical code: authz policy changes, new public API surface, a schema migration impact (per-team categories with cascade), and behavioral change to several existing paths (FMA add, VPP batch associate, software installer update, gitops). The PR also targets a feature branch (feat/39018-self-service-categories), which softens the immediate blast radius, but the changes still need human eyes on the authz rules and the team-scoping invariants.
Other factors
Test coverage is reasonable — new policy_test cases cover all roles × team combinations, a new datastore CRUD test, and a full integration test exercising create/list/rename/delete/activities/cross-fleet uniqueness/cascade-on-team-delete. Codecov flags 68% patch coverage with the bulk of misses in the new service files, mostly error paths. The bug-hunting system found no issues. A human reviewer should still confirm the GitOps-denied TODO is intentional for this PR and that the legacy global software_categories rows (team_id=0) interact correctly with all the changed call sites.
2 participants
Related issue: Resolves #46391
Checklist for submitter
If some of the following don't apply, delete the relevant line.
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Input data is properly validated,
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.Timeouts are implemented and retries are limited to avoid infinite loops
If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes
Testing
Added/updated automated tests
Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)
QA'd all new/changed functionality manually