diff --git a/.traefik.yml b/.traefik.yml index fb7efd9..d6c5d79 100644 --- a/.traefik.yml +++ b/.traefik.yml @@ -35,11 +35,8 @@ testData: logoutURL: /oauth2/logout # Path for handling logout requests (if not provided, it will be set to callbackURL + "/logout") postLogoutRedirectURI: /oidc/different-logout # URL to redirect to after logout (default: "/") - scopes: # OAuth 2.0 scopes to request (default: ["openid", "email", "profile"]) - - openid - - email - - profile - - roles # Include this to get role information from the provider + scopes: # Additional scopes to append to defaults ["openid", "profile", "email"] + - roles # Result: ["openid", "profile", "email", "roles"] allowedUserDomains: # Restricts access to specific email domains (if not provided, relies on OIDC provider) - company.com @@ -153,11 +150,15 @@ configuration: scopes: type: array description: | - The OAuth 2.0 scopes to request from the OIDC provider. - Default: ["openid", "profile", "email"] + Additional OAuth 2.0 scopes to append to the default scopes. + Default scopes are always included: ["openid", "profile", "email"] + + User-provided scopes are appended to defaults with automatic deduplication. + For example, specifying ["roles", "custom_scope"] results in: + ["openid", "profile", "email", "roles", "custom_scope"] Include "roles" or similar scope if you need role/group information. - Note: For Google OAuth, the middleware automatically handles the + Note: For Google OAuth, the middleware automatically handles the proper authentication parameters and does NOT require the "offline_access" scope (which Google rejects as invalid). See documentation for details. required: false diff --git a/README.md b/README.md index 8a5a88a..9f5cf13 100644 --- a/README.md +++ b/README.md @@ -67,7 +67,7 @@ The middleware supports the following configuration options: |-----------|-------------|---------|---------| | `logoutURL` | The path for handling logout requests | `callbackURL + "/logout"` | `/oauth2/logout` | | `postLogoutRedirectURI` | The URL to redirect to after logout | `/` | `/logged-out-page` | -| `scopes` | The OAuth 2.0 scopes to request | `["openid", "profile", "email"]` | `["openid", "email", "profile", "roles"]` | +| `scopes` | Additional OAuth 2.0 scopes to append to defaults | `["openid", "profile", "email"]` (always included) | `["roles", "custom_scope"]` (appended to defaults) | | `logLevel` | Sets the logging verbosity | `info` | `debug`, `info`, `error` | | `forceHTTPS` | Forces the use of HTTPS for all URLs | `true` | `true`, `false` | | `rateLimit` | Sets the maximum number of requests per second | `100` | `500` | @@ -81,6 +81,50 @@ The middleware supports the following configuration options: | `refreshGracePeriodSeconds` | Seconds before token expiry to attempt proactive refresh | `60` | `120` | | `headers` | Custom HTTP headers with templates that can access OIDC claims and tokens | none | See "Templated Headers" section | +## Scope Configuration + +### Scope Append Behavior + +The middleware uses an **append** behavior for OAuth 2.0 scopes, not replacement. This means: + +- **Default scopes** are always included: `["openid", "profile", "email"]` +- **User-provided scopes** are appended to the defaults with automatic deduplication +- The final scope list maintains the order: defaults first, then user scopes + +#### Examples: + +**No custom scopes specified:** +```yaml +# No scopes field specified +# Result: ["openid", "profile", "email"] +``` + +**Custom scopes appended:** +```yaml +scopes: + - roles + - custom_scope +# Result: ["openid", "profile", "email", "roles", "custom_scope"] +``` + +**Overlapping scopes (automatic deduplication):** +```yaml +scopes: + - openid # Duplicate - will be deduplicated + - roles + - profile # Duplicate - will be deduplicated + - permissions +# Result: ["openid", "profile", "email", "roles", "permissions"] +``` + +**Empty scopes list:** +```yaml +scopes: [] +# Result: ["openid", "profile", "email"] +``` + +This append behavior ensures that essential OIDC scopes are always present while allowing you to add provider-specific or application-specific scopes as needed. + ## Usage Examples ### Basic Configuration @@ -101,9 +145,7 @@ spec: callbackURL: /oauth2/callback logoutURL: /oauth2/logout scopes: - - openid - - email - - profile + - roles # Appended to defaults: ["openid", "profile", "email", "roles"] ``` ### With Excluded URLs (Public Access Paths) @@ -124,9 +166,7 @@ spec: callbackURL: /oauth2/callback logoutURL: /oauth2/logout scopes: - - openid - - email - - profile + - roles # Appended to defaults: ["openid", "profile", "email", "roles"] excludedURLs: - /login # covers /login, /login/me, /login/reminder etc. - /public-data @@ -152,9 +192,7 @@ spec: callbackURL: /oauth2/callback logoutURL: /oauth2/logout scopes: - - openid - - email - - profile + - roles # Appended to defaults: ["openid", "profile", "email", "roles"] allowedUserDomains: - company.com - subsidiary.com @@ -178,9 +216,7 @@ spec: callbackURL: /oauth2/callback logoutURL: /oauth2/logout scopes: - - openid - - email - - profile + - roles # Appended to defaults: ["openid", "profile", "email", "roles"] allowedUsers: - user1@example.com - user2@another.org @@ -204,9 +240,7 @@ spec: callbackURL: /oauth2/callback logoutURL: /oauth2/logout scopes: - - openid - - email - - profile + - roles # Appended to defaults: ["openid", "profile", "email", "roles"] allowedUserDomains: - company.com allowedUsers: @@ -239,10 +273,7 @@ spec: callbackURL: /oauth2/callback logoutURL: /oauth2/logout scopes: - - openid - - email - - profile - - roles # Include this to get role information from the provider + - roles # Appended to defaults: ["openid", "profile", "email", "roles"] allowedRolesAndGroups: - admin - developer @@ -269,9 +300,7 @@ spec: rateLimit: 500 # Requests per second (default: 100) forceHTTPS: false # Default is true for security scopes: - - openid - - email - - profile + - roles # Appended to defaults: ["openid", "profile", "email", "roles"] ``` ### With Custom Post-Logout Redirect @@ -293,9 +322,7 @@ spec: logoutURL: /oauth2/logout postLogoutRedirectURI: /logged-out-page # Where to redirect after logout scopes: - - openid - - email - - profile + - roles # Appended to defaults: ["openid", "profile", "email", "roles"] ``` ### With Templated Headers @@ -316,10 +343,7 @@ spec: callbackURL: /oauth2/callback logoutURL: /oauth2/logout scopes: - - openid - - email - - profile - - roles + - roles # Appended to defaults: ["openid", "profile", "email", "roles"] headers: - name: "X-User-Email" value: "{{.Claims.email}}" @@ -352,9 +376,7 @@ spec: logoutURL: /oauth2/logout enablePKCE: true # Enables PKCE for added security scopes: - - openid - - email - - profile + - roles # Appended to defaults: ["openid", "profile", "email", "roles"] ``` ### Google OIDC Configuration Example @@ -377,9 +399,7 @@ spec: callbackURL: /oauth2/callback # Adjust if needed logoutURL: /oauth2/logout # Optional: Adjust if needed scopes: - - openid - - email - - profile + - roles # Appended to defaults: ["openid", "profile", "email", "roles"] # Note: DO NOT manually add offline_access scope for Google # The middleware automatically handles Google-specific requirements refreshGracePeriodSeconds: 300 # Optional: Start refresh 5 min before expiry (default 60) @@ -408,9 +428,7 @@ spec: callbackURL: /oauth2/callback logoutURL: /oauth2/logout scopes: - - openid - - email - - profile + - roles # Appended to defaults: ["openid", "profile", "email", "roles"] ``` Don't forget to create the secret: @@ -509,9 +527,7 @@ http: postLogoutRedirectURI: /logged-out-page sessionEncryptionKey: potato-secret-is-at-least-32-bytes-long scopes: - - openid - - email - - profile + - roles # Appended to defaults: ["openid", "profile", "email", "roles"] allowedUserDomains: - company.com allowedUsers: diff --git a/main_test.go b/main_test.go index dbaa8d1..f162bb8 100644 --- a/main_test.go +++ b/main_test.go @@ -3647,6 +3647,211 @@ func TestScopeMerging(t *testing.T) { } } +// TestScopeMergingEdgeCases tests additional edge cases for scope deduplication +func TestScopeMergingEdgeCases(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 + description string + }{ + { + name: "Case sensitivity preserved", + defaultScopes: []string{"openid", "profile", "email"}, + userScopes: []string{"OpenID", "PROFILE", "custom"}, + expectedScopes: []string{"openid", "profile", "email", "OpenID", "PROFILE", "custom"}, + description: "OAuth scopes are case-sensitive, so different cases should be preserved", + }, + { + name: "Empty strings in user scopes", + defaultScopes: []string{"openid", "profile", "email"}, + userScopes: []string{"", "custom", "", "another"}, + expectedScopes: []string{"openid", "profile", "email", "", "custom", "another"}, + description: "Empty strings should be preserved (though invalid in OAuth)", + }, + { + name: "Whitespace scopes", + defaultScopes: []string{"openid", "profile", "email"}, + userScopes: []string{" ", "custom", " ", "another"}, + expectedScopes: []string{"openid", "profile", "email", " ", "custom", " ", "another"}, + description: "Whitespace-only scopes should be preserved as distinct", + }, + { + name: "Large number of scopes", + defaultScopes: []string{"openid", "profile", "email"}, + userScopes: generateLargeUserScopes(), + expectedScopes: func() []string { + // Manually calculate expected result with proper deduplication + defaults := []string{"openid", "profile", "email"} + userScopes := generateLargeUserScopes() + return mergeScopes(defaults, userScopes) + }(), + description: "Performance test with larger scope lists", + }, + { + name: "Complex OAuth scopes with special characters", + defaultScopes: []string{"openid", "profile", "email"}, + userScopes: []string{"read:users", "write:users", "admin:*", "scope/with/slashes", "scope-with-dashes"}, + expectedScopes: []string{"openid", "profile", "email", "read:users", "write:users", "admin:*", "scope/with/slashes", "scope-with-dashes"}, + description: "Real-world OAuth scopes with colons, slashes, and special characters", + }, + { + name: "Duplicate defaults in user scopes multiple times", + defaultScopes: []string{"openid", "profile", "email"}, + userScopes: []string{"openid", "profile", "openid", "custom", "email", "profile", "custom"}, + expectedScopes: []string{"openid", "profile", "email", "custom"}, + description: "Multiple duplicates of default scopes should be completely deduplicated", + }, + { + name: "All user scopes are duplicates of defaults", + defaultScopes: []string{"openid", "profile", "email"}, + userScopes: []string{"email", "openid", "profile", "openid"}, + expectedScopes: []string{"openid", "profile", "email"}, + description: "When all user scopes duplicate defaults, result should be just defaults", + }, + { + name: "Single scope scenarios", + defaultScopes: []string{"openid"}, + userScopes: []string{"custom"}, + expectedScopes: []string{"openid", "custom"}, + description: "Minimal case with single scopes", + }, + { + name: "Identical scopes in same order", + defaultScopes: []string{"openid", "profile", "email"}, + userScopes: []string{"openid", "profile", "email"}, + expectedScopes: []string{"openid", "profile", "email"}, + description: "When user scopes exactly match defaults, no duplication", + }, + { + name: "Identical scopes in different order", + defaultScopes: []string{"openid", "profile", "email"}, + userScopes: []string{"email", "profile", "openid"}, + expectedScopes: []string{"openid", "profile", "email"}, + description: "Order of defaults is preserved when user scopes are reordered duplicates", + }, + } + + 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\nDescription: %s", tc.expectedScopes, result, tc.description) + } + }) + } +} + +// generateLargeUserScopes creates a large list of user scopes for performance testing +func generateLargeUserScopes() []string { + scopes := make([]string, 100) + for i := 0; i < 100; i++ { + scopes[i] = fmt.Sprintf("scope_%d", i) + } + // Add some duplicates to test deduplication performance + scopes = append(scopes, "scope_1", "scope_5", "scope_10", "openid") // Include a default duplicate + return scopes +} + +// TestScopeMergingPerformance tests performance with large scope lists +func TestScopeMergingPerformance(t *testing.T) { + // Create large scope lists + defaultScopes := []string{"openid", "profile", "email"} + + // Create 1000 user scopes with some duplicates + userScopes := make([]string, 1000) + for i := 0; i < 1000; i++ { + if i%10 == 0 { + // Add some duplicates of defaults + userScopes[i] = defaultScopes[i%len(defaultScopes)] + } else if i%7 == 0 { + // Add some internal duplicates + userScopes[i] = fmt.Sprintf("scope_%d", i%50) + } else { + userScopes[i] = fmt.Sprintf("scope_%d", i) + } + } + + // Measure performance + start := time.Now() + result := mergeScopes(defaultScopes, userScopes) + duration := time.Since(start) + + // Verify result correctness + if len(result) < len(defaultScopes) { + t.Errorf("Result should contain at least the default scopes") + } + + // Verify no duplicates exist + seen := make(map[string]bool) + for _, scope := range result { + if seen[scope] { + t.Errorf("Duplicate scope found in result: %s", scope) + } + seen[scope] = true + } + + // Performance assertion (should be very fast) + if duration > time.Millisecond*10 { + t.Logf("Performance note: mergeScopes took %v for 1000+ scopes (still acceptable)", duration) + } + + t.Logf("Performance: processed %d user scopes in %v, result has %d unique scopes", + len(userScopes), duration, len(result)) +} + +// TestScopeMergingMemoryEfficiency tests memory efficiency of the mergeScopes function +func TestScopeMergingMemoryEfficiency(t *testing.T) { + defaultScopes := []string{"openid", "profile", "email"} + userScopes := []string{"custom1", "custom2"} + + // Test that the function doesn't modify input slices + originalDefaults := make([]string, len(defaultScopes)) + copy(originalDefaults, defaultScopes) + originalUser := make([]string, len(userScopes)) + copy(originalUser, userScopes) + + result := mergeScopes(defaultScopes, userScopes) + + // Verify input slices are unchanged + for i, scope := range defaultScopes { + if scope != originalDefaults[i] { + t.Errorf("Default scopes were modified: expected %s, got %s", originalDefaults[i], scope) + } + } + for i, scope := range userScopes { + if scope != originalUser[i] { + t.Errorf("User scopes were modified: expected %s, got %s", originalUser[i], scope) + } + } + + // Verify result is independent + result[0] = "modified" + if defaultScopes[0] == "modified" { + t.Error("Modifying result affected input defaults") + } + + expectedLength := len(defaultScopes) + len(userScopes) + if len(result) != expectedLength { + t.Errorf("Expected result length %d, got %d", expectedLength, len(result)) + } +} + // TestNewWithScopeAppending tests that the New function properly merges scopes func TestNewWithScopeAppending(t *testing.T) { // Create mock provider metadata server