From 4b94ed869bf7a24b36e97038cf57c0c0362e514c Mon Sep 17 00:00:00 2001 From: shaurya Date: Thu, 1 Oct 2026 12:37:31 +0530 Subject: [PATCH] fix: surface Microsoft Graph errors during Entra group overage instead of logging in with an incomplete group set (#3535) * Propagate Microsoft Graph errors during Entra group overage When a session hit AAD group overage, addGraphGroupsToSession logged a failed Graph request and returned nil, so EnrichSession's error handling never ran and the session kept the overage placeholder instead of the real groups. Return the error like the rest of the file does, and add a regression test using the existing 401 Graph mock. Signed-off-by: Ishan Shaurya Jaiswal <19599684+no-hup@users.noreply.github.com> * Tighten the Graph-error test and wrap the error, add a changelog entry Wrap the Graph error with %w so callers can errors.Is through EnrichSession. Pin the test to the Graph path with ErrorContains and assert the session's groups are left untouched after the failed call, so the test fails if the fix regresses rather than on any error. Add the behaviour change to the changelog since a Graph outage during overage now fails the login instead of proceeding with the overage placeholder. Signed-off-by: Ishan Shaurya Jaiswal <19599684+no-hup@users.noreply.github.com> --------- Signed-off-by: Ishan Shaurya Jaiswal <19599684+no-hup@users.noreply.github.com> Co-authored-by: Ishan Shaurya Jaiswal <19599684+no-hup@users.noreply.github.com> --- CHANGELOG.md | 1 + providers/ms_entra_id.go | 3 +-- providers/ms_entra_id_test.go | 38 +++++++++++++++++++++++++++++++++++ 3 files changed, 40 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c974da0d..4c240122 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ Additionally refer to OAuth client configuration for Bitbucket provider in the [ - [#3546](https://github.com/oauth2-proxy/oauth2-proxy/pull/3546) fix: strip the port from the request host when matching cookie domains @kirilju - [#3547](https://github.com/oauth2-proxy/oauth2-proxy/pull/3547) fix: refresh additional claims for OIDC and MS Entra ID providers and properly populate additional claims during login (@Apollo3zehn) - [#3477](https://github.com/oauth2-proxy/oauth2-proxy/pull/3477) fix(bitbucket): auth failure due to Bitbucket OAuth 2.0 [changes on May 4th 2026](https://developer.atlassian.com/cloud/bitbucket/changelog/#CHANGE-3052) @mfouad +- [#3535](https://github.com/oauth2-proxy/oauth2-proxy/pull/3535) fix: surface Microsoft Graph errors during Entra group overage instead of logging in with an incomplete group set @no-hup # V7.15.4 diff --git a/providers/ms_entra_id.go b/providers/ms_entra_id.go index 052d45a8..1229c0de 100644 --- a/providers/ms_entra_id.go +++ b/providers/ms_entra_id.go @@ -248,8 +248,7 @@ func (p *MicrosoftEntraIDProvider) addGraphGroupsToSession(ctx context.Context, UnmarshalSimpleJSON() if err != nil { - logger.Errorf("invalid response from microsoft graph, no groups added to session: %v", err) - return nil + return fmt.Errorf("invalid response from microsoft graph: %w", err) } reqGroups := response.Get("value").MustArray() diff --git a/providers/ms_entra_id_test.go b/providers/ms_entra_id_test.go index b153006e..0cd807d1 100644 --- a/providers/ms_entra_id_test.go +++ b/providers/ms_entra_id_test.go @@ -82,6 +82,44 @@ func TestAzureEntraOIDCProviderEnrichSessionGroupOverage(t *testing.T) { assert.Contains(t, session.Groups, "b1aef995-6b55-4ac6-bbfe-e829810e9352", "Pagination using $skiptoken failed") } +func TestAzureEntraOIDCProviderEnrichSessionGraphError(t *testing.T) { + // Create ID Token that indicates group overage with _claim_names + key, _ := rsa.GenerateKey(rand.Reader, 2048) + claimsWithGroupOverage := &claimsWithGroupOverage{ + jwt.RegisteredClaims{ + Issuer: "https://login.microsoftonline.com/18014347-dd57-41a1-8191-7a1f734ea457/v2.0", + }, + map[string]string{"groups": "src1"}, + } + + jwtWithClaims := jwt.NewWithClaims(jwt.SigningMethodRS256, claimsWithGroupOverage) + signedJWT, err := jwtWithClaims.SignedString(key) + assert.NoError(t, err) + + session := CreateAuthorizedSession() + session.IDToken = signedJWT + session.Email = "mock@example.com" + + provider := NewMicrosoftEntraIDProvider(&ProviderData{}, + options.Provider{OIDCConfig: options.OIDCOptions{ + IssuerURL: "https://login.microsoftonline.com/18014347-dd57-41a1-8191-7a1f734ea457/v2.0", + }}, + ) + + // Mock a Graph server that rejects the groups request + mockedGraph := mockGraphAPI(true) + mockedGraphURL, _ := url.Parse(mockedGraph.URL) + updateURL(provider.microsoftGraphURL, mockedGraphURL.Host) + + // A failed Graph lookup during overage must surface as an error, not a + // silently under-populated session. + groupsBefore := session.Groups + err = provider.EnrichSession(context.Background(), session) + assert.ErrorContains(t, err, "invalid response from microsoft graph") + // The session must be left exactly as it was, not half-populated. + assert.Equal(t, groupsBefore, session.Groups) +} + func TestAzureEntraOIDCProviderValidateSessionAllowedTenants(t *testing.T) { // Create multi-tenant Azure Entra provider with allowed tenants provider := NewMicrosoftEntraIDProvider(