43885: MLAPR migration + UUID capture - #44244
Conversation
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Failure
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #44244 +/- ##
==========================================
+ Coverage 66.78% 66.79% +0.01%
==========================================
Files 2632 2633 +1
Lines 211581 211574 -7
Branches 9424 9400 -24
==========================================
+ Hits 141304 141326 +22
+ Misses 57446 57414 -32
- Partials 12831 12834 +3
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:
|
GitHub Actions
Error
GitHub Actions
Inactive
GitHub Actions
Inactive
![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 (4)
✅ Files skipped from review due to trivial changes (1)
WalkthroughAdds schema and logic to enable managed local admin password rotation and capture the managed account UUID: a migration adds Possibly related PRs
🚥 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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 (2)
server/service/integration_mdm_test.go (1)
23684-23709: Optional cleanup: extract duplicated row-read SQL into a helper.The two
account_uuid, updated_atreads are identical; pulling them into a small local helper would reduce repetition and keep assertions more readable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/integration_mdm_test.go` around lines 23684 - 23709, Extract the duplicated row-read into a small helper function (e.g., readManagedLocalAccountPasswordRow or fetchAccountUUIDAndUpdatedAt) that accepts the context, data store (s.ds) or sqlx.ExtContext via mysql.ExecAdhocSQL, and the mdmDevice.UUID/host ID, executes the SELECT account_uuid, updated_at FROM host_managed_local_account_passwords WHERE host_uuid = ?, and returns (*string, time.Time, error); then replace both inline mysql.ExecAdhocSQL blocks with calls to this helper and update the subsequent assertions to use the helper's return values. Ensure the helper is local to the test file and use existing symbols submitUsersResult, mdmDevice.UUID, and mysql.ExecAdhocSQL to locate where to call it.server/service/osquery_utils/queries_test.go (1)
1924-1927: Usefleet.ManagedLocalAccountUsernameinstead of hardcoded"_fleetadmin"in test fixtures.Using the shared constant here reduces drift risk if the managed username ever changes.
Suggested diff
- baseRows := []map[string]string{ - {"uid": "501", "username": "alice", "type": "standard", "groupname": "staff", "shell": "/bin/zsh", "uuid": "alice-uuid"}, - {"uid": "502", "username": "_fleetadmin", "type": "standard", "groupname": "staff", "shell": "/bin/zsh", "uuid": "fleetadmin-uuid"}, - } + baseRows := []map[string]string{ + {"uid": "501", "username": "alice", "type": "standard", "groupname": "staff", "shell": "/bin/zsh", "uuid": "alice-uuid"}, + {"uid": "502", "username": fleet.ManagedLocalAccountUsername, "type": "standard", "groupname": "staff", "shell": "/bin/zsh", "uuid": "fleetadmin-uuid"}, + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/osquery_utils/queries_test.go` around lines 1924 - 1927, Replace the hardcoded managed username literal "_fleetadmin" in the test fixture baseRows with the shared constant fleet.ManagedLocalAccountUsername in server/service/osquery_utils/queries_test.go so the test uses the canonical value; locate the baseRows slice (variable name baseRows) and substitute the string for fleet.ManagedLocalAccountUsername to avoid drift if the managed username changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@changes/37142-local-admin-password-rotation`:
- Line 1: The PR adds scheduling fields and helpers (auto_rotate_at in the
migration, MarkRecoveryLockPasswordViewed(), GetHostsForAutoRotation(),
pending_encrypted_password/pending_command_uuid) but no background executor to
perform rotations; either implement a cron/background worker that periodically
calls GetHostsForAutoRotation() and enqueues/executes the rotation flow
(processing pending_* columns and invoking the same rotation logic used for
manual rotations) or update the changes/37142-local-admin-password-rotation note
to state this PR only adds infrastructure (auto_rotate_at and scheduling) and
that the executor will be delivered separately, and if the executor already
exists under a different name/path, add a reference to that service/worker and
the function that triggers rotations.
In `@server/service/osquery_utils/queries.go`:
- Around line 982-989: The shared usersQueryStr currently includes the
`_fleetadmin` allowlist which leaks that service account into withCachedUsers;
remove the allowlist from the shared query and apply it only in the dedicated
host-users/detail query used by directIngestUsers. Concretely, split
usersQueryStr into two identifiers (e.g., cachedUsersQueryStr without the
"(username = '...ManagedLocalAccountUsername...')" clause and
detailUsersQueryStr that retains the allowlist), update withCachedUsers(...) to
use cachedUsersQueryStr, and ensure directIngestUsers (or the function that
builds the host detail query) uses detailUsersQueryStr so `_fleetadmin` stays
excluded from cached/shared user collections.
---
Nitpick comments:
In `@server/service/integration_mdm_test.go`:
- Around line 23684-23709: Extract the duplicated row-read into a small helper
function (e.g., readManagedLocalAccountPasswordRow or
fetchAccountUUIDAndUpdatedAt) that accepts the context, data store (s.ds) or
sqlx.ExtContext via mysql.ExecAdhocSQL, and the mdmDevice.UUID/host ID, executes
the SELECT account_uuid, updated_at FROM host_managed_local_account_passwords
WHERE host_uuid = ?, and returns (*string, time.Time, error); then replace both
inline mysql.ExecAdhocSQL blocks with calls to this helper and update the
subsequent assertions to use the helper's return values. Ensure the helper is
local to the test file and use existing symbols submitUsersResult,
mdmDevice.UUID, and mysql.ExecAdhocSQL to locate where to call it.
In `@server/service/osquery_utils/queries_test.go`:
- Around line 1924-1927: Replace the hardcoded managed username literal
"_fleetadmin" in the test fixture baseRows with the shared constant
fleet.ManagedLocalAccountUsername in
server/service/osquery_utils/queries_test.go so the test uses the canonical
value; locate the baseRows slice (variable name baseRows) and substitute the
string for fleet.ManagedLocalAccountUsername to avoid drift if the managed
username changes.
🪄 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: 315a7189-57b1-4cf6-a8cd-f62ff709c777
⛔ Files ignored due to path filters (1)
docs/Contributing/product-groups/orchestration/understanding-host-vitals.mdis excluded by!**/*.md
📒 Files selected for processing (13)
changes/37142-local-admin-password-rotationserver/datastore/mysql/managed_local_account.goserver/datastore/mysql/managed_local_account_test.goserver/datastore/mysql/migrations/tables/20260428125634_AddManagedLocalAccountRotationColumns.goserver/datastore/mysql/migrations/tables/20260428125634_AddManagedLocalAccountRotationColumns_test.goserver/datastore/mysql/schema.sqlserver/fleet/apple_mdm.goserver/fleet/datastore.goserver/mock/datastore_mock.goserver/service/apple_mdm.goserver/service/integration_mdm_test.goserver/service/osquery_utils/queries.goserver/service/osquery_utils/queries_test.go
| // Usernames starting with an underscore are excluded (macOS system accounts), | ||
| // with the managed local admin (_fleetadmin) allowlisted so the ingest layer | ||
| // can capture its UUID for MDM rotation. The ingest layer is responsible for | ||
| // excluding _fleetadmin from host_users — see directIngestUsers. | ||
| const usersQueryStr = `WITH cached_groups AS (select * from groups) | ||
| SELECT uid, uuid, username, type, groupname, shell | ||
| FROM users LEFT JOIN cached_groups USING (gid) | ||
| WHERE type <> 'special' AND shell NOT LIKE '%/false' AND shell NOT LIKE '%/nologin' AND shell NOT LIKE '%/shutdown' AND shell NOT LIKE '%/halt' AND username NOT LIKE '%$' AND username NOT LIKE '\_%' ESCAPE '\' AND NOT (username = 'sync' AND shell ='/bin/sync' AND directory <> '')` | ||
| WHERE type <> 'special' AND shell NOT LIKE '%/false' AND shell NOT LIKE '%/nologin' AND shell NOT LIKE '%/shutdown' AND shell NOT LIKE '%/halt' AND username NOT LIKE '%$' AND (username NOT LIKE '\_%' ESCAPE '\' OR username = '` + fleet.ManagedLocalAccountUsername + `') AND NOT (username = 'sync' AND shell ='/bin/sync' AND directory <> '')` |
There was a problem hiding this comment.
Keep the _fleetadmin allowlist out of the shared usersQueryStr.
usersQueryStr is also the source for withCachedUsers(...) (Lines 991-993), so this change now pulls _fleetadmin into the user-scoped software queries too, not just directIngestUsers. That widens unrelated collection for a service account we still intentionally exclude from host_users. Please split the macOS host-users query from the shared cached-users query, or apply the allowlist only to the dedicated users detail query.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/service/osquery_utils/queries.go` around lines 982 - 989, The shared
usersQueryStr currently includes the `_fleetadmin` allowlist which leaks that
service account into withCachedUsers; remove the allowlist from the shared query
and apply it only in the dedicated host-users/detail query used by
directIngestUsers. Concretely, split usersQueryStr into two identifiers (e.g.,
cachedUsersQueryStr without the "(username =
'...ManagedLocalAccountUsername...')" clause and detailUsersQueryStr that
retains the allowlist), update withCachedUsers(...) to use cachedUsersQueryStr,
and ensure directIngestUsers (or the function that builds the host detail query)
uses detailUsersQueryStr so `_fleetadmin` stays excluded from cached/shared user
collections.
There was a problem hiding this comment.
Pull request overview
Implements the DB schema and ingestion plumbing needed to support managed local admin password rotation on macOS by adding rotation-state columns and capturing the managed admin account UUID (_fleetadmin) from osquery, with an MDM-driven refetch kickstart to shorten capture latency.
Changes:
- Adds rotation-related columns (and index) to
host_managed_local_account_passwordsand regeneratesschema.sql. - Captures
_fleetadmin’susers.uuidduringdirectIngestUsersand persists it via new datastore methods. - Requests a host refetch after
AccountConfigurationack when the managed local account exists butaccount_uuidis still NULL; adds unit/integration test coverage.
Reviewed changes
Copilot reviewed 12 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/osquery_utils/queries.go | Allowlists _fleetadmin in the users query and captures/persists its UUID during direct ingest. |
| server/service/osquery_utils/queries_test.go | Adds unit tests covering managed-local-account UUID capture behavior and filtering from host_users. |
| server/service/apple_mdm.go | Triggers a best-effort host refetch after AccountConfiguration ack when account_uuid is still NULL. |
| server/service/integration_mdm_test.go | Extends managed local account integration test to validate refetch + UUID capture + idempotency. |
| server/fleet/datastore.go | Adds datastore interface methods for get/set managed local account UUID. |
| server/fleet/apple_mdm.go | Promotes _fleetadmin to fleet.ManagedLocalAccountUsername. |
| server/datastore/mysql/managed_local_account.go | Clears account_uuid on managed-local-account upsert; implements Get/Set UUID methods. |
| server/datastore/mysql/managed_local_account_test.go | Adds datastore tests for Get/Set UUID semantics. |
| server/mock/datastore_mock.go | Extends mock datastore with Get/Set managed local account UUID methods. |
| server/datastore/mysql/migrations/tables/20260428125634_AddManagedLocalAccountRotationColumns.go | Adds migration for rotation-state columns and auto_rotate_at index. |
| server/datastore/mysql/migrations/tables/20260428125634_AddManagedLocalAccountRotationColumns_test.go | Verifies migration behavior on existing/new rows and presence of the new index. |
| server/datastore/mysql/schema.sql | Updates schema snapshot for new managed-local-account columns/index. |
| docs/Contributing/product-groups/orchestration/understanding-host-vitals.md | Updates documented cached-users query to reflect _fleetadmin allowlist. |
| changes/37142-local-admin-password-rotation | Adds a user-visible changes entry for the feature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Usernames starting with an underscore are excluded (macOS system accounts), | ||
| // with the managed local admin (_fleetadmin) allowlisted so the ingest layer | ||
| // can capture its UUID for MDM rotation. The ingest layer is responsible for | ||
| // excluding _fleetadmin from host_users — see directIngestUsers. | ||
| const usersQueryStr = `WITH cached_groups AS (select * from groups) | ||
| SELECT uid, uuid, username, type, groupname, shell | ||
| FROM users LEFT JOIN cached_groups USING (gid) | ||
| WHERE type <> 'special' AND shell NOT LIKE '%/false' AND shell NOT LIKE '%/nologin' AND shell NOT LIKE '%/shutdown' AND shell NOT LIKE '%/halt' AND username NOT LIKE '%$' AND username NOT LIKE '\_%' ESCAPE '\' AND NOT (username = 'sync' AND shell ='/bin/sync' AND directory <> '')` | ||
| WHERE type <> 'special' AND shell NOT LIKE '%/false' AND shell NOT LIKE '%/nologin' AND shell NOT LIKE '%/shutdown' AND shell NOT LIKE '%/halt' AND username NOT LIKE '%$' AND (username NOT LIKE '\_%' ESCAPE '\' OR username = '` + fleet.ManagedLocalAccountUsername + `') AND NOT (username = 'sync' AND shell ='/bin/sync' AND directory <> '')` |
There was a problem hiding this comment.
usersQueryStr is reused by withCachedUsers(...), so allowlisting _fleetadmin here affects all queries that build a cached_users CTE (software queries, VSCode extensions, JetBrains plugins, python packages with users, etc.), not just the users detail query. If the goal is only to fetch the UUID for managed-local-account capture, consider splitting this into two query strings (one for users ingest that includes _fleetadmin, and a separate one for cached_users joins that keeps excluding underscore/system accounts) to avoid unintentionally including _fleetadmin-scoped data in other ingestions.
MagnusHJensen
left a comment
MagnusHJensen
left a comment
There was a problem hiding this comment.
Just a small comment, I'll run through the flow in a second
| } | ||
|
|
||
| func (ds *Datastore) GetManagedLocalAccountUUID(ctx context.Context, hostUUID string) (*string, error) { | ||
| const stmt = `SELECT account_uuid FROM host_managed_local_account_passwords WHERE host_uuid = ?` |
There was a problem hiding this comment.
Is there ever a chance where we have more tha none managed local account for the same host UUID?
There was a problem hiding this comment.
Nah we shouldn't. Likely a bug somewhere if we think we do
| const stmt = ` | ||
| UPDATE host_managed_local_account_passwords | ||
| SET account_uuid = ? | ||
| WHERE host_uuid = ? AND (account_uuid IS NULL OR account_uuid <> ?)` |
There was a problem hiding this comment.
I just got bamboozled by the <> operator. 🤦♂️
I thought it was equal, but it's actually not equals, I guess it depends on the background how easy one understands that. I'll just do a quick advocation for !=, but that is only my personal preference.
There was a problem hiding this comment.
I don't think it functionally matters much in modern SQL but I usually use <> out of habit as some older DBs I worked on didn't support or didn't fully support != but I am happy to change it. <> is the SQL standard form whereas != is one of those non-standard-but-everyone-supports-it things
There was a problem hiding this comment.
It definitely doesn't matter, so I'll leave it up to you what you think fits best.
MagnusHJensen
left a comment
MagnusHJensen
left a comment
There was a problem hiding this comment.
There seems to be another local reference to the _fleetadmin here:
fleet/server/worker/apple_mdm.go
Line 251 in d03dd5b
|
I see the new columns, and on a new enrollment (also for existing) it orbit eventually queries the managed account UUID. Once the small comment above is addressed, ping for re-review and I'll approve. |
GitHub Actions
Error
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
GitHub Actions
Inactive
Fixed |
GitHub Actions
Inactive
sgress454
left a comment
sgress454
left a comment
There was a problem hiding this comment.
host vitals docs matches new query 👍
Related issue: Resolves #43885
Adds a migration and code to capture the value of the fleet managed admin account if one exists. Changes file added for entire feature
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
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
Database migrations
COLLATE utf8mb4_unicode_ci).Summary by CodeRabbit
New Features
Chores
Tests