diff --git a/main.go b/main.go index e634466..611d9a5 100644 --- a/main.go +++ b/main.go @@ -371,6 +371,45 @@ func (t *TraefikOidc) VerifyJWTSignatureAndClaims(jwt *JWT, token string) error return nil } +// mergeScopes merges default scopes with user-provided scopes, ensuring no duplicates +// while preserving order (defaults first, then user scopes). +// It handles edge cases like empty user scopes or nil inputs. +// +// Parameters: +// - defaultScopes: The default scopes to include (e.g., ["openid", "profile", "email"]) +// - userScopes: The user-provided scopes to append +// +// Returns: +// - A slice containing merged scopes with defaults first, then user scopes, with duplicates removed +func mergeScopes(defaultScopes, userScopes []string) []string { + if len(userScopes) == 0 { + // If no user scopes provided, return only defaults + return append([]string(nil), defaultScopes...) + } + + // Create a map to track which scopes we've already seen (for deduplication) + seen := make(map[string]bool) + var result []string + + // Add default scopes first + for _, scope := range defaultScopes { + if !seen[scope] { + seen[scope] = true + result = append(result, scope) + } + } + + // Add user scopes, skipping duplicates + for _, scope := range userScopes { + if !seen[scope] { + seen[scope] = true + result = append(result, scope) + } + } + + return result +} + // New is the constructor for the TraefikOidc middleware plugin. // It is called by Traefik during plugin initialization. It performs the following steps: // 1. Creates a default configuration if none is provided. @@ -450,7 +489,7 @@ func New(ctx context.Context, next http.Handler, config *Config, name string) (h clientSecret: config.ClientSecret, forceHTTPS: config.ForceHTTPS, enablePKCE: config.EnablePKCE, - scopes: config.Scopes, + scopes: mergeScopes([]string{"openid", "profile", "email"}, config.Scopes), limiter: rate.NewLimiter(rate.Every(time.Second), config.RateLimit), tokenCache: NewTokenCache(), httpClient: httpClient, diff --git a/main_test.go b/main_test.go index 32423c4..dbaa8d1 100644 --- a/main_test.go +++ b/main_test.go @@ -3564,3 +3564,220 @@ func TestEdgeCasesWithDifferentTokenTypes(t *testing.T) { }) } } + +// TestScopeMerging tests the scope append functionality +func TestScopeMerging(t *testing.T) { + // Helper function to compare string slices + equalSlices := func(a, b []string) bool { + if len(a) != len(b) { + return false + } + for i, v := range a { + if v != b[i] { + return false + } + } + return true + } + + tests := []struct { + name string + defaultScopes []string + userScopes []string + expectedScopes []string + }{ + { + name: "Empty user scopes", + defaultScopes: []string{"openid", "profile", "email"}, + userScopes: []string{}, + expectedScopes: []string{"openid", "profile", "email"}, + }, + { + name: "Nil user scopes", + defaultScopes: []string{"openid", "profile", "email"}, + userScopes: nil, + expectedScopes: []string{"openid", "profile", "email"}, + }, + { + name: "New scopes are appended", + defaultScopes: []string{"openid", "profile", "email"}, + userScopes: []string{"custom_scope", "another_scope"}, + expectedScopes: []string{"openid", "profile", "email", "custom_scope", "another_scope"}, + }, + { + name: "Deduplication - user scope already in defaults", + defaultScopes: []string{"openid", "profile", "email"}, + userScopes: []string{"openid", "custom_scope"}, + expectedScopes: []string{"openid", "profile", "email", "custom_scope"}, + }, + { + name: "Duplicate user scopes are removed", + defaultScopes: []string{"openid", "profile", "email"}, + userScopes: []string{"custom_scope", "custom_scope", "another_scope"}, + expectedScopes: []string{"openid", "profile", "email", "custom_scope", "another_scope"}, + }, + { + name: "Multiple overlapping scopes", + defaultScopes: []string{"openid", "profile", "email"}, + userScopes: []string{"profile", "custom_scope", "email", "another_scope", "profile"}, + expectedScopes: []string{"openid", "profile", "email", "custom_scope", "another_scope"}, + }, + { + name: "Only custom scopes", + defaultScopes: []string{"openid", "profile", "email"}, + userScopes: []string{"read:users", "write:users", "admin"}, + expectedScopes: []string{"openid", "profile", "email", "read:users", "write:users", "admin"}, + }, + { + name: "Empty defaults", + defaultScopes: []string{}, + userScopes: []string{"custom1", "custom2"}, + expectedScopes: []string{"custom1", "custom2"}, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + // Test the mergeScopes function directly + result := mergeScopes(tc.defaultScopes, tc.userScopes) + if !equalSlices(result, tc.expectedScopes) { + t.Errorf("Expected %v, got %v", tc.expectedScopes, result) + } + }) + } +} + +// TestNewWithScopeAppending tests that the New function properly merges scopes +func TestNewWithScopeAppending(t *testing.T) { + // Create mock provider metadata server + mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + metadata := ProviderMetadata{ + Issuer: "https://test-issuer.com", + AuthURL: "https://test-issuer.com/auth", + TokenURL: "https://test-issuer.com/token", + JWKSURL: "https://test-issuer.com/jwks", + RevokeURL: "https://test-issuer.com/revoke", + EndSessionURL: "https://test-issuer.com/end-session", + } + json.NewEncoder(w).Encode(metadata) + })) + defer mockServer.Close() + + tests := []struct { + name string + configScopes []string + expectedScopes []string + }{ + { + name: "Default scopes only", + configScopes: []string{}, + expectedScopes: []string{"openid", "profile", "email"}, + }, + { + name: "Custom scopes appended", + configScopes: []string{"custom_scope", "another_scope"}, + expectedScopes: []string{"openid", "profile", "email", "custom_scope", "another_scope"}, + }, + { + name: "Overlapping scopes deduplicated", + configScopes: []string{"openid", "custom_scope"}, + expectedScopes: []string{"openid", "profile", "email", "custom_scope"}, + }, + { + name: "OAuth scopes", + configScopes: []string{"read:users", "write:users", "admin"}, + expectedScopes: []string{"openid", "profile", "email", "read:users", "write:users", "admin"}, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + // Create config with test scopes + config := &Config{ + ProviderURL: mockServer.URL, + ClientID: "test-client", + ClientSecret: "test-secret", + CallbackURL: "/callback", + SessionEncryptionKey: "test-encryption-key-thats-long-enough", + Scopes: tc.configScopes, + } + + // Create middleware instance + middleware, err := New(context.Background(), http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }), config, "test") + if err != nil { + t.Fatalf("Failed to create middleware: %v", err) + } + + // Wait for initialization + if m, ok := middleware.(*TraefikOidc); ok { + select { + case <-m.initComplete: + case <-time.After(5 * time.Second): + t.Fatalf("Middleware failed to initialize") + } + + // Check that scopes were properly merged + if !equalSlices(m.scopes, tc.expectedScopes) { + t.Errorf("Expected scopes %v, got %v", tc.expectedScopes, m.scopes) + } + } else { + t.Fatalf("Middleware is not of type *TraefikOidc") + } + }) + } +} + +// TestBuildAuthURLWithMergedScopes tests that the auth URL includes the properly merged scopes +func TestBuildAuthURLWithMergedScopes(t *testing.T) { + ts := &TestSuite{t: t} + ts.Setup() + + tests := []struct { + name string + scopes []string + expectedScopes string + }{ + { + name: "Default scopes only", + scopes: []string{"openid", "profile", "email"}, + expectedScopes: "openid profile email offline_access", + }, + { + name: "Custom scopes appended", + scopes: []string{"openid", "profile", "email", "custom_scope", "another_scope"}, + expectedScopes: "openid profile email custom_scope another_scope offline_access", + }, + { + name: "OAuth scopes", + scopes: []string{"openid", "profile", "email", "read:users", "write:users"}, + expectedScopes: "openid profile email read:users write:users offline_access", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + // Configure the test instance with specific scopes + tOidc := ts.tOidc + tOidc.scopes = tc.scopes + tOidc.authURL = "https://auth.example.com/oauth/authorize" + tOidc.issuerURL = "https://auth.example.com" + + // Build auth URL + result := tOidc.buildAuthURL("https://app.example.com/callback", "test-state", "test-nonce", "") + + // Parse the resulting URL to verify scopes + parsedURL, err := url.Parse(result) + if err != nil { + t.Fatalf("Failed to parse resulting URL: %v", err) + } + + query := parsedURL.Query() + actualScopes := query.Get("scope") + if actualScopes != tc.expectedScopes { + t.Errorf("Expected scopes %q, got %q", tc.expectedScopes, actualScopes) + } + }) + } +} diff --git a/settings_test.go b/settings_test.go index fd95d0a..110b60d 100644 --- a/settings_test.go +++ b/settings_test.go @@ -7,6 +7,19 @@ import ( "testing" ) +// Helper function to compare string slices +func equalSlices(a, b []string) bool { + if len(a) != len(b) { + return false + } + for i, v := range a { + if v != b[i] { + return false + } + } + return true +} + func TestCreateConfig(t *testing.T) { t.Run("Default Values", func(t *testing.T) { config := CreateConfig() @@ -38,25 +51,25 @@ func TestCreateConfig(t *testing.T) { } }) - t.Run("Custom Values Preserved", func(t *testing.T) { + t.Run("Config Can Hold Custom Values", func(t *testing.T) { config := CreateConfig() config.Scopes = []string{"custom_scope"} config.LogLevel = "debug" config.RateLimit = 50 config.ForceHTTPS = false - // Verify custom values are not overwritten + // Verify config struct can hold custom values if len(config.Scopes) != 1 || config.Scopes[0] != "custom_scope" { - t.Error("Custom scopes were overwritten") + t.Error("Config struct cannot hold custom scopes") } if config.LogLevel != "debug" { - t.Error("Custom log level was overwritten") + t.Error("Config struct cannot hold custom log level") } if config.RateLimit != 50 { - t.Error("Custom rate limit was overwritten") + t.Error("Config struct cannot hold custom rate limit") } if config.ForceHTTPS { - t.Error("Custom ForceHTTPS value was overwritten") + t.Error("Config struct cannot hold custom ForceHTTPS value") } }) }