Repository navigation
feat: reload OIDC policy at login - #2461
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughOIDC provisioning policies can now be reloaded from merged configuration files during login. The active policy is retained when reloads fail, while server wiring and user-list API responses expose the effective policy and role mappings. ChangesOIDC policy reload
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LoginClient
participant ProcessLogin
participant ConfigPolicyLoader
participant OIDCProvisioningService
participant UsersAPI
LoginClient->>ProcessLogin: submit OIDC claims
ProcessLogin->>ConfigPolicyLoader: load current provisioning policy
ConfigPolicyLoader-->>OIDCProvisioningService: return validated policy
OIDCProvisioningService->>OIDCProvisioningService: apply access and role mapping
UsersAPI->>OIDCProvisioningService: request current policy
OIDCProvisioningService-->>UsersAPI: return effective role mapping
UsersAPI-->>LoginClient: report OIDC synchronization state
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/service/oidcprovision/service.go`:
- Around line 250-279: Update Service.loadPolicy so s.config.LoadPolicy(ctx) and
newPolicySnapshot(loaded) execute without holding policyMu; acquire the lock
only when returning the current policy after either failure and when swapping
s.policy after successful parsing. Preserve the existing fallback and successful
reload behavior while preventing synchronous reload I/O from blocking concurrent
logins.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 914a0be9-99be-49b4-a66d-7b7663fb7675
📒 Files selected for processing (11)
internal/cmn/config/config.gointernal/cmn/config/loader.gointernal/cmn/config/loader_test.gointernal/cmn/config/oidc_policy.gointernal/cmn/config/oidc_policy_test.gointernal/service/frontend/api/v1/api.gointernal/service/frontend/api/v1/users.gointernal/service/frontend/api/v1/users_internal_test.gointernal/service/frontend/server.gointernal/service/oidcprovision/service.gointernal/service/oidcprovision/service_test.go
| func (s *Service) loadPolicy(ctx context.Context) *policySnapshot { | ||
| s.policyMu.Lock() | ||
| defer s.policyMu.Unlock() | ||
|
|
||
| if s.config.LoadPolicy == nil { | ||
| return s.policy | ||
| } | ||
|
|
||
| loaded, err := s.config.LoadPolicy(ctx) | ||
| if err != nil { | ||
| s.logger.Warn("OIDC authorization policy reload rejected", | ||
| slog.String("error", err.Error())) | ||
| return s.policy | ||
| } | ||
| policy, err := newPolicySnapshot(loaded) | ||
| if err != nil { | ||
| s.logger.Warn("OIDC authorization policy reload rejected", | ||
| slog.String("error", err.Error())) | ||
| return s.policy | ||
| } | ||
| s.policy = policy | ||
| return policy | ||
| } | ||
|
|
||
| // CurrentPolicy returns the latest successfully loaded provisioning policy. | ||
| func (s *Service) CurrentPolicy() ProvisioningPolicy { | ||
| s.policyMu.Lock() | ||
| defer s.policyMu.Unlock() | ||
| return s.policy.ProvisioningPolicy | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Reload lock is held across synchronous file I/O, serializing all concurrent OIDC logins.
loadPolicy takes policyMu and keeps it locked while calling s.config.LoadPolicy(ctx), which (per the wiring in server.go) performs os.ReadFile + a full Viper re-parse of every tracked config file on every login (see internal/cmn/config/oidc_policy.go's Load()). Since every OIDC login goes through this path, concurrent logins fully serialize on synchronous disk I/O for the duration of the reload, rather than merely contending on an in-memory pointer swap.
Move the load/parse work outside the critical section and only take the lock to read the current policy on failure or to swap the pointer on success.
⚡ Proposed fix: shrink the lock scope around the reload
func (s *Service) loadPolicy(ctx context.Context) *policySnapshot {
- s.policyMu.Lock()
- defer s.policyMu.Unlock()
-
if s.config.LoadPolicy == nil {
return s.policy
}
loaded, err := s.config.LoadPolicy(ctx)
if err != nil {
s.logger.Warn("OIDC authorization policy reload rejected",
slog.String("error", err.Error()))
+ s.policyMu.Lock()
+ defer s.policyMu.Unlock()
return s.policy
}
policy, err := newPolicySnapshot(loaded)
if err != nil {
s.logger.Warn("OIDC authorization policy reload rejected",
slog.String("error", err.Error()))
+ s.policyMu.Lock()
+ defer s.policyMu.Unlock()
return s.policy
}
+
+ s.policyMu.Lock()
+ defer s.policyMu.Unlock()
s.policy = policy
return policy
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/service/oidcprovision/service.go` around lines 250 - 279, Update
Service.loadPolicy so s.config.LoadPolicy(ctx) and newPolicySnapshot(loaded)
execute without holding policyMu; acquire the lock only when returning the
current policy after either failure and when swapping s.policy after successful
parsing. Preserve the existing fallback and successful reload behavior while
preventing synchronous reload I/O from blocking concurrent logins.
Summary
Why
The OIDC role mapper was compiled once during server startup and retained by the provisioning service. Changes to
auth.oidc.role_mappingtherefore required restarting the server even though the policy is only consumed when an OIDC login is processed.Loading at the authentication boundary avoids a file watcher, reload endpoint, or separate runtime configuration store. Retaining one last-known-good snapshot also prevents a malformed runtime edit from causing an OIDC login outage.
Behavior
auto_signup,allowed_domains,whitelist, androle_mappingare evaluated from current configuration on each login.Testing
make fmtgo test ./internal/cmn/config ./internal/service/oidcprovision ./internal/service/frontend/api/v1 -count=1make test TEST_TARGET='./internal/cmn/config ./internal/service/oidcprovision ./internal/service/frontend/api/v1'Closes #2457
Summary by CodeRabbit
New Features
Bug Fixes