feat: support provider-specific OAuth whitelists#882
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR enables per-provider OAuth email whitelisting by extending the configuration model with provider-specific allowlists, loading them during bootstrap, updating the auth service to accept and evaluate provider context, and integrating these changes into OAuth callback and session middleware paths. ChangesProvider-specific OAuth whitelists
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
4852cfd to
a61870a
Compare
|
Rebased this branch on current Verified locally:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/service/auth_service_test.go (1)
19-34: ⚡ Quick winAdd one edge-case assertion for configured-but-empty provider whitelist.
This test covers unknown-provider fallback, but not a configured provider whose whitelist resolves to empty. Adding that case will lock the intended
len(...) == 0fallback behavior.Proposed test extension
runtime: model.RuntimeConfig{ OAuthWhitelist: []string{"[email protected]"}, OAuthProviders: map[string]model.OAuthServiceConfig{ "github": { Whitelist: []string{"[email protected]"}, }, "pocketid": { Whitelist: []string{"[email protected]"}, }, + "gitlab": { + Whitelist: []string{}, + }, }, }, } assert.True(t, auth.IsEmailWhitelisted("github", "[email protected]")) assert.False(t, auth.IsEmailWhitelisted("github", "[email protected]")) assert.True(t, auth.IsEmailWhitelisted("pocketid", "[email protected]")) assert.True(t, auth.IsEmailWhitelisted("google", "[email protected]")) + assert.True(t, auth.IsEmailWhitelisted("gitlab", "[email protected]"))🤖 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/auth_service_test.go` around lines 19 - 34, Add a test case for a configured provider whose Whitelist is present but empty: in the existing OAuthProviders map (used by auth.IsEmailWhitelisted), add a provider key like "emptyprov" with Whitelist: []string{} and then assert the function falls back to the global whitelist behavior (e.g., assert.True(t, auth.IsEmailWhitelisted("emptyprov", "[email protected]")) and assert.False for an email not in global). This ensures the len(...) == 0 fallback path in IsEmailWhitelisted is exercised.
🤖 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/controller/oauth_controller.go`:
- Line 186: Change the whitelist check to use the session-bound provider
identity: first verify provider/session consistency (compare req.Provider with
the session service identity svc.ID() or return an error), and only after that
call controller.auth.IsEmailWhitelisted with svc.ID() (not req.Provider); update
the condition around the existing IsEmailWhitelisted call so whitelist
evaluation uses svc.ID() and occurs after the provider mismatch check.
---
Nitpick comments:
In `@internal/service/auth_service_test.go`:
- Around line 19-34: Add a test case for a configured provider whose Whitelist
is present but empty: in the existing OAuthProviders map (used by
auth.IsEmailWhitelisted), add a provider key like "emptyprov" with Whitelist:
[]string{} and then assert the function falls back to the global whitelist
behavior (e.g., assert.True(t, auth.IsEmailWhitelisted("emptyprov",
"[email protected]")) and assert.False for an email not in global). This
ensures the len(...) == 0 fallback path in IsEmailWhitelisted is exercised.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 44388392-b334-4569-9e45-432b77db26f1
📒 Files selected for processing (7)
.env.exampleinternal/bootstrap/app_bootstrap.gointernal/controller/oauth_controller.gointernal/middleware/context_middleware.gointernal/model/config.gointernal/service/auth_service.gointernal/service/auth_service_test.go
Verify the OAuth service identity from the pending session before evaluating provider-specific whitelists, and cover the configured-empty provider fallback path. AI-assisted: Codex
|
Addressed the review on the current head
Verification run locally: AI-assisted: Codex helped draft this small follow-up; I reviewed the diff and verification output. |
steveiliop56
left a comment
There was a problem hiding this comment.
LGTM, @Rycochet and @scottmckendry everything ok with you?
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Summary
Closes #622
Validation
AI assistance
OpenAI GPT-5 assisted with repository navigation, drafting the focused regression test, and summarizing validation. I reviewed the change and take responsibility for the submitted code.
Summary by CodeRabbit
New Features
Tests