Add Windows admin account config - #49863
Merged
Merged
Conversation
Member
Author
|
@coderabbitai full review |
Member
Author
|
/agentic_review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new Windows MDM configuration surface for Fleet’s “managed local admin account” toggle, ensuring it round-trips through API responses, persistence, GitOps apply, and fleetctl generate-gitops, and logs the existing managed-local-account activities when the Windows toggle changes.
Changes:
- Introduces
ManagedLocalAccountSettings{ enabled }and wires it intoWindowsSettings, with default-to-false behavior for marshaled output and persisted team/app config. - Adds AppConfig PATCH handling: null/absent semantics, premium gating, Windows-MDM precondition validation, and activity logging for Windows toggle changes.
- Updates GitOps apply/generate to support declarative disable (absent key => disabled), plus adds/updates unit + integration + fleetctl fixture tests.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/integration_enterprise_test.go | Updates integration fixtures to include defaulted Windows managed-local-account setting. |
| server/service/client.go | GitOps client defaults Windows toggle to disabled when absent for declarative semantics. |
| server/service/appconfig.go | AppConfig PATCH merge/validate/activity support for Windows managed-local-account toggle. |
| server/service/appconfig_test.go | Adds unit tests covering persistence, gating, activities, and null semantics for Windows toggle. |
| server/fleet/teams.go | Adds team PATCH payload type for Windows toggle and defaults toggle to false for marshal/value/spec output. |
| server/fleet/teams_test.go | Adds tests for team JSON/value defaults and TeamMDM copy behavior for the new setting. |
| server/fleet/app.go | Defines ManagedLocalAccountSettings, adds it to WindowsSettings, and defaults output to enabled=false. |
| server/fleet/app_test.go | Adds tests for AppConfig marshal defaults and clone behavior for the new setting. |
| ee/server/service/teams.go | Adds team PATCH + team-spec apply support for Windows toggle (incl. MDM precondition + activities). |
| ee/server/service/teams_test.go | Adds tests for team PATCH and team-spec apply persistence for Windows toggle. |
| ee/server/service/mdm.go | Renames/refactors activity helper to be platform-agnostic for managed-local-account enable/disable. |
| cmd/fleetctl/fleetctl/testdata/macosSetupExpectedTeam1Set.yml | Updates expected YAML to include Windows toggle default output. |
| cmd/fleetctl/fleetctl/testdata/macosSetupExpectedTeam1Empty.yml | Updates expected YAML to include Windows toggle default output. |
| cmd/fleetctl/fleetctl/testdata/macosSetupExpectedTeam1And2Set.yml | Updates expected YAML to include Windows toggle default output. |
| cmd/fleetctl/fleetctl/testdata/macosSetupExpectedTeam1And2Empty.yml | Updates expected YAML to include Windows toggle default output. |
| cmd/fleetctl/fleetctl/testdata/macosSetupExpectedAppConfigSet.yml | Updates expected YAML to include Windows toggle default output. |
| cmd/fleetctl/fleetctl/testdata/macosSetupExpectedAppConfigEmpty.yml | Updates expected YAML to include Windows toggle default output. |
| cmd/fleetctl/fleetctl/gitops_test.go | Adds end-to-end GitOps test ensuring absent key disables the toggle declaratively. |
| cmd/fleetctl/fleetctl/generate_gitops.go | Emits Windows toggle only when enabled (and premium), to support declarative disable via omission. |
| cmd/fleetctl/fleetctl/generate_gitops_test.go | Adds generate-gitops test coverage for Windows toggle emission rules. |
Comments suppressed due to low confidence (1)
ee/server/service/teams.go:416
- The invalid-argument field path includes an extra "mdm." prefix ("mdm.windows_settings...") whereas other MDM sub-field errors in this handler typically omit the top-level wrapper (e.g. "windows_updates", "setup_experience..."). For this new field, using "windows_settings.managed_local_account_settings.enabled" would better match app-config validation keys and other surfaces.
return nil, fleet.NewInvalidArgumentError("mdm.windows_settings.managed_local_account_settings.enabled",
"Couldn't update windows_settings.managed_local_account_settings because Windows MDM isn't turned on in Fleet.")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughAdds Windows managed local account settings to Fleet configuration models, API payloads, serialization, validation, team PATCH, GitOps application, and GitOps generation. Enabling is gated by premium licensing and configured Windows MDM. Configuration changes emit platform-specific activities, and the activity feed renders Windows or macOS host labels while preserving the macOS fallback for legacy events. Tests and expected configuration fixtures cover default-disabled serialization, enable/disable transitions, validation, persistence, and generated output. Possibly related issues
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@ee/server/service/teams.go`:
- Around line 2013-2022: Update the edit handling around
ManagedLocalAccountSettings.Enabled in the team configuration flow to default an
omitted GitOps toggle to false, while preserving explicit values and existing
validation for enabling it. Ensure the assignment and update tracking treat the
omitted field as a disable operation, and add an edit test beginning with the
setting enabled and omitting the field to verify it becomes false.
🪄 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: e7e23154-418c-4900-99ee-3507cce03cc1
📒 Files selected for processing (20)
cmd/fleetctl/fleetctl/generate_gitops.gocmd/fleetctl/fleetctl/generate_gitops_test.gocmd/fleetctl/fleetctl/gitops_test.gocmd/fleetctl/fleetctl/testdata/macosSetupExpectedAppConfigEmpty.ymlcmd/fleetctl/fleetctl/testdata/macosSetupExpectedAppConfigSet.ymlcmd/fleetctl/fleetctl/testdata/macosSetupExpectedTeam1And2Empty.ymlcmd/fleetctl/fleetctl/testdata/macosSetupExpectedTeam1And2Set.ymlcmd/fleetctl/fleetctl/testdata/macosSetupExpectedTeam1Empty.ymlcmd/fleetctl/fleetctl/testdata/macosSetupExpectedTeam1Set.ymlee/server/service/mdm.goee/server/service/teams.goee/server/service/teams_test.goserver/fleet/app.goserver/fleet/app_test.goserver/fleet/teams.goserver/fleet/teams_test.goserver/service/appconfig.goserver/service/appconfig_test.goserver/service/client.goserver/service/integration_enterprise_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #49863 +/- ##
==========================================
+ Coverage 67.92% 68.00% +0.07%
==========================================
Files 3906 3925 +19
Lines 249746 250239 +493
Branches 13337 13367 +30
==========================================
+ Hits 169649 170165 +516
+ Misses 64822 64761 -61
- Partials 15275 15313 +38
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:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
ee/server/service/teams.go:1561
- This error message refers to enabling
windows_settings.managed_local_account_settings, but the actual setting is theenabledfield. Using the full field path in the message avoids confusion when troubleshooting spec-apply failures.
if spec.MDM.WindowsSettings.ManagedLocalAccountSettings.Enabled.Value && !windowsEnabledAndConfigured {
return nil, ctxerr.Wrap(ctx, fleet.NewInvalidArgumentError("windows_settings.managed_local_account_settings.enabled",
"Couldn't enable windows_settings.managed_local_account_settings. "+fleet.ErrWindowsMDMNotConfigured.Error()))
}
ee/server/service/teams.go:2020
- This error message refers to enabling
windows_settings.managed_local_account_settings, but the invalid-argument key iswindows_settings.managed_local_account_settings.enabled. Updating the message to match the field path will make the error clearer.
if didUpdateWindowsManagedLocalAccount && newWindowsManagedLocalAccount.Value && !windowsEnabledAndConfigured {
return ctxerr.Wrap(ctx, fleet.NewInvalidArgumentError("windows_settings.managed_local_account_settings.enabled",
"Couldn't enable windows_settings.managed_local_account_settings. "+fleet.ErrWindowsMDMNotConfigured.Error()))
}
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
cmd/fleetctl/fleetctl/generate_gitops.go:1381
- Avoid embedding a specific date in this inline comment. Dates become stale quickly and don’t communicate durable intent; prefer a timeless rationale and/or an issue reference for why disabled settings aren’t emitted.
// emit the managed local account toggle only when enabled
// Product guidance (2026/07/24): we generally do not output all settings, only what's configured
if cmd.AppConfig.License.IsPremium() && teamMdm != nil && teamMdm.WindowsSettings.ManagedLocalAccountSettings.Enabled.Value {
server/service/appconfig.go:1988
- The error message says it couldn’t enable "windows_settings.managed_local_account_settings" but the validated field/key is "windows_settings.managed_local_account_settings.enabled". Using the full field path here makes the response clearer and consistent with the invalid-argument key.
invalid.Append("windows_settings.managed_local_account_settings.enabled",
`Couldn’t enable windows_settings.managed_local_account_settings. Windows MDM isn’t turned on. This can be enabled by setting "controls.windows_enabled_and_configured: true" in the default configuration. Visit https://fleetdm.com/guides/windows-mdm-setup and https://fleetdm.com/docs/configuration/yaml-files#controls to learn more about enabling MDM.`)
}
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 38 out of 38 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
ee/server/service/teams.go:426
- Team PATCH path allows enabling Windows managed local account without any premium-license gate. This lets a Free-tier instance turn on a premium-only feature via /teams/:id, unlike the global /config path which enforces ErrMissingLicense when enabling.
if payload.MDM.WindowsSettings != nil && payload.MDM.WindowsSettings.ManagedLocalAccountSettings.Enabled.Valid {
newEnabled := payload.MDM.WindowsSettings.ManagedLocalAccountSettings.Enabled
windowsManagedLocalAccountUpdated = team.Config.MDM.WindowsSettings.ManagedLocalAccountSettings.Enabled.Value != newEnabled.Value
if windowsManagedLocalAccountUpdated && newEnabled.Value && !appCfg.MDM.WindowsEnabledAndConfigured {
return nil, fleet.NewInvalidArgumentError("windows_settings.managed_local_account_settings.enabled",
"Couldn't update windows_settings.managed_local_account_settings because Windows MDM isn't turned on in Fleet.")
}
team.Config.MDM.WindowsSettings.ManagedLocalAccountSettings.Enabled = newEnabled
}
ee/server/service/teams.go:2019
- GitOps/team-spec apply path (editTeamFromSpec) enables the Windows managed local account toggle without checking for a premium license. This makes license enforcement inconsistent with ModifyAppConfig/validateMDM and allows enabling the feature via fleetctl gitops on a Free-tier instance.
var didUpdateWindowsManagedLocalAccount bool
if spec.MDM.WindowsSettings.ManagedLocalAccountSettings.Enabled.Valid {
newWindowsManagedLocalAccount := spec.MDM.WindowsSettings.ManagedLocalAccountSettings.Enabled
didUpdateWindowsManagedLocalAccount = team.Config.MDM.WindowsSettings.ManagedLocalAccountSettings.Enabled.Value != newWindowsManagedLocalAccount.Value
if didUpdateWindowsManagedLocalAccount && newWindowsManagedLocalAccount.Value && !windowsEnabledAndConfigured {
return ctxerr.Wrap(ctx, fleet.NewInvalidArgumentError("windows_settings.managed_local_account_settings.enabled",
"Couldn't enable windows_settings.managed_local_account_settings. "+fleet.ErrWindowsMDMNotConfigured.Error()))
}
team.Config.MDM.WindowsSettings.ManagedLocalAccountSettings.Enabled = newWindowsManagedLocalAccount
}
Member
Author
|
@lucasmrod, assigning this one to you to review for Monday. Feel free to assign PRs to me to review, since I don't have another engineer on my team right now. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
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/teams.go (1)
418-425: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winEnforce premium licensing before enabling the team-level setting.
Both paths allow a free-tier deployment with configured Windows MDM to enable this premium capability. Gate only false→true transitions; disabling must remain allowed for license downgrades.
ee/server/service/teams.go#L418-L425: require premium entitlement before persisting an enabled PATCH value.ee/server/service/teams.go#L2010-L2019: apply the same entitlement check during GitOps team edits.🤖 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 `@ee/server/service/teams.go` around lines 418 - 425, The team-level Windows managed local account setting must require premium entitlement only for false-to-true transitions. In ee/server/service/teams.go:418-425, update the PATCH handling around the WindowsSettings managed-local-account Enabled assignment to reject enabling when the deployment lacks the premium entitlement, while preserving disabling and existing Windows MDM validation. Apply the same entitlement check to the GitOps team-edit path in ee/server/service/teams.go:2010-2019 before persisting an enabled value; no restriction should apply when disabling.
🧹 Nitpick comments (1)
frontend/pages/DashboardPage/cards/ActivityFeed/GlobalActivityItem/GlobalActivityItem.tests.tsx (1)
722-745: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the legacy fallback for enabled activities too.
The legacy case at Line 737 uses
DisabledManagedLocalAccount, soenabledManagedLocalAccountis never tested whendetails.platformis absent. Add an enabled legacy assertion and keep the disabled case separately.🤖 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/DashboardPage/cards/ActivityFeed/GlobalActivityItem/GlobalActivityItem.tests.tsx` around lines 722 - 745, Update the test around the enabledManagedLocalAccount activity to add a legacy enabled activity with empty details and assert it defaults to macOS. Keep the existing Windows assertion, and retain the disabled legacy activity as a separate case so both activity types’ fallbacks are covered.
🤖 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 `@server/service/integration_enterprise_test.go`:
- Around line 221-229: Update TeamConfig.Value() to normalize Windows
managed-local-account settings alongside the existing macOS defaults: when
ManagedLocalAccountSettings.Enabled is not valid, assign optjson.SetBool(false)
before serialization. Preserve explicitly configured Windows values and apply
the same normalization consistently across the affected TeamConfig.Value()
paths.
---
Outside diff comments:
In `@ee/server/service/teams.go`:
- Around line 418-425: The team-level Windows managed local account setting must
require premium entitlement only for false-to-true transitions. In
ee/server/service/teams.go:418-425, update the PATCH handling around the
WindowsSettings managed-local-account Enabled assignment to reject enabling when
the deployment lacks the premium entitlement, while preserving disabling and
existing Windows MDM validation. Apply the same entitlement check to the GitOps
team-edit path in ee/server/service/teams.go:2010-2019 before persisting an
enabled value; no restriction should apply when disabling.
---
Nitpick comments:
In
`@frontend/pages/DashboardPage/cards/ActivityFeed/GlobalActivityItem/GlobalActivityItem.tests.tsx`:
- Around line 722-745: Update the test around the enabledManagedLocalAccount
activity to add a legacy enabled activity with empty details and assert it
defaults to macOS. Keep the existing Windows assertion, and retain the disabled
legacy activity as a separate case so both activity types’ fallbacks are
covered.
🪄 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: f2011994-32a9-4198-bba7-f032094e09a5
📒 Files selected for processing (32)
cmd/fleetctl/fleetctl/apply_deprecated_test.gocmd/fleetctl/fleetctl/apply_test.gocmd/fleetctl/fleetctl/generate_gitops.gocmd/fleetctl/fleetctl/generate_gitops_test.gocmd/fleetctl/fleetctl/gitops_test.gocmd/fleetctl/fleetctl/testdata/expectedGetConfigAppConfigJson.jsoncmd/fleetctl/fleetctl/testdata/expectedGetConfigAppConfigTeamMaintainerJson.jsoncmd/fleetctl/fleetctl/testdata/expectedGetConfigAppConfigTeamMaintainerYaml.ymlcmd/fleetctl/fleetctl/testdata/expectedGetConfigAppConfigYaml.ymlcmd/fleetctl/fleetctl/testdata/expectedGetConfigIncludeServerConfigJson.jsoncmd/fleetctl/fleetctl/testdata/expectedGetConfigIncludeServerConfigYaml.ymlcmd/fleetctl/fleetctl/testdata/expectedGetTeamsJson.jsoncmd/fleetctl/fleetctl/testdata/expectedGetTeamsYaml.ymlee/server/service/mdm.goee/server/service/teams.goee/server/service/teams_test.gofrontend/pages/DashboardPage/cards/ActivityFeed/GlobalActivityItem/GlobalActivityItem.tests.tsxfrontend/pages/DashboardPage/cards/ActivityFeed/GlobalActivityItem/GlobalActivityItem.tsxserver/datastore/mysql/schema.sqlserver/datastore/mysql/teams_test.goserver/fleet/activities.goserver/fleet/app.goserver/fleet/app_test.goserver/fleet/mdm.goserver/fleet/teams.goserver/service/appconfig.goserver/service/appconfig_test.goserver/service/integration_enterprise_test.goserver/service/integration_mdm_test.gotools/cloner-check/generated_files/appconfig.txttools/cloner-check/generated_files/teamconfig.txttools/cloner-check/generated_files/teammdm.txt
🚧 Files skipped from review as they are similar to previous changes (3)
- ee/server/service/mdm.go
- cmd/fleetctl/fleetctl/generate_gitops.go
- ee/server/service/teams_test.go
lucasmrod
reviewed
Jul 28, 2026
lucasmrod
left a comment
lucasmrod
left a comment
Member
There was a problem hiding this comment.
Looks good! Left two questions.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 38 out of 38 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
ee/server/service/teams.go:424
- The Windows MDM precondition error message is inconsistent with the rest of the API and doesn’t mention the actual leaf key (
...enabled). It also omits the standard guidance used elsewhere (seefleet.WindowsMDMNotTurnedOnMessage).
if windowsManagedLocalAccountUpdated && newEnabled.Value && !appCfg.MDM.WindowsEnabledAndConfigured {
return nil, fleet.NewInvalidArgumentError("windows_settings.managed_local_account_settings.enabled",
"Couldn't update windows_settings.managed_local_account_settings because Windows MDM isn't turned on in Fleet.")
}
ee/server/service/teams.go:2017
- This GitOps/team-spec validation error message uses a different Windows-MDM-not-configured message than
ModifyAppConfig/validateMDM, and it also refers tomanaged_local_account_settingsinstead of the actual leaf keymanaged_local_account_settings.enabled. Consider reusingfleet.WindowsMDMNotTurnedOnMessagefor consistent guidance and error text.
if didUpdateWindowsManagedLocalAccount && newWindowsManagedLocalAccount.Value && !windowsEnabledAndConfigured {
return ctxerr.Wrap(ctx, fleet.NewInvalidArgumentError("windows_settings.managed_local_account_settings.enabled",
"Couldn't enable windows_settings.managed_local_account_settings. "+fleet.ErrWindowsMDMNotConfigured.Error()))
}
server/service/appconfig.go:1985
- The invalid-argument message mentions
windows_settings.managed_local_account_settings, but the rejected field iswindows_settings.managed_local_account_settings.enabled. Aligning the message with the actual leaf key makes the error clearer (and matches other MDM validation messages).
Member
Author
|
@lucasmrod I made fixes for your 2 comments. Ready for re-review. |
lucasmrod
approved these changes
Jul 28, 2026
nulmete
approved these changes
Jul 28, 2026
4 tasks
This was referenced Jul 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issue: Resolves #48720
Subtask of #43488
This PR only adds the Windows config, and doesn't mess with macOS configs.
Checklist for submitter
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
New Fleet configuration settings
fleetctl generate-gitopsSummary by CodeRabbit
New Features
Bug Fixes