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.
sgress454
changed the title
| allow { | ||
| object.type == "software_inventory" | ||
| subject.global_role == [admin, maintainer, technician, observer, observer_plus][_] | ||
| subject.global_role == [admin, maintainer, technician, observer, observer_plus, gitops][_] |
There was a problem hiding this comment.
the global role updates might be overkill, but they don't hurt and could be needed in the future 🤷
There was a problem hiding this comment.
Pull request overview
Expands GitOps role authorization so GitOps users can successfully call GET /api/latest/fleet/software/titles (and related “installable entity”/maintained app reads), addressing the 403 reported in #44696.
Changes:
- Allow
gitopstoreadsoftware_inventoryglobally and for owned teams inserver/authz/policy.rego. - Update/extend authz tests and integration tests to assert GitOps can list software titles but still cannot fetch a single title (due to
Host:listrequirement). - Add a changelog entry for the permission update.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/software_titles_test.go | Extends service-level auth tests to cover GitOps list vs. get-by-id behavior. |
| server/service/integration_enterprise_test.go | Updates end-to-end GitOps expectations: /software/titles now OK; /software/titles/:id still forbidden; adds team-scope assertions. |
| server/authz/policy.rego | Adds gitops to software_inventory, maintained_app, and installable_entity read allow-lists (plus team-scoped variants). |
| server/authz/policy_test.go | Updates policy tests for the new GitOps allowances and adds coverage for maintained app reads. |
| changes/44696-add-list-software-perms-for-gitops | Adds user-visible changelog entry describing the permission change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Global admins, maintainers, technician, observers, and observer_plus can read all software. | ||
| allow { | ||
| object.type == "software_inventory" | ||
| subject.global_role == [admin, maintainer, technician, observer, observer_plus][_] | ||
| subject.global_role == [admin, maintainer, technician, observer, observer_plus, gitops][_] | ||
| action == read |
| # Global admins and maintainers can read all maintained apps. | ||
| allow { | ||
| object.type == "maintained_app" | ||
| subject.global_role == [admin, maintainer][_] | ||
| subject.global_role == [admin, maintainer, gitops][_] | ||
| action == read |
| # Team admins and maintainers can read all maintained apps (no team constraint, unlike installers) | ||
| allow { | ||
| object.type == "maintained_app" | ||
| team_role(subject, subject.teams[_].id) == [admin, maintainer][_] | ||
| team_role(subject, subject.teams[_].id) == [admin, maintainer, gitops][_] | ||
| action == read |
| # Global admins, maintainers, and technicians can read any installable entity (software installer or VPP app) | ||
| allow { | ||
| object.type == "installable_entity" | ||
| subject.global_role == [admin, maintainer, technician][_] | ||
| subject.global_role == [admin, maintainer, technician, gitops][_] | ||
| action == read |
| @@ -0,0 +1 @@ | |||
| - Added permissions for GitOps user to list installable software No newline at end of file | |||
WalkthroughThis change adds the 🚥 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.
Actionable comments posted: 1
🧹 Nitpick comments (3)
server/authz/policy.rego (1)
743-797: ⚡ Quick winUpdate the surrounding role comments to include
gitops.These rules now grant
gitopsread access, but the adjacent comments still describe the old role sets. In this file, stale comments are easy to treat as the source of truth during auth reviews, so it’s worth updating them alongside the policy changes.🤖 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/authz/policy.rego` around lines 743 - 797, Update the nearby human-readable comments that describe role membership to include "gitops" wherever the policy rules grant gitops access (e.g., the comment blocks immediately above the allow rules that reference object.type == "software_inventory", object.type == "maintained_app", object.type == "installable_entity", and the team_role/subject.global_role lists). Ensure each comment that previously listed roles (admin, maintainer, technician, observer, observer_plus) now also mentions gitops so the prose matches the policy logic using subject.global_role and team_role.server/service/integration_enterprise_test.go (1)
7479-7484: ⚡ Quick winTeam-scoped gitops boundary cases are well-covered — one untested case worth considering.
The three assertions correctly cover the permission matrix for team-scoped gitops:
Scenario Expected team_id= owned team200 ✓ No team_id(global)403 ✓ team_id= unowned team403 ✓ One case not currently exercised here: global gitops user listing software titles scoped to a specific
team_id. The Rego policy grants global-scope gitops unrestricted read onsoftware_inventory, so this should return 200. Adding a check in the global gitops block (hunk 1) would confirm that the global-scope grant also covers team-filtered requests, preventing a future regression if someone inadvertently restricts it.➕ Suggested addition in the global gitops test block (after line 6925)
// Listing software titles is allowed for gitops so they can reconcile software state. s.DoJSON("GET", "/api/latest/fleet/software/titles", listSoftwareTitlesRequest{}, http.StatusOK, &listSoftwareTitlesResponse{}) + // Global gitops can also list software titles scoped to a specific team. + s.DoJSON("GET", "/api/latest/fleet/software/titles", listSoftwareTitlesRequest{}, http.StatusOK, &listSoftwareTitlesResponse{}, "team_id", fmt.Sprint(t1.ID))🤖 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/integration_enterprise_test.go` around lines 7479 - 7484, Add a test in the global gitops block to assert that a global-scope gitops user can list software titles when filtering by a specific team_id: call s.DoJSON("GET", "/api/latest/fleet/software/titles", listSoftwareTitlesRequest{}, http.StatusOK, &listSoftwareTitlesResponse{}, "team_id", fmt.Sprint(t1.ID)) (use the same t1 from the existing team tests), placing it near the other global gitops assertions so we confirm that the global grant on software_inventory allows team-scoped requests; reuse the same request/response types (listSoftwareTitlesRequest, listSoftwareTitlesResponse) and the s.DoJSON helper to match the style of the surrounding tests.server/authz/policy_test.go (1)
775-793: ⚡ Quick winAdd explicit team-scoped
MaintainedAppcases to harden boundary coverage.This test currently validates role behavior against only a zero-value object. If maintained apps are team-scoped in authz, adding team1/team2 fixtures (allow same team, deny other team) would better protect against cross-team regressions.
🤖 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/authz/policy_test.go` around lines 775 - 793, The test only exercises a zero-value MaintainedApp; create explicit team-scoped fixtures (e.g., maintainedAppTeam1 and maintainedAppTeam2 with their TeamID/Team or equivalent set to test.Team1.ID and test.Team2.ID) and add authTestCase entries calling runTestCases to assert that team-scoped users (test.UserTeamAdminTeam1, test.UserTeamMaintainerTeam1, test.UserTeamGitOpsTeam1) are allowed to read maintainedAppTeam1 but denied on maintainedAppTeam2, and conversely that team2 users are denied on maintainedAppTeam1; keep the existing zero-value cases and reuse the same action/read expectations for consistency.
🤖 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.
Inline comments:
In `@changes/44696-add-list-software-perms-for-gitops`:
- Line 1: Update the changelog entry wording: replace the phrase "GitOps can
list installable software" with a clearer description such as "GitOps can read
software titles during GitOps sync" (or similar wording) so the release note
reflects that the fix allows GitOps to read software titles during sync; ensure
the updated sentence appears in the same changelog/PR description and any
related release-note metadata or header (e.g., the entry that currently contains
"list installable software") is updated to match.
---
Nitpick comments:
In `@server/authz/policy_test.go`:
- Around line 775-793: The test only exercises a zero-value MaintainedApp;
create explicit team-scoped fixtures (e.g., maintainedAppTeam1 and
maintainedAppTeam2 with their TeamID/Team or equivalent set to test.Team1.ID and
test.Team2.ID) and add authTestCase entries calling runTestCases to assert that
team-scoped users (test.UserTeamAdminTeam1, test.UserTeamMaintainerTeam1,
test.UserTeamGitOpsTeam1) are allowed to read maintainedAppTeam1 but denied on
maintainedAppTeam2, and conversely that team2 users are denied on
maintainedAppTeam1; keep the existing zero-value cases and reuse the same
action/read expectations for consistency.
In `@server/authz/policy.rego`:
- Around line 743-797: Update the nearby human-readable comments that describe
role membership to include "gitops" wherever the policy rules grant gitops
access (e.g., the comment blocks immediately above the allow rules that
reference object.type == "software_inventory", object.type == "maintained_app",
object.type == "installable_entity", and the team_role/subject.global_role
lists). Ensure each comment that previously listed roles (admin, maintainer,
technician, observer, observer_plus) now also mentions gitops so the prose
matches the policy logic using subject.global_role and team_role.
In `@server/service/integration_enterprise_test.go`:
- Around line 7479-7484: Add a test in the global gitops block to assert that a
global-scope gitops user can list software titles when filtering by a specific
team_id: call s.DoJSON("GET", "/api/latest/fleet/software/titles",
listSoftwareTitlesRequest{}, http.StatusOK, &listSoftwareTitlesResponse{},
"team_id", fmt.Sprint(t1.ID)) (use the same t1 from the existing team tests),
placing it near the other global gitops assertions so we confirm that the global
grant on software_inventory allows team-scoped requests; reuse the same
request/response types (listSoftwareTitlesRequest, listSoftwareTitlesResponse)
and the s.DoJSON helper to match the style of the surrounding tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a9b1e2d0-3ac4-4aa3-9008-d8653b5b2305
📒 Files selected for processing (5)
changes/44696-add-list-software-perms-for-gitopsserver/authz/policy.regoserver/authz/policy_test.goserver/service/integration_enterprise_test.goserver/service/software_titles_test.go
| @@ -0,0 +1 @@ | |||
| - Added permissions for GitOps user to list installable software No newline at end of file | |||
There was a problem hiding this comment.
Clarify the changelog entry.
This says GitOps can "list installable software", but the user-facing fix here is about GitOps being able to read software titles during GitOps sync. Tightening the wording will keep the release notes aligned with the actual fix.
Suggested wording
-- Added permissions for GitOps user to list installable software
+- Added permissions for GitOps users to read software titles during GitOps sync📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - Added permissions for GitOps user to list installable software | |
| - Added permissions for GitOps users to read software titles during GitOps sync |
🤖 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 `@changes/44696-add-list-software-perms-for-gitops` at line 1, Update the
changelog entry wording: replace the phrase "GitOps can list installable
software" with a clearer description such as "GitOps can read software titles
during GitOps sync" (or similar wording) so the release note reflects that the
fix allows GitOps to read software titles during sync; ensure the updated
sentence appears in the same changelog/PR description and any related
release-note metadata or header (e.g., the entry that currently contains "list
installable software") is updated to match.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #44721 +/- ##
==========================================
- Coverage 66.68% 66.67% -0.01%
==========================================
Files 2651 2651
Lines 213531 213573 +42
Branches 9613 9613
==========================================
+ Hits 142395 142406 +11
- Misses 58172 58193 +21
- Partials 12964 12974 +10
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:
|
Related issue: Resolves #44696
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
fleetctl gitopsfailed for a gitops user with a 403)fleetctl gitopssynced successfullyFor unreleased bug fixes in a release candidate, one of:
Summary by CodeRabbit