gkarr 23242 fe#47754
Merged
Merged
Conversation
![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.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends BYOD support by (1) distinguishing personal manual enrollments in the enrollment status string and (2) enforcing Apple MDM command permissions for personal (BYOD) manual enrollments via stored AccessRights, with corresponding UI updates.
Changes:
- Add per-host Apple MDM enrollment AccessRights storage and use it to gate wipe/lock/clear-passcode (esp. for BYOD manual enrollments).
- Rename the personal enrollment status string from
On (personal)toOn (manual - personal)across backend, migrations, and frontend. - Update enrollment flows and UI (/enroll, OTA) to support a “Personal (BYOD) vs Company-owned” selection and propagate
byodthrough to the server URL.
Reviewed changes
Copilot reviewed 41 out of 41 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/integration_mdm_test.go | Updates expected enrollment status string in MDM integration tests. |
| server/service/integration_mdm_setup_experience_test.go | Updates expected enrollment status string for BYOD iOS setup experience test. |
| server/service/hosts.go | Populates new wipe_allowed/lock_allowed/clear_passcode_allowed flags for Apple manual/personal hosts. |
| server/service/hosts_test.go | Updates Android BYOD test fixtures to the renamed status string. |
| server/service/apple_mdm.go | Propagates byod through manual + OTA enrollment flows, persists AccessRights, and uses stored rights in renewal. |
| server/service/apple_mdm_test.go | Updates service tests for new method signatures and AccessRights parameter. |
| server/mock/service/service_mock.go | Updates mock service interface signatures for new params. |
| server/mock/datastore_mock.go | Adds mock datastore functions for Apple enrollment permissions get/set. |
| server/mdm/lifecycle/lifecycle.go | Plumbs IsPersonalEnrollment into Apple lifecycle reset logic. |
| server/mdm/apple/apple_mdm.go | Introduces AccessRights constants, AccessRights computation, and URL helper for personal flag; templates now render AccessRights. |
| server/mdm/apple/apple_mdm_test.go | Updates tests for new enrollment profile generator signature. |
| server/fleet/service.go | Updates Fleet Service interface for new enrollment/OTA method signatures. |
| server/fleet/hosts.go | Adds new MDM permission fields to host payload; renames personal enrollment status constant; adds HostMDMApplePermissions model. |
| server/fleet/hosts_test.go | Updates expected enrollment status for personal enrollment. |
| server/fleet/datastore.go | Adds datastore interface methods for reading/writing stored Apple enrollment permissions. |
| server/datastore/mysql/schema.sql | Updates generated enrollment_status enum value and adds host_mdm_apple_enrollment_permissions table. |
| server/datastore/mysql/migrations/tables/20260616144538_RenamePersonalEnrollmentStatus.go | Migration to rename generated personal enrollment_status value. |
| server/datastore/mysql/migrations/tables/20260616144538_RenamePersonalEnrollmentStatus_test.go | Migration test validating new generated enrollment_status value. |
| server/datastore/mysql/migrations/tables/20260616144537_AddHostMDMAppleEnrollmentPermissions.go | Migration creating + backfilling permissions table for Apple manual enrollments. |
| server/datastore/mysql/migrations/tables/20260616144537_AddHostMDMAppleEnrollmentPermissions_test.go | Migration test validating backfill rules and upsert behavior. |
| server/datastore/mysql/hosts.go | Implements get/set datastore methods and host UUID cleanup mapping for new permissions table. |
| frontend/utilities/constants.tsx | Updates MDM status tooltips for renamed status string. |
| frontend/templates/enroll-ota.html | Adds BYOD/company-owned selector and propagates byod=true into the download URL. |
| frontend/pages/hosts/details/HostDetailsPage/modals/ClearPasscodeModal/ClearPasscodeModal.tsx | Updates Android BYOD detection for renamed status string. |
| frontend/pages/hosts/details/HostDetailsPage/HostDetailsPage.tsx | Passes new *_allowed fields through to HostActionsDropdown. |
| frontend/pages/hosts/details/HostDetailsPage/HostActionsDropdown/HostActionsDropdown.tsx | Adds props for permission gating and passes them into option generation. |
| frontend/pages/hosts/details/HostDetailsPage/HostActionsDropdown/HostActionsDropdown.tests.tsx | Updates tests for renamed personal enrollment status. |
| frontend/pages/hosts/details/HostDetailsPage/HostActionsDropdown/helpers.tsx | Disables wipe/lock/clear-passcode options when *_allowed is false and adds BYOD-specific tooltips. |
| frontend/pages/hosts/details/cards/Vitals/Vitals.tests.tsx | Updates test fixtures for renamed personal enrollment status. |
| frontend/pages/hosts/details/cards/Software/SelfService/SelfService.tests.tsx | Updates test fixtures for renamed personal enrollment status. |
| frontend/pages/hosts/details/cards/Software/helpers.tsx | Updates iOS personal enrollment string checks for renamed status. |
| frontend/pages/hosts/details/cards/Software/helpers.tests.ts | Updates tests for renamed personal enrollment status. |
| frontend/pages/DashboardPage/DashboardPage.tsx | Updates dashboard MDM status bucket label for renamed personal enrollment status. |
| frontend/pages/DashboardPage/cards/MDM/MDM.tests.tsx | Updates MDM card tests for renamed personal enrollment status. |
| frontend/interfaces/mdm.ts | Updates enrollment status union + helpers to use renamed personal status. |
| frontend/interfaces/host.ts | Adds new optional wipe_allowed/lock_allowed/clear_passcode_allowed fields on host MDM payload. |
| frontend/components/TableContainer/DataTable/HostMdmStatusCell/HostMdmStatusCell.tests.tsx | Updates status cell tests for renamed personal enrollment status. |
| frontend/components/AddHostsModal/PlatformWrapper/IosIpadosPanel/IosIpadosPanel.tsx | Adds BYOD/company-owned selection and includes byod=true in generated enroll URL for personal. |
| frontend/components/AddHostsModal/PlatformWrapper/AndroidPanel/AndroidPanel.tsx | Updates radio label copy to “Personal (BYOD)” / “Company-owned (fully-managed)”. |
| frontend/components/AddHostsModal/AddHostsModal.tests.tsx | Updates AddHostsModal tests for new iOS/iPadOS enrollment instructions UI. |
| ee/server/service/mdm.go | Generates manual enrollment profile with BYOD-aware AccessRights and embeds personal flag into ServerURL. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
WalkthroughThis PR adds end-to-end BYOD (Personal enrollment) support for Apple MDM. The enrollment status string 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/service/apple_mdm.go (1)
3786-3811:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPersist permissions from the effective personal enrollment state.
resetAppletreatsEnrollmentIDaccount-driven enrollments as personal, but this path stores access rights from only the URL param. That can persist full wipe/lock rights for account-driven BYOD hosts; also, silently continuing after a personal permission write failure leaves renewals/UI gating without the required restricted-rights row.Proposed fix
- isPersonal := r.Params != nil && r.Params[apple_mdm.FleetPersonalEnrollmentKey] == "1" + isPersonal := r.Params != nil && r.Params[apple_mdm.FleetPersonalEnrollmentKey] == "1" + effectivePersonal := isPersonal || m.EnrollmentID != "" if err := svc.mdmLifecycle.Do(r.Context, mdmlifecycle.HostOptions{ Action: mdmlifecycle.HostActionReset, Platform: platform, UUID: m.UDID, HardwareSerial: m.SerialNumber, HardwareModel: m.Model, SCEPRenewalInProgress: scepRenewalInProgress, UserEnrollmentID: m.EnrollmentID, - IsPersonalEnrollment: isPersonal, + IsPersonalEnrollment: effectivePersonal, }); err != nil { svc.logger.WarnContext(r.Context, "could not reset Apple mdm information", "UDID", m.UDID, "EnrollmentID", m.EnrollmentID, "err", err) return err } // Persist the access rights for this host so SCEP/ACME renewal can honour // the monotonic-narrowing invariant (Apple disallows widening on replace). - accessRights := apple_mdm.AppleEnrollmentAccessRights(isPersonal) + accessRights := apple_mdm.AppleEnrollmentAccessRights(effectivePersonal) if err := svc.ds.SetHostMDMAppleEnrollmentPermissions(r.Context, r.ID, accessRights); err != nil { + if effectivePersonal { + return ctxerr.Wrap(r.Context, err, "persisting personal enrollment permissions") + } svc.logger.ErrorContext(r.Context, "failed to persist enrollment permissions", "host_uuid", r.ID, "err", err) - // Non-fatal: worst-case the next SCEP renewal uses MDMAccessRightAll (the pre-feature default). }🤖 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/apple_mdm.go` around lines 3786 - 3811, The code determines personal enrollment status for the mdmlifecycle.Do call using IsPersonalEnrollment, which treats account-driven enrollments as personal, but then uses only the URL parameter isPersonal when calling AppleEnrollmentAccessRights. Update the logic to pass to AppleEnrollmentAccessRights the same effective personal enrollment state that considers both the URL parameter and account-driven enrollment status (based on EnrollmentID), and remove or change the non-fatal error handling for SetHostMDMAppleEnrollmentPermissions since silently continuing after a permission write failure can leave renewals and UI gating without required restricted-rights rows.
🧹 Nitpick comments (4)
frontend/pages/hosts/details/HostDetailsPage/modals/ClearPasscodeModal/ClearPasscodeModal.tsx (1)
10-10: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUse the shared BYOD enrollment helper instead of a raw status string.
Line 37 hardcodes
"On (manual - personal)", which duplicates enrollment-status contract logic in UI code. Prefer the centralized helper to avoid future drift.Proposed change
-import { MdmEnrollmentStatus } from "interfaces/mdm"; +import { MdmEnrollmentStatus, isAndroidBYO } from "interfaces/mdm"; @@ - const isAndroidBYO = - isAndroidHost && hostMdmEnrollmentStatus === "On (manual - personal)"; + const isAndroidBYODEnrollment = + isAndroidHost && isAndroidBYO(hostMdmEnrollmentStatus); @@ - if (isAndroidBYO) { + if (isAndroidBYODEnrollment) {Also applies to: 35-37, 60-60
🤖 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/modals/ClearPasscodeModal/ClearPasscodeModal.tsx` at line 10, The code contains hardcoded enrollment status strings like "On (manual - personal)" at lines 35-37 and line 60, which duplicates enrollment-status contract logic. Replace these hardcoded strings with calls to a shared BYOD enrollment helper function that centralizes this logic. Import the necessary helper function from the shared utilities if not already imported, and refactor the locations where the enrollment status string is used to call the helper function instead of using the raw hardcoded string.frontend/components/AddHostsModal/PlatformWrapper/IosIpadosPanel/IosIpadosPanel.tsx (1)
65-82: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd a legend to the enrollment-type radio group.
At Line 65, the new
<fieldset>has no<legend>, so the two radio options lose group context for screen-reader users.Proposed change
<fieldset className="form-field"> + <legend>Enrollment type</legend> <Radio name="iosIpadosEnrollmentType" id="iosIpadosPersonal" label="Personal (BYOD)"🤖 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/AddHostsModal/PlatformWrapper/IosIpadosPanel/IosIpadosPanel.tsx` around lines 65 - 82, The fieldset element in IosIpadosPanel lacks a legend element, which is required for accessibility. Add a legend element as the first child inside the fieldset that describes the purpose of the radio group (e.g., describing that users are selecting between personal and company-owned enrollment types). The legend should be placed immediately after the opening fieldset tag and before the Radio components for iosIpadosPersonal and iosIpadosCompanyOwned.server/mdm/apple/apple_mdm_test.go (1)
243-274: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider adding test coverage for the
AccessRightsvalue in the generated profile.The test verifies SCEP and MDM URLs but doesn't assert that the
AccessRightsinteger is correctly embedded in the output. Consider adding a case withAppleEnrollmentAccessRights(true)(personal enrollment) and verifying the resulting plist contains the expected value (8179 instead of 8191).🤖 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 243 - 274, The test for GenerateEnrollmentProfileMobileconfig in the loop starting at line 243 currently verifies SCEP and MDM URLs but does not assert the AccessRights value in the generated profile. Add a new test case with AppleEnrollmentAccessRights(true) for personal enrollment, and within the switch statement for payload type checking, add a case to verify that the AccessRights integer value in the profile matches the expected value (8179 for personal enrollment versus 8191 for MDMAccessRightAll). This can be done by checking the appropriate field in the profile structure or payload after unmarshaling the plist result.server/mdm/apple/apple_mdm.go (1)
1726-1744: 🧹 Nitpick | 🔵 TrivialInconsistency in
byodparameter representation: unify to single format.The
byodparameter is set to"true"inGenerateOTAEnrollmentProfileMobileconfigbut to"1"inAddPersonalEnrollmentToFleetURL. While downstream handlers (lines 6675, 6804) already accommodate both formats with explicit checks ("true" || "1"), consolidating to a single representation would improve consistency and reduce the need for defensive parsing logic.🤖 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 1726 - 1744, The byod parameter is being set inconsistently across functions: GenerateOTAEnrollmentProfileMobileconfig uses "true" while AddPersonalEnrollmentToFleetURL uses "1". Unify this parameter to use a single consistent format across both functions. Choose one representation (either "true" or "1") and update both GenerateOTAEnrollmentProfileMobileconfig and AddPersonalEnrollmentToFleetURL to use the same value when setting the byod query parameter. This will eliminate the need for defensive parsing logic in the downstream handlers.
🤖 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 `@frontend/templates/enroll-ota.html`:
- Around line 365-367: The buttons with classes byod-tab, byod-tab--personal,
and byod-tab--company are incorrectly marked with role="tab" and aria-selected
attributes without implementing full tab pattern semantics. Replace the tab
roles with proper toggle button semantics by removing role="tab", removing the
aria-selected attributes, and adding aria-pressed="false" for the inactive
button (Personal) and aria-pressed="true" for the active button (Company-owned).
Update the JavaScript logic that switches between these buttons to toggle
aria-pressed instead of aria-selected. Apply the same changes to all occurrences
mentioned in lines 439-440 and 734-739.
In `@server/datastore/mysql/hosts.go`:
- Line 643: The host_mdm_apple_enrollment_permissions table is being removed as
part of a generic UUID-based cleanup that affects all host types. This creates a
risk where deleting a Windows or Linux host with a duplicate UUID could remove
Apple MDM enrollment permissions for a surviving Apple host with the same UUID.
Modify the cleanup logic to scope the deletion of
host_mdm_apple_enrollment_permissions specifically to Apple hosts (verify the
host being deleted is an Apple host) or add a safeguard check to only delete
this table when no other host with that UUID remains in the database, similar to
the duplicate-UUID safeguard mentioned in the comment.
- Around line 4531-4534: In the INSERT...ON DUPLICATE KEY UPDATE statement for
the host_mdm_apple_enrollment_permissions table, the ON DUPLICATE KEY UPDATE
clause currently only updates the access_rights field. Modify this clause to
also refresh the delivered_at column to the current timestamp when a duplicate
key is encountered, so that repeated profile delivery updates reflect the latest
delivery metadata rather than keeping the stale original timestamp.
---
Outside diff comments:
In `@server/service/apple_mdm.go`:
- Around line 3786-3811: The code determines personal enrollment status for the
mdmlifecycle.Do call using IsPersonalEnrollment, which treats account-driven
enrollments as personal, but then uses only the URL parameter isPersonal when
calling AppleEnrollmentAccessRights. Update the logic to pass to
AppleEnrollmentAccessRights the same effective personal enrollment state that
considers both the URL parameter and account-driven enrollment status (based on
EnrollmentID), and remove or change the non-fatal error handling for
SetHostMDMAppleEnrollmentPermissions since silently continuing after a
permission write failure can leave renewals and UI gating without required
restricted-rights rows.
---
Nitpick comments:
In
`@frontend/components/AddHostsModal/PlatformWrapper/IosIpadosPanel/IosIpadosPanel.tsx`:
- Around line 65-82: The fieldset element in IosIpadosPanel lacks a legend
element, which is required for accessibility. Add a legend element as the first
child inside the fieldset that describes the purpose of the radio group (e.g.,
describing that users are selecting between personal and company-owned
enrollment types). The legend should be placed immediately after the opening
fieldset tag and before the Radio components for iosIpadosPersonal and
iosIpadosCompanyOwned.
In
`@frontend/pages/hosts/details/HostDetailsPage/modals/ClearPasscodeModal/ClearPasscodeModal.tsx`:
- Line 10: The code contains hardcoded enrollment status strings like "On
(manual - personal)" at lines 35-37 and line 60, which duplicates
enrollment-status contract logic. Replace these hardcoded strings with calls to
a shared BYOD enrollment helper function that centralizes this logic. Import the
necessary helper function from the shared utilities if not already imported, and
refactor the locations where the enrollment status string is used to call the
helper function instead of using the raw hardcoded string.
In `@server/mdm/apple/apple_mdm_test.go`:
- Around line 243-274: The test for GenerateEnrollmentProfileMobileconfig in the
loop starting at line 243 currently verifies SCEP and MDM URLs but does not
assert the AccessRights value in the generated profile. Add a new test case with
AppleEnrollmentAccessRights(true) for personal enrollment, and within the switch
statement for payload type checking, add a case to verify that the AccessRights
integer value in the profile matches the expected value (8179 for personal
enrollment versus 8191 for MDMAccessRightAll). This can be done by checking the
appropriate field in the profile structure or payload after unmarshaling the
plist result.
In `@server/mdm/apple/apple_mdm.go`:
- Around line 1726-1744: The byod parameter is being set inconsistently across
functions: GenerateOTAEnrollmentProfileMobileconfig uses "true" while
AddPersonalEnrollmentToFleetURL uses "1". Unify this parameter to use a single
consistent format across both functions. Choose one representation (either
"true" or "1") and update both GenerateOTAEnrollmentProfileMobileconfig and
AddPersonalEnrollmentToFleetURL to use the same value when setting the byod
query parameter. This will eliminate the need for defensive parsing logic in the
downstream handlers.
🪄 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: f2447533-28a2-4dc6-b69b-724e9c96b0a7
📒 Files selected for processing (41)
ee/server/service/mdm.gofrontend/components/AddHostsModal/AddHostsModal.tests.tsxfrontend/components/AddHostsModal/PlatformWrapper/AndroidPanel/AndroidPanel.tsxfrontend/components/AddHostsModal/PlatformWrapper/IosIpadosPanel/IosIpadosPanel.tsxfrontend/components/TableContainer/DataTable/HostMdmStatusCell/HostMdmStatusCell.tests.tsxfrontend/interfaces/host.tsfrontend/interfaces/mdm.tsfrontend/pages/DashboardPage/DashboardPage.tsxfrontend/pages/DashboardPage/cards/MDM/MDM.tests.tsxfrontend/pages/hosts/details/HostDetailsPage/HostActionsDropdown/HostActionsDropdown.tests.tsxfrontend/pages/hosts/details/HostDetailsPage/HostActionsDropdown/HostActionsDropdown.tsxfrontend/pages/hosts/details/HostDetailsPage/HostActionsDropdown/helpers.tsxfrontend/pages/hosts/details/HostDetailsPage/HostDetailsPage.tsxfrontend/pages/hosts/details/HostDetailsPage/modals/ClearPasscodeModal/ClearPasscodeModal.tsxfrontend/pages/hosts/details/cards/Software/SelfService/SelfService.tests.tsxfrontend/pages/hosts/details/cards/Software/helpers.tests.tsfrontend/pages/hosts/details/cards/Software/helpers.tsxfrontend/pages/hosts/details/cards/Vitals/Vitals.tests.tsxfrontend/templates/enroll-ota.htmlfrontend/utilities/constants.tsxserver/datastore/mysql/hosts.goserver/datastore/mysql/migrations/tables/20260616144537_AddHostMDMAppleEnrollmentPermissions.goserver/datastore/mysql/migrations/tables/20260616144537_AddHostMDMAppleEnrollmentPermissions_test.goserver/datastore/mysql/migrations/tables/20260616144538_RenamePersonalEnrollmentStatus.goserver/datastore/mysql/migrations/tables/20260616144538_RenamePersonalEnrollmentStatus_test.goserver/datastore/mysql/schema.sqlserver/fleet/datastore.goserver/fleet/hosts.goserver/fleet/hosts_test.goserver/fleet/service.goserver/mdm/apple/apple_mdm.goserver/mdm/apple/apple_mdm_test.goserver/mdm/lifecycle/lifecycle.goserver/mock/datastore_mock.goserver/mock/service/service_mock.goserver/service/apple_mdm.goserver/service/apple_mdm_test.goserver/service/hosts.goserver/service/hosts_test.goserver/service/integration_mdm_setup_experience_test.goserver/service/integration_mdm_test.go
Aligns the OTA endpoint and the MDM ServerURL query key with the
"byod" param the /enroll page and add-hosts modal already use, so the
URL key is consistent across the personal-enrollment surface area:
- getManualEnrollmentProfileRequest.Personal: query:"personal" -> "byod"
- getOTAProfileRequest.Personal: query:"personal" -> "byod"
- OTA decoder + POST-back decoder read Query().Get("byod")
- GenerateOTAEnrollmentProfileMobileconfig writes "byod=true"
- FleetPersonalEnrollmentKey constant value: "is_personal" -> "byod"
Consumer at apple_mdm.go:3771 reads via the constant, so the rename
propagates through the Authenticate checkin without code changes.
The Authenticate handler runs on every checkin, including the one triggered by a SCEP cert renewal. The renewed enrollment profile's ServerURL does not carry byod=1, so isPersonal would be derived as false during the renewal Authenticate. Without this guard, SetHostMDMAppleEnrollmentPermissions would overwrite the previously-stored narrow bitmask (MDMAccessRightAll &^ DeviceLock &^ DeviceErase) with MDMAccessRightAll. The next SCEP renewal would then generate a profile requesting full rights, which Apple rejects as a widening violation on replacement. The rights row is a write-once-at-enrollment value; renewal must preserve it.
georgekarrv
force-pushed
the
gkarr-23242-fe
branch
from
9d713af to
d3efee8
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #47754 +/- ##
==========================================
- Coverage 67.47% 67.47% -0.01%
==========================================
Files 3672 3673 +1
Lines 232703 232973 +270
Branches 12405 12421 +16
==========================================
+ Hits 157025 157193 +168
- Misses 61588 61657 +69
- Partials 14090 14123 +33
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:
|
Apple rejects ServerURL changes on enrollment-profile replacement, so the
URL the renewal generator builds must exactly match the URL the device was
originally enrolled with. The OTA / EE manual-enrollment paths bake byod=1
into the ServerURL for personal devices (AddPersonalEnrollmentToFleetURL),
but the renewal generator at server/service/apple_mdm.go was passing the
raw MDMUrl(), dropping byod=1 on every BYOD renewal and triggering Apple
rejection.
Fix:
- Extend GetHostMDMAppleEnrollmentPermissions to JOIN host_mdm so
IsPersonalEnrollment (the authoritative BYOD signal) and AccessRights
are returned together. The lookup is now driven off hosts.uuid so it
returns a row whenever the device exists, even if the permissions
persist failed during initial Authenticate (that write is non-fatal).
- Add renewalEnrollmentParams() helper that returns (personal, rights),
re-narrowing the bitmask when a personal device's stored rights came
back unrestricted (covers the persist-failure edge case).
- Apply at all three renewal sites:
* manual SCEP without enroll ref (filteredAssocs)
* manual SCEP with enroll ref (assocsWithRefs)
* ACME (defensive; OTA is the only BYOD path and OTA doesn't use ACME,
but match URL shape in case that combination becomes possible).
Matrix verified:
- Pre-feature device: is_personal_enrollment=0, no permissions row
-> personal=false, rights=MDMAccessRightAll, URL=MDMUrl. Matches.
- New company-owned: is_personal_enrollment=0, rights=MDMAccessRightAll
-> personal=false, URL=MDMUrl. Matches.
- New BYOD: is_personal_enrollment=1, rights=narrow
-> personal=true, URL=MDMUrl?byod=1, rights=narrow. Matches.
- New BYOD with persist-failed permissions row:
is_personal_enrollment=1, no permissions row -> personal=true,
rights renarrowed from personal flag, URL=MDMUrl?byod=1. Matches.
- ADUE: uses GenerateAccountDrivenEnrollmentProfileMobileconfig (no byod
in initial URL) and a different renewal path that does not call
AddPersonalEnrollmentToFleetURL -> URL unchanged. Matches.
georgekarrv
force-pushed
the
gkarr-23242-fe
branch
from
d3efee8 to
fa258d2
Compare
# Conflicts: # server/datastore/mysql/hosts.go # server/fleet/hosts.go # server/service/apple_mdm.go
…al - personal)' Cosmetic: the actual asserted values already used the renamed status; only two test/describe description strings still referenced the old label.
CodeQL flagged a high-severity alert: the download link's href, read from the DOM (data-base-href/href), was string-concatenated back into the href in the catch fallback, reinterpreting DOM-sourced text unsafely. The primary path already routes baseHref through the URL constructor (which resolves relative URLs against the origin and normalizes the value), so the raw-concat fallback was both unnecessary and the flagged sink. Drop it: if URL parsing ever fails, leave the server-rendered href untouched rather than reinterpreting it.
Comment on lines
+4
to
+56
| @@ -21,6 +29,11 @@ interface IosIpadosPanelProps { | |||
| const IosIpadosPanel = ({ enrollSecret }: IosIpadosPanelProps) => { | |||
| const { config, isMacMdmEnabledAndConfigured } = useContext(AppContext); | |||
|
|
|||
| // Default to "Personal (BYOD)" per #23242 design. | |||
| const [enrollmentType, setEnrollmentType] = useState<EnrollmentType>( | |||
| "personal" | |||
| ); | |||
|
|
|||
| const helpText = | |||
| "When the end user navigates to this URL, the enrollment profile " + | |||
| "will download in their browser. End users will have to install the profile " + | |||
| @@ -40,19 +53,43 @@ const IosIpadosPanel = ({ enrollSecret }: IosIpadosPanelProps) => { | |||
| ); | |||
| } | |||
|
|
|||
| const url = generateUrl(config.server_settings.server_url, enrollSecret); | |||
| const url = generateUrl( | |||
Member
There was a problem hiding this comment.
Could we drop generateUrl and inline getPathWithQueryParams from utilities/url at the call site? It URL-encodes and filters
undefined/null/"" for us, so the byod toggle falls out naturally and we stay consistent with the rest of the frontend (the TODO at
frontend/utilities/url/index.ts:102 calls it out as the preferred helper):
const url = getPathWithQueryParams(
${config.server_settings.server_url}/enroll,
{
enroll_secret: enrollSecret,
byod: enrollmentType === "personal" ? "true" : undefined,
}
);
Member
Author
There was a problem hiding this comment.
Done in 6dc4005. Dropped generateUrl and inlined getPathWithQueryParams exactly as suggested:
const url = getPathWithQueryParams(
`${config.server_settings.server_url}/enroll`,
{
enroll_secret: enrollSecret,
byod: enrollmentType === "personal" ? "true" : undefined,
}
);The helper URL-encodes and filters out the undefined byod value, so the company-owned case drops the param naturally. (Left the matching generateUrl in AndroidPanel.tsx alone since it's pre-existing on main from #39468 and outside this PR's scope — happy to do that as a follow-up if you'd like.)
| const BYOD_DISABLED_TOOLTIPS: Record<string, JSX.Element> = { | ||
| wipe: <>Wipe permissions are disabled for this host.</>, | ||
| lock: <>Lock permissions are disabled for this host.</>, | ||
| clearPasscode: <>Clear passcode permissions are disabled for this host.</>, |
Member
There was a problem hiding this comment.
styling nit, adding a <br/> halfway through so the tooltip doesn't have weird hanging words, for example this might render like:
Clear passcode permissions are disabled for
this host.
instead of:
Clear passcode permissions
are disabled for this host.
Member
Author
There was a problem hiding this comment.
Done in 6dc4005 — added a <br/> after "permissions" so it renders as:
Clear passcode permissions
are disabled for this host.
Applied the same break to the wipe and lock tooltips too so the three stay visually consistent in the dropdown.
RachelElysia
requested changes
Jun 29, 2026
RachelElysia
left a comment
RachelElysia
left a comment
Member
There was a problem hiding this comment.
Looks good, just a) replace one helper function with an existing helper function and b) some tooltip styling consideration nit
…YOD tooltips - IosIpadosPanel: drop the custom generateUrl helper and build the enroll URL with getPathWithQueryParams from utilities/url, which URL-encodes and drops undefined/null/empty values (the byod toggle falls out naturally) — matches the preferred helper called out in utilities/url and stays consistent with the rest of the frontend. - HostActionsDropdown BYOD-disabled tooltips: add a <br/> after "permissions" so the copy wraps cleanly instead of leaving hanging words.
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
RachelElysia
approved these changes
Jun 29, 2026
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: #23242
Frontend changes for Apple BYOD (personal) enrollment, layered on the merged backend work.
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.If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes
Testing
Test plan
Summary by CodeRabbit
Release Notes
New Features
Improvements