🪙 fix: Resolve Azure AD Group Overage via OBO Token Exchange for OpenID#12187
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes Azure Entra ID group overage handling in the OpenID strategy by exchanging the app-audience access token for a Microsoft Graph-audience token (OBO flow) before resolving groups, and extends overage handling to OPENID_ADMIN_ROLE checks.
Changes:
- Added an OBO token exchange + caching path to obtain a Graph-scoped token for
/me/getMemberObjectsoverage resolution. - Updated group overage resolution to use the exchanged Graph token instead of the raw
tokenset.access_token. - Added admin-role overage detection and reuse of already-resolved overage groups to avoid duplicate Graph calls.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| api/strategies/openidStrategy.js | Implements Graph OBO exchange + cache for overage resolution; reuses resolved groups for admin role checks. |
| api/strategies/openidStrategy.spec.js | Updates existing Graph resolution assertions and adds tests for OBO exchange, caching, and admin overage behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| await tokensCache.set( | ||
| cacheKey, | ||
| { access_token: grantResponse.access_token }, | ||
| grantResponse.expires_in * 1000, |
There was a problem hiding this comment.
exchangeTokenForOverage() assumes grantResponse.expires_in is always present; if it’s missing, grantResponse.expires_in * 1000 becomes NaN and the cache set TTL may break or immediately expire. Consider defaulting to a sane value (e.g., (grantResponse.expires_in || 3600) * 1000) and/or skipping caching when expires_in is not a finite number.
| await tokensCache.set( | |
| cacheKey, | |
| { access_token: grantResponse.access_token }, | |
| grantResponse.expires_in * 1000, | |
| const expiresInSeconds = Number(grantResponse.expires_in); | |
| const ttlMs = | |
| Number.isFinite(expiresInSeconds) && expiresInSeconds > 0 | |
| ? expiresInSeconds * 1000 | |
| : 3600 * 1000; | |
| await tokensCache.set( | |
| cacheKey, | |
| { access_token: grantResponse.access_token }, | |
| ttlMs, |
| const cacheKey = `${sub}:overage`; | ||
|
|
||
| const cached = await tokensCache.get(cacheKey); | ||
| if (cached) { |
There was a problem hiding this comment.
The cache read path returns cached.access_token whenever cached is truthy, but doesn’t validate that access_token is actually present. If the cache contains an unexpected value (or a prior failed exchange cached an object without access_token), this would return undefined and lead to Authorization: Bearer undefined. Consider checking if (cached?.access_token) before returning and falling back to a fresh exchange otherwise.
| if (cached) { | |
| if (cached?.access_token) { |
| it('caches the exchanged token for subsequent calls', async () => { | ||
| const getLogStores = require('~/cache/getLogStores'); | ||
| const mockSet = jest.fn(); | ||
| getLogStores.mockReturnValue({ get: jest.fn().mockResolvedValue(undefined), set: mockSet }); | ||
|
|
||
| process.env.OPENID_REQUIRED_ROLE = 'group-required'; | ||
| process.env.OPENID_REQUIRED_ROLE_PARAMETER_PATH = 'groups'; | ||
| process.env.OPENID_REQUIRED_ROLE_TOKEN_KIND = 'id'; | ||
|
|
||
| jwtDecode.mockReturnValue({ hasgroups: true }); | ||
|
|
||
| await setupOpenId(); | ||
| verifyCallback = require('openid-client/passport').__getVerifyCallbackByName('openid'); | ||
|
|
||
| undici.fetch.mockResolvedValue({ | ||
| ok: true, | ||
| status: 200, | ||
| statusText: 'OK', | ||
| json: async () => ({ value: ['group-required'] }), | ||
| }); | ||
|
|
||
| await validate(tokenset); | ||
|
|
||
| expect(mockSet).toHaveBeenCalledWith( | ||
| '1234:overage', | ||
| { access_token: 'exchanged_graph_token' }, | ||
| 3600000, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
This test is named as if it verifies reuse of the cached exchanged token, but it only asserts that set() was called once. To actually test caching behavior, consider running the overage flow twice (or calling the exchange helper twice) and asserting genericGrantRequest is only called once and the second run uses the cached token.
ebb4dc8 to
bc21856
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const grantResponse = await client.genericGrantRequest( | ||
| openidConfig, | ||
| 'urn:ietf:params:oauth:grant-type:jwt-bearer', | ||
| { | ||
| scope: 'https://graph.microsoft.com/User.Read', | ||
| assertion: accessToken, | ||
| requested_token_use: 'on_behalf_of', | ||
| }, |
There was a problem hiding this comment.
exchangeTokenForOverage() hardcodes the OBO scope to https://graph.microsoft.com/User.Read. Elsewhere in the repo, Graph OBO scopes are configurable (e.g., OPENID_GRAPH_SCOPES in GraphApiService.exchangeTokenForGraphAccess), and different tenants may require additional Graph delegated permissions for membership resolution. Consider making the overage exchange scope configurable (new env var or reuse OPENID_GRAPH_SCOPES with a safe default) so deployments can adjust without code changes.
There was a problem hiding this comment.
Hmm... The /me/getMemberObjects endpoint only requires the User.Read delegated permission as it's the least privileged scope needed (link).
Making it configurable would add complexity for a case it's not so common, but it can be implemented if needed.
|
Taking a look at this today. Thanks for your patience |
|
Here's an automated review I run on all my PR's via LibreChat that has some findings. If you have the time to validate them, that would be great: PASS 1: CORE LOGIC[P1-A] Removal of
|
| Property | exchangeAccessTokenIfNeeded |
exchangeTokenForOverage |
|---|---|---|
| Config source | Explicit parameter | Module-level global |
| Trigger | Feature flag | Always |
| Cache key | sub |
${sub}:overage |
| TTL guard | None (no isFinite check) |
Number.isFinite guard |
The naming also differs: one is action+condition (exchangeAccessTokenIfNeeded), the other is action+purpose (exchangeTokenForOverage). Both functions do nearly the same thing. Per AGENTS.md: "Parameterized helpers instead of near-duplicate functions." These could be refactored into a single, parameterized OBO helper.
[P3-B] CRLF line endings in new test additions
The new test code in openidStrategy.spec.js uses CRLF line endings, visible as \r in the diff. The entire spec file uses CRLF. The implementation file uses LF. While consistent within the file, AGENTS.md says formatting lint errors must be fixed. This should be normalized to LF project-wide.
[P3-C] Inline comment violates the "no narrating comments" rule
// Handle Azure AD group overage for admin role when using ID token groupsAGENTS.md: "Write self-documenting code; no inline comments narrating what code does." This comment narrates mechanics. It's borderline acceptable since it names the business domain ("Azure AD group overage"), but the function names and conditions should speak for themselves.
[P3-D] resolvedOverageGroups pollution of function scope
The let resolvedOverageGroups = null variable is declared at the top of processOpenIDAuth and threaded through required-role and admin-role blocks. This is implicit shared mutable state within a function. Per AGENTS.md: "functional first, immutable data." A cleaner approach would extract the overage resolution into a helper that takes both config objects and returns a memoized resolver.
PASS 4: PERFORMANCE
[P4-A] Cache TTL not reduced by a clock skew safety buffer
const ttlMs =
Number.isFinite(grantResponse.expires_in) && grantResponse.expires_in > 0
? grantResponse.expires_in * 1000
: 3600 * 1000;The cached Graph token's TTL is set to exactly expires_in seconds. If there's clock skew between the Azure AD server and the application server, or network/processing latency, a cached entry near its expiry could arrive as expired at the Microsoft Graph API, causing a 401 and silent login failure. The standard defense is subtracting a 60-second safety buffer (expires_in * 1000 - 60_000). Note: exchangeAccessTokenIfNeeded has the same omission. Fixing here without fixing there would be inconsistent.
[P4-B] resolvedOverageGroups in-memory reuse correctly avoids duplicate Graph call
When both requiredRole and adminRole use ID token groups with overage, the Graph call is made exactly once per auth flow (from the required role block), and resolvedOverageGroups is reused for the admin check. The OPENID_EXCHANGED_TOKENS cache further deduplicates the OBO exchange across requests. This is correct and efficient.
[P4-C] No pagination for /me/getMemberObjects
The Graph API call is not paginated. /me/getMemberObjects returns up to 1,000 group IDs but does not use @odata.nextLink pagination. For users in 1,000+ groups, the API returns an error rather than truncating. This is a pre-existing limitation, not introduced by this PR, and documented by Microsoft's 1,000-group hard cap for this endpoint.
[P4-D] Thundering herd on cold cache
Two concurrent login requests for the same user both trigger a cache miss and issue two parallel OBO exchanges. The second write overwrites the first. No correctness issue (both tokens are valid), but it is wasted work. This is inherent to the Keyv-based caching design throughout the module, not a regression.
STEP 4 & 5: SYNTHESIZED FINAL REPORT
1. Change Summary
When Azure AD users belong to more than 200 groups, Microsoft removes all group claims from the ID token ("group overage") and sets a marker (hasgroups: true or _claim_names/_claim_sources). This PR fixes two bugs: (1) resolveGroupsFromOverage() was calling Microsoft Graph with the app-audience access token, which Graph rejected with HTTP 401; it now exchanges the token via the OBO flow to get a Graph-scoped token first. (2) The OPENID_ADMIN_ROLE block never checked for group overage, silently failing to grant the admin role. The PR also adds shared caching of the resolved overage groups to avoid a redundant Graph call when both checks are active.
2. Files Modified
| File | Change |
|---|---|
api/strategies/openidStrategy.js |
New exchangeTokenForOverage() OBO helper; updated resolveGroupsFromOverage() to use it; added admin-role overage detection with resolvedOverageGroups sharing |
api/strategies/openidStrategy.spec.js |
Updated existing assertion for Graph token; added 8 new test cases covering OBO exchange, caching, and admin-role overage scenarios |
3. Numbered Findings
Finding 1 — MAJOR | Confidence 75 | api/strategies/openidStrategy.js ~line 360
Problem: grantResponse.access_token is never validated before caching or returning
await tokensCache.set(cacheKey, { access_token: grantResponse.access_token }, ttlMs);
return grantResponse.access_token; // ← could be undefinedIf the OBO endpoint returns HTTP 200 with a valid JSON body that omits access_token (misbehaving IdP, or a future AAD policy change), the function caches { access_token: undefined } and returns undefined. resolveGroupsFromOverage then sends Authorization: Bearer undefined to Graph, gets HTTP 401, and logs a completely misleading "Failed to resolve groups via Microsoft Graph getMemberObjects: HTTP 401 Unauthorized" — the actual failure (broken OBO response) is invisible. Login is denied without any actionable diagnostic. The cache entry { access_token: undefined } is harmless long-term (cache miss on next check), but every subsequent login attempt for that user retries the broken OBO exchange.
Fix:
if (!grantResponse.access_token) {
throw new Error(
'[openidStrategy] OBO exchange succeeded but returned no access_token; cannot call Graph API',
);
}
await tokensCache.set(cacheKey, { access_token: grantResponse.access_token }, ttlMs);
return grantResponse.access_token;Question: Does the openid-client library guarantee access_token is present in all non-error grant responses, or can it return a partial token response?
Finding 2 — MINOR | Confidence 90 | api/strategies/openidStrategy.js ~line 346
Problem: exchangeTokenForOverage implicitly depends on module-level openidConfig with no guard
const grantResponse = await client.genericGrantRequest(
openidConfig, // ← module-level, could be null
...
);The sibling function exchangeAccessTokenIfNeeded(config, accessToken, sub) receives config as an explicit parameter. This function breaks that pattern. The implicit module-level dependency makes the function impure, untestable in isolation, and inconsistent with the established design. If openidConfig is null for any reason (e.g., setupOpenId failed midway), genericGrantRequest(null, ...) throws, caught by resolveGroupsFromOverage's catch block, logging a misleading "Error resolving groups via Microsoft Graph" when the real problem is an uninitialized OpenID config.
Fix (minimal):
async function exchangeTokenForOverage(accessToken, sub) {
if (!openidConfig) {
throw new Error('[openidStrategy] OpenID config not initialized; cannot exchange OBO token');
}
...
}Fix (proper, aligned with existing design): Add config as the first parameter, consistent with exchangeAccessTokenIfNeeded.
Finding 3 — MINOR | Confidence 55 | api/strategies/openidStrategy.js ~line 336
Problem: sub is not validated before being used as a cache key
const cacheKey = `${sub}:overage`;If claims.sub is undefined, null, or '' (possible with non-compliant IdPs, or if tokenset.claims() returns a malformed object), the cache key becomes undefined:overage, null:overage, or :overage. All such users share the same cache entry, meaning User A could receive User B's Graph token on a cache hit. While sub is required by the OpenID Connect spec and always present in Azure AD tokens, this is a defense-in-depth gap in security-sensitive authentication code.
Fix:
async function exchangeTokenForOverage(accessToken, sub) {
if (!sub || typeof sub !== 'string') {
throw new Error('[openidStrategy] Valid subject (sub) is required for OBO token cache keying');
}
...
}Finding 4 — MINOR | Confidence 95 | api/strategies/openidStrategy.js ~lines 624–628
Problem: Dead assignment — resolvedOverageGroups is updated inside the admin block but never read again
if (overageGroups) {
adminRoles = overageGroups;
if (!resolvedOverageGroups) {
resolvedOverageGroups = overageGroups; // ← never read after this point
}
}After the admin role block completes, processOpenIDAuth only deals with user persistence and avatar. resolvedOverageGroups has no more consumers. This dead write implies an anticipated future consumer that doesn't exist, misleading future maintainers.
Fix: Remove the dead update entirely:
if (overageGroups) {
adminRoles = overageGroups;
}Finding 5 — MINOR | Confidence 90 | api/strategies/openidStrategy.spec.js
Problem: No test for admin-only overage (OPENID_REQUIRED_ROLE absent or non-groups path)
Every test in the 'admin role group overage' suite configures both OPENID_REQUIRED_ROLE and OPENID_ADMIN_ROLE. The code path where resolvedOverageGroups is null (because the required role block didn't run or used a non-groups path) and the admin block independently calls resolveGroupsFromOverage is never exercised. Admin escalation is a security-sensitive decision; every code path leading to user.role = ADMIN needs direct test coverage.
Fix — add two tests:
it('resolves admin via Graph independently when OPENID_REQUIRED_ROLE is not configured', async () => {
delete process.env.OPENID_REQUIRED_ROLE;
process.env.OPENID_ADMIN_ROLE = 'admin-group-id';
process.env.OPENID_ADMIN_ROLE_PARAMETER_PATH = 'groups';
process.env.OPENID_ADMIN_ROLE_TOKEN_KIND = 'id';
jwtDecode.mockReturnValue({ hasgroups: true });
await setupOpenId();
verifyCallback = require('openid-client/passport').__getVerifyCallbackByName('openid');
undici.fetch.mockResolvedValue({
ok: true, status: 200, statusText: 'OK',
json: async () => ({ value: ['admin-group-id'] }),
});
const { user } = await validate(tokenset);
expect(user.role).toBe('ADMIN');
expect(undici.fetch).toHaveBeenCalledTimes(1);
});
it('denies admin when OPENID_REQUIRED_ROLE is absent and Graph does not contain admin group', async () => {
delete process.env.OPENID_REQUIRED_ROLE;
process.env.OPENID_ADMIN_ROLE = 'admin-group-id';
process.env.OPENID_ADMIN_ROLE_PARAMETER_PATH = 'groups';
process.env.OPENID_ADMIN_ROLE_TOKEN_KIND = 'id';
jwtDecode.mockReturnValue({ hasgroups: true });
await setupOpenId();
verifyCallback = require('openid-client/passport').__getVerifyCallbackByName('openid');
undici.fetch.mockResolvedValue({
ok: true, status: 200, statusText: 'OK',
json: async () => ({ value: ['other-group'] }),
});
const { user } = await validate(tokenset);
expect(user.role).toBeUndefined();
});Finding 6 — MINOR | Confidence 85 | api/strategies/openidStrategy.spec.js
Problem: No test for OBO exchange failure — genericGrantRequest throwing
All tests mock genericGrantRequest to succeed. If the OBO exchange itself fails (network error, Azure AD rejects the assertion with HTTP 400/401, tenant policy blocks OBO), genericGrantRequest throws. The exception propagates through exchangeTokenForOverage to resolveGroupsFromOverage's catch block, returning null. Login is denied. But the logged error says "Error resolving groups via Microsoft Graph getMemberObjects" — the error object contains the OBO failure, not a Graph API failure. There should be a test verifying this graceful degradation and that the error is logged correctly.
Fix — add a test:
it('denies login and logs error when OBO exchange throws', async () => {
const openidClient = require('openid-client');
process.env.OPENID_REQUIRED_ROLE = 'group-required';
process.env.OPENID_REQUIRED_ROLE_PARAMETER_PATH = 'groups';
process.env.OPENID_REQUIRED_ROLE_TOKEN_KIND = 'id';
jwtDecode.mockReturnValue({ hasgroups: true });
openidClient.genericGrantRequest.mockRejectedValue(new Error('OBO exchange rejected'));
await setupOpenId();
verifyCallback = require('openid-client/passport').__getVerifyCallbackByName('openid');
const { user, details } = await validate(tokenset);
expect(user).toBe(false);
expect(details.message).toBe('You must have "group-required" role to log in.');
expect(undici.fetch).not.toHaveBeenCalled();
});Finding 7 — NIT | Confidence 65 | api/strategies/openidStrategy.js ~line 355
Problem: Cache TTL uses full expires_in without a clock-skew safety buffer
const ttlMs =
Number.isFinite(grantResponse.expires_in) && grantResponse.expires_in > 0
? grantResponse.expires_in * 1000
: 3600 * 1000;A cached Graph token served near the end of its validity window could be expired by the time it reaches Microsoft's servers. Standard OAuth practice subtracts a small safety margin (60s). Note: exchangeAccessTokenIfNeeded has the same omission (no isFinite guard, and no clock-skew buffer). Fixing here without fixing there adds inconsistency. Worth addressing in both places together.
Fix:
const CLOCK_SKEW_BUFFER_MS = 60 * 1000;
const ttlMs =
Number.isFinite(grantResponse.expires_in) && grantResponse.expires_in > 0
? Math.max(0, grantResponse.expires_in * 1000 - CLOCK_SKEW_BUFFER_MS)
: 3600 * 1000;Finding 8 — NIT | Confidence 85 | api/strategies/openidStrategy.js overall
Problem: Two near-duplicate OBO helper functions should be consolidated
exchangeAccessTokenIfNeeded and exchangeTokenForOverage both:
- Get the
OPENID_EXCHANGED_TOKENScache - Check for a cached token
- Call
client.genericGrantRequestwithurn:ietf:params:oauth:grant-type:jwt-bearer - Store the result and return the access token
The only differences are: the cache key suffix, the scope, whether a feature flag gates execution, and whether config is passed as a param. Per AGENTS.md: "Parameterized helpers instead of near-duplicate functions." These should be unified:
async function exchangeOBOToken(config, accessToken, sub, scope, cacheKeySuffix = '') {
const tokensCache = getLogStores(CacheKeys.OPENID_EXCHANGED_TOKENS);
const cacheKey = cacheKeySuffix ? `${sub}:${cacheKeySuffix}` : sub;
const cached = await tokensCache.get(cacheKey);
if (cached?.access_token) { return cached.access_token; }
const grantResponse = await client.genericGrantRequest(config, 'urn:ietf:params:oauth:grant-type:jwt-bearer', {
scope,
assertion: accessToken,
requested_token_use: 'on_behalf_of',
});
if (!grantResponse.access_token) {
throw new Error(`[openidStrategy] OBO exchange for scope "${scope}" returned no access_token`);
}
const ttlMs = Number.isFinite(grantResponse.expires_in) && grantResponse.expires_in > 0
? grantResponse.expires_in * 1000
: 3600 * 1000;
await tokensCache.set(cacheKey, { access_token: grantResponse.access_token }, ttlMs);
return grantResponse.access_token;
}4. Summary Table
| # | Severity | Confidence | Description |
|---|---|---|---|
| 1 | MAJOR | 75 | grantResponse.access_token not validated before cache/return — Authorization: Bearer undefined silently sent to Graph |
| 2 | MINOR | 90 | exchangeTokenForOverage relies on implicit module-level openidConfig instead of explicit parameter |
| 3 | MINOR | 55 | sub not validated before cache key construction — potential key collision if sub is absent |
| 4 | MINOR | 95 | Dead code: resolvedOverageGroups = overageGroups in admin block, variable never read again |
| 5 | MINOR | 90 | No test for admin-only overage when OPENID_REQUIRED_ROLE is not configured |
| 6 | MINOR | 85 | No test for OBO exchange throwing — genericGrantRequest error path untested |
| 7 | NIT | 65 | Cache TTL uses full expires_in with no clock-skew safety buffer (pre-existing pattern) |
| 8 | NIT | 85 | exchangeTokenForOverage and exchangeAccessTokenIfNeeded are near-duplicate OBO functions; should be consolidated |
5. Verdict
The core bug fix is correct and necessary. The OBO token exchange is the right solution for the HTTP 401 problem. The overage detection logic (hasgroups / _claim_names/_claim_sources) is technically sound. The resolvedOverageGroups sharing is a real performance optimization. The tests added are meaningful and cover the primary happy and error paths. The existing Copilot review comments (1–3) were all already addressed in the final commit.
Must address before merge:
- Finding 1 is the most actionable: a one-line guard on
grantResponse.access_tokenbefore caching. The current code turns a broken OBO response into a misleading Graph 401 error. Easy fix, high value.
Should address before merge:
- Finding 2 (null config guard) — a two-line defensive check that makes failures diagnosable.
- Finding 4 (dead assignment) — remove two lines of dead code that mislead readers.
- Finding 5 (missing admin-only test) — admin escalation paths require direct test coverage; adding it now is straightforward.
- Finding 6 (missing OBO failure test) — error path test for the new function.
Can be follow-up work:
- Finding 3 (sub validation), Finding 7 (clock skew), Finding 8 (consolidate OBO helpers) — these are quality improvements that belong in a broader refactor of the OBO exchange infrastructure rather than this focused bug fix PR.
|
Thanks! I will work over them as soon as possible 😄 |
When Azure AD users belong to 200+ groups, group claims are moved out of the ID token (overage). The existing resolveGroupsFromOverage() called Microsoft Graph directly with the app-audience access token, which Graph rejected (401/403). Changes: - Add exchangeTokenForOverage() dedicated OBO exchange with User.Read scope - Update resolveGroupsFromOverage() to exchange token before Graph call - Add overage handling to OPENID_ADMIN_ROLE block (was silently failing) - Share resolved overage groups between required role and admin role checks - Always resolve via Graph when overage detected (even with partial groups) - Remove debug-only bypass that forced Graph resolution - Add tests for OBO exchange, caching, and admin role overage scenarios
bc21856 to
c9772a9
Compare
|
I addressed some of the issues. Implemented:
Deferred (as suggested in the verdict):
If there's something else you want to be implemented I can do it. 😄 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
danny-avila
changed the title
…ID (danny-avila#12187) When Azure AD users belong to 200+ groups, group claims are moved out of the ID token (overage). The existing resolveGroupsFromOverage() called Microsoft Graph directly with the app-audience access token, which Graph rejected (401/403). Changes: - Add exchangeTokenForOverage() dedicated OBO exchange with User.Read scope - Update resolveGroupsFromOverage() to exchange token before Graph call - Add overage handling to OPENID_ADMIN_ROLE block (was silently failing) - Share resolved overage groups between required role and admin role checks - Always resolve via Graph when overage detected (even with partial groups) - Remove debug-only bypass that forced Graph resolution - Add tests for OBO exchange, caching, and admin role overage scenarios Co-authored-by: Airam Hernández Hernández <airam.hernandez@intelequia.com>
…ID (danny-avila#12187) When Azure AD users belong to 200+ groups, group claims are moved out of the ID token (overage). The existing resolveGroupsFromOverage() called Microsoft Graph directly with the app-audience access token, which Graph rejected (401/403). Changes: - Add exchangeTokenForOverage() dedicated OBO exchange with User.Read scope - Update resolveGroupsFromOverage() to exchange token before Graph call - Add overage handling to OPENID_ADMIN_ROLE block (was silently failing) - Share resolved overage groups between required role and admin role checks - Always resolve via Graph when overage detected (even with partial groups) - Remove debug-only bypass that forced Graph resolution - Add tests for OBO exchange, caching, and admin role overage scenarios Co-authored-by: Airam Hernández Hernández <airam.hernandez@intelequia.com>
Summary
When Azure AD users belong to 200+ groups, group claims are removed from the ID token (overage). Two bugs existed:
resolveGroupsFromOverage()used the wrong token for Graph API it passed the rawtokenset.access_token(audienceapi://<client-id>) directly to Microsoft Graph, which rejected it with HTTP 401 because Graph requiresaud: https://graph.microsoft.com.OPENID_ADMIN_ROLEhad no overage handling the admin role block never checked forhasgroupsor_claim_names/_claim_sources, so admins were silently demoted toUSERduring overage.Fixes #12185 and #12186
Change Type
Testing
Test Configuration
OPENID_REQUIRED_ROLE=<role-id>OPENID_REQUIRED_ROLE_TOKEN_KIND=idOPENID_REQUIRED_ROLE_PARAMETER_PATH=groupsOPENID_ADMIN_ROLE=<role-id>OPENID_ADMIN_ROLE_TOKEN_KIND=id,OPENID_ADMIN_ROLE_PARAMETER_PATH=groupsOPENID_SCOPE="api://<client-id>/.default openid profile email offline_access"Checklist