Bugfix #51: Ensures that user provided scopes overrides work.

This commit is contained in:
2025-06-20 23:58:42 +01:00
parent 667b4213fe
commit de91edb514
4 changed files with 530 additions and 79 deletions
+17
View File
@@ -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
}
+36 -16
View File
@@ -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
+183 -1
View File
@@ -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)
}
}
+294 -62
View File
@@ -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
})
}
}