Added support for validating Microsoft Entra v2 access tokens - #46416
Merged
Merged
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
WalkthroughThis pull request adds support for Microsoft Entra v2 access tokens during Windows MDM enrollment. It introduces a new AppConfig field for an Entra application client ID allowlist, a DB migration to initialize it, validation/normalization and activity emission on config changes, JWT audience authorization that accepts v2 client-ID GUIDs alongside existing v1 URL audiences, updated Azure claim extraction (UPN fallback and optional unique_name), GitOps and fleetctl support, frontend UI to manage client IDs, and tests covering unit, migration, and integration scenarios. 🚥 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.85% +0.04%
==========================================
Files 2804 2813 +9
Lines 223574 223893 +319
Branches 11346 11336 -10
==========================================
+ Hits 149379 149689 +310
+ Misses 60640 60630 -10
- Partials 13555 13574 +19
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}
Contributor
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.
ksykulev
reviewed
Jun 1, 2026
|
|
||
| // do an additional validation to check if the client id already exists in the config | ||
| const clientIdExists = | ||
| config?.mdm.windows_entra_client_ids?.includes(clientId ?? "") ?? false; |
Contributor
There was a problem hiding this comment.
Confused a bit.
Array.includes() is strict equality. The input has been lowercased, but config.mdm.windows_entra_client_ids contains whatever was stored which could be uppercase if the list was populated via GitOps or the API directly. The backend hasAuthorizedAzureAudience uses case-insensitive comparison, so both entries would authorize the same v2 token. So could the UI display two list entries that are functionally identical?
| upnClaim, ok := claims["upn"].(string) | ||
| if !ok || len(upnClaim) == 0 { | ||
| return AzureData{}, ctxerr.New(ctx, "invalid UPN claim") | ||
| upnClaim, ok = claims["preferred_username"].(string) |
Contributor
There was a problem hiding this comment.
On https://learn.microsoft.com/en-us/entra/identity-platform/access-token-claims-reference
The preferred_username entry there states: "Its value is mutable and might change over time. Since it's mutable, this value can't be used to make authorization decisions." Is it safe to use preferred_username.
Also just making sure but is the profile scope present in the token? As far as I understand the preferred_username needs that to appear at all, so if a v2 token is issued without that scope, the fallback would also fail and Fleet would reject the token entirely.
Member
Author
There was a problem hiding this comment.
Correct, preferred_username (or upn) is never compared against anything to grant or deny enrollment
I'll make the change to make it optional like unique_name.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/service/microsoft_mdm_test.go (1)
1748-1752: ⚡ Quick winAdd the reverse case-insensitivity regression for configured client IDs.
These cases only prove that an upper-cased token
audmatches a lower-cased configured ID. Please also cover the inverse (clientIDs: []string{strings.ToUpper(clientID)},audiences: []string{clientID}), otherwise an implementation that only normalizes the token side would still pass.🤖 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/microsoft_mdm_test.go` around lines 1748 - 1752, Add complementary test rows to microsoft_mdm_test.go to cover the reverse case-insensitivity of configured client IDs (so implementations that only normalize the token side fail). Specifically add cases like "v2 configured client ID uppercase" with clientIDs: []string{strings.ToUpper(clientID)} and audiences: []string{clientID} expecting true, and "v2 configured client ID uppercase with surrounding whitespace" with clientIDs: []string{" " + strings.ToUpper(clientID) + " "} and audiences: []string{clientID} expecting true; mirror the existing tests that reference clientID and clientID2 to ensure both single and multi-config scenarios are covered.
🤖 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 `@server/service/appconfig.go`:
- Around line 517-533: The code currently normalizes and de-duplicates
admin-provided Entra client IDs by mutating
appConfig.MDM.WindowsEntraClientIDs.Value; remove this normalization so stored
values remain exactly as provided (do not trim, lowercase, or dedupe on write).
Instead, perform canonicalization (trim+lowercase and dedupe) only at comparison
time where the values are checked (e.g., in hasAuthorizedAzureAudience or the
authorization-checking path that consumes appConfig.MDM.WindowsEntraClientIDs)
so comparisons remain case-insensitive but saved config is unchanged.
---
Nitpick comments:
In `@server/service/microsoft_mdm_test.go`:
- Around line 1748-1752: Add complementary test rows to microsoft_mdm_test.go to
cover the reverse case-insensitivity of configured client IDs (so
implementations that only normalize the token side fail). Specifically add cases
like "v2 configured client ID uppercase" with clientIDs:
[]string{strings.ToUpper(clientID)} and audiences: []string{clientID} expecting
true, and "v2 configured client ID uppercase with surrounding whitespace" with
clientIDs: []string{" " + strings.ToUpper(clientID) + " "} and audiences:
[]string{clientID} expecting true; mirror the existing tests that reference
clientID and clientID2 to ensure both single and multi-config scenarios are
covered.
🪄 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: d9fc46e6-a1e4-496d-ac00-3f26e49c067f
📒 Files selected for processing (31)
changes/46388-windows-entra-v2-access-tokenscmd/fleetctl/fleetctl/gitops_test.gocmd/fleetctl/fleetctl/testdata/expectedGetConfigAppConfigJson.jsoncmd/fleetctl/fleetctl/testdata/expectedGetConfigAppConfigTeamMaintainerJson.jsoncmd/fleetctl/fleetctl/testdata/expectedGetConfigAppConfigTeamMaintainerYaml.ymlcmd/fleetctl/fleetctl/testdata/expectedGetConfigAppConfigYaml.ymlcmd/fleetctl/fleetctl/testdata/expectedGetConfigIncludeServerConfigJson.jsoncmd/fleetctl/fleetctl/testdata/expectedGetConfigIncludeServerConfigYaml.ymlcmd/fleetctl/fleetctl/testdata/generateGitops/appConfig.jsoncmd/fleetctl/fleetctl/testdata/generateGitops/expectedGlobalControls.yamlcmd/fleetctl/fleetctl/testdata/generateGitops/test_dir_premium/fleets/unassigned.ymlcmd/fleetctl/fleetctl/testdata/macosSetupExpectedAppConfigEmpty.ymlcmd/fleetctl/fleetctl/testdata/macosSetupExpectedAppConfigSet.ymlfrontend/interfaces/config.tsfrontend/pages/DashboardPage/cards/ActivityFeed/GlobalActivityItem/GlobalActivityItem.tsxfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/WindowsAutomaticEnrollmentPage/EntraClientIDsListItem/EntraClientIDsListItem.tsxfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/WindowsAutomaticEnrollmentPage/EntraTenantsListItem/EntraTenantsListItem.tsxfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/WindowsAutomaticEnrollmentPage/WindowsAutomaticEnrollmentPage.tsxfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/WindowsAutomaticEnrollmentPage/_styles.scssfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/components/AddEntraClientIDModal/AddEntraClientIDModal.tests.tsxfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/components/AddEntraClientIDModal/AddEntraClientIDModal.tsxfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/components/DeleteEntraClientIDModal/DeleteEntraClientIDModal.tsxserver/datastore/mysql/schema.sqlserver/fleet/app.goserver/mdm/microsoft/wstep.goserver/mdm/microsoft/wstep_test.goserver/service/appconfig.goserver/service/appconfig_test.goserver/service/microsoft_mdm.goserver/service/microsoft_mdm_test.gotools/cloner-check/generated_files/appconfig.txt
✅ Files skipped from review due to trivial changes (9)
- cmd/fleetctl/fleetctl/testdata/expectedGetConfigIncludeServerConfigJson.json
- tools/cloner-check/generated_files/appconfig.txt
- cmd/fleetctl/fleetctl/testdata/expectedGetConfigAppConfigTeamMaintainerJson.json
- cmd/fleetctl/fleetctl/testdata/generateGitops/expectedGlobalControls.yaml
- cmd/fleetctl/fleetctl/testdata/expectedGetConfigAppConfigYaml.yml
- cmd/fleetctl/fleetctl/testdata/expectedGetConfigAppConfigTeamMaintainerYaml.yml
- cmd/fleetctl/fleetctl/testdata/expectedGetConfigIncludeServerConfigYaml.yml
- changes/46388-windows-entra-v2-access-tokens
- server/mdm/microsoft/wstep_test.go
🚧 Files skipped from review as they are similar to previous changes (15)
- cmd/fleetctl/fleetctl/testdata/macosSetupExpectedAppConfigSet.yml
- frontend/pages/admin/IntegrationsPage/cards/MdmSettings/WindowsAutomaticEnrollmentPage/EntraTenantsListItem/EntraTenantsListItem.tsx
- cmd/fleetctl/fleetctl/testdata/expectedGetConfigAppConfigJson.json
- frontend/interfaces/config.ts
- frontend/pages/admin/IntegrationsPage/cards/MdmSettings/WindowsAutomaticEnrollmentPage/EntraClientIDsListItem/EntraClientIDsListItem.tsx
- frontend/pages/admin/IntegrationsPage/cards/MdmSettings/WindowsAutomaticEnrollmentPage/_styles.scss
- server/fleet/app.go
- frontend/pages/admin/IntegrationsPage/cards/MdmSettings/WindowsAutomaticEnrollmentPage/WindowsAutomaticEnrollmentPage.tsx
- server/mdm/microsoft/wstep.go
- frontend/pages/DashboardPage/cards/ActivityFeed/GlobalActivityItem/GlobalActivityItem.tsx
- cmd/fleetctl/fleetctl/testdata/generateGitops/appConfig.json
- frontend/pages/admin/IntegrationsPage/cards/MdmSettings/components/DeleteEntraClientIDModal/DeleteEntraClientIDModal.tsx
- frontend/pages/admin/IntegrationsPage/cards/MdmSettings/components/AddEntraClientIDModal/AddEntraClientIDModal.tsx
- server/datastore/mysql/schema.sql
- server/service/microsoft_mdm.go
Comment on lines
+517
to
533
| // Normalize Entra client IDs to canonical lower-case and de-duplicate them. They are authorized | ||
| // case-insensitively (see hasAuthorizedAzureAudience), so storing them canonically prevents | ||
| // functionally-identical duplicates that differ only in case (for example, an upper-case ID added via | ||
| // GitOps or the API alongside a lower-case one added through the UI). | ||
| if appConfig.MDM.WindowsEntraClientIDs.Set && appConfig.MDM.WindowsEntraClientIDs.Valid { | ||
| seen := make(map[string]struct{}, len(appConfig.MDM.WindowsEntraClientIDs.Value)) | ||
| normalized := make([]string, 0, len(appConfig.MDM.WindowsEntraClientIDs.Value)) | ||
| for _, clientID := range appConfig.MDM.WindowsEntraClientIDs.Value { | ||
| id := strings.ToLower(strings.TrimSpace(clientID)) | ||
| if _, ok := seen[id]; ok { | ||
| continue | ||
| } | ||
| seen[id] = struct{}{} | ||
| normalized = append(normalized, id) | ||
| } | ||
| appConfig.MDM.WindowsEntraClientIDs.Value = normalized | ||
| } |
Contributor
There was a problem hiding this comment.
Don't canonicalize stored Entra client IDs.
This mutates admin-provided config by trimming, lower-casing, and de-duplicating it before save. The PR contract says client IDs should be compared case-insensitively, not rewritten on write, so this will cause GitOps/API read-after-write drift and case-only edits to disappear. Keep the raw stored values and do the case-insensitive handling only at comparison/activity time.
🤖 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/appconfig.go` around lines 517 - 533, The code currently
normalizes and de-duplicates admin-provided Entra client IDs by mutating
appConfig.MDM.WindowsEntraClientIDs.Value; remove this normalization so stored
values remain exactly as provided (do not trim, lowercase, or dedupe on write).
Instead, perform canonicalization (trim+lowercase and dedupe) only at comparison
time where the values are checked (e.g., in hasAuthorizedAzureAudience or the
authorization-checking path that consumes appConfig.MDM.WindowsEntraClientIDs)
so comparisons remain case-insensitive but saved config is unchanged.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@cmd/fleetctl/fleetctl/gitops_test.go`:
- Around line 571-573: The test is asserting that client IDs are stored
lower-cased and de-duplicated, but the feature requires case-insensitive
matching for validation while preserving the original stored values; update the
assertions in gitops_test.go (the `fleetctl gitops apply` test) so they no
longer expect canonical lower-casing or case-only de-duplication — instead
assert that the stored client IDs equal the original input list (including the
upper-case GUID and its case-only duplicate) at the three assertion sites
mentioned (around the current checks at ~571, ~596, and ~619-621), i.e. replace
expectations that downcase/deduplicate with equality to the original client ID
entries while leaving token-validation tests unchanged.
🪄 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: 66f43c2b-1edc-4015-bb63-38c9e9d54875
📒 Files selected for processing (1)
cmd/fleetctl/fleetctl/gitops_test.go
Comment on lines
+571
to
+573
| // `fleetctl gitops` apply (issue #46388). The client IDs include an upper-case GUID and a case-only duplicate of it, | ||
| // which exercises server-side normalization: client IDs are authorized case-insensitively, so they are stored | ||
| // canonically (lower-cased and de-duplicated) to avoid functionally-identical entries that differ only in case. |
Contributor
There was a problem hiding this comment.
This test now asserts the wrong storage contract.
The PR objective says Entra client IDs should be matched case-insensitively for token validation but stored without normalization. Expecting lower-casing and case-only de-duplication here would fail a correct implementation and bakes in behavior the feature explicitly says not to add.
Suggested expectation update
-// which exercises server-side normalization: client IDs are authorized case-insensitively, so they are stored
-// canonically (lower-cased and de-duplicated) to avoid functionally-identical entries that differ only in case.
+// which exercises case-insensitive audience matching. Stored config values should still be preserved as provided.
- // The upper-case client ID and its case-only duplicate are normalized to a single canonical lower-case entry.
+ // Client IDs should round-trip without storage normalization.
require.Equal(t,
- []string{"abcdef12-3456-7890-abcd-ef1234567890", "11111111-2222-3333-4444-555555555555"},
+ []string{
+ "ABCDEF12-3456-7890-ABCD-EF1234567890",
+ "abcdef12-3456-7890-abcd-ef1234567890",
+ "11111111-2222-3333-4444-555555555555",
+ },
(*savedAppConfigPtr).MDM.WindowsEntraClientIDs.Value)Also applies to: 596-596, 619-621
🤖 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 `@cmd/fleetctl/fleetctl/gitops_test.go` around lines 571 - 573, The test is
asserting that client IDs are stored lower-cased and de-duplicated, but the
feature requires case-insensitive matching for validation while preserving the
original stored values; update the assertions in gitops_test.go (the `fleetctl
gitops apply` test) so they no longer expect canonical lower-casing or case-only
de-duplication — instead assert that the stored client IDs equal the original
input list (including the upper-case GUID and its case-only duplicate) at the
three assertion sites mentioned (around the current checks at ~571, ~596, and
~619-621), i.e. replace expectations that downcase/deduplicate with equality to
the original client ID entries while leaving token-validation tests unchanged.
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
This branch was previously deployed
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
Documentation