47700 abm token invalid errors#49770
Conversation
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
This PR expands Apple Business Manager (ABM/ADE/DEP) error detection and reporting so Fleet can (a) persist a token_invalid flag on ABM tokens when Apple rejects/revokes the token, and (b) return a structured dep_error classification on the DEP assignment endpoint instead of surfacing Apple API errors directly.
Changes:
- Add detection for DEP
token_rejectedandsignature_invaliderrors and persist token invalid state (abm_tokens.token_invalid). - Add
dep_errorto the host DEP assignment response and classify Apple DEP “device details” failures into a stable enum. - Add/extend unit and datastore tests to cover the new token-invalid and classification behavior.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/hosts.go | Extends DEP assignment endpoint response with dep_error and threads classification through service method. |
| server/service/apple_mdm_test.go | Stubs new datastore method in service tests to satisfy updated ABM updater interface. |
| server/mock/service/service_mock.go | Updates mock service signature for GetHostDEPAssignmentDetails to include DEPDeviceErrorType. |
| server/mock/datastore_mock.go | Adds mocked datastore method for updating ABM token invalid state. |
| server/mdm/nanodep/godep/client.go | Adds IsServerError helper to detect DEP 5xx HTTP errors. |
| server/mdm/nanodep/godep/account.go | Adds IsTokenRejected and IsSignatureInvalid error classifiers. |
| server/mdm/nanodep/godep/account_test.go | Adds unit tests for the new DEP error classifier helpers. |
| server/mdm/apple/apple_mdm.go | Updates DEP client after-hook to set/clear token_invalid; adds ClassifyDEPDeviceError. |
| server/mdm/apple/apple_mdm_test.go | Adds tests verifying token_invalid updates and DEP device error classification. |
| server/fleet/service.go | Updates Fleet service interface to return (depError, err) for DEP assignment details. |
| server/fleet/mdm.go | Adds TokenInvalid to ABMToken and expands ABMTermsUpdater with token-invalid setter. |
| server/fleet/mdm_test.go | Extends DEP client tests to account for token_invalid bookkeeping behavior. |
| server/fleet/datastore.go | Adds datastore interface method SetABMTokenInvalidForOrgName. |
| server/fleet/apple_mdm.go | Introduces DEPDeviceErrorType enum used by dep_assignment response. |
| server/datastore/mysql/apple_mdm.go | Selects token_invalid in ABM token queries and implements SetABMTokenInvalidForOrgName. |
| server/datastore/mysql/apple_mdm_test.go | Adds datastore integration test coverage for ABM token invalid flag updates. |
| changes/47700-abm-token-invalid-dep-error | User-visible changelog entry (content excluded from diff). |
Files excluded by content exclusion policy (1)
- changes/47700-abm-token-invalid-dep-error
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WalkthroughAdds Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/mdm/apple/apple_mdm.go (1)
1062-1062: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
IsTokenRejected || IsSignatureInvalidpredicate.This OR-condition is repeated verbatim in the after-hook and in
ClassifyDEPDeviceError. Extracting a singlegodep.IsTokenInvalid(err) boolhelper would avoid the two call sites drifting if a third "token invalid" cause is added later.♻️ Proposed helper (in the godep package)
// IsTokenInvalid returns true if err indicates the ABM token itself is no // longer usable (rejected, or its signature is invalid). func IsTokenInvalid(err error) bool { return IsTokenRejected(err) || IsSignatureInvalid(err) }Also applies to: 1142-1142
🤖 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` at line 1062, Extract the repeated token-invalid predicate into a godep.IsTokenInvalid(err) helper that combines IsTokenRejected and IsSignatureInvalid, then replace the duplicate OR conditions in the after-hook and ClassifyDEPDeviceError with this helper while preserving the existing reqErr nil guard.
🤖 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.
Nitpick comments:
In `@server/mdm/apple/apple_mdm.go`:
- Line 1062: Extract the repeated token-invalid predicate into a
godep.IsTokenInvalid(err) helper that combines IsTokenRejected and
IsSignatureInvalid, then replace the duplicate OR conditions in the after-hook
and ClassifyDEPDeviceError with this helper while preserving the existing reqErr
nil guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6737e8d3-e03f-4e90-a7cc-0908ca9f7100
📒 Files selected for processing (17)
changes/47700-abm-token-invalid-dep-errorserver/datastore/mysql/apple_mdm.goserver/datastore/mysql/apple_mdm_test.goserver/fleet/apple_mdm.goserver/fleet/datastore.goserver/fleet/mdm.goserver/fleet/mdm_test.goserver/fleet/service.goserver/mdm/apple/apple_mdm.goserver/mdm/apple/apple_mdm_test.goserver/mdm/nanodep/godep/account.goserver/mdm/nanodep/godep/account_test.goserver/mdm/nanodep/godep/client.goserver/mock/datastore_mock.goserver/mock/service/service_mock.goserver/service/apple_mdm_test.goserver/service/hosts.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 16 out of 17 changed files in this pull request and generated 1 comment.
Files excluded by content exclusion policy (1)
- changes/47700-abm-token-invalid-dep-error
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 16 out of 17 changed files in this pull request and generated 1 comment.
Files excluded by content exclusion policy (1)
- changes/47700-abm-token-invalid-dep-error
Comments suppressed due to low confidence (1)
server/mdm/apple/apple_mdm.go:1073
- The token_invalid flag is only cleared on successful requests. If Apple stops returning token_rejected/signature_invalid but returns a different definitive error like T_C_NOT_SIGNED (terms expired), token_invalid will remain stuck true until a successful request happens (which may not occur while terms are expired). Consider also updating token_invalid when terms-expired errors occur so the flag reflects the current Apple response.
tokenInvalid := reqErr != nil && (godep.IsTokenRejected(reqErr) || godep.IsSignatureInvalid(reqErr))
if reqErr == nil || tokenInvalid {
if _, err := updater.SetABMTokenInvalidForOrgName(ctx, orgName, tokenInvalid); err != nil {
logger.ErrorContext(ctx, "Apple DEP client: failed to update token invalid status of ABM token", "err", err)
}
}
// if the request failed due to terms not signed, or if it succeeded,
// update the ABM token's (and possibly the app config's) flag accordingly.
// If it failed for any other reason, do not update the flag.
termsExpired := reqErr != nil && godep.IsTermsNotSigned(reqErr)
if reqErr == nil || termsExpired {
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #49770 +/- ##
==========================================
+ Coverage 67.85% 67.91% +0.06%
==========================================
Files 3905 3905
Lines 249669 249499 -170
Branches 13142 13064 -78
==========================================
+ Hits 169403 169449 +46
+ Misses 65042 64766 -276
- Partials 15224 15284 +60
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 22 out of 23 changed files in this pull request and generated no new comments.
Files excluded by content exclusion policy (1)
- changes/47700-abm-token-invalid-dep-error
Comments suppressed due to low confidence (1)
server/mdm/apple/apple_mdm.go:1066
token_invalidis only cleared on successful DEP requests. If a token previously hittoken_rejected/signature_invalid(setting the flag) and later Apple stops returning those errors but instead returns a different DEP error (notablyT_C_NOT_SIGNED/ terms expired), the flag will remain stucktrueeven though the token is no longer rejected. This conflicts with the requirement to clear the bit once Apple stops returning those errors.
Consider clearing token_invalid when the token is clearly accepted but the request failed for a different known reason (e.g. terms expired).
tokenInvalid := reqErr != nil && (godep.IsTokenRejected(reqErr) || godep.IsSignatureInvalid(reqErr))
if reqErr == nil || tokenInvalid {
if _, err := updater.SetABMTokenInvalidForOrgName(ctx, orgName, tokenInvalid); err != nil {
logger.ErrorContext(ctx, "Apple DEP client: failed to update token invalid status of ABM token", "err", err)
}
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 22 out of 23 changed files in this pull request and generated no new comments.
Files excluded by content exclusion policy (1)
- changes/47700-abm-token-invalid-dep-error
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 22 out of 23 changed files in this pull request and generated no new comments.
Files excluded by content exclusion policy (1)
- changes/47700-abm-token-invalid-dep-error
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 22 out of 23 changed files in this pull request and generated 1 comment.
Files excluded by content exclusion policy (1)
- changes/47700-abm-token-invalid-dep-error
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 23 out of 24 changed files in this pull request and generated 2 comments.
Files excluded by content exclusion policy (1)
- changes/47700-abm-token-invalid-dep-error
Comments suppressed due to low confidence (2)
server/service/hosts.go:2529
- The related issue/PR description call out a structured
dep_errorattribute on the dep_assignment response (null when no error). The endpoint currently only returns the human-readabledep_device_errormessage, so API consumers can't reliably branch on error type without parsing text. Consider returning the typed error code as a separate JSON field (e.g.dep_error) alongside the message.
ID uint `json:"id"`
HostDEPAssignment *fleet.HostDEPAssignment `json:"host_dep_assignment"`
DEPDevice *godep.DeviceDetails `json:"dep_device"`
DEPDeviceError *string `json:"dep_device_error"`
Err error `json:"error,omitempty"`
server/service/hosts.go:2545
- If you add a structured
dep_errorcode to the response, the endpoint should also populate it (as null when depError=="") alongside the existingdep_device_errormessage. That keeps the API contract explicit and lets the frontend display a message while branching on a stable enum.
var depErrorMessage *string
if depError != "" {
msg := depError.Message()
depErrorMessage = &msg
}
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 23 out of 24 changed files in this pull request and generated no new comments.
Files excluded by content exclusion policy (1)
- changes/47700-abm-token-invalid-dep-error
Comments suppressed due to low confidence (2)
server/service/hosts.go:2600
- The comment says errors are classified via
dep_error, but the response field and related docs usedep_device_error. This mismatch is confusing for maintainers and API consumers; please align the comment with the actual JSON attribute name.
// Call Apple's "Get Device Details" API. Per the issue spec: on error, log
// and classify the error via dep_error rather than surfacing it to the
// caller.
server/fleet/apple_mdm.go:780
- This is user-facing text, and it appears to refer to "Apple Business" and an "AB token". The product name is Apple Business Manager (ABM) and the remediation is typically to upload/replace the ABM token (not "renew an AB token"). Clarifying this message will reduce user confusion.
case DEPDeviceErrorTokenInvalid:
return "Fleet can't connect to Apple Business. An admin needs to renew the AB token."
JordanMontgomery
left a comment
JordanMontgomery
left a comment
There was a problem hiding this comment.
Overall I think this looks good, one tiny comment, two things that are mostly just me griping about this awesome API - the response status change seems a bit odd to me but I assume it's something that's there and just not documented
| return depAssignment, nil, apple_mdm.ClassifyDEPDeviceError(err), nil | ||
| } | ||
|
|
||
| // Apple returns an entry for every requested serial even when it isn't |
There was a problem hiding this comment.
I've said it so many times but I really hate this API 😂
| tokenAccepted := reqErr == nil || godep.IsTermsNotSigned(reqErr) | ||
| if tokenAccepted || tokenInvalid { | ||
| if _, err := updater.SetABMTokenInvalidForOrgName(ctx, orgName, tokenInvalid); err != nil { | ||
| logger.ErrorContext(ctx, "Apple DEP client: failed to update token invalid status of ABM token", "err", err) |
There was a problem hiding this comment.
the below block does something like:
// on API call success, if the global terms expired flag is not set and
// the count is 0, no need to do anything else (it means this ABM token
// already had the flag cleared).
if reqErr == nil && count == 0 && !appCfg.MDM.AppleBMTermsExpired {
return reqErr
}
Does it make sense to do something like this here so this only executes against the writer when a change is needed? As best I can tell this will cause a query against the writer DB each time even though most times it won't be needed
| case "FOUND123": | ||
| devices[serial] = map[string]any{ | ||
| "serial_number": serial, | ||
| "response_status": "SUCCESS", |
There was a problem hiding this comment.
is this an actual part of the Apple API? they don't list response_status on their API docs. https://developer.apple.com/documentation/devicemanagement/device-details but @MagnusHJensen just added it and seemed to think it is so I'm good with it if it's there.
Have I mentioned how I hate this API and its documentation? 😂
Related issue: Resolves #47700
Checklist for submitter
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
QA'd all new/changed functionality manually. Not able to do for all code paths yet
Summary by CodeRabbit
token_invalidstatus for Apple Business Manager tokens, updated automatically based on Apple responses.dep_errordetails (including an error classification and helpful messaging) for device lookup failures.