diff --git a/helpers.go b/helpers.go index f7534ca..fde8710 100644 --- a/helpers.go +++ b/helpers.go @@ -404,3 +404,20 @@ func BuildLogoutURL(endSessionURL, idToken, postLogoutRedirectURI string) (strin return u.String(), nil } + +// deduplicateScopes removes duplicate strings from a slice while preserving order. +// The first occurrence of each scope is kept. +func deduplicateScopes(scopes []string) []string { + if len(scopes) == 0 { + return []string{} + } + seen := make(map[string]struct{}) + result := []string{} + for _, scope := range scopes { + if _, ok := seen[scope]; !ok { + seen[scope] = struct{}{} + result = append(result, scope) + } + } + return result +} diff --git a/main.go b/main.go index 3a4f8f2..762bf19 100644 --- a/main.go +++ b/main.go @@ -614,6 +614,7 @@ func New(ctx context.Context, next http.Handler, config *Config, name string) (h // Initialize logger logger := NewLogger(config.LogLevel) + // Log the scopes received from Traefik to help diagnose duplication issues // Ensure key meets minimum length requirement if len(config.SessionEncryptionKey) < minEncryptionKeyLength { if runtime.Compiler == "yaegi" { @@ -660,10 +661,16 @@ func New(ctx context.Context, next http.Handler, config *Config, name string) (h enablePKCE: config.EnablePKCE, overrideScopes: config.OverrideScopes, scopes: func() []string { + // Deduplicate scopes from config first + uniqueConfigScopes := deduplicateScopes(config.Scopes) if config.OverrideScopes { - return append([]string(nil), config.Scopes...) + return uniqueConfigScopes } - return mergeScopes([]string{"openid", "profile", "email"}, config.Scopes) + // If not overriding, merge with defaults (which also handles deduplication) + defaultInitialScopes := []string{"openid", "profile", "email"} // Explicitly define for logging + merged := mergeScopes(defaultInitialScopes, uniqueConfigScopes) + return merged + // return mergeScopes([]string{"openid", "profile", "email"}, uniqueConfigScopes) }(), limiter: rate.NewLimiter(rate.Every(time.Second), config.RateLimit), tokenCache: cacheManager.GetSharedTokenCache(), @@ -723,7 +730,7 @@ func New(ctx context.Context, next http.Handler, config *Config, name string) (h } startReplayCacheCleanup(pluginCtx, logger) - + logger.Debugf("TraefikOidc.New: Final t.scopes initialized to: %v", t.scopes) go t.initializeMetadata(config.ProviderURL) return t, nil @@ -1687,27 +1694,40 @@ func (t *TraefikOidc) buildAuthURL(redirectURL, state, nonce, codeChallenge stri } } - if !hasOfflineAccess { - scopes = append(scopes, "offline_access") - t.logger.Debug("Azure AD provider detected, added offline_access scope for refresh tokens") + // For Azure AD, add offline_access scope if not overriding or if overriding with no user scopes + if !t.overrideScopes || (t.overrideScopes && len(t.scopes) == 0) { + if !hasOfflineAccess { + scopes = append(scopes, "offline_access") + t.logger.Debugf("Azure AD provider: Added offline_access scope (overrideScopes: %t, user scopes count: %d)", t.overrideScopes, len(t.scopes)) + } + } else { + t.logger.Debugf("Azure AD provider: User is overriding scopes (count: %d), offline_access not automatically added.", len(t.scopes)) } } else { // For other providers, use the standard offline_access scope - hasOfflineAccess := false - for _, scope := range scopes { - if scope == "offline_access" { - hasOfflineAccess = true - break + // Only add offline_access if overrideScopes is false, + // or if overrideScopes is true AND no scopes were provided by the user (edge case, effectively defaults) + if !t.overrideScopes || (t.overrideScopes && len(t.scopes) == 0) { + hasOfflineAccess := false + for _, scope := range scopes { + if scope == "offline_access" { + hasOfflineAccess = true + break + } } - } - - if !hasOfflineAccess { - scopes = append(scopes, "offline_access") + if !hasOfflineAccess { + scopes = append(scopes, "offline_access") + t.logger.Debugf("Standard provider: Added offline_access scope (overrideScopes: %t, user scopes count: %d)", t.overrideScopes, len(t.scopes)) + } + } else { + t.logger.Debugf("Standard provider: User is overriding scopes (count: %d), offline_access not automatically added.", len(t.scopes)) } } if len(scopes) > 0 { - params.Set("scope", strings.Join(scopes, " ")) + finalScopeString := strings.Join(scopes, " ") + params.Set("scope", finalScopeString) + t.logger.Debugf("TraefikOidc.buildAuthURL: Final scope string being sent to OIDC provider: %s", finalScopeString) } // Use buildURLWithParams which handles potential relative authURL from metadata diff --git a/main_test.go b/main_test.go index 1c8daab..046fd89 100644 --- a/main_test.go +++ b/main_test.go @@ -3998,9 +3998,12 @@ func TestBuildAuthURLWithMergedScopes(t *testing.T) { t.Run(tc.name, func(t *testing.T) { // Configure the test instance with specific scopes tOidc := ts.tOidc - tOidc.scopes = tc.scopes + tOidc.scopes = tc.scopes // These scopes are already deduplicated by New() tOidc.authURL = "https://auth.example.com/oauth/authorize" tOidc.issuerURL = "https://auth.example.com" + // Reset overrideScopes for each test case, as it's part of tOidc state + // Default to false, specific tests will set it. + tOidc.overrideScopes = false // Build auth URL result := tOidc.buildAuthURL("https://app.example.com/callback", "test-state", "test-nonce", "") @@ -4019,3 +4022,182 @@ func TestBuildAuthURLWithMergedScopes(t *testing.T) { }) } } + +// TestBuildAuthURL_OverrideScopes_And_OfflineAccess tests the offline_access logic in buildAuthURL +// considering the overrideScopes flag. +func TestBuildAuthURL_OverrideScopes_And_OfflineAccess(t *testing.T) { + ts := &TestSuite{t: t} + ts.Setup() // Sets up ts.tOidc + + tests := []struct { + name string + initialScopes []string // Scopes as they would be in tOidc.scopes (after New processing) + overrideScopes bool + isGoogle bool // To test Google-specific handling + isAzure bool // To test Azure-specific handling + expectedParams map[string]string + expectedScope string // The final scope string expected in the URL + }{ + { + name: "Override false, no user scopes, non-Google/Azure", + initialScopes: []string{"openid", "profile", "email"}, // Defaults from New() when config.Scopes is empty + overrideScopes: false, + expectedScope: "openid profile email offline_access", + }, + { + name: "Override false, user scopes without offline_access, non-Google/Azure", + initialScopes: []string{"openid", "profile", "email", "custom1"}, // Merged and deduplicated by New() + overrideScopes: false, + expectedScope: "openid profile email custom1 offline_access", + }, + { + name: "Override false, user scopes with offline_access, non-Google/Azure", + initialScopes: []string{"openid", "profile", "email", "offline_access", "custom1"}, + overrideScopes: false, + expectedScope: "openid profile email offline_access custom1", // Order might vary based on merge, but offline_access present + }, + { + name: "Override true, user scopes without offline_access, non-Google/Azure", + initialScopes: []string{"custom1", "custom2"}, // Directly from config.Scopes, deduplicated + overrideScopes: true, + expectedScope: "custom1 custom2", // offline_access NOT added + }, + { + name: "Override true, user scopes with offline_access, non-Google/Azure", + initialScopes: []string{"custom1", "offline_access", "custom2"}, + overrideScopes: true, + expectedScope: "custom1 offline_access custom2", // User explicitly included it + }, + { + name: "Override true, no user scopes (edge case), non-Google/Azure", + initialScopes: []string{}, // config.Scopes was empty + overrideScopes: true, + // In this edge case, buildAuthURL's logic `(t.overrideScopes && len(t.scopes) == 0)` + // will lead to offline_access being added, as it behaves like defaults. + expectedScope: "offline_access", + }, + // Google Provider Tests (access_type=offline, prompt=consent) + { + name: "Google, Override false, no user scopes", + initialScopes: []string{"openid", "profile", "email"}, + overrideScopes: false, + isGoogle: true, + expectedParams: map[string]string{"access_type": "offline", "prompt": "consent"}, + expectedScope: "openid profile email", // No offline_access scope for Google + }, + { + name: "Google, Override true, user scopes", + initialScopes: []string{"custom1", "custom2"}, + overrideScopes: true, + isGoogle: true, + expectedParams: map[string]string{"access_type": "offline", "prompt": "consent"}, + expectedScope: "custom1 custom2", // No offline_access scope for Google + }, + // Azure Provider Tests (response_mode=query, offline_access scope added if not present by user) + { + name: "Azure, Override false, no user scopes", + initialScopes: []string{"openid", "profile", "email"}, + overrideScopes: false, + isAzure: true, + expectedParams: map[string]string{"response_mode": "query"}, + expectedScope: "openid profile email offline_access", + }, + { + name: "Azure, Override true, user scopes without offline_access", + initialScopes: []string{"custom1", "custom2"}, + overrideScopes: true, + isAzure: true, + expectedParams: map[string]string{"response_mode": "query"}, + expectedScope: "custom1 custom2", // offline_access NOT added by default when override is true + }, + { + name: "Azure, Override true, user scopes with offline_access", + initialScopes: []string{"custom1", "offline_access"}, + overrideScopes: true, + isAzure: true, + expectedParams: map[string]string{"response_mode": "query"}, + expectedScope: "custom1 offline_access", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + tOidc := ts.tOidc + tOidc.scopes = tc.initialScopes // Set the scopes as if they came from New() + tOidc.overrideScopes = tc.overrideScopes + + // Adjust issuerURL for provider-specific tests + originalIssuerURL := tOidc.issuerURL + if tc.isGoogle { + tOidc.issuerURL = "https://accounts.google.com" + } else if tc.isAzure { + tOidc.issuerURL = "https://login.microsoftonline.com/common" + } else { + tOidc.issuerURL = "https://generic-provider.com" // Non-Google/Azure + } + + authURLString := tOidc.buildAuthURL("http://localhost/callback", "state123", "nonce123", "challenge123") + parsedAuthURL, err := url.Parse(authURLString) + if err != nil { + t.Fatalf("Failed to parse auth URL: %v", err) + } + query := parsedAuthURL.Query() + + actualScope := query.Get("scope") + if actualScope != tc.expectedScope { + t.Errorf("Expected scope string %q, got %q", tc.expectedScope, actualScope) + } + + if tc.expectedParams != nil { + for k, v := range tc.expectedParams { + if query.Get(k) != v { + t.Errorf("Expected param %s=%s, got %s", k, v, query.Get(k)) + } + } + } + + // Restore original issuerURL for next test + tOidc.issuerURL = originalIssuerURL + }) + } +} + +// TestBuildAuthURL_SpecificUserCase tests the buildAuthURL function with the specific user-reported scenario. +func TestBuildAuthURL_SpecificUserCase(t *testing.T) { + ts := &TestSuite{t: t} + ts.Setup() // Basic setup for tOidc + + // Configure the TraefikOidc instance for the specific scenario + tOidc := ts.tOidc + tOidc.scopes = []string{"email", "test3"} // This is what t.scopes should be after New() + tOidc.overrideScopes = true + tOidc.issuerURL = "https://generic-provider.com" // Non-Google/Azure + tOidc.authURL = "https://generic-provider.com/auth" // Dummy auth URL + tOidc.clientID = "test-client-id" + + // Expected scope string in the URL + expectedScopeString := "email test3" + + // Call buildAuthURL + authURLString := tOidc.buildAuthURL("http://localhost/callback", "test-state", "test-nonce", "") + + // Parse the resulting URL + parsedAuthURL, err := url.Parse(authURLString) + if err != nil { + t.Fatalf("Failed to parse generated auth URL %q: %v", authURLString, err) + } + + // Get the 'scope' query parameter + actualScopeString := parsedAuthURL.Query().Get("scope") + + // Assert that the scope string is as expected + if actualScopeString != expectedScopeString { + t.Errorf("Expected scope parameter to be %q, but got %q. Full URL: %s", + expectedScopeString, actualScopeString, authURLString) + } + + // Additionally, ensure 'offline_access' was not added + if strings.Contains(actualScopeString, "offline_access") { + t.Errorf("Scope parameter %q should not contain 'offline_access' when overrideScopes is true and it's not in tOidc.scopes", actualScopeString) + } +} diff --git a/scopes_test.go b/scopes_test.go index 2ba6837..d30a0e0 100644 --- a/scopes_test.go +++ b/scopes_test.go @@ -1,6 +1,7 @@ package traefikoidc import ( + "net/url" "reflect" "testing" ) @@ -54,71 +55,302 @@ func TestMergeScopes(t *testing.T) { } } +func TestDeduplicateScopes(t *testing.T) { + testCases := []struct { + name string + inputScopes []string + expectedScopes []string + }{ + { + name: "No duplicates", + inputScopes: []string{"openid", "profile", "email"}, + expectedScopes: []string{"openid", "profile", "email"}, + }, + { + name: "Simple duplicates", + inputScopes: []string{"openid", "profile", "openid", "email"}, + expectedScopes: []string{"openid", "profile", "email"}, + }, + { + name: "Multiple duplicates", + inputScopes: []string{"scope1", "scope2", "scope1", "scope2", "scope1"}, + expectedScopes: []string{"scope1", "scope2"}, + }, + { + name: "Empty input", + inputScopes: []string{}, + expectedScopes: []string{}, + }, + { + name: "Nil input", + inputScopes: nil, + expectedScopes: []string{}, + }, + { + name: "Single element", + inputScopes: []string{"openid"}, + expectedScopes: []string{"openid"}, + }, + { + name: "All duplicates", + inputScopes: []string{"test", "test", "test"}, + expectedScopes: []string{"test"}, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + result := deduplicateScopes(tc.inputScopes) + if !reflect.DeepEqual(result, tc.expectedScopes) { + t.Errorf("Expected %v, got %v", tc.expectedScopes, result) + } + }) + } +} + func TestScopesConfiguration(t *testing.T) { defaultScopes := []string{"openid", "profile", "email"} - userScopes := []string{"roles", "custom_scope"} - t.Run("Default Append Behavior", func(t *testing.T) { - // Create config with user scopes but overrideScopes=false - config := &Config{ - Scopes: userScopes, - OverrideScopes: false, - } + testCases := []struct { + name string + configScopes []string // Scopes from Traefik config + overrideScopes bool + expectedResult []string + }{ + { + name: "Default Append Behavior - No user scopes", + configScopes: []string{}, + overrideScopes: false, + expectedResult: []string{"openid", "profile", "email"}, + }, + { + name: "Default Append Behavior - With user scopes", + configScopes: []string{"roles", "custom_scope"}, + overrideScopes: false, + expectedResult: []string{"openid", "profile", "email", "roles", "custom_scope"}, + }, + { + name: "Default Append Behavior - With duplicate user scopes", + configScopes: []string{"roles", "custom_scope", "roles"}, + overrideScopes: false, + expectedResult: []string{"openid", "profile", "email", "roles", "custom_scope"}, + }, + { + name: "Default Append Behavior - User scopes overlap with defaults", + configScopes: []string{"openid", "roles", "profile"}, + overrideScopes: false, + expectedResult: []string{"openid", "profile", "email", "roles"}, + }, + { + name: "Override Behavior - With user scopes", + configScopes: []string{"roles", "custom_scope"}, + overrideScopes: true, + expectedResult: []string{"roles", "custom_scope"}, + }, + { + name: "Override Behavior - With duplicate user scopes", + configScopes: []string{"roles", "custom_scope", "roles"}, + overrideScopes: true, + expectedResult: []string{"roles", "custom_scope"}, + }, + { + name: "Override Behavior - Empty user scopes", + configScopes: []string{}, + overrideScopes: true, + expectedResult: []string{}, + }, + { + name: "Override Behavior - Nil user scopes", + configScopes: nil, + overrideScopes: true, + expectedResult: []string{}, // Deduplicate will handle nil as empty + }, + } - // Simulate middleware initialization - var result []string - if config.OverrideScopes { - result = append([]string(nil), config.Scopes...) - } else { - result = mergeScopes(defaultScopes, config.Scopes) - } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Simulate the logic within TraefikOidc.New for setting t.scopes + var result []string + uniqueConfigScopes := deduplicateScopes(tc.configScopes) + if tc.overrideScopes { + result = uniqueConfigScopes + } else { + result = mergeScopes(defaultScopes, uniqueConfigScopes) + } - // Expect defaultScopes + userScopes with deduplication - expectedScopes := []string{"openid", "profile", "email", "roles", "custom_scope"} - if !reflect.DeepEqual(result, expectedScopes) { - t.Errorf("Expected %v, got %v", expectedScopes, result) - } - }) - - t.Run("Override Behavior", func(t *testing.T) { - // Create config with user scopes and overrideScopes=true - config := &Config{ - Scopes: userScopes, - OverrideScopes: true, - } - - // Simulate middleware initialization - var result []string - if config.OverrideScopes { - result = append([]string(nil), config.Scopes...) - } else { - result = mergeScopes(defaultScopes, config.Scopes) - } - - // Expect only userScopes - if !reflect.DeepEqual(result, userScopes) { - t.Errorf("Expected %v, got %v", userScopes, result) - } - }) - - t.Run("Empty Scopes with Override", func(t *testing.T) { - // Create config with empty scopes and overrideScopes=true - config := &Config{ - Scopes: []string{}, - OverrideScopes: true, - } - - // Simulate middleware initialization - var result []string - if config.OverrideScopes { - result = append([]string(nil), config.Scopes...) - } else { - result = mergeScopes(defaultScopes, config.Scopes) - } - - // Expect empty scopes - check length instead of DeepEqual - if len(result) != 0 { - t.Errorf("Expected empty slice, got %v with length %d", result, len(result)) - } - }) + if !reflect.DeepEqual(result, tc.expectedResult) { + t.Errorf("Expected scopes %v, got %v", tc.expectedResult, result) + } + }) + } +} + +func TestBuildAuthURLScopeHandling(t *testing.T) { + ts := &TestSuite{t: t} + ts.Setup() // Basic setup for TraefikOidc instance + + // Default scopes expected if not overridden and no user scopes provided + defaultInitialScopes := []string{"openid", "profile", "email"} + + testCases := []struct { + name string + configScopes []string // Scopes from Traefik config + overrideScopes bool + isGoogle bool + isAzure bool + expectedScopeString string // Expected final scope string in the auth URL + expectedParams map[string]string + }{ + { + name: "Deduplication: Default append, duplicate in user scopes", + configScopes: []string{"openid", "custom", "profile", "custom"}, + overrideScopes: false, + expectedScopeString: "openid profile email custom offline_access", + }, + { + name: "Deduplication: Override, duplicate in user scopes", + configScopes: []string{"openid", "custom", "profile", "custom"}, + overrideScopes: true, + expectedScopeString: "openid custom profile", // offline_access not added + }, + { + name: "Override True: No automatic offline_access", + configScopes: []string{"scope1", "scope2"}, + overrideScopes: true, + expectedScopeString: "scope1 scope2", + }, + { + name: "Override True: User includes offline_access", + configScopes: []string{"scope1", "offline_access", "scope2"}, + overrideScopes: true, + expectedScopeString: "scope1 offline_access scope2", + }, + { + name: "Override False: Automatic offline_access added", + configScopes: []string{"scope1", "scope2"}, + overrideScopes: false, + expectedScopeString: "openid profile email scope1 scope2 offline_access", + }, + { + name: "Override False: User includes offline_access (deduplicated)", + configScopes: []string{"scope1", "offline_access", "scope2"}, + overrideScopes: false, + expectedScopeString: "openid profile email scope1 offline_access scope2", + }, + { + name: "Integration: Duplicate scopes in config, override true", + configScopes: []string{"scope1", "scope1", "scope2"}, + overrideScopes: true, + expectedScopeString: "scope1 scope2", + }, + { + name: "Integration: No auto offline_access with override true", + configScopes: []string{"scope1", "scope2"}, + overrideScopes: true, + expectedScopeString: "scope1 scope2", + }, + { + name: "Integration: Duplicates and no auto offline_access with override true", + configScopes: []string{"scope1", "scope1", "scope2"}, + overrideScopes: true, + expectedScopeString: "scope1 scope2", + }, + { + name: "Integration: Google provider, override false, no user scopes", + configScopes: []string{}, + overrideScopes: false, + isGoogle: true, + expectedScopeString: "openid profile email", // Google uses access_type=offline param + expectedParams: map[string]string{"access_type": "offline", "prompt": "consent"}, + }, + { + name: "Integration: Google provider, override true, user scopes", + configScopes: []string{"custom1", "custom2"}, + overrideScopes: true, + isGoogle: true, + expectedScopeString: "custom1 custom2", // Google uses access_type=offline param + expectedParams: map[string]string{"access_type": "offline", "prompt": "consent"}, + }, + { + name: "Integration: Azure provider, override false, no user scopes", + configScopes: []string{}, + overrideScopes: false, + isAzure: true, + expectedScopeString: "openid profile email offline_access", // Azure adds offline_access scope + expectedParams: map[string]string{"response_mode": "query"}, + }, + { + name: "Integration: Azure provider, override true, user scopes without offline_access", + configScopes: []string{"custom1", "custom2"}, + overrideScopes: true, + isAzure: true, + expectedScopeString: "custom1 custom2", // Azure respects override + expectedParams: map[string]string{"response_mode": "query"}, + }, + { + name: "Integration: Azure provider, override true, user scopes with offline_access", + configScopes: []string{"custom1", "offline_access"}, + overrideScopes: true, + isAzure: true, + expectedScopeString: "custom1 offline_access", // Azure respects override + expectedParams: map[string]string{"response_mode": "query"}, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Simulate the TraefikOidc instance's scope initialization + var initializedScopes []string + uniqueConfigScopes := deduplicateScopes(tc.configScopes) + if tc.overrideScopes { + initializedScopes = uniqueConfigScopes + } else { + initializedScopes = mergeScopes(defaultInitialScopes, uniqueConfigScopes) + } + + // Create a new TraefikOidc instance for this test case + // to ensure proper isolation of 'scopes' and 'overrideScopes' fields. + // We use parts of the TestSuite's tOidc for common setup like logger, clientID etc. + // but override the scope-related fields. + testOidc := &TraefikOidc{ + clientID: ts.tOidc.clientID, + logger: ts.tOidc.logger, + scopes: initializedScopes, // Use scopes processed as New() would + overrideScopes: tc.overrideScopes, + // Set other necessary fields for buildAuthURL to function + authURL: "https://provider.com/auth", // Dummy authURL + issuerURL: "https://provider.com", // Dummy issuerURL + httpClient: ts.tOidc.httpClient, // Reuse from TestSuite + } + + originalIssuerURL := testOidc.issuerURL + if tc.isGoogle { + testOidc.issuerURL = "https://accounts.google.com" + } else if tc.isAzure { + testOidc.issuerURL = "https://login.microsoftonline.com/common" + } + + authURLString := testOidc.buildAuthURL("http://localhost/callback", "state", "nonce", "challenge") + parsedURL, err := url.Parse(authURLString) + if err != nil { + t.Fatalf("Failed to parse auth URL: %v", err) + } + + query := parsedURL.Query() + actualScopeString := query.Get("scope") + + if actualScopeString != tc.expectedScopeString { + t.Errorf("Expected scope string %q, got %q", tc.expectedScopeString, actualScopeString) + } + + if tc.expectedParams != nil { + for k, v := range tc.expectedParams { + if query.Get(k) != v { + t.Errorf("Expected param %s=%s, got %s", k, v, query.Get(k)) + } + } + } + testOidc.issuerURL = originalIssuerURL // Restore + }) + } }