47701 abm errors UI#49896
Draft
andymFleet wants to merge 31 commits into
Draft
Conversation
# Conflicts: # server/fleet/service.go # server/mock/service/service_mock.go # server/service/hosts.go
…into 47701-abm-errors-ui
Contributor
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 updates Fleet’s Apple Business Manager (ABM) error handling end-to-end so the UI can show a new “token invalid” banner and more specific DEP assignment errors, while keeping underlying Apple API failures out of the API error surface (logged + classified instead).
Changes:
- Add ABM token invalid tracking (
token_invalid) and a datastore method to update it based on definitive Apple DEP error signals. - Classify DEP “Get Device Details” failures into typed error codes and surface a user-facing
dep_device_errormessage in the host DEP assignment response. - Update frontend state + UI to render the new ABM token invalid banner and show the API-provided DEP assignment error message (with test coverage).
Reviewed changes
Copilot reviewed 38 out of 39 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| changes/47700-abm-token-invalid-dep-error | DB migration for new ABM token invalid / DEP error support (content excluded). |
| server/service/hosts.go | Extend host DEP assignment details to return a classified DEP error and expose dep_device_error in the API response. |
| server/service/hosts_test.go | Add coverage for “not found” classification based on Apple /devices response shape/statuses. |
| server/service/apple_mdm_test.go | Update test stubs for new ABM token invalid updater hook dependency. |
| server/mock/service/service_mock.go | Update service mock signature for GetHostDEPAssignmentDetails. |
| server/mock/datastore_mock.go | Add datastore mock support for SetABMTokenInvalidForOrgName. |
| server/mdm/nanodep/godep/client.go | Add IsServerError helper to classify Apple-side 5xx failures. |
| server/mdm/nanodep/godep/client_test.go | Add unit tests for IsServerError. |
| server/mdm/nanodep/godep/account.go | Add helpers to detect token_rejected and signature_invalid error codes. |
| server/mdm/nanodep/godep/account_test.go | Add unit tests for token/signature error helpers. |
| server/mdm/apple/apple_mdm.go | Update DEP client after-hook to set/clear token_invalid; add ClassifyDEPDeviceError. |
| server/mdm/apple/apple_mdm_test.go | Add tests for token invalid after-hook behavior and device error classification. |
| server/fleet/service.go | Update service interface contract to return DEP error classification. |
| server/fleet/mdm.go | Add TokenInvalid to ABMToken; extend ABMTermsUpdater to include token invalid updates. |
| server/fleet/mdm_test.go | Update DEP client tests to account for token-invalid bookkeeping. |
| server/fleet/datastore.go | Add datastore interface method SetABMTokenInvalidForOrgName. |
| server/fleet/apple_mdm.go | Add DEPDeviceErrorType enum + end-user messages. |
| server/fleet/apple_mdm_test.go | Add tests for DEPDeviceErrorType.Message(). |
| server/datastore/mysql/apple_mdm.go | Include token_invalid in ABM token selects; implement SetABMTokenInvalidForOrgName. |
| server/datastore/mysql/apple_mdm_test.go | Add integration coverage for setting/clearing token_invalid. |
| frontend/services/entities/hosts.ts | Make dep_device / host_dep_assignment nullable; add dep_device_error. |
| frontend/pages/hosts/details/modals/MDMStatusModal/MDMStatusModal.tsx | Show API-provided DEP error message when device details are missing; harden null handling. |
| frontend/pages/hosts/details/modals/MDMStatusModal/MDMStatusModal.tests.tsx | Add tests for non-DEP hosts and dep_device_error rendering/fallback behavior. |
| frontend/pages/hosts/details/modals/MDMStatusModal/_styles.scss | Add styling for the DEP error block in the modal. |
| frontend/pages/admin/IntegrationsPage/cards/MdmSettings/AppleBusinessManagerPage/AppleBusinessManagerPage.tsx | Populate app context with invalid-token banner inputs from ABM tokens list. |
| frontend/interfaces/mdm.ts | Add token_invalid to ABM token interface. |
| frontend/context/app.tsx | Extend ABM expiry state to include invalid-token banner fields. |
| frontend/context/app.tests.tsx | Move/expand app context tests and add coverage for new ABM expiry fields. |
| frontend/context/app.tests.ts | Remove old non-JSX app context tests file. |
| frontend/components/MDM/AppleBMTokenInvalidMessage/index.ts | Barrel export for new invalid-token banner component. |
| frontend/components/MDM/AppleBMTokenInvalidMessage/AppleBMTokenInvalidMessage.tsx | New ABM token invalid banner UI with singular/plural and org-name list formatting. |
| frontend/components/MDM/AppleBMTokenInvalidMessage/AppleBMTokenInvalidMessage.tests.tsx | Add tests for banner copy formatting. |
| frontend/components/MainContent/MainContent.tsx | Render the new token invalid banner with precedence ordering. |
| frontend/components/MainContent/_styles.scss | Adjust banner spacing when the invalid-token banner is shown. |
| frontend/components/App/App.tsx | Set ABM expiry state with token-invalid banner inputs when loading ABM tokens. |
| ee/server/service/mdm_external_test.go | Update EE test stubs for token-invalid updater dependency. |
| ee/server/service/apple_mdm_test.go | Update EE test setup to short-circuit new token-invalid after-hook. |
| cmd/fleetctl/fleetctl/apply_test.go | Update fleetctl tests stubbing datastore for token-invalid updater dependency. |
| cmd/fleetctl/fleetctl/apply_deprecated_test.go | Update deprecated fleetctl tests stubbing datastore for token-invalid updater dependency. |
Files excluded by content exclusion policy (1)
- changes/47700-abm-token-invalid-dep-error
Comments suppressed due to low confidence (1)
server/fleet/apple_mdm.go:781
- The token-invalid message says "renew the AB token", but elsewhere in this PR the feature is described as ABM tokens / Apple Business (AB) tokens. Consider updating this copy to consistently refer to "ABM token" (or "Apple Business (AB) token") to reduce user confusion.
case DEPDeviceErrorTokenInvalid:
return "Fleet can't connect to Apple Business. An admin needs to renew the AB token."
case DEPDeviceErrorTermsExpired:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+751
to
+753
| // DEPDeviceErrorType describes why Fleet could not retrieve a host's DEP | ||
| // device details from Apple, for the dep_error attribute of the | ||
| // dep_assignment endpoint. It is empty when there was no error. |
Comment on lines
2598
to
+2600
| // Call Apple's "Get Device Details" API. Per the issue spec: on error, log | ||
| // and return dep_device as nil rather than surfacing the error to the caller. | ||
| // and classify the error via dep_error rather than surfacing it to the | ||
| // caller. |
Contributor
WalkthroughThe change tracks invalid ABM tokens in datastore and API responses, classifies Apple DEP retrieval failures, and exposes 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}
Contributor
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/components/App/App.tsx (1)
133-144: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReset ABM state when the successful token list is empty.
When
ab_tokenschanges from non-empty to[], the length guard skipssetABMExpiry, leavinghasAbmTokenInvalidandinvalidAbmTokenOrgNamesfrom the previous response. The global invalid-token banner can therefore remain visible after the invalid token is removed. Handle the empty-success case explicitly and reset all ABM-derived state using the existing no-token semantics; add a regression test for non-empty → empty responses.🤖 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/components/App/App.tsx` around lines 133 - 144, Update the onSuccess handler in App to always call setABMExpiry, including when ab_tokens is empty, and reset all ABM-derived fields using the existing no-token semantics so stale invalid-token state is cleared. Add a regression test covering a successful non-empty response followed by an empty ab_tokens response.
🤖 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 `@changes/47700-abm-token-invalid-dep-error`:
- Line 1: Update the changelog entry to document the serialized response
property dep_device_error instead of dep_error, while preserving the existing
endpoint and ABM token context.
In `@server/fleet/apple_mdm.go`:
- Around line 751-753: Update the DEPDeviceErrorType comment to reference the
documented API/UI response field dep_device_error instead of dep_error, leaving
the rest of the description unchanged.
---
Outside diff comments:
In `@frontend/components/App/App.tsx`:
- Around line 133-144: Update the onSuccess handler in App to always call
setABMExpiry, including when ab_tokens is empty, and reset all ABM-derived
fields using the existing no-token semantics so stale invalid-token state is
cleared. Add a regression test covering a successful non-empty response followed
by an empty ab_tokens response.
🪄 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 Plus
Run ID: c7905ad6-e8a3-40b3-9bae-b97667b48b72
📒 Files selected for processing (39)
changes/47700-abm-token-invalid-dep-errorcmd/fleetctl/fleetctl/apply_deprecated_test.gocmd/fleetctl/fleetctl/apply_test.goee/server/service/apple_mdm_test.goee/server/service/mdm_external_test.gofrontend/components/App/App.tsxfrontend/components/MDM/AppleBMTokenInvalidMessage/AppleBMTokenInvalidMessage.tests.tsxfrontend/components/MDM/AppleBMTokenInvalidMessage/AppleBMTokenInvalidMessage.tsxfrontend/components/MDM/AppleBMTokenInvalidMessage/index.tsfrontend/components/MainContent/MainContent.tsxfrontend/components/MainContent/_styles.scssfrontend/context/app.tests.tsfrontend/context/app.tests.tsxfrontend/context/app.tsxfrontend/interfaces/mdm.tsfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/AppleBusinessManagerPage/AppleBusinessManagerPage.tsxfrontend/pages/hosts/details/modals/MDMStatusModal/MDMStatusModal.tests.tsxfrontend/pages/hosts/details/modals/MDMStatusModal/MDMStatusModal.tsxfrontend/pages/hosts/details/modals/MDMStatusModal/_styles.scssfrontend/services/entities/hosts.tsserver/datastore/mysql/apple_mdm.goserver/datastore/mysql/apple_mdm_test.goserver/fleet/apple_mdm.goserver/fleet/apple_mdm_test.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/mdm/nanodep/godep/client_test.goserver/mock/datastore_mock.goserver/mock/service/service_mock.goserver/service/apple_mdm_test.goserver/service/hosts.goserver/service/hosts_test.go
💤 Files with no reviewable changes (1)
- frontend/context/app.tests.ts
| @@ -0,0 +1 @@ | |||
| - Added `token_invalid` to the ABM token API responses and `dep_error` to `GET /hosts/:id/dep_assignment`, to help identify why a host's Apple Business Manager device lookup or ABM token isn't returning expected data (e.g. a rejected or invalid-signature token, unsigned terms, a server-side error, or the device no longer being assigned to Fleet). | |||
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Document the correct response property.
The endpoint serializes dep_device_error, not dep_error; clients following this note will look for a field that is never returned.
Proposed fix
- `dep_error` to `GET /hosts/:id/dep_assignment`
+ `dep_device_error` to `GET /hosts/:id/dep_assignment`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - Added `token_invalid` to the ABM token API responses and `dep_error` to `GET /hosts/:id/dep_assignment`, to help identify why a host's Apple Business Manager device lookup or ABM token isn't returning expected data (e.g. a rejected or invalid-signature token, unsigned terms, a server-side error, or the device no longer being assigned to Fleet). | |
| - Added `token_invalid` to the ABM token API responses and `dep_device_error` to `GET /hosts/:id/dep_assignment`, to help identify why a host's Apple Business Manager device lookup or ABM token isn't returning expected data (e.g. a rejected or invalid-signature token, unsigned terms, a server-side error, or the device no longer being assigned to Fleet). |
🤖 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 `@changes/47700-abm-token-invalid-dep-error` at line 1, Update the changelog
entry to document the serialized response property dep_device_error instead of
dep_error, while preserving the existing endpoint and ABM token context.
Comment on lines
+751
to
+753
| // DEPDeviceErrorType describes why Fleet could not retrieve a host's DEP | ||
| // device details from Apple, for the dep_error attribute of the | ||
| // dep_assignment endpoint. It is empty when there was no error. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the documented response field name.
Line 752 says dep_error, but the API/UI contract uses dep_device_error. Keep the comment aligned to prevent callers from implementing the wrong field.
🤖 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/apple_mdm.go` around lines 751 - 753, Update the
DEPDeviceErrorType comment to reference the documented API/UI response field
dep_device_error instead of dep_error, leaving the rest of the description
unchanged.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #49896 +/- ##
========================================
Coverage 67.90% 67.91%
========================================
Files 3899 3905 +6
Lines 249026 249483 +457
Branches 13099 13050 -49
========================================
+ Hits 169104 169437 +333
- Misses 64688 64762 +74
- Partials 15234 15284 +50
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:
|
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issue: Resolves #47701
Checklist for submitter
Testing
Added/updated automated tests
QA'd all new/changed functionality manually: Not able to test token_rejected, terms and conditions or apple server error.
Summary by CodeRabbit