Improve fleet scope validation for software title lookups#48034
Conversation
sharon-fdm
force-pushed
the
fix-software-title-scope-validation
branch
from
d0ee47e to
193e553
Compare
![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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughIn 🚥 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 |
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
This PR tightens authorization behavior for SoftwareTitleByID so that providing a non-nil team_id/fleet_id pointer always triggers software-inventory scope authorization, including the previously-unchecked team_id=0 case.
Changes:
- Apply
AuthzSoftwareInventoryauthorization wheneverteamID != nil(not only when*teamID != 0). - Keep team existence validation only for non-zero team IDs.
- Add a unit test covering the
team_id=0authorization behavior.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| server/service/software_titles.go | Adjusts scope authorization to run for all non-nil team IDs, including 0, and retains existence checks for non-zero IDs. |
| server/service/software_titles_test.go | Adds a focused unit test for SoftwareTitleByID with team_id=0 behavior across team-scoped vs global admin users. |
| changes/software-title-scope-validation | User-visible changes entry (diff content excluded by policy). |
Files excluded by content exclusion policy (1)
- changes/software-title-scope-validation
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // This auth check ensures we return 403 if the user doesn't have access to the team | ||
| if err := svc.authz.Authorize(ctx, &fleet.AuthzSoftwareInventory{TeamID: teamID}, fleet.ActionRead); err != nil { | ||
| return nil, err |
| require.Error(t, err) | ||
| require.Contains(t, err.Error(), "forbidden") | ||
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/service/software_titles_test.go (1)
281-309: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAssert
TeamExistsis never called forteam_id=0.This test verifies auth outcomes, but Line 281’s always-true mock won’t catch regressions where
SoftwareTitleByIDstarts callingTeamExists(0)again.Suggested test hardening
func TestSoftwareTitleByIDTeamIDZero(t *testing.T) { ds := new(mock.Store) + teamExistsCalled := false ds.SoftwareTitleByIDFunc = func(ctx context.Context, id uint, teamID *uint, tmFilter fleet.TeamFilter) (*fleet.SoftwareTitle, error) { return &fleet.SoftwareTitle{BundleIdentifier: ptr.String("com.example.app")}, nil } - ds.TeamExistsFunc = func(ctx context.Context, teamID uint) (bool, error) { return true, nil } + ds.TeamExistsFunc = func(ctx context.Context, teamID uint) (bool, error) { + teamExistsCalled = true + return true, nil + } @@ _, err = svc.SoftwareTitleByID(adminCtx, 1, teamIDZero) require.NoError(t, err) + require.False(t, teamExistsCalled, "TeamExists should not be called when team_id=0") }🤖 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/software_titles_test.go` around lines 281 - 309, The test mocks TeamExistsFunc to always return true but does not verify that it is never called with team_id=0. Modify the ds.TeamExistsFunc mock to track invocations with their parameters, then add assertions after both SoftwareTitleByID calls to verify that TeamExistsFunc was either not called at all or never called with a team_id argument of 0. This will prevent regressions where SoftwareTitleByID might incorrectly start calling TeamExists(0) when handling the zero team_id case.
🤖 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/software_titles_test.go`:
- Around line 281-309: The test mocks TeamExistsFunc to always return true but
does not verify that it is never called with team_id=0. Modify the
ds.TeamExistsFunc mock to track invocations with their parameters, then add
assertions after both SoftwareTitleByID calls to verify that TeamExistsFunc was
either not called at all or never called with a team_id argument of 0. This will
prevent regressions where SoftwareTitleByID might incorrectly start calling
TeamExists(0) when handling the zero team_id case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2ba9f3a9-761b-4e5a-b56f-48189a5ad28e
📒 Files selected for processing (3)
changes/software-title-scope-validationserver/service/software_titles.goserver/service/software_titles_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #48034 +/- ##
========================================
Coverage 67.31% 67.31%
========================================
Files 3655 3655
Lines 231242 231244 +2
Branches 12224 12076 -148
========================================
+ Hits 155658 155660 +2
- Misses 61618 61620 +2
+ Partials 13966 13964 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Summary
Ensures that
SoftwareTitleByIDvalidates fleet scope for all non-nilteam_idvalues, including zero. Previously the scope check was only applied whenteam_id > 0.Reproduction
Added a unit test (
TestSoftwareTitleByIDTeamIDZero) that sets up a fleet-scoped user on fleet 1, then callsSoftwareTitleByIDwithteam_id=0. Before this change, the call succeeded. After, it correctly returns 403.Also confirmed that a global admin calling with
team_id=0still succeeds, and that all existingTestServiceSoftwareTitlesAuthsubtests continue to pass.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.Testing
Added/updated automated tests
QA'd all new/changed functionality manually
Summary by CodeRabbit
Improvements
0.Tests
0.