Added support for validating Microsoft Entra v2 access tokens#46416
Open
getvictor wants to merge 15 commits into
Open
Added support for validating Microsoft Entra v2 access tokens#46416getvictor wants to merge 15 commits into
getvictor wants to merge 15 commits into
Conversation
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Member
Author
|
/agentic_review |
Code Review by Qodo
1.
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds support for validating Microsoft Entra (Azure AD) v2 access tokens during Windows MDM automatic enrollment by authorizing tokens whose aud matches a configured Entra application client ID, while preserving the existing v1 aud (Fleet server URL host) behavior.
Changes:
- Add
mdm.windows_entra_client_idsapp config setting (including migration) and validate it (premium-only, GUID format, Windows MDM enabled). - Update Windows MDM JWT audience authorization logic to accept either configured client IDs (v2) or server URL host (v1), plus add unit coverage for the matching helper.
- Extend GitOps parsing/generation and anonymous statistics payload to include the new setting.
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/microsoft_mdm.go | Adds shared audience-matching helper and uses it when authorizing Entra JWTs for Windows automatic enrollment. |
| server/service/microsoft_mdm_test.go | Adds unit tests for the audience authorization helper (v1 URL-host and v2 client-ID paths). |
| server/service/client.go | Ensures GitOps/app config patching includes windows_entra_client_ids with stable empty-array defaults. |
| server/service/appconfig.go | Validates the new client ID allowlist, clears it when disabling Windows MDM, and emits add/remove activities. |
| server/service/appconfig_test.go | Updates MDM config test fixtures and adds validation-focused test cases for the new setting. |
| server/fleet/app.go | Adds WindowsEntraClientIDs to the fleet.MDM config struct and deep-copies it in AppConfig.Copy(). |
| server/fleet/activities.go | Introduces new activity types for adding/deleting Microsoft Entra client IDs. |
| server/fleet/statistics.go | Adds numWindowsEntraClientIDs to the anonymous statistics payload. |
| server/datastore/mysql/statistics.go | Populates the new statistics field from app config. |
| server/datastore/mysql/migrations/tables/20260529120000_AddAppConfigMDMWindowsEntraClientIDs.go | Migration initializes mdm.windows_entra_client_ids to [] for upgraded installs. |
| server/datastore/mysql/migrations/tables/20260529120000_AddAppConfigMDMWindowsEntraClientIDs_test.go | Verifies the migration initializes windows_entra_client_ids to an empty array in app config JSON. |
| pkg/spec/gitops.go | Extends GitOps controls schema and Set() detection to include windows_entra_client_ids. |
| pkg/spec/gitops_test.go | Asserts GitOps parsing includes the new key. |
| pkg/spec/testdata/team_config_no_paths.yml | Adds windows_entra_client_ids: [] to GitOps testdata. |
| pkg/spec/testdata/team_config_invalid_sha.yml | Adds windows_entra_client_ids: [] to GitOps testdata. |
| pkg/spec/testdata/global_config_no_paths.yml | Adds windows_entra_client_ids: [] to GitOps testdata. |
| pkg/spec/testdata/controls.yml | Adds windows_entra_client_ids: [] to GitOps testdata. |
| pkg/spec/testdata/controls_new_names.yml | Adds windows_entra_client_ids: [] to GitOps testdata. |
| cmd/fleetctl/fleetctl/generate_gitops.go | Emits windows_entra_client_ids in generated GitOps controls when configured and Windows MDM is enabled. |
| changes/46388-windows-entra-v2-access-tokens | Adds a release note for v2 access token support and the new configuration key. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
Caution Review failedFailed to post review comments WalkthroughThis pull request adds support for Microsoft Entra v2 access tokens during Windows MDM enrollment. The change introduces a configurable allowlist of Entra application client IDs ( 🚥 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/46388-windows-entra-v2-access-tokens`:
- Line 1: Update the release note text to match Microsoft's wording and include
the official doc link: change the cutoff phrasing from "after 2026-07-01" to
"effective July 1, 2026" (or "starting July 1, 2026") and specify that this
applies to new on-premises MDM applications created via the Entra Portal flow;
keep that v1 tokens continue to work unchanged and retain the configuration
reference mdm.windows_entra_client_ids (or controls.windows_entra_client_ids)
and append the Microsoft link
https://learn.microsoft.com/en-us/windows/client-management/azure-active-directory-integration-with-mdm
for reference.
In
`@server/datastore/mysql/migrations/tables/20260529120000_AddAppConfigMDMWindowsEntraClientIDs_test.go`:
- Line 17: The test currently reads the app_config_json row with a
non-deterministic query (`SELECT json_value FROM app_config_json LIMIT 1`),
which relies on an implicit singleton invariant; update the test to explicitly
target the singleton row by querying with WHERE id = 1 (e.g., use `SELECT
json_value FROM app_config_json WHERE id = 1`) or alternatively add a clear
comment documenting the singleton invariant for app_config_json, referencing the
test that runs this query
(20260529120000_AddAppConfigMDMWindowsEntraClientIDs_test.go) so future changes
don’t introduce additional rows that break the assertion.
In `@server/service/microsoft_mdm.go`:
- Around line 969-988: The v1 audience host check in hasAuthorizedAzureAudience
currently does a case-sensitive compare between audURL.Host and serverHost which
can fail due to casing or default-port formatting; update the logic to
canonicalize both sides by parsing/normalizing hosts (split host and port,
remove default ports like :443 for https or :80 for http, or normalize to just
hostname) and then perform a case-insensitive comparison (e.g., use
strings.EqualFold on the normalized hostnames). Ensure you normalize serverHost
once (parse expected URL or host string) and apply the same normalization to
audURL.Host before comparing so v1 audience matches ignore case and default-port
differences.
🪄 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: ba6db45b-c762-43fd-8355-1bc3dd5bbd13
📒 Files selected for processing (20)
changes/46388-windows-entra-v2-access-tokenscmd/fleetctl/fleetctl/generate_gitops.gopkg/spec/gitops.gopkg/spec/gitops_test.gopkg/spec/testdata/controls.ymlpkg/spec/testdata/controls_new_names.ymlpkg/spec/testdata/global_config_no_paths.ymlpkg/spec/testdata/team_config_invalid_sha.ymlpkg/spec/testdata/team_config_no_paths.ymlserver/datastore/mysql/migrations/tables/20260529120000_AddAppConfigMDMWindowsEntraClientIDs.goserver/datastore/mysql/migrations/tables/20260529120000_AddAppConfigMDMWindowsEntraClientIDs_test.goserver/datastore/mysql/statistics.goserver/fleet/activities.goserver/fleet/app.goserver/fleet/statistics.goserver/service/appconfig.goserver/service/appconfig_test.goserver/service/client.goserver/service/microsoft_mdm.goserver/service/microsoft_mdm_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #46416 +/- ##
=======================================
Coverage 66.81% 66.81%
=======================================
Files 2804 2806 +2
Lines 223574 223652 +78
Branches 11346 11315 -31
=======================================
+ Hits 149379 149437 +58
- Misses 60640 60662 +22
+ Partials 13555 13553 -2
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:
|
Member
Author
|
@coderabbitai full review |
Member
Author
|
/agentic_review |
Contributor
✅ Actions performedFull review triggered. |
|
Code review by qodo was updated up to the latest commit c001e70 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/mdm/microsoft/wstep.go (1)
411-484:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t require
unique_nameinazureDataFromClaims(v2 tokens)
azureDataFromClaimscurrently fails withinvalid UniqueName claimwhenclaims["unique_name"]is missing. This blocks Entra v2 access tokens that usepreferred_usernameinstead. The automatic-enrollment auth path only usestokenData.Audience,tokenData.TenantID, and returnstokenData.UPN—UniqueNameisn’t used—sounique_nameshould be optional. Microsoft documentsunique_nameas v1-only andpreferred_usernameas v2.
https://learn.microsoft.com/en-us/entra/identity-platform/access-token-claims-reference💡 Minimal fix
- // Get UniqueName claim - uniqueNameClaim, ok := claims["unique_name"].(string) - if !ok { - return AzureData{}, ctxerr.New(ctx, "invalid UniqueName claim") - } + // `unique_name` is v1-only. Keep it when present, but don't reject v2 tokens that + // identify the user via `preferred_username`. + uniqueNameClaim, _ := claims["unique_name"].(string) + if uniqueNameClaim == "" { + uniqueNameClaim = upnClaim + }🤖 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/microsoft/wstep.go` around lines 411 - 484, azureDataFromClaims currently returns an error if claims["unique_name"] is missing; make UniqueName optional to support Entra v2 tokens that use preferred_username instead: in azureDataFromClaims remove the hard failure on missing unique_name (the block referencing uniqueNameClaim and the error "invalid UniqueName claim"), instead set AzureData.UniqueName to the value if present (claims["unique_name"].(string)) or leave it empty when absent; ensure downstream use (e.g., automatic-enrollment path) continues to rely only on Audience, TenantID and UPN.
🤖 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/pages/admin/IntegrationsPage/cards/MdmSettings/components/AddEntraClientIDModal/AddEntraClientIDModal.tsx`:
- Around line 60-68: The add flow treats clientId as possibly undefined and does
a case-sensitive duplicate check; update the AddEntraClientIDModal logic to
first narrow/guard clientId (e.g., early return if !clientId) so TypeScript
knows it's a string before using it in [...currentClientIds, clientId] and
configAPI.update, and perform a case-insensitive duplicate check by normalizing
both currentClientIds and clientId (e.g., compare currentClientIds.map(id =>
id.toLowerCase()) includes clientId.toLowerCase()) when computing clientIdExists
and before pushing into windows_entra_client_ids.
In
`@frontend/pages/admin/IntegrationsPage/cards/MdmSettings/WindowsAutomaticEnrollmentPage/EntraClientIDsListItem/EntraClientIDsListItem.tsx`:
- Around line 27-34: The icon-only delete Button in the EntraClientIDsListItem
component lacks an accessible name; update the Button (the element using props
disabled={disableChildren}, onClick={onClickDelete},
className={`${baseClass}__action-button`} and variant="icon") to include an
appropriate aria-label (e.g., "Delete client ID" or similar contextual text) so
screen readers can identify the action.
In
`@frontend/pages/DashboardPage/cards/ActivityFeed/GlobalActivityItem/GlobalActivityItem.tsx`:
- Around line 2098-2106: The addedMicrosoftEntraClientId and
deletedMicrosoftEntraClientId renderers currently interpolate
activity.details?.client_id directly which can produce "(undefined)"; update
both functions to guard the value and either omit the parenthetical when
client_id is falsy or supply a safe fallback (e.g., "unknown" or "unspecified")
and render accordingly so the UI never shows "undefined" in the activity text.
In `@server/service/appconfig.go`:
- Around line 1522-1525: The windowsEntraGUIDRegex only accepts lowercase hex
and rejects valid GUIDs with A-F; update its pattern to accept uppercase hex as
well (e.g. make character classes include A-F or use a case-insensitive regex
flag) so the Entra tenant and application client ID validations that use
windowsEntraGUIDRegex (the validation calls around the current checks) will
accept valid UUIDs in either case.
---
Outside diff comments:
In `@server/mdm/microsoft/wstep.go`:
- Around line 411-484: azureDataFromClaims currently returns an error if
claims["unique_name"] is missing; make UniqueName optional to support Entra v2
tokens that use preferred_username instead: in azureDataFromClaims remove the
hard failure on missing unique_name (the block referencing uniqueNameClaim and
the error "invalid UniqueName claim"), instead set AzureData.UniqueName to the
value if present (claims["unique_name"].(string)) or leave it empty when absent;
ensure downstream use (e.g., automatic-enrollment path) continues to rely only
on Audience, TenantID and UPN.
🪄 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: 20f97e63-6dc1-47af-88d1-ebd26479dfd8
📒 Files selected for processing (38)
changes/46388-windows-entra-v2-access-tokenscmd/fleetctl/fleetctl/generate_gitops.gofrontend/__mocks__/configMock.tsfrontend/interfaces/activity.tsfrontend/interfaces/config.tsfrontend/pages/DashboardPage/cards/ActivityFeed/GlobalActivityItem/GlobalActivityItem.tsxfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/WindowsAutomaticEnrollmentPage/EntraClientIDsListHeader/EntraClientIDsListHeader.tsxfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/WindowsAutomaticEnrollmentPage/EntraClientIDsListHeader/_styles.scssfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/WindowsAutomaticEnrollmentPage/EntraClientIDsListHeader/index.tsfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/WindowsAutomaticEnrollmentPage/EntraClientIDsListItem/EntraClientIDsListItem.tsxfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/WindowsAutomaticEnrollmentPage/EntraClientIDsListItem/_styles.scssfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/WindowsAutomaticEnrollmentPage/EntraClientIDsListItem/index.tsfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/WindowsAutomaticEnrollmentPage/WindowsAutomaticEnrollmentPage.tsxfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/components/AddEntraClientIDModal/AddEntraClientIDModal.tsxfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/components/AddEntraClientIDModal/helpers.tsfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/components/AddEntraClientIDModal/index.tsfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/components/DeleteEntraClientIDModal/DeleteEntraClientIDModal.tsxfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/components/DeleteEntraClientIDModal/index.tspkg/spec/gitops.gopkg/spec/gitops_test.gopkg/spec/testdata/controls.ymlpkg/spec/testdata/controls_new_names.ymlpkg/spec/testdata/global_config_no_paths.ymlpkg/spec/testdata/team_config_invalid_sha.ymlpkg/spec/testdata/team_config_no_paths.ymlserver/datastore/mysql/migrations/tables/20260529120000_AddAppConfigMDMWindowsEntraClientIDs.goserver/datastore/mysql/migrations/tables/20260529120000_AddAppConfigMDMWindowsEntraClientIDs_test.goserver/datastore/mysql/statistics.goserver/fleet/activities.goserver/fleet/app.goserver/fleet/statistics.goserver/mdm/microsoft/wstep.goserver/mdm/microsoft/wstep_test.goserver/service/appconfig.goserver/service/appconfig_test.goserver/service/client.goserver/service/microsoft_mdm.goserver/service/microsoft_mdm_test.go
![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.
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 #46388
Video demo: https://www.youtube.com/watch?v=t3yuGh0kwP8
Docs PR: #46483
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.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.Testing
Database migrations
New Fleet configuration settings
If you didn't check the box above, follow this checklist for GitOps-enabled settings:
fleetctl generate-gitopsSummary by CodeRabbit
New Features
Enhancements