Fix get policy by id endpoint and unify access in UI#45048
Conversation
![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.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #45048 +/- ##
==========================================
+ Coverage 66.77% 66.81% +0.03%
==========================================
Files 2718 2722 +4
Lines 218816 218987 +171
Branches 10622 10754 +132
==========================================
+ Hits 146123 146307 +184
+ Misses 59530 59517 -13
Partials 13163 13163
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.
Pull request overview
This PR fixes policy detail fetching for inherited/global policies in team context by switching the UI to a single policy “get by id” API and hardening the backend endpoint to safely return team-scoped policies.
Changes:
- Added a new
GetPolicyByIDservice method/endpoint implementation that re-authorizes on the fetched policy to prevent cross-team reads. - Refactored team policy endpoints to use a shared automation-population helper and renamed service interface methods (
Get*ByIDQueries→Get*ByID). - Updated several policy-related UI pages to always load policy details via
/fleet/policies/:id(global endpoint), fixing the inherited-policy 404.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/team_policies.go | Refactors automation population and renames team policy “get by id” service method. |
| server/service/team_policies_test.go | Updates auth test to new method name; adds coverage ensuring automation fields are populated for team policies. |
| server/service/policies.go | Introduces unified GetPolicyByID endpoint/service method with re-authorization after fetch. |
| server/service/integration_core_test.go | Adds integration test for cross-team access control on GET /fleet/policies/:id. |
| server/service/global_policies.go | Removes legacy global-policy “get by id” implementation in favor of the unified one. |
| server/service/global_policies_test.go | Updates auth test to call GetPolicyByID; adds regression test for cross-team reads. |
| server/mock/service/service_mock.go | Updates mock service interface to renamed GetPolicyByID/GetTeamPolicyByID methods. |
| server/fleet/service.go | Updates fleet.Service interface method names. |
| frontend/services/entities/policies.ts | Adds a unified “policy by id” API client wrapper. |
| frontend/pages/policies/ManagePoliciesPage/components/PoliciesTable/PoliciesTableConfig.tsx | Preserves team context in links for inherited policies. |
| frontend/pages/policies/live/LivePolicyPage/LivePolicyPage.tsx | Switches policy load to unified policies API. |
| frontend/pages/policies/edit/EditPolicyPage.tsx | Switches policy load to unified policies API (removes prior endpoint branching). |
| frontend/pages/policies/details/PolicyDetailsPage/PolicyDetailsPage.tsx | Switches policy load to unified policies API; drives displayed fleet/team from returned policy ownership. |
| frontend/pages/hosts/ManageHostsPage/ManageHostsPage.tsx | Switches policy load to unified policies API. |
| changes/fix-get-policy-id-endpoint-and-unify-access-in-ui | Adds a user-visible changelog entry for the endpoint/UI behavior changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR fixes team-observer 404s for inherited policies by unifying policy retrieval under GET /api/latest/fleet/policies/:id. Backend adds Service.GetPolicyByID and getPolicyByIDEndpoint, centralizes team-policy automation enrichment via populateAutomationsForTeamPolicy, and renames service/mocks. Frontend adds policiesAPI.load and updates pages (PolicyDetails/Edit/Live/ManageHosts/PoliciesTable) to use it and derive fleet display from storedPolicy.team_id. Tests added/updated cover cross-team authorization, integration access checks, and automation population. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
server/service/global_policies_test.go (1)
178-208: 💤 Low valueMissing coverage: "team admin of the policy's own team can read it"
TestGetPolicyByIDCrossTeamAuthchecks team observer of the policy's team (case 3), but not team admin of the policy's team. Adding this case gives full coverage of the positive-access surface for team-scoped users.🧪 Suggested additional test case
{ "team observer of the policy's team can read it", &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: policyTeamID}, Role: fleet.RoleObserver}}}, false, }, + { + "team admin of the policy's team can read it", + &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: policyTeamID}, Role: fleet.RoleAdmin}}}, + false, + }, { "team observer of a different team cannot read it",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/service/global_policies_test.go` around lines 178 - 208, Add a missing positive test case to the TestGetPolicyByIDCrossTeamAuth table: include a case where the user is a team admin of the policy's own team (use Teams: []fleet.UserTeam{{Team: fleet.Team{ID: policyTeamID}, Role: fleet.RoleAdmin}}) and set shouldFailRead to false so the test verifies team admins on the policy's team can read the policy; place this alongside the existing cases in the testCases slice.server/service/team_policies_test.go (1)
243-287: 💤 Low value
setupDSmock functions capture the outert, not the sub-test'st.The
require.Equal(t, ...)assertions inside the DS mock closures (e.g.GetSoftwareInstallerMetadataByIDFunc,ScriptFunc) use thetfromTestTeamPolicyAutomationsPopulated, not from the individual sub-test. When arequirefails in a sub-test's goroutine using the outert, the failure is attributed to the parent test rather than to the specific sub-test, making it harder to diagnose which sub-test triggered the assertion failure.Consider threading the sub-test's
tthroughsetupDS(e.g., as a parameter) or moving therequire.Equalcalls intorequireAutomationsPopulated(which already accepts the correctt).♻️ Suggested approach
-setupDS := func() *mock.Store { +setupDS := func(t *testing.T) *mock.Store { ds := new(mock.Store) ... ds.GetSoftwareInstallerMetadataByIDFunc = func(ctx context.Context, id uint) (*fleet.SoftwareInstaller, error) { require.Equal(t, softwareInstallerID, id) ... } ... } t.Run("NewTeamPolicy", func(t *testing.T) { - ds := setupDS() + ds := setupDS(t) ... })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/service/team_policies_test.go` around lines 243 - 287, The mock closures in setupDS (e.g., GetSoftwareInstallerMetadataByIDFunc, ScriptFunc, GetSoftwareInstallerMetadataByTeamAndTitleIDFunc) capture the outer test variable t, so assertion failures are attributed to the parent test instead of the sub-test; fix by making setupDS accept a *testing.T parameter (or remove those require.Equal checks from setupDS and move them into requireAutomationsPopulated which already takes the sub-test t) and use that sub-test t inside the mock functions (or perform the equality assertions inside requireAutomationsPopulated), ensuring failures are reported against the correct sub-test.frontend/pages/policies/details/PolicyDetailsPage/PolicyDetailsPage.tsx (1)
115-119: ⚡ Quick win
teamIdForApiin the"policy"query key is now a stale dependency.The fetch function unconditionally calls
policiesAPI.load(policyId)and no longer usesteamIdForApi. Keeping it in the key causes React Query to treat the same policy as a separate cache entry for each team context, triggering redundant network requests whenever the user's selected team changes (e.g., navigating from an inherited-policy view on Team A's list to the same policy's detail page).React Query's own guidance is to "treat the query key like a dependency array" — only include values that actually affect what the fetch returns.
♻️ Proposed fix
- ["policy", policyId, teamIdForApi], + ["policy", policyId], () => policiesAPI.load(policyId as number),
teamIdForApican remain inbackToPoliciesPathand the "Run"/"Edit" button links where it's still relevant.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/pages/policies/details/PolicyDetailsPage/PolicyDetailsPage.tsx` around lines 115 - 119, The query key for the policy useQuery includes teamIdForApi but the fetcher (policiesAPI.load in the call that builds ["policy", policyId, teamIdForApi]) does not use it, causing stale cache entries per team; remove teamIdForApi from the query key (leave it in backToPoliciesPath and button link generation where needed) so the key becomes ["policy", policyId] and keep the enabled: isRouteOk && !!policyId logic unchanged; update any references to the old key usage (e.g., places that invalidate or read the query) to use the new ["policy", policyId] key.frontend/pages/policies/edit/EditPolicyPage.tsx (1)
149-152: ⚡ Quick winSame stale
teamIdForApiin the"policy"query key.Identical to
PolicyDetailsPage.tsx: the fetch is nowpoliciesAPI.load(policyId)but the key still includesteamIdForApi, splitting the cache unnecessarily across team-context changes.♻️ Proposed fix
- ["policy", policyId, teamIdForApi], + ["policy", policyId], () => policiesAPI.load(policyId as number),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/pages/policies/edit/EditPolicyPage.tsx` around lines 149 - 152, The query key for useQuery in EditPolicyPage.tsx currently includes teamIdForApi but the fetcher is policiesAPI.load(policyId) (no team), which causes unnecessary cache splits; update the key in the useQuery call from ["policy", policyId, teamIdForApi] to ["policy", policyId] (or alternatively pass teamIdForApi into the fetcher and keep the key consistent) so the cache key matches the fetcher—adjust the useQuery invocation that references ["policy", policyId, teamIdForApi"] accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@frontend/pages/policies/details/PolicyDetailsPage/PolicyDetailsPage.tsx`:
- Around line 115-119: The query key for the policy useQuery includes
teamIdForApi but the fetcher (policiesAPI.load in the call that builds
["policy", policyId, teamIdForApi]) does not use it, causing stale cache entries
per team; remove teamIdForApi from the query key (leave it in backToPoliciesPath
and button link generation where needed) so the key becomes ["policy", policyId]
and keep the enabled: isRouteOk && !!policyId logic unchanged; update any
references to the old key usage (e.g., places that invalidate or read the query)
to use the new ["policy", policyId] key.
In `@frontend/pages/policies/edit/EditPolicyPage.tsx`:
- Around line 149-152: The query key for useQuery in EditPolicyPage.tsx
currently includes teamIdForApi but the fetcher is policiesAPI.load(policyId)
(no team), which causes unnecessary cache splits; update the key in the useQuery
call from ["policy", policyId, teamIdForApi] to ["policy", policyId] (or
alternatively pass teamIdForApi into the fetcher and keep the key consistent) so
the cache key matches the fetcher—adjust the useQuery invocation that references
["policy", policyId, teamIdForApi"] accordingly.
In `@server/service/global_policies_test.go`:
- Around line 178-208: Add a missing positive test case to the
TestGetPolicyByIDCrossTeamAuth table: include a case where the user is a team
admin of the policy's own team (use Teams: []fleet.UserTeam{{Team:
fleet.Team{ID: policyTeamID}, Role: fleet.RoleAdmin}}) and set shouldFailRead to
false so the test verifies team admins on the policy's team can read the policy;
place this alongside the existing cases in the testCases slice.
In `@server/service/team_policies_test.go`:
- Around line 243-287: The mock closures in setupDS (e.g.,
GetSoftwareInstallerMetadataByIDFunc, ScriptFunc,
GetSoftwareInstallerMetadataByTeamAndTitleIDFunc) capture the outer test
variable t, so assertion failures are attributed to the parent test instead of
the sub-test; fix by making setupDS accept a *testing.T parameter (or remove
those require.Equal checks from setupDS and move them into
requireAutomationsPopulated which already takes the sub-test t) and use that
sub-test t inside the mock functions (or perform the equality assertions inside
requireAutomationsPopulated), ensuring failures are reported against the correct
sub-test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d9c1e5a9-3006-4976-808c-f65a06aca466
📒 Files selected for processing (15)
changes/fix-get-policy-id-endpoint-and-unify-access-in-uifrontend/pages/hosts/ManageHostsPage/ManageHostsPage.tsxfrontend/pages/policies/ManagePoliciesPage/components/PoliciesTable/PoliciesTableConfig.tsxfrontend/pages/policies/details/PolicyDetailsPage/PolicyDetailsPage.tsxfrontend/pages/policies/edit/EditPolicyPage.tsxfrontend/pages/policies/live/LivePolicyPage/LivePolicyPage.tsxfrontend/services/entities/policies.tsserver/fleet/service.goserver/mock/service/service_mock.goserver/service/global_policies.goserver/service/global_policies_test.goserver/service/integration_core_test.goserver/service/policies.goserver/service/team_policies.goserver/service/team_policies_test.go
💤 Files with no reviewable changes (1)
- server/service/global_policies.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/service/global_policies_test.go (1)
162-226: ⚡ Quick winPrevious feedback addressed; consider adding team admin test case for completeness.
The three new test functions (
TestGetPolicyByIDCrossTeamAuth,TestGetPolicyByIDGlobalPolicyAuth,TestGetPolicyByIDNoTeamPolicyAuth) comprehensively address the earlier request for global and no-team policy test coverage.Minor: The cross-team test includes positive cases for team observer (line 194) and team gitops (line 199) on the policy's team, but doesn't have a corresponding positive case for "team admin of the policy's team can read it". Adding this would make the test coverage more symmetric, though team admins are likely covered in integration tests.
➕ Optional: Add team admin positive test case
{ "team gitops of the policy's team can read it", &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: policyTeamID}, Role: fleet.RoleGitOps}}}, false, }, + { + "team admin of the policy's team can read it", + &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: policyTeamID}, Role: fleet.RoleAdmin}}}, + false, + }, { "team observer of a different team cannot read it",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/service/global_policies_test.go` around lines 162 - 226, Add a positive assertion that a team admin on the policy's team can read the policy in the TestGetPolicyByIDCrossTeamAuth test: create or reuse a team-admin identity for the policy's team (matching how other roles are constructed in that test), perform the same GetPolicyByID request used for team observer/gitops checks, and assert a successful response/expected policy body; update setup/teardown or helper calls used in TestGetPolicyByIDCrossTeamAuth to include this team-admin case for symmetry with the other role checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@server/service/global_policies_test.go`:
- Around line 162-226: Add a positive assertion that a team admin on the
policy's team can read the policy in the TestGetPolicyByIDCrossTeamAuth test:
create or reuse a team-admin identity for the policy's team (matching how other
roles are constructed in that test), perform the same GetPolicyByID request used
for team observer/gitops checks, and assert a successful response/expected
policy body; update setup/teardown or helper calls used in
TestGetPolicyByIDCrossTeamAuth to include this team-admin case for symmetry with
the other role checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a368ca44-bccb-4c2e-b7fd-3786ee3fce89
📒 Files selected for processing (4)
changes/fix-get-policy-id-endpoint-and-unify-access-in-uifrontend/pages/policies/details/PolicyDetailsPage/PolicyDetailsPage.tsxserver/service/global_policies_test.goserver/service/policies.go
✅ Files skipped from review due to trivial changes (1)
- changes/fix-get-policy-id-endpoint-and-unify-access-in-ui
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/pages/policies/details/PolicyDetailsPage/PolicyDetailsPage.tsx
Related issue: Resolves #44949.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
For unreleased bug fixes in a release candidate, one of:
Summary by CodeRabbit
Bug Fixes
New Features
Tests