Remove unenroll pending and add Android COBO wipe to Free - #46653
Conversation
|
@coderabbitai full review |
|
/agentic_review |
✅ Actions performedFull review triggered. |
Code Review by Qodo
1.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #46653 +/- ##
==========================================
- Coverage 66.88% 66.87% -0.01%
==========================================
Files 2814 2814
Lines 224028 224315 +287
Branches 11491 11383 -108
==========================================
+ Hits 149834 150013 +179
- Misses 60601 60675 +74
- Partials 13593 13627 +34
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:
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
WalkthroughThis PR implements Android device wipe commands for Fleet Free users targeting company-owned hosts exclusively, while suppressing transient wipe states for personally-owned Android enrollments. The backend implements the complete WipeHost service with authorization, Android-only validation, and pending state checks. A new helper function suppresses device status and pending action for Android BYO hosts across all host-loading paths to hide UI artifacts. The frontend simplifies the unenroll modal by removing optimistic status updates, updates wipe dropdown gating to allow Fleet Free access for Android COBO, and removes special-case status display logic now that suppression is handled server-side. Possibly related issues
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. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/pages/hosts/details/HostDetailsPage/HostDetailsPage.tsx (1)
1718-1725:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep BYO Android unenroll hidden after submit.
Removing the success callback leaves this page with no way to mark BYO Android unenroll as pending. In this PR the host reads are normalized back to
device_status="unlocked"/ no pending action, while the dropdown still hides AndroidUnenrollonly whenhostMdmDeviceStatus !== "unlocked". That means the action stays visible immediately after a successful unenroll and can be fired again until some other field changes. Preserve a local pending flag here, or gate the dropdown on a dedicated server-side unenroll-pending signal instead.🤖 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/hosts/details/HostDetailsPage/HostDetailsPage.tsx` around lines 1718 - 1725, HostDetailsPage currently loses the "unenroll pending" state after a successful BYO Android unenroll because the success callback was removed; add a local state flag (e.g., pendingByoAndroidUnenroll) in HostDetailsPage and set it true from the UnenrollMdmModal success handler (pass a prop like onSuccess to UnenrollMdmModal), then use that flag alongside hostMdmDeviceStatus (and/or host.mdm.device_status) to hide the Android Unenroll action in the dropdown; clear the flag when the modal closes or when the host is refreshed/host.mdm.device_status changes away from "unlocked" so the UI stays consistent until the server-side state changes.
🧹 Nitpick comments (1)
server/service/hosts_test.go (1)
3541-3569: ⚡ Quick winAdd one core-service COBO happy-path test.
These cases only prove the early exits. They never reach the new Android COBO branch in
server/service/scripts.gothat checks config/connectivity, callsandroidSvc.WipeAndroidHost, and recordsActivityTypeWipedHost. Because Premium overridesWipeHost, premium integration coverage won’t exercise this core implementation.🤖 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/hosts_test.go` around lines 3541 - 3569, Add a COBO Android happy-path subtest to TestWipeHostFreeTierGating that exercises the core-service android branch: mock ds.HostFunc/HostLiteFunc to return a host with Platform "android" and MDM.EnrollmentStatus set to a company/COBO value (e.g., "On (company)"), inject a mocked android service on svc where androidSvc.WipeAndroidHost returns nil, call svc.WipeHost(ctx, hostID, nil) and assert no error, and also assert that androidSvc.WipeAndroidHost was invoked and that an activity of type ActivityTypeWipedHost was recorded (or the DS activity create/update mock was called accordingly) so the core implementation in server/service/scripts.go is 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/scripts.go`:
- Around line 1285-1294: When svc.ds.Host returns an error, don’t immediately
reveal host existence; follow the RunHostScript/GetScriptResult fallback-auth
pattern: if fleet.IsNotFound(err) then call svc.authz.Authorize(ctx, <no-team
wipe target>) (i.e. authorize a wipe action with no TeamID) and only return the
not-found error if that authorization succeeds; for other errors keep the
existing ctxerr.Wrap behavior; otherwise continue to the existing team-scoped
Authorize call (svc.authz.Authorize with fleet.MDMCommandAuthz{TeamID:
host.TeamID}) so team-scoped users cannot distinguish “missing host” from
“exists in another team.”
- Around line 1312-1316: The error branch is checking the wrong sentinel so the
Android-specific error handling never runs; update the errors.Is comparison in
the VerifyMDMAndroidConfigured error path to test for
fleet.ErrAndroidMDMNotConfigured (instead of fleet.ErrMDMNotConfigured) so that
when svc.VerifyMDMAndroidConfigured(ctx) returns that sentinel you construct the
fleet.NewInvalidArgumentError("host_id",
fleet.AndroidMDMNotConfiguredMessage).WithStatus(http.StatusBadRequest) before
returning via ctxerr.Wrap(ctx, err, "check android MDM enabled").
---
Outside diff comments:
In `@frontend/pages/hosts/details/HostDetailsPage/HostDetailsPage.tsx`:
- Around line 1718-1725: HostDetailsPage currently loses the "unenroll pending"
state after a successful BYO Android unenroll because the success callback was
removed; add a local state flag (e.g., pendingByoAndroidUnenroll) in
HostDetailsPage and set it true from the UnenrollMdmModal success handler (pass
a prop like onSuccess to UnenrollMdmModal), then use that flag alongside
hostMdmDeviceStatus (and/or host.mdm.device_status) to hide the Android Unenroll
action in the dropdown; clear the flag when the modal closes or when the host is
refreshed/host.mdm.device_status changes away from "unlocked" so the UI stays
consistent until the server-side state changes.
---
Nitpick comments:
In `@server/service/hosts_test.go`:
- Around line 3541-3569: Add a COBO Android happy-path subtest to
TestWipeHostFreeTierGating that exercises the core-service android branch: mock
ds.HostFunc/HostLiteFunc to return a host with Platform "android" and
MDM.EnrollmentStatus set to a company/COBO value (e.g., "On (company)"), inject
a mocked android service on svc where androidSvc.WipeAndroidHost returns nil,
call svc.WipeHost(ctx, hostID, nil) and assert no error, and also assert that
androidSvc.WipeAndroidHost was invoked and that an activity of type
ActivityTypeWipedHost was recorded (or the DS activity create/update mock was
called accordingly) so the core implementation in server/service/scripts.go is
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
Run ID: 4f6f15bb-1462-4218-83a6-5db774c3a7e1
📒 Files selected for processing (11)
changes/41683-android-lock-wipe-clear-passcodefrontend/pages/hosts/details/HostDetailsPage/HostActionsDropdown/HostActionsDropdown.tests.tsxfrontend/pages/hosts/details/HostDetailsPage/HostActionsDropdown/helpers.tsxfrontend/pages/hosts/details/HostDetailsPage/HostDetailsPage.tsxfrontend/pages/hosts/details/HostDetailsPage/modals/UnenrollMdmModal/UnenrollMdmModal.tsxfrontend/pages/hosts/details/cards/HostHeader/HostHeader.tests.tsxfrontend/pages/hosts/details/cards/HostHeader/HostHeader.tsxserver/service/hosts.goserver/service/hosts_test.goserver/service/labels.goserver/service/scripts.go
💤 Files with no reviewable changes (1)
- frontend/pages/hosts/details/HostDetailsPage/modals/UnenrollMdmModal/UnenrollMdmModal.tsx
There was a problem hiding this comment.
Pull request overview
This PR adjusts Android MDM command behavior to (1) stop surfacing a transient “unenroll pending” state for Android BYOD unenroll (which internally triggers a work-profile wipe) and (2) make Android COBO wipe available on Fleet Free (UI + core service implementation), while keeping wipe on other platforms Premium-only.
Changes:
- Implemented Fleet Free
WipeHostbehavior for Android (COBO), including authz + validations + activity creation. - Added backend suppression of the transient BYOD-unenroll wipe state so UI no longer shows an “Unenroll pending” badge.
- Updated frontend gating + tests to show Android COBO Wipe on Free and removed the special BYOD “Unenroll pending” UI override.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/scripts.go | Implements core (Fleet Free) Android wipe flow and license gating for other platforms. |
| server/service/labels.go | Applies BYOD wipe-status suppression in label host listings when device status is included. |
| server/service/hosts.go | Adds helper to suppress transient BYOD-unenroll wipe status in host list/detail flows. |
| server/service/hosts_test.go | Adds unit tests for suppression helper and Free-tier wipe gating. |
| frontend/pages/hosts/details/HostDetailsPage/modals/UnenrollMdmModal/UnenrollMdmModal.tsx | Removes now-unneeded onSuccess callback used for optimistic “unenroll pending” UI state. |
| frontend/pages/hosts/details/HostDetailsPage/HostDetailsPage.tsx | Removes Android BYOD “set host mdm device state to wiping” optimistic update. |
| frontend/pages/hosts/details/HostDetailsPage/HostActionsDropdown/HostActionsDropdown.tests.tsx | Adds/updates dropdown tests for Free-tier Android COBO wipe and removes BYOD “pending unenroll” case. |
| frontend/pages/hosts/details/HostDetailsPage/HostActionsDropdown/helpers.tsx | Updates wipe eligibility logic to allow Android COBO wipe on Fleet Free. |
| frontend/pages/hosts/details/cards/HostHeader/HostHeader.tsx | Removes special-case “Unenroll pending” label/tooltip override for BYOD Android. |
| frontend/pages/hosts/details/cards/HostHeader/HostHeader.tests.tsx | Updates HostHeader tests to reflect removal of “Unenroll pending” badge behavior. |
| changes/41683-android-lock-wipe-clear-passcode | Documents Free-tier availability of Android COBO wipe while keeping other platforms Premium-only. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
There was a problem hiding this comment.
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 (2)
ee/server/service/hosts.go (1)
123-128:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSame sentinel mismatch exists in
LockHostfor Android.
VerifyMDMAndroidConfiguredreturnsfleet.ErrAndroidMDMNotConfigured, but line 124 checks againstfleet.ErrMDMNotConfigured. This branch never fires, so the intended Android-specific bad-request message is skipped when Android MDM is disabled—identical to the bug fixed inWipeHostat line 333.Suggested fix
case "android": // Lock is supported for BYO and COBO. The Android Service.LockAndroidHost call enforces // that the host is enrolled (svc.ds.AndroidHostLiteByHostUUID -> NotFound otherwise). if err := svc.VerifyMDMAndroidConfigured(ctx); err != nil { - if errors.Is(err, fleet.ErrMDMNotConfigured) { + if errors.Is(err, fleet.ErrAndroidMDMNotConfigured) { err = fleet.NewInvalidArgumentError("host_id", fleet.AndroidMDMNotConfiguredMessage).WithStatus(http.StatusBadRequest) } return "", ctxerr.Wrap(ctx, err, "check android MDM enabled") }🤖 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/hosts.go` around lines 123 - 128, The Android sentinel mismatch in LockHost means the branch never fires; update the error check in the LockHost handling of svc.VerifyMDMAndroidConfigured(ctx) to compare against fleet.ErrAndroidMDMNotConfigured (not fleet.ErrMDMNotConfigured) and, when matched, build the bad-request using fleet.NewInvalidArgumentError("host_id", fleet.AndroidMDMNotConfiguredMessage). Keep the existing ctxerr.Wrap(ctx, err, "check android MDM enabled") behavior otherwise.server/service/hosts.go (1)
307-307:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove debug print statements.
These
fmt.Printlnstatements appear to be debugging artifacts that were left in the code. They should be removed before merging to production as they will pollute logs and provide no value.🐛 Proposed fix
st, err := svc.SoftwareTitleByID(ctx, titleID, req.Opts.TeamFilter) switch { case err == nil: - fmt.Println("regular") softwareTitle = st case fleet.IsNotFound(err): // Not found: only ID + Name as string from helper. name, displayName, errName := svc.SoftwareTitleNameForHostFilter(ctx, titleID) if errName != nil && !fleet.IsNotFound(errName) { return listHostsResponse{Err: errName}, nil } if errName == nil { - fmt.Println("here") softwareTitle = &fleet.SoftwareTitle{ ID: titleID, }Also applies to: 317-317
🤖 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/hosts.go` at line 307, Remove the stray debug print statements by deleting the fmt.Println("regular") calls (and any other plain fmt.Println usages) left in hosts.go; if you need to surface runtime info keep or replace them with the project's logger (e.g., processLogger or the existing logging helper) at the appropriate level, and ensure no plain fmt.Println calls remain (specifically remove the occurrences of fmt.Println("regular") around the previously noted lines).
🤖 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_core_test.go`:
- Around line 8356-8360: Add a positive test that creates an Android COBO host
and asserts the wipe endpoint returns 200 OK: use s.createHosts(t, "android")
(or the appropriate helper variant that produces an Android COBO host) to obtain
wipeHost, then call s.Do("POST", fmt.Sprintf("/api/v1/fleet/hosts/%d/wipe",
wipeHost.ID), nil, http.StatusOK). Place this alongside the existing Darwin
checks so the test verifies Android COBO wipe succeeds on Fleet Free while
Darwin still returns StatusPaymentRequired.
---
Outside diff comments:
In `@ee/server/service/hosts.go`:
- Around line 123-128: The Android sentinel mismatch in LockHost means the
branch never fires; update the error check in the LockHost handling of
svc.VerifyMDMAndroidConfigured(ctx) to compare against
fleet.ErrAndroidMDMNotConfigured (not fleet.ErrMDMNotConfigured) and, when
matched, build the bad-request using fleet.NewInvalidArgumentError("host_id",
fleet.AndroidMDMNotConfiguredMessage). Keep the existing ctxerr.Wrap(ctx, err,
"check android MDM enabled") behavior otherwise.
In `@server/service/hosts.go`:
- Line 307: Remove the stray debug print statements by deleting the
fmt.Println("regular") calls (and any other plain fmt.Println usages) left in
hosts.go; if you need to surface runtime info keep or replace them with the
project's logger (e.g., processLogger or the existing logging helper) at the
appropriate level, and ensure no plain fmt.Println calls remain (specifically
remove the occurrences of fmt.Println("regular") around the previously noted
lines).
🪄 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: babd1067-0821-4def-90d0-6461d30c7e14
📒 Files selected for processing (11)
ee/server/service/hosts.gofrontend/pages/hosts/details/HostDetailsPage/HostActionsDropdown/HostActionsDropdown.tests.tsxfrontend/pages/hosts/details/HostDetailsPage/HostActionsDropdown/helpers.tsxfrontend/pages/hosts/details/cards/HostHeader/HostHeader.tests.tsxserver/fleet/datastore.goserver/mdm/android/service/service.goserver/service/hosts.goserver/service/hosts_test.goserver/service/integration_core_test.goserver/service/integration_mdm_test.goserver/service/scripts.go
💤 Files with no reviewable changes (2)
- frontend/pages/hosts/details/cards/HostHeader/HostHeader.tests.tsx
- frontend/pages/hosts/details/HostDetailsPage/HostActionsDropdown/HostActionsDropdown.tests.tsx
✅ Files skipped from review due to trivial changes (1)
- server/mdm/android/service/service.go
🚧 Files skipped from review as they are similar to previous changes (2)
- server/service/hosts_test.go
- frontend/pages/hosts/details/HostDetailsPage/HostActionsDropdown/helpers.tsx
| // NOTE: keep this Android wipe flow in sync with the Premium implementation in ee/server/service/hosts.go | ||
| // (WipeHost + enqueueWipeHostRequest). Wipe is COBO-only for Android; BYO unenroll already runs an AMAPI WIPE | ||
| // under the hood (see UnenrollAndroidHost), so routing BYO hosts through Wipe would be redundant + misleading. | ||
| if host.MDM.EnrollmentStatus != nil && *host.MDM.EnrollmentStatus == "On (personal)" { |
There was a problem hiding this comment.
Non-blocking. But I like how the front end does this
https://github.com/fleetdm/fleet/blob/main/frontend/interfaces/mdm.ts#L55-L61
makes these feel more like constants vs random strings littered in the code. Could we do the same in go?
const (
MDMEnrollmentStatusPersonal = "On (personal)"
MDMEnrollmentStatusAutomatic = "On (automatic)"
MDMEnrollmentStatusManual = "On (manual)"
MDMEnrollmentStatusPending = "Pending"
MDMEnrollmentStatusOff = "Off"
)
Perhaps in server/fleet/hosts.go.
Then these become *host.MDM.EnrollmentStatus == fleet.MDMEnrollmentStatusPersonal which make it feel a bit cleaner.
There was a problem hiding this comment.
I agree, but that would have been a bunch more lines of changes, so I didn't do this fix at this time.
| return fleet.ErrMissingLicense | ||
| } | ||
|
|
||
| // NOTE: keep this Android wipe flow in sync with the Premium implementation in ee/server/service/hosts.go |
There was a problem hiding this comment.
Non-blocking. But seems like a better way to do this would be to extract a validateAndroidWipeRequest(ctx, host, ds) error helper that both call.
There was a problem hiding this comment.
I can do a follow up PR for this.
Related issue: Resolves #41683
Unenroll/wipe Android on Fleet Free: https://www.youtube.com/watch?v=JvsD3WBcDgE
Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.Testing
Summary by CodeRabbit
New Features
Bug Fixes