Release from AB backend support - #49680
Conversation
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
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 an Apple Business device release API with per-host statuses, admin authorization, Apple DEP disowning across multiple ABM tokens, activity logging, and DEP assignment cleanup. Introduces release response and authorization models, datastore access for active DEP assignments, service and datastore mocks, and authorization tests. Adds an Apple DEP throttling CLI that transfers a host between teams while polling assignment status. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 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: 5
🧹 Nitpick comments (2)
server/authz/policy.rego (1)
1009-1024: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
==for equality comparison.Using
==is preferred over=for equality comparison in Rego to maintain idiomatic syntax and avoid confusion with assignment.♻️ Proposed refactor
# Global admins can write/modify AB release devices allow { - object.type = "mdm_ab_release" + object.type == "mdm_ab_release" subject.global_role == admin action == write } # Team admins can write/modify AB release devices on their teams. allow { not is_null(object.team_id) - object.type = "mdm_ab_release" + object.type == "mdm_ab_release" object.team_id != 0 team_role(subject, object.team_id) == admin action == write }🤖 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/authz/policy.rego` around lines 1009 - 1024, Update the two AB release device allow rules to use Rego’s == equality operator wherever the diff currently uses =, including comparisons involving object.type, subject.global_role, and object.team_id. Preserve the existing authorization conditions and behavior.Source: Linters/SAST tools
server/datastore/mysql/apple_mdm.go (1)
8437-8450: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd an early return for an empty
hostIDsslice.
sqlx.Inwill return an error if thehostIDsslice is empty. While the current call site ensures the slice is populated, adding an early return is a good defensive programming practice to prevent potential runtime errors for future callers.🛡️ Proposed refactor
func (ds *Datastore) GetHostDEPAssignmentsByHostIDs(ctx context.Context, hostIDs []uint) ([]*fleet.HostDEPAssignment, error) { + if len(hostIDs) == 0 { + return nil, nil + } var res []*fleet.HostDEPAssignment query, args, err := sqlx.In(`SELECT host_id, added_at, deleted_at, abm_token_id, mdm_migration_deadline, mdm_migration_completed, hardware_serial FROM host_dep_assignments hdep🤖 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/apple_mdm.go` around lines 8437 - 8450, Add an early return at the start of GetHostDEPAssignmentsByHostIDs when hostIDs is empty, returning an empty assignment slice and nil error before calling sqlx.In. Preserve the existing query and error handling for non-empty hostIDs.
🤖 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/apple_mdm.go`:
- Line 525: Remove the debug fmt.Printf call that logs dedupedTokenIDs and
tokens, leaving the surrounding Apple MDM token-deduplication flow unchanged and
ensuring sensitive ABMToken contents are not written to stdout.
- Around line 561-577: Guard the disownResp.Devices loop before using hostID:
verify each serial exists in serialToHostID and skip unknown serials. Apply this
before both setResponse paths so unknown Apple-echoed serials cannot dereference
hostIDToLiteHost or record a response with hostID 0; preserve existing handling
for known hosts.
In `@tools/mdm/apple/throttle.go`:
- Around line 61-64: Extend the startup validation alongside the existing token,
baseURL, and hostID checks to reject an interval value of 0 or less before any
ticker is created. Include the interval flag in the usage error, preserving the
existing fatal validation behavior and preventing subsequent time.NewTicker
calls from receiving an invalid duration.
- Around line 69-74: Configure a finite request timeout when creating the HTTP
client in the client initialization block, using the existing fleethttp client
construction mechanism and an appropriate duration. Keep the baseURL, token, and
hostID initialization unchanged, and ensure requests cannot block indefinitely.
- Around line 52-64: Update the token handling in the flag setup and validation
around token to prefer a suitable environment variable when the command-line
token is omitted, while preserving explicit CLI token support. Ensure the
resolved token is used by the transfer logic and retain the existing
required-token validation.
---
Nitpick comments:
In `@server/authz/policy.rego`:
- Around line 1009-1024: Update the two AB release device allow rules to use
Rego’s == equality operator wherever the diff currently uses =, including
comparisons involving object.type, subject.global_role, and object.team_id.
Preserve the existing authorization conditions and behavior.
In `@server/datastore/mysql/apple_mdm.go`:
- Around line 8437-8450: Add an early return at the start of
GetHostDEPAssignmentsByHostIDs when hostIDs is empty, returning an empty
assignment slice and nil error before calling sqlx.In. Preserve the existing
query and error handling for non-empty hostIDs.
🪄 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: af5dd32b-bf88-4ed2-82c3-3c5cdfa2671f
📒 Files selected for processing (17)
changes/47633-release-ab-deviceee/server/service/apple_mdm.goee/server/service/apple_mdm_test.goserver/authz/policy.regoserver/datastore/mysql/apple_mdm.goserver/datastore/mysql/apple_mdm_test.goserver/fleet/activities.goserver/fleet/apple_mdm.goserver/fleet/datastore.goserver/fleet/service.goserver/mdm/nanodep/client/transport.goserver/mdm/nanodep/godep/profile.goserver/mock/datastore_mock.goserver/mock/service/service_mock.goserver/service/apple_mdm.goserver/service/handler.gotools/mdm/apple/throttle.go
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Adds backend support for releasing Apple devices from Apple Business Manager (ABM) via Fleet, including new API routing, authz policy, datastore support for DEP assignment lookup by host IDs, and EE service implementation with tests. This fits into Fleet’s Apple MDM/DEP tooling by enabling “disown device” operations and recording related activities.
Changes:
- Added
POST /api/_version_/fleet/hosts/release_abendpoint and service interface plumbing (CE returnsErrMissingLicense, EE implements behavior). - Implemented EE
ReleaseABDevicesflow: validate hosts, authorize across teams, group by ABM token, call DEP Disown Devices, clear DEP assignments, and create host-level activity. - Added datastore method + MySQL query + tests to fetch DEP assignments by host IDs; updated nanodep protocol/version structs.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/mdm/apple/throttle.go | New utility to reproduce Apple DEP throttling via repeated team transfers + polling. |
| server/service/handler.go | Registers the new hosts/release_ab API route behind Apple MDM-config middleware. |
| server/service/apple_mdm.go | Adds request/response types + CE stub endpoint returning ErrMissingLicense. |
| server/mock/service/service_mock.go | Extends mock service with ReleaseABDevices. |
| server/mock/datastore_mock.go | Extends mock datastore with GetHostDEPAssignmentsByHostIDs. |
| server/mdm/nanodep/godep/profile.go | Extends DEP assign-profile response model with retry_after_seconds. |
| server/mdm/nanodep/client/transport.go | Bumps default DEP server protocol version to 10. |
| server/fleet/service.go | Adds ReleaseABDevices to the Fleet service interface. |
| server/fleet/datastore.go | Adds GetHostDEPAssignmentsByHostIDs to datastore interface + docs. |
| server/fleet/apple_mdm.go | Adds HardwareSerial to HostDEPAssignment and defines AB release status/response + authz type. |
| server/fleet/activities.go | Adds host-scoped activity type released_from_ab. |
| server/datastore/mysql/apple_mdm.go | Implements GetHostDEPAssignmentsByHostIDs query. |
| server/datastore/mysql/apple_mdm_test.go | Adds unit test coverage for GetHostDEPAssignmentsByHostIDs. |
| server/authz/policy.rego | Adds authz rules for the new mdm_ab_release object type. |
| ee/server/service/apple_mdm.go | Implements AB release logic: grouping by token, calling DEP disown, per-host results, activity logging, cleanup. |
| ee/server/service/apple_mdm_test.go | Adds comprehensive tests for authz, per-host errors, multi-token behavior, activity logging, and cleanup behavior. |
Files excluded by content exclusion policy (1)
- changes/47633-release-ab-device
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #49680 +/- ##
==========================================
+ Coverage 67.89% 67.92% +0.02%
==========================================
Files 3890 3890
Lines 248437 248798 +361
Branches 13022 13022
==========================================
+ Hits 168679 168988 +309
- Misses 64543 64577 +34
- Partials 15215 15233 +18
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:
|
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 3 comments.
Files excluded by content exclusion policy (1)
- changes/47633-release-ab-device
| const ( | ||
| ABReleaseDeviceStatusSuccess ABReleaseDeviceStatus = "success" | ||
| ABReleaseDeviceStatusError ABReleaseDeviceStatus = "failed" | ||
| ) | ||
|
|
There was a problem hiding this comment.
If we get another status than success from the API we embed the status in the error message, and mark it as failed for our own response_type that way we can type it, and still allow whatever to be shown and returned from the DEP API, let me know if we want this different
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/datastore/mysql/apple_mdm.go (1)
8443-8445: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDo not filter out assignment states handled by the release service.
ReleaseABDevicesexplicitly handles assignments with a missingABMTokenID(and missing hardware serial), but these predicates prevent the real datastore from returning them. Those hosts therefore fall through to “not found in Apple Business” instead of receiving the intended per-device error; the supplied test only passes because the datastore is mocked.Remove these predicates, or change the service/test contract to intentionally treat such assignments as absent.
🤖 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/apple_mdm.go` around lines 8443 - 8445, The query used by ReleaseABDevices must not exclude assignments with missing abm_token_id or hardware_serial. Remove the hdep.abm_token_id IS NOT NULL and hdep.hardware_serial != '' predicates so those assignments reach the service and receive their intended per-device errors.Source: Path instructions
🤖 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.
Outside diff comments:
In `@server/datastore/mysql/apple_mdm.go`:
- Around line 8443-8445: The query used by ReleaseABDevices must not exclude
assignments with missing abm_token_id or hardware_serial. Remove the
hdep.abm_token_id IS NOT NULL and hdep.hardware_serial != '' predicates so those
assignments reach the service and receive their intended per-device errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 56ad606b-c61a-4eb7-b1f8-90044d9fcf3c
📒 Files selected for processing (7)
ee/server/service/apple_mdm.goee/server/service/apple_mdm_test.goserver/authz/policy.regoserver/authz/policy_test.goserver/datastore/mysql/apple_mdm.goserver/fleet/apple_mdm.gotools/mdm/apple/throttle.go
🚧 Files skipped from review as they are similar to previous changes (4)
- tools/mdm/apple/throttle.go
- server/authz/policy.rego
- ee/server/service/apple_mdm.go
- ee/server/service/apple_mdm_test.go
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 3 comments.
Files excluded by content exclusion policy (1)
- changes/47633-release-ab-device
Comments suppressed due to low confidence (1)
ee/server/service/apple_mdm.go:439
serialToHostIDassumes hardware serials are unique across the requested host IDs. If multiple hosts share the sameHardwareSerial(possible;host_dep_assignments.hardware_serialis not unique), later entries overwrite earlier ones, which can cause some requested host IDs to never get a result inresponse(they’ll be omitted from the returned slice).
serialToHostID := make(map[string]uint, len(liteHosts))
teamIDs := make(map[uint]struct{}, len(liteHosts))
for _, h := range liteHosts {
hostIDToLiteHost[h.ID] = h
delete(unseenHostIDs, h.ID)
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Copilot reviewed 18 out of 19 changed files in this pull request and generated 3 comments.
Files excluded by content exclusion policy (1)
- changes/47633-release-ab-device
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Copilot reviewed 19 out of 20 changed files in this pull request and generated 1 comment.
Files excluded by content exclusion policy (1)
- changes/47633-release-ab-device
Comments suppressed due to low confidence (2)
ee/server/service/apple_mdm.go:468
serialToHostIDis amap[string]uint, so if multiple selected hosts share the sameHardwareSerial(duplicate host records are possible in Fleet), the later host overwrites the earlier one. That can lead to missing/incorrect per-host results (one host may never get a success/failure reflecting the release), and the activity/logging will only reference one of the hosts for that serial.
Consider tracking serial -> []hostID (or maintaining a separate hostID -> serial) so a single disown result can be applied to all host records with that serial, and all requested host IDs get a deterministic result.
serialToHostID := make(map[string]uint, len(liteHosts))
teamIDs := make(map[uint]struct{}, len(liteHosts))
for _, h := range liteHosts {
hostIDToLiteHost[h.ID] = h
delete(unseenHostIDs, h.ID)
if h.TeamID != nil {
teamIDs[*h.TeamID] = struct{}{}
} else {
teamIDs[0] = struct{}{}
}
if !fleet.IsApplePlatform(h.FleetPlatform()) {
setErrorResponse(h.ID, fleet.ABReleaseDeviceStatusError, "This is not an eligible Apple host.")
continue
}
if h.HardwareSerial == "" {
setErrorResponse(h.ID, fleet.ABReleaseDeviceStatusError, "Host has no hardware serial.")
continue
}
serialToHostID[h.HardwareSerial] = h.ID
}
ee/server/service/apple_mdm.go:557
- This debug log includes the full
serialsslice for each ABM token. For large bulk releases this can generate very large log lines and unnecessarily expose device serials in logs. Logging a count (and optionally a small sample) is safer and avoids log-volume spikes.
svc.logger.DebugContext(ctx, "Releasing AB devices", "token_id", tokenID, "organization_name", token.OrganizationName, "serials", serials)
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Copilot reviewed 19 out of 20 changed files in this pull request and generated 3 comments.
Files excluded by content exclusion policy (1)
- changes/47633-release-ab-device
Comments suppressed due to low confidence (2)
ee/server/service/apple_mdm.go:473
hardware_serialis not unique onhosts, soserialToHostID[h.HardwareSerial] = h.IDcan overwrite an earlier host with the same serial. That can lead to missing per-host results (some requested host IDs never get a response entry). Consider trackingserial -> []hostID(or explicitly erroring on duplicates) so every input host ID gets a status.
setErrorResponse(h.ID, fleet.ABReleaseDeviceStatusError, "Host has no hardware serial.")
continue
}
serialToHostID[h.HardwareSerial] = h.ID
ee/server/service/apple_mdm.go:563
- Logging the full
serialsslice can produce extremely large log entries (up to 32k serials) and may include device identifiers. Consider logging a count (and maybe token/org) instead.
svc.logger.DebugContext(ctx, "Releasing AB devices", "token_id", tokenID, "organization_name", token.OrganizationName, "serials", serials)
Related issue: Resolves #49367
Checklist for submitter
If some of the following don't apply, delete the relevant line.
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Input data is properly validated,
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.Timeouts are implemented and retries are limited to avoid infinite loops
If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes
Testing
Added/updated automated tests
Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)
QA'd all new/changed functionality manually
Summary by CodeRabbit