Patch when closed: policy and package api - #49871
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feat/39962-patch-when-closed #49871 +/- ##
===============================================================
Coverage ? 67.90%
===============================================================
Files ? 3891
Lines ? 248652
Branches ? 13022
===============================================================
Hits ? 168837
Misses ? 64593
Partials ? 15222
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:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
WalkthroughAdds Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 2
🧹 Nitpick comments (4)
server/service/team_policies_test.go (1)
290-301: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest doesn't verify what its name claims.
With
ContinuousAutomationsEnabledomitted (defaultsfalse), thepatch_when_closed && !continuous_automations_enabledguard innewTeamPolicyPayloadToPolicyPayloadfires first, so this test passes due toerrPatchWhenClosedRequiresContinuousAutomations(which happens to also contain the substring"patch_when_closed"), not because of any type-based rejection. AddContinuousAutomationsEnabled: trueto isolate the intended case.See consolidated comment — the source constraint this test intends to cover doesn't appear to exist anywhere in the codebase.
🤖 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 290 - 301, Update the test case “create rejects patch_when_closed on non-patch policy” to set ContinuousAutomationsEnabled: true in the NewTeamPolicyPayload, bypassing the unrelated continuous-automations guard and isolating the non-patch policy validation in newTeamPolicyPayloadToPolicyPayload.server/service/team_policies.go (2)
47-53: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing validation:
patch_when_closedshould requiretype == "patch".
newTeamPolicyPayloadToPolicyPayloadvalidates thatPatchWhenClosedrequiresContinuousAutomationsEnabled, but nothing here rejectspatch_when_closed: trueon a non-patch (dynamic) policy. If a caller setsTypeomitted/dynamic,PatchWhenClosed: true, andContinuousAutomationsEnabled: true, this passes silently and persists a meaninglesspatch_when_closed=trueon a dynamic policy.See consolidated comment for related gaps in
ApplyPolicySpecsand the masked test coverage inteam_policies_test.go.Also applies to: 107-112, 300-339, 717-728, 804-809
🤖 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.go` around lines 47 - 53, The team policy conversion and application paths, including newTeamPolicyPayloadToPolicyPayload and the referenced ApplyPolicySpecs flows, must reject PatchWhenClosed=true unless the policy Type is "patch". Add this validation alongside the existing ContinuousAutomationsEnabled check and preserve the existing behavior for patch policies and false values; update the affected team policy test coverage accordingly.
1-1: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNo validation anywhere restricts
patch_when_closedtotype == "patch"policies. All three sites below share this root cause: the API and GitOps paths both acceptpatch_when_closed: trueon a non-patch policy (silently storing meaningless state), and the one test intended to cover this doesn't actually exercise it.
server/service/team_policies.go#L300-339: add a check innewTeamPolicyPayloadToPolicyPayload(and the equivalent logic inmodifyPolicy) rejectingpatch_when_closed: truewhen the policy type isn'tpatch.server/datastore/mysql/policies.go#L1809-1821:ApplyPolicySpecsneeds the samespec.Type == fleet.PolicyTypePatchguard forspec.PatchWhenClosed, and should decide whether to match the API's explicit-rejection behavior for a conflictingcontinuous_automations_enabled: falseinstead of silently overriding it.server/service/team_policies_test.go#L290-301: once the guard exists, addContinuousAutomationsEnabled: trueto this subtest so the assertion actually isolates the type-rejection path instead of incidentally passing viaerrPatchWhenClosedRequiresContinuousAutomations.🤖 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.go` at line 1, The policy validation paths currently allow patch_when_closed on non-patch policies. Update newTeamPolicyPayloadToPolicyPayload, modifyPolicy, and ApplyPolicySpecs to reject patch_when_closed: true unless the policy type is fleet.PolicyTypePatch, while preserving the API’s explicit conflict handling for continuous_automations_enabled: false. In the relevant team policy test, set ContinuousAutomationsEnabled to true so it specifically verifies type rejection.server/datastore/mysql/policies.go (1)
1809-1821: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGitOps apply diverges from the API for
patch_when_closed/continuous_automations_enabledvalidation.Here, an explicit
continuous_automations_enabled: falsecombined withpatch_when_closed: trueis silently overridden totrue, whereasteam_policies.go'snewTeamPolicyPayloadToPolicyPayload/modifyPolicyexplicitly reject that same combination witherrPatchWhenClosedRequiresContinuousAutomations. There's also no check here thatpatch_when_closedis only meaningful forspec.Type == fleet.PolicyTypePatch.See consolidated comment for the related gap in
team_policies.goand the masked test inteam_policies_test.go.🤖 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/datastore/mysql/policies.go` around lines 1809 - 1821, The GitOps policy apply path must match API validation for patch-when-closed settings. In the flow containing the `spec.PatchWhenClosed` assignment before `tx.ExecContext`, reject an explicit false `spec.ContinuousAutomationsEnabled` when `spec.PatchWhenClosed` is true using `errPatchWhenClosedRequiresContinuousAutomations`, and validate that `PatchWhenClosed` is only set for `fleet.PolicyTypePatch`; do not silently override the caller’s value.
🤖 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 `@pkg/patch_policy/patch_policy.go`:
- Line 172: Update the openTemplate query in the patch policy code to replace
LEFT(p.path, LENGTH(a.path) + 1) with substr(p.path, 1, LENGTH(a.path) + 1),
preserving the existing predicate semantics. Update both corresponding
expected-query strings in the patch policy tests to match the generated
SQLite-compatible query.
In `@server/fleet/policies.go`:
- Around line 617-618: Update Service.ApplyPolicySpecs to check
license.IsPremium(ctx) whenever a policy spec requests patch_when_closed, and
reject the request for non-premium viewers using the existing error pattern. Add
a regression test covering a free-tier caller applying this option, while
preserving successful application for premium viewers.
---
Nitpick comments:
In `@server/datastore/mysql/policies.go`:
- Around line 1809-1821: The GitOps policy apply path must match API validation
for patch-when-closed settings. In the flow containing the
`spec.PatchWhenClosed` assignment before `tx.ExecContext`, reject an explicit
false `spec.ContinuousAutomationsEnabled` when `spec.PatchWhenClosed` is true
using `errPatchWhenClosedRequiresContinuousAutomations`, and validate that
`PatchWhenClosed` is only set for `fleet.PolicyTypePatch`; do not silently
override the caller’s value.
In `@server/service/team_policies_test.go`:
- Around line 290-301: Update the test case “create rejects patch_when_closed on
non-patch policy” to set ContinuousAutomationsEnabled: true in the
NewTeamPolicyPayload, bypassing the unrelated continuous-automations guard and
isolating the non-patch policy validation in
newTeamPolicyPayloadToPolicyPayload.
In `@server/service/team_policies.go`:
- Around line 47-53: The team policy conversion and application paths, including
newTeamPolicyPayloadToPolicyPayload and the referenced ApplyPolicySpecs flows,
must reject PatchWhenClosed=true unless the policy Type is "patch". Add this
validation alongside the existing ContinuousAutomationsEnabled check and
preserve the existing behavior for patch policies and false values; update the
affected team policy test coverage accordingly.
- Line 1: The policy validation paths currently allow patch_when_closed on
non-patch policies. Update newTeamPolicyPayloadToPolicyPayload, modifyPolicy,
and ApplyPolicySpecs to reject patch_when_closed: true unless the policy type is
fleet.PolicyTypePatch, while preserving the API’s explicit conflict handling for
continuous_automations_enabled: false. In the relevant team policy test, set
ContinuousAutomationsEnabled to true so it specifically verifies type rejection.
🪄 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 Plus
Run ID: 4678e792-28a4-4bd7-9415-ab3ed7ae4ff9
📒 Files selected for processing (19)
ee/maintained-apps/ingesters/homebrew/ingester_test.goee/server/service/software_installers.goee/server/service/software_installers_test.gopkg/patch_policy/patch_policy.gopkg/patch_policy/patch_policy_test.goserver/datastore/mysql/migrations/tables/20260721173820_PatchWhenClosed_test.goserver/datastore/mysql/policies.goserver/datastore/mysql/software_installers.goserver/datastore/mysql/software_installers_test.goserver/fleet/api_policies.goserver/fleet/datastore.goserver/fleet/policies.goserver/fleet/software_installer.goserver/mock/datastore_mock.goserver/service/global_policies.goserver/service/software_installers.goserver/service/software_titles.goserver/service/team_policies.goserver/service/team_policies_test.go
…install query Thread patch_when_closed through the team-policy create/update API and the update-package endpoint, and resolve the Fleet-managed "is app open" query at install time. Policy create/update: - Add patch_when_closed to the create request, payload mapping, and modify path. - Require type=patch (with a patch_software_title_id) when enabling it. - Auto-set continuous_automations_enabled when patch_when_closed is on, and reject an explicit continuous_automations_enabled=false in the same request. - Persist the column in the INSERT, UPDATE, ApplyPolicySpecs upsert, and the shared policy read column list; add PatchWhenClosed to PolicySpec. Update-package: - Add patch and patch_when_closed (FMA-only) to the request/payload. - Reconcile the title's patch policy: create/update on patch=true, delete on patch=false, keyed on patch_software_title_id. - Reject a direct pre_install_query edit while the managed query owns it. - Return patch_when_closed from GetPatchPolicy. GetSoftwareInstallDetails: - Join the triggering policy and return app_open_query as the pre-install condition only on the policy-automation path when patch_when_closed is set; self-service, manual, and setup-experience installs keep the user query.
Collapse the two-phase planPatchPolicy/applyPatchPolicyPlan (struct, managed method, and both functions) into one reconcilePatchPolicy call. It validates, rejects a managed pre-install query edit, and creates, updates, or deletes the title's patch policy. Keep the FMA-only guard as an early fail-fast at the call site, and run the reconcile at the "persist changes" boundary so a validation error can't leave a stray policy behind. Reconcile re-derives state from the datastore, so a retry after a partial failure converges without special conflict handling. Consolidate the plan/apply tests into TestReconcilePatchPolicy and cover the FMA-only rejection through the update-package flow.
… query in Go Update-package: a newly created patch policy defaults patch_when_closed on (Force patch is the explicit false), matching the documented default. An existing policy keeps its value. GetSoftwareInstallDetails: select the raw pre_install_query, app_open_query, and the triggering policy's patch_when_closed, and pick the managed query in Go instead of a SQL CASE. Policy-automation installs of a patch-when-closed policy get the managed app-open query; every other path keeps the user query.
…en_closed is enabled Enabling patch_when_closed hands the pre-install condition to the managed app open query, so the installer's user pre-install query is deleted and the title's pending installs are canceled. This applies from both the update-package endpoint and policy create/update.
The update-package endpoint no longer errors when patch_when_closed is set with patch off; it's ignored, since there's no policy to carry it. planPatchPolicy is now a pure resolver that takes the current patch policy as a parameter.
…abled=false with it Per the API docs PR: update-package rejects patch_when_closed set while patch isn't enabled; policy create/update reject an explicit continuous_automations_enabled=false alongside patch_when_closed=true.
Replace the LIKE-based process path match with an exact prefix check so a sibling app whose path shares the same prefix can't match.
…lers Guard the patch-policy fetch, planning, and pre-install query clear behind a Fleet-maintained-app check. A regular package edit no longer queries or touches a patch policy, which also fixes a nil pointer dereference when the datastore mock has no GetPatchPolicy stub.
osquery runs queries in SQLite, which has no LEFT function, so the generated open query would error on-device. substr(p.path, 1, LENGTH(a.path) + 1) matches the same leftmost characters.
The GitOps policy spec path already rejects the other premium-only fields for free-tier callers but let patch_when_closed through. Reject it the same way.
7ea13d3 to
06b8e4f
Compare
| } | ||
| } | ||
|
|
||
| // Create, update, or delete the patch policy after the installer save |
There was a problem hiding this comment.
Query is blanked & saved before this policy write; if NewTeamPolicy fails, query's gone and no policy exists. Should these be atomic?
There was a problem hiding this comment.
I think that would be great but I'm not sure how to fit these in a transaction or something similar. I tried to add as much validation as possible before this point to help avoid this situation but that's not guaranteed.
Maybe I should move the part that deletes the pre_install_query until after New/Modify/DeleteTeamPolicy ?
There was a problem hiding this comment.
I think you can actually just delete 824 because NewTeamPolicy/ModifyTeamPolicy already call ClearPreInstallQueryForTitle internally and those run here, i.e. after the installer save. I think you'd just get the ordering for free.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if patchFlag && patchWhenClosedFlag && existingInstaller.PreInstallQuery != "" { |
There was a problem hiding this comment.
Enabling patch_when_closed overwrites pre_install_query in place; disabling never restores it. Intended, or should we preserve/restore it?
There was a problem hiding this comment.
I think what we discussed on standup is that it's fine to just delete it. Maybe it could be an error to tell the user to manually delete the query?
| @@ -278,8 +300,8 @@ func TestTeamPolicyPatchWhenClosed(t *testing.T) { | |||
| require.ErrorContains(t, err, "patch_when_closed") | |||
There was a problem hiding this comment.
This trips the continuous-automations check first (both messages contain patch_when_closed), so the non-patch guard is never hit, seems like. Add ContinuousAutomationsEnabled: true?
| continuousAutomationsEnabled := p.ContinuousAutomationsEnabled | ||
| if p.PatchWhenClosed { | ||
| continuousAutomationsEnabled = true | ||
| if p.PatchWhenClosed && !p.ContinuousAutomationsEnabled { |
There was a problem hiding this comment.
The gist of the PR is to make the API reject patch_when_closed & continuous_automations_enabled=false, right? But it seems like here, ApplyPolicySpecs still silently coerces it. Should specs reject too?
There was a problem hiding this comment.
What I got from the API docs changes is:
"If patch_when_closed is true on a policy (existing or being set in the same request) and the request explicitly sets continuous_automations_enabled to false, Fleet rejects the request instead of silently overriding it."
So I think it makes sense in ApplyPolicySpecs to silently coerce it if the field is missing, and reject it if the field is explicitly set to false. I think this is already the behaviour in the gitops changes for this feature right?
There was a problem hiding this comment.
I think only the modify path does that. It's the one field typed *bool. PolicySpec and create are plain bool, so they can't tell false from omitted. Specs always coerce, create always rejects. Fine to leave if we document gitops as normalizing; for real parity they'd need to be *bool I think.
There was a problem hiding this comment.
Yeah, I would think that plain bool values will get the explicit error. And *bool will only get it if not nil.
There was a problem hiding this comment.
I think that's true for create, but the opposite for specs. If what we want is "coerce-if-missing / reject-if-explicit-false" everywhere, then I think create and specs would need *bool and that same nil check, if I'm not mistaken here.
Also reject patch_when_closed in policy specs when the package already has a pre-install query, and fix the non-patch patch_when_closed test so it reaches its guard instead of tripping the continuous-automations check first.
2 participants
Related issue: Resolves #49416
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
Summary by CodeRabbit
New Features
Bug Fixes