Allow conditional downloads across fleets - #43679
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #43679 +/- ##
==========================================
+ Coverage 66.91% 66.94% +0.02%
==========================================
Files 2600 2600
Lines 208710 208718 +8
Branches 9339 9339
==========================================
+ Hits 139666 139716 +50
+ Misses 56327 56297 -30
+ Partials 12717 12705 -12
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:
|
0e31e56 to
dd651d9
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ee/server/service/software_installers.go (1)
2690-2712:⚠️ Potential issue | 🟡 MinorCross-team 304 cache-hit bypasses the authz check used by the sibling cross-team copy path.
When the cross-team lookup succeeds and the server returns 304,
fillSoftwareInstallerPayloadFromExistingcopies metadata (filename, version, title, bundle_identifier, etc.) from an installer owned by a potentially different team, with no check that the current viewer has write access to that team. Contrast this with the pre-existing cross-team copy path at Lines 2559–2595, which explicitly walks each team and skips ones the user isn't authorized for (svc.authz.Authorize(userctx, &fleet.SoftwareInstaller{TeamID: tmIDPtr}, fleet.ActionWrite)).In practice this is narrow — the bytes are the same (keyed by SHA-256 / StorageID), and a user reaching this point has already authorized write on the current team — but the metadata being copied (e.g.,
Title,BundleIdentifier,Filename) can leak details of an installer on a team the user can't see. Consider either (a) restricting the cross-team fallback lookup at Line 2663 to installers the user can access, or (b) only adoptingexistingForCachefor same-team matches and using cross-team solely to obtain an ETag.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/server/service/software_installers.go` around lines 2690 - 2712, When handling the 304 + existingForCache cache-hit path, enforce the same authorization check used in the cross-team copy path: call svc.authz.Authorize(userctx, &fleet.SoftwareInstaller{TeamID: existingForCache.TeamID}, fleet.ActionWrite) before invoking fillSoftwareInstallerPayloadFromExisting or copying metadata from existingForCache; if the authorization fails, do not copy metadata (either fall back to re-downloading via retryDownload or only adopt the ETag/StorageID for caching but leave Title/Filename/BundleIdentifier untouched). Update the branch that currently sets p.MaintainedApp.SHA256, installer.HTTPETag, and cacheHit to occur only after the authz check (or constrain to same-team matches) so metadata from other teams is never leaked.
🧹 Nitpick comments (1)
server/datastore/mysql/software_installers_test.go (1)
4666-4676: Consider adding coverage for the multi-fleet cross-team fallback.The nil-team assertion here is correct but doesn't fully exercise the PR's stated objective (reusing an already-downloaded package when multiple fleets declare the same URL). Only
team1has an installer athttps://example.com/app/latest, so theORDER BY si.id DESC LIMIT 1path is trivially deterministic.Consider adding a case where two teams both have an installer with the same URL (but different
StorageIDs) and verifying that a nil-team lookup returns the most-recent active one. That would also guard against regressions in theORDER BY id DESC+is_active = 1interplay when teams share a URL — which is the core scenario this PR is intended to support.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/datastore/mysql/software_installers_test.go` around lines 4666 - 4676, Add a test that exercises the multi-fleet cross-team fallback for GetInstallerByTeamAndURL: create two teams (e.g., team1 and team2), insert active installers for both with identical URL "https://example.com/app/latest" but different StorageID values and different ids/timestamps, then call ds.GetInstallerByTeamAndURL(ctx, nil, "https://example.com/app/latest") and assert it returns the installer with the most-recent id (the expected active StorageID); ensure one installer can be inactive (is_active = 0) to validate the is_active filtering and the ORDER BY si.id DESC behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@ee/server/service/software_installers.go`:
- Around line 2690-2712: When handling the 304 + existingForCache cache-hit
path, enforce the same authorization check used in the cross-team copy path:
call svc.authz.Authorize(userctx, &fleet.SoftwareInstaller{TeamID:
existingForCache.TeamID}, fleet.ActionWrite) before invoking
fillSoftwareInstallerPayloadFromExisting or copying metadata from
existingForCache; if the authorization fails, do not copy metadata (either fall
back to re-downloading via retryDownload or only adopt the ETag/StorageID for
caching but leave Title/Filename/BundleIdentifier untouched). Update the branch
that currently sets p.MaintainedApp.SHA256, installer.HTTPETag, and cacheHit to
occur only after the authz check (or constrain to same-team matches) so metadata
from other teams is never leaked.
---
Nitpick comments:
In `@server/datastore/mysql/software_installers_test.go`:
- Around line 4666-4676: Add a test that exercises the multi-fleet cross-team
fallback for GetInstallerByTeamAndURL: create two teams (e.g., team1 and team2),
insert active installers for both with identical URL
"https://example.com/app/latest" but different StorageID values and different
ids/timestamps, then call ds.GetInstallerByTeamAndURL(ctx, nil,
"https://example.com/app/latest") and assert it returns the installer with the
most-recent id (the expected active StorageID); ensure one installer can be
inactive (is_active = 0) to validate the is_active filtering and the ORDER BY
si.id DESC behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 68205ec6-5872-4769-9af7-12a07f571b72
📒 Files selected for processing (9)
cmd/fleetctl/fleetctl/gitops_test.gocmd/fleetctl/fleetctl/testing_utils.gocmd/fleetctl/integrationtest/gitops/software_test.goee/server/service/software_installers.goserver/datastore/mysql/software_installers.goserver/datastore/mysql/software_installers_test.goserver/fleet/datastore.goserver/mock/datastore_mock.goserver/service/software_installers_test.go
![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.
There was a problem hiding this comment.
Pull request overview
Enables cross-team reuse of cached URL-based software installers during GitOps runs by allowing installer lookups to optionally fall back to “any team”, so the same URL isn’t re-downloaded separately per fleet/team.
Changes:
- Updated
GetInstallerByTeamAndURLto accept an optionalteamID(*uint) wherenilmeans “search across teams”. - Implemented cross-team fallback lookup in the conditional download path for software batch uploads.
- Updated MySQL datastore implementation and expanded tests/mocks to cover the new signature and nil-team behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/software_installers_test.go | Updates datastore mock signature used by service tests. |
| server/mock/datastore_mock.go | Updates mock datastore interface + method signature to teamID *uint. |
| server/fleet/datastore.go | Updates datastore interface contract and documentation for cross-team fallback behavior. |
| server/datastore/mysql/software_installers_test.go | Adjusts calls to pass *uint and adds coverage for teamID=nil fallback. |
| server/datastore/mysql/software_installers.go | Implements optional team filtering; supports teamID=nil to search all teams. |
| ee/server/service/software_installers.go | Adds same-team lookup first, then cross-team fallback for conditional downloads. |
| cmd/fleetctl/integrationtest/gitops/software_test.go | Updates GitOps integration test mocks for new signature. |
| cmd/fleetctl/fleetctl/testing_utils.go | Updates GitOps test mock setup for new signature. |
| cmd/fleetctl/fleetctl/gitops_test.go | Updates fleetctl GitOps test mocks for new signature. |
Comments suppressed due to low confidence (1)
ee/server/service/software_installers.go:2674
- The cross-team fallback (
GetInstallerByTeamAndURL(ctx, nil, ...)) can reuse an installer from another team without checking whether the requesting user has access to that source team. Unlike the earlier cross-team copy-by-hash logic in this same function (which authorizes per team), this could let a team-scoped user leverage cached bytes/metadata from a team they’re not permitted to access. Consider authorizing againstexisting.TeamID(including the nil/global case) before using its ETag/storage_id; if unauthorized, ignore the fallback and proceed with a normal download.
// First try same-team lookup, then fall back to any team.
existing, lookupErr := svc.ds.GetInstallerByTeamAndURL(ctx, &tmID, p.URL)
if lookupErr != nil {
svc.logger.WarnContext(ctx, "conditional download lookup failed, will download normally", "url", p.URL, "err", lookupErr)
} else if existing == nil {
// Cross-team fallback: another team may already have this URL cached.
existing, lookupErr = svc.ds.GetInstallerByTeamAndURL(ctx, nil, p.URL)
if lookupErr != nil {
svc.logger.WarnContext(ctx, "cross-team conditional download lookup failed, will download normally", "url", p.URL, "err", lookupErr)
}
}
if lookupErr == nil && existing != nil && existing.StorageID != "" &&
existing.HTTPETag != nil && *existing.HTTPETag != "" &&
existing.Extension != "ipa" && // skip conditional download for .ipa (multi-platform extraInstallers)
validETag(*existing.HTTPETag) { // re-validate before use as defense-in-depth
existingForCache = existing
ifNoneMatch = *existing.HTTPETag
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Related issue: Resolves #43417
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.done in Add
cacheoption for software packages to skip re-downloading unchanged content #42216Testing
fleetctl gitops. Verified that the first fleet downloaded the file, the second fleet used the cache, and both fleet showed the software installer in the UI.Summary by CodeRabbit