43887 MLAPR backend - #44726
Conversation
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #44726 +/- ##
==========================================
+ Coverage 66.67% 66.70% +0.03%
==========================================
Files 2652 2660 +8
Lines 213648 215429 +1781
Branches 9806 9806
==========================================
+ Hits 142449 143702 +1253
- Misses 58236 58621 +385
- Partials 12963 13106 +143
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
Inactive
GitHub Actions
Error
![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.
GitHub Actions
Inactive
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds end-to-end managed local account password rotation for macOS: datastore state machine and APIs to mark password views, initiate/clear/complete/fail/defer rotations, and list auto-rotation candidates; new SetAutoAdminPassword MDM command and commander method; cron schedule running every 5 minutes to enqueue rotations; service endpoint to request rotation; command ack handling to complete or fail rotations; new activity types for success/failure; host model/API fields for auto-rotation metadata; and unit/integration tests. Password-availability now depends on presence of an encrypted password (excluding 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 unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 8
🧹 Nitpick comments (4)
server/mdm/apple/apple_mdm_test.go (1)
945-950: 💤 Low valueStrengthen activity payload assertions.
The activity type per PR objectives carries
HostID,HostDisplayName, and an automation flag. The mock injectsHostID: 1andDisplayName: "host-1", so it's cheap to assert that those land on the activity, not justFleetInitiated. This guards against future regressions in activity field mapping.♻️ Proposed assertion tightening
require.Len(t, loggedActivities, 1) rotated, ok := loggedActivities[0].(fleet.ActivityTypeRotatedManagedLocalAccountPassword) require.True(t, ok) require.True(t, rotated.FleetInitiated) + assert.Equal(t, uint(1), rotated.HostID) + assert.Equal(t, "host-1", rotated.HostDisplayName)🤖 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/mdm/apple/apple_mdm_test.go` around lines 945 - 950, Add assertions to verify the HostID and host display name made it into the activity payload: after casting to ActivityTypeRotatedManagedLocalAccountPassword, assert rotated.HostID == 1 and rotated.HostDisplayName == "host-1" in addition to the existing require.True(t, rotated.FleetInitiated) so the test ensures host fields are mapped correctly.server/mdm/apple/apple_mdm.go (1)
2190-2214: 💤 Low valueActivity gating logic is correct, but the hard-coded
FleetInitiated: truedeserves a one-line note.The early return on
!host.InitiatedByFleetalready guarantees we only ever log when the row was view-driven, so settingFleetInitiated: trueunconditionally on the activity payload is correct (and matches the comment on lines 2191–2193 about not double-counting deferred-manual rotations).Optional nit: since this branch is now load-bearing for the "no double-count" guarantee, consider adding a short
// invariant: only reached when host.InitiatedByFleet == truecomment immediately above thenewActivityFncall so a future refactor doesn't accidentally lift the guard. Not blocking.🤖 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/mdm/apple/apple_mdm.go` around lines 2190 - 2214, Add a one-line invariant comment above the activity creation call in logManagedLocalAccountRotationActivity to document that this code path is only reached when host.InitiatedByFleet is true; specifically, just before invoking newActivityFn(ctx, ... , fleet.ActivityTypeRotatedManagedLocalAccountPassword{... FleetInitiated: true }), add a short comment like "// invariant: reached only when host.InitiatedByFleet == true" to prevent future refactors from accidentally removing the guard and to clarify why FleetInitiated is hard-coded true.server/fleet/datastore.go (1)
1731-1734: ⚡ Quick winAlign this contract with the new
password_availablebehavior.This comment says the previous password “remains usable,” but the new flow in this PR makes
password_availablefalse when status isfailed, andGetHostManagedAccountPasswordrejects those rows. Please update the interface comment so callers and tests do not encode the wrong post-failure behavior.Suggested wording
- // FailManagedLocalAccountRotation marks the row's status='failed' and clears pending - // columns; encrypted_password (the previous-known-good password) is left intact so - // the password remains usable. + // FailManagedLocalAccountRotation marks the row's status='failed' and clears pending + // columns. encrypted_password (the previous-known-good password) is left intact for + // audit/recovery purposes, but password_available becomes false while status='failed'. FailManagedLocalAccountRotation(ctx context.Context, hostUUID, cmdUUID, errorMessage string) error🤖 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/fleet/datastore.go` around lines 1731 - 1734, The comment for FailManagedLocalAccountRotation is outdated: it says the previous password "remains usable" but the new flow sets password_available=false when status='failed' and GetHostManagedAccountPassword will reject such rows. Update the interface comment for FailManagedLocalAccountRotation to reflect that on failure the row is marked status='failed', pending columns are cleared, and password_available is set to false so the stored encrypted_password is not considered available by GetHostManagedAccountPassword; reference FailManagedLocalAccountRotation, password_available, status='failed', and GetHostManagedAccountPassword in the comment so callers/tests do not assume the password remains usable after failure.server/datastore/mysql/managed_local_account.go (1)
153-155: ⚡ Quick winWhitelist the lookup column instead of interpolating it raw.
Line 154 is only safe because the current callers pass literals. A small whitelist/switch for
command_uuidandpending_command_uuidwould enforce that invariant in code and avoid turning this helper into an injection footgun later.♻️ Possible hardening
func (ds *Datastore) lookupManagedLocalAccountHost(ctx context.Context, column, commandUUID string) (*fleet.Host, error) { + switch column { + case "command_uuid", "pending_command_uuid": + default: + return nil, ctxerr.Wrap(ctx, fmt.Errorf("unsupported column %q", column), "getting managed local account by command uuid") + } stmt := fmt.Sprintf(`SELECT host_uuid FROM host_managed_local_account_passwords WHERE %s = ?`, column)As per coding guidelines, "Review all SQL queries for possible SQL injection."
🤖 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/managed_local_account.go` around lines 153 - 155, The lookupManagedLocalAccountHost helper currently interpolates the column directly into the SQL; instead validate/whitelist the incoming column parameter (allowed values: "command_uuid" and "pending_command_uuid") using a small switch or map inside lookupManagedLocalAccountHost, return an error for any other value, then construct the SELECT using the validated column name (e.g. fmt.Sprintf with the whitelisted value) and keep the query parameter for the commandUUID argument; this ensures host_managed_local_account_passwords and the column parameter are referenced safely and prevents SQL injection.
🤖 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/hosts.go`:
- Around line 822-838: The manual-rotation path must avoid replica-backed reads:
replace the call to svc.ds.GetManagedLocalAccountUUID(ctx, host.UUID) with a
primary-backed read (e.g., implement and call
svc.ds.GetManagedLocalAccountUUIDPrimary(ctx, host.UUID) or an equivalent method
that queries the primary DB) so replica lag cannot cause a user-triggered
rotation to be deferred; keep the subsequent logic using
svc.logRotateManagedLocalAccountActivity and
svc.ds.MarkManagedLocalAccountRotationDeferred unchanged.
- Around line 830-837: The activity log is written before persisting the
deferred state which can leave a false-positive entry if
MarkManagedLocalAccountRotationDeferred fails; change the order in the
accountUUID==nil branch so you call
svc.ds.MarkManagedLocalAccountRotationDeferred(ctx, host.UUID) first and only
call svc.logRotateManagedLocalAccountActivity(ctx, host, false) after that
succeeds, returning/propagating any error from
MarkManagedLocalAccountRotationDeferred immediately; keep the existing error
wrapping for MarkManagedLocalAccountRotationDeferred and preserve the final nil
return on success.
- Around line 793-803: In RotateManagedLocalAccountPassword ensure you use the
MDM-command authorization gate rather than a generic host write check: after
retrieving host (host := svc.ds.HostLite(...)) replace the call that authorizes
with fleet.ActionWrite (authz.Authorize(ctx, host, fleet.ActionWrite)) with an
authorization using fleet.MDMCommandAuthz{TeamID: host.TeamID} (i.e., call
svc.authz.Authorize(ctx, fleet.MDMCommandAuthz{TeamID: host.TeamID})). This
mirrors the other MDM command flows in this file and enforces the correct
permission model for queuing device commands.
In `@server/datastore/mysql/managed_local_account.go`:
- Around line 322-329: ClearManagedLocalAccountRotation currently only NULLs
pending_encrypted_password and pending_command_uuid which can leave
auto-initiated rotations stopped because InitiateManagedLocalAccountRotation
cleared auto_rotate_at; update ClearManagedLocalAccountRotation to also restore
scheduling—set auto_rotate_at back to the saved prior value (e.g.,
previous_auto_rotate_at) if that column exists, otherwise re-schedule by setting
auto_rotate_at = NOW() + INTERVAL <appropriate_interval> (or the account’s
rotation interval column), and clear any previous_auto_rotate_at after
restoring; locate the logic in ClearManagedLocalAccountRotation and coordinate
with InitiateManagedLocalAccountRotation to use the same
“previous_auto_rotate_at”/interval semantics so cleanup re-arms automation when
enqueue/persistence fails.
In `@server/service/apple_mdm.go`:
- Around line 4342-4344: The current errMsg only uses cmdResult.Status and loses
device error details; update the block that calls
svc.ds.FailManagedLocalAccountRotation (where r.Context, host.UUID,
cmdResult.CommandUUID are used) to build a richer errMsg that includes the
device's error chain when present (e.g., prefer cmdResult.ErrorChain
formatted/trimmed, falling back to cmdResult.Status if ErrorChain is empty), and
pass that combined message into FailManagedLocalAccountRotation so the persisted
failure contains the formatted error chain along with the status.
- Around line 4346-4353: Remove the activity logging from the
SetAutoAdminPassword command-results handler: delete the svc.newActivityFn call
and its error handling (the block that creates
fleet.ActivityTypeFailedToRotateManagedLocalAccountPassword and returns
ctxerr.Wrap on error). The handler should only reconcile the command result into
the datastore rotation state and not call svc.newActivityFn or return errors
from that logging path (remove references to svc.newActivityFn and ctxerr.Wrap
related to this activity type).
In `@server/service/integration_mdm_test.go`:
- Around line 24202-24206: The test checks the ManagedLocalAccount status flips
to "failed" after NACK but misses asserting PasswordAvailable becomes false;
update the subtest around getHostResponse/hostResp to add an assertion that
hostResp.Host.MDM.OSSettings.ManagedLocalAccount.PasswordAvailable is false (use
require.False or equivalent) alongside the existing checks for Status,
PendingRotation, and AutoRotateAt.
- Around line 24106-24118: The assertion currently accepts any fleet-initiated
rotate activity; narrow it to this host by, inside the loop that iterates
fleetActivities.Activities (using rotateActivityName), also checking the
activity is for the test host (compare the activity's host identifier to host.ID
or activity's display name to host.DisplayName()), e.g. add an additional
condition like a.HostID == host.ID || a.HostDisplayName == host.DisplayName() so
sawFleetRotation only becomes true when the rotate activity is both
FleetInitiated and targets this host.
---
Nitpick comments:
In `@server/datastore/mysql/managed_local_account.go`:
- Around line 153-155: The lookupManagedLocalAccountHost helper currently
interpolates the column directly into the SQL; instead validate/whitelist the
incoming column parameter (allowed values: "command_uuid" and
"pending_command_uuid") using a small switch or map inside
lookupManagedLocalAccountHost, return an error for any other value, then
construct the SELECT using the validated column name (e.g. fmt.Sprintf with the
whitelisted value) and keep the query parameter for the commandUUID argument;
this ensures host_managed_local_account_passwords and the column parameter are
referenced safely and prevents SQL injection.
In `@server/fleet/datastore.go`:
- Around line 1731-1734: The comment for FailManagedLocalAccountRotation is
outdated: it says the previous password "remains usable" but the new flow sets
password_available=false when status='failed' and GetHostManagedAccountPassword
will reject such rows. Update the interface comment for
FailManagedLocalAccountRotation to reflect that on failure the row is marked
status='failed', pending columns are cleared, and password_available is set to
false so the stored encrypted_password is not considered available by
GetHostManagedAccountPassword; reference FailManagedLocalAccountRotation,
password_available, status='failed', and GetHostManagedAccountPassword in the
comment so callers/tests do not assume the password remains usable after
failure.
In `@server/mdm/apple/apple_mdm_test.go`:
- Around line 945-950: Add assertions to verify the HostID and host display name
made it into the activity payload: after casting to
ActivityTypeRotatedManagedLocalAccountPassword, assert rotated.HostID == 1 and
rotated.HostDisplayName == "host-1" in addition to the existing require.True(t,
rotated.FleetInitiated) so the test ensures host fields are mapped correctly.
In `@server/mdm/apple/apple_mdm.go`:
- Around line 2190-2214: Add a one-line invariant comment above the activity
creation call in logManagedLocalAccountRotationActivity to document that this
code path is only reached when host.InitiatedByFleet is true; specifically, just
before invoking newActivityFn(ctx, ... ,
fleet.ActivityTypeRotatedManagedLocalAccountPassword{... FleetInitiated: true
}), add a short comment like "// invariant: reached only when
host.InitiatedByFleet == true" to prevent future refactors from accidentally
removing the guard and to clarify why FleetInitiated is hard-coded true.
🪄 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: 97adc4cc-2d03-4944-8a85-f261f3893066
📒 Files selected for processing (22)
cmd/fleet/cron.gocmd/fleet/serve.goee/server/service/hosts.goee/server/service/hosts_test.goserver/datastore/mysql/managed_local_account.goserver/datastore/mysql/managed_local_account_test.goserver/fleet/activities.goserver/fleet/apple_mdm.goserver/fleet/cron_schedules.goserver/fleet/datastore.goserver/fleet/hosts.goserver/fleet/service.goserver/mdm/apple/apple_mdm.goserver/mdm/apple/apple_mdm_test.goserver/mdm/apple/commander.goserver/mdm/apple/commander_test.goserver/mock/datastore_mock.goserver/mock/service/service_mock.goserver/service/apple_mdm.goserver/service/handler.goserver/service/hosts.goserver/service/integration_mdm_test.go
There was a problem hiding this comment.
Pull request overview
Implements the backend password-rotation lifecycle for the macOS managed local admin account (_fleetadmin), including a datastore-driven state machine, MDM command enqueue/ack handling, an API endpoint to trigger rotation, and a cron job to perform auto-rotations after password view (mirroring the recovery-lock rotation pattern). This aligns with #43887’s requirements to decouple password_available from the status lifecycle and to ensure activities are logged at enqueue time (not at ack).
Changes:
- Add managed-local-account rotation datastore APIs/state transitions (
pending/verified/failed),auto_rotate_at, andpending_*fields handling. - Add
SetAutoAdminPasswordcommander support + ack handler plumbing and a premium-onlyPOST /hosts/:id/managed_local_account/rotateendpoint. - Add a premium cron schedule to enqueue rotations when
auto_rotate_atelapses, with activity logging rules for view-driven vs deferred-manual rows; expand integration/unit tests accordingly.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/integration_mdm_test.go | Extends integration coverage to exercise end-to-end rotation flows (view timer, manual rotate, auto rotate, deferred rotate, failure path). |
| server/service/hosts.go | Adds the non-EE endpoint/handler stub for rotate (returns ErrMissingLicense) and wires endpoint response. |
| server/service/handler.go | Registers the new rotate route under authenticated API routes. |
| server/service/apple_mdm.go | Handles SetAutoAdminPassword acknowledgements to complete/fail rotation state and log failure activity. |
| server/mock/service/service_mock.go | Extends service mock to include RotateManagedLocalAccountPassword. |
| server/mock/datastore_mock.go | Extends datastore mock with managed-local-account rotation state-machine methods. |
| server/mdm/apple/commander.go | Implements SetAutoAdminPassword MDM command enqueueing (base64-encoded passwordHash plist). |
| server/mdm/apple/commander_test.go | Adds unit test validating the SetAutoAdminPassword payload contains expected GUID, cmd UUID, and base64 hash data. |
| server/mdm/apple/apple_mdm.go | Adds cron job logic to enqueue managed-local-account rotations and log activity only when appropriate. |
| server/mdm/apple/apple_mdm_test.go | Adds tests for cron behavior (logging rules, benign races, APNs vs persistence errors). |
| server/fleet/service.go | Updates managed-account password doc semantics and adds service interface method for rotation. |
| server/fleet/hosts.go | Extends host MDM managed-local-account model with auto_rotate_at and pending_rotation; updates password_available semantics. |
| server/fleet/datastore.go | Adds rotation state-machine methods to the datastore interface with eligibility/behavior contracts. |
| server/fleet/cron_schedules.go | Introduces cron schedule name for managed-local-account rotation commands. |
| server/fleet/apple_mdm.go | Adds sentinel errors, command name constant, and cron host info struct for managed-local-account rotation. |
| server/fleet/activities.go | Adds rotated/failed-to-rotate managed-local-account activity types and automation attribution logic. |
| server/datastore/mysql/managed_local_account.go | Implements the rotation state machine in MySQL (view marking, initiate/defer/clear/complete/fail, due-for-rotation query). |
| server/datastore/mysql/managed_local_account_test.go | Adds MySQL unit tests covering rotation state transitions and eligibility logic. |
| ee/server/service/hosts.go | Implements premium rotation endpoint logic and updates password view behavior to gate on PasswordAvailable and start view-driven timers. |
| ee/server/service/hosts_test.go | Updates auth test to stub MarkManagedLocalAccountPasswordViewed. |
| cmd/fleet/serve.go | Registers the new premium cron schedule during server startup. |
| cmd/fleet/cron.go | Adds the cron schedule constructor that calls SendManagedLocalAccountRotationCommands. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
GitHub Actions
Error
GitHub Actions
Inactive
| if err != nil { | ||
| return ctxerr.Wrap(ctx, err, "get host lite") | ||
| } | ||
| // Authorize again with team loaded as "execute mdm_command", matching |
There was a problem hiding this comment.
nit: this comment seems slightly redundant.
There was a problem hiding this comment.
Fixed comment(just used the one used elsewhere - basically as a reminder that we have to do the double auth check)
| } | ||
|
|
||
| cmdUUID := uuid.NewString() | ||
| if err := svc.ds.InitiateManagedLocalAccountRotation(ctx, host.UUID, newPassword, cmdUUID); err != nil { |
There was a problem hiding this comment.
In this method, we allow initating on status=failed, but further up we fail if !acct.PasswordAvailable which it can only be if not failed, can you help me out a bit on the reasoning here?
There was a problem hiding this comment.
Perhaps I misunderstand but InitiatedManagedLocalAccountRotation should fail if status is failed with ErrManagedLocalAccountNotEligible since it checks the status and errors if no rows updated
| return svc.logRotateManagedLocalAccountActivity(ctx, host, false) | ||
| } | ||
|
|
||
| func (svc *Service) logRotateManagedLocalAccountActivity(ctx context.Context, host *fleet.Host, fleetInitiated bool) error { |
There was a problem hiding this comment.
seems this helper is never called with fleetInitiated=true, should we just remove the flag and always do true in this method? I assume the cron does not use this helper, and creates the struct itself with the value = true.
There was a problem hiding this comment.
You're right. That changed during development so the param is not needed
| // First view sets auto_rotate_at ~65 minutes in the future and flips status to pending. | ||
| rotateAt, err := ds.MarkManagedLocalAccountPasswordViewed(ctx, hostUUID) | ||
| require.NoError(t, err) | ||
| assert.WithinDuration(t, time.Now().Add(65*time.Minute), rotateAt, 30*time.Second) |
There was a problem hiding this comment.
nit: should we assert, this always sets initiated_by_fleet=true
There was a problem hiding this comment.
Added an assertion
| assert.Equal(t, "the-new-password", pwd.Password) | ||
|
|
||
| // Mismatched cmdUUID → notFound. | ||
| require.NoError(t, ds.InitiateManagedLocalAccountRotation(ctx, hostUUID, "another-password", "rot-cmd-comp2")) |
There was a problem hiding this comment.
nit: do we need to run this Initiate, to test the not found on mismatched pending cmd UUID?
There was a problem hiding this comment.
You're right. Removed the extra initiate and just added it earlier in the test
| assert.True(t, fleet.IsNotFound(err)) | ||
| } | ||
|
|
||
| func testManagedLocalAccountFailRotation(t *testing.T, ds *Datastore) { |
There was a problem hiding this comment.
nit: should we also exercise the not found path here?
MagnusHJensen
left a comment
MagnusHJensen
left a comment
There was a problem hiding this comment.
Other than some small comments looks and works great.
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/datastore/mysql/managed_local_account_test.go (1)
13-41: ⚡ Quick winIsolate each rotation case with its own datastore.
These subtests currently share one
Datastore, so rows left behind by earlier cases can leak into later ones.DeferredRotationleaves an auto-rotation candidate behind, andGetForAutoRotationwill happily see that extra row, which makes the assertions order-dependent and can hide query regressions.Suggested change
func TestManagedLocalAccount(t *testing.T) { - ds := CreateMySQLDS(t) - cases := []struct { name string fn func(t *testing.T, ds *Datastore) }{ @@ for _, c := range cases { t.Run(c.name, func(t *testing.T) { + ds := CreateMySQLDS(t) c.fn(t, ds) }) } }🤖 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/managed_local_account_test.go` around lines 13 - 41, The subtests under TestManagedLocalAccount share a single Datastore (ds) which lets earlier rotation tests leak rows into later ones; fix this by creating a fresh Datastore for each subtest: move the call to CreateMySQLDS(t) into each t.Run (or call CreateMySQLDS(t) at the start of each c.fn invocation) so each test function like testManagedLocalAccountDeferredRotation and testManagedLocalAccountGetForAutoRotation receives its own Datastore instance and cannot observe leftover rows from other subtests (ensure any teardown in CreateMySQLDS is still executed per subtest).
🤖 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/hosts.go`:
- Around line 767-778: The NotFound branch of
svc.ds.MarkManagedLocalAccountPasswordViewed is treated as "rotation in flight"
but currently doesn't update the response's pending flag; modify the branch so
that when fleet.IsNotFound(err) you explicitly set pwd.PendingRotation = true
(keeping the existing pwd.AutoRotateAt logic for the err==nil path), so the API
returns the pending rotation state even when
MarkManagedLocalAccountPasswordViewed returned NotFound.
---
Nitpick comments:
In `@server/datastore/mysql/managed_local_account_test.go`:
- Around line 13-41: The subtests under TestManagedLocalAccount share a single
Datastore (ds) which lets earlier rotation tests leak rows into later ones; fix
this by creating a fresh Datastore for each subtest: move the call to
CreateMySQLDS(t) into each t.Run (or call CreateMySQLDS(t) at the start of each
c.fn invocation) so each test function like
testManagedLocalAccountDeferredRotation and
testManagedLocalAccountGetForAutoRotation receives its own Datastore instance
and cannot observe leftover rows from other subtests (ensure any teardown in
CreateMySQLDS is still executed per subtest).
🪄 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: b7d95a1c-3bd4-41cf-8a88-1654ebc1ebbc
📒 Files selected for processing (4)
ee/server/service/hosts.goserver/datastore/mysql/managed_local_account.goserver/datastore/mysql/managed_local_account_test.goserver/mdm/apple/apple_mdm.go
🚧 Files skipped from review as they are similar to previous changes (1)
- server/datastore/mysql/managed_local_account.go
GitHub Actions
Inactive
Related issue: Resolves #43887
Adds the password rotation state machine for macOS local admin accounts. Changes file covered in prior PR
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
Summary by CodeRabbit
New Features
Behavior Changes
Tests