Conversation
|
Warning Review limit reached
More reviews will be available in 26 minutes and 7 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis PR refactors OAuth provider handling to use the goth library with dynamic provider selection. The StateStore interface now persists session values alongside state keys, enabling stateless OAuth session reconstruction. A new goth-based adapter replaces per-provider implementations and eliminates HTTP client injection from service wiring. ChangesOAuth Provider Abstraction and State Store Refactoring
Sequence Diagram(s)sequenceDiagram
participant Client
participant OAuthService
participant ProviderService
participant StateStore
participant GothProvider
Client->>OAuthService: GetOAuthURL(provider, state)
OAuthService->>ProviderService: GetAuthCodeURL(provider, state)
ProviderService->>GothProvider: BeginAuth(state)
GothProvider-->>ProviderService: AuthURL, Session
ProviderService->>StateStore: Marshal session
ProviderService-->>OAuthService: AuthURL, SessionString
OAuthService->>StateStore: Store(state, sessionString, expiry)
OAuthService-->>Client: redirectURL
Client->>OAuthService: HandleOAuthCallback(provider, code, state)
OAuthService->>StateStore: VerifyAndDelete(state)
StateStore-->>OAuthService: sessionString, err
OAuthService->>ProviderService: HandleCallback(provider, code, sessionString)
ProviderService->>StateStore: Unmarshal sessionString
ProviderService->>GothProvider: Authorize(code)
GothProvider->>GothProvider: FetchUser()
GothProvider-->>ProviderService: UserInfo
ProviderService-->>OAuthService: UserInfo
OAuthService-->>Client: UserInfo
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/adapters/core/service/store/nats_kv_state_store_test.go (1)
50-103: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winTest file should use
testify/assertfor assertions.Per coding guidelines, test files must use
testify/assertfor assertions. The current tests use manualt.Fatalandt.Fatalfcalls instead.Example refactor for one test:
+import ( + "github.com/stretchr/testify/assert" +) func TestNATSKVStateStore_Store(t *testing.T) { kv := newMockKV() s := NewNATSKVStateStore(kv) err := s.Store(context.Background(), "abc123", "session-data", 5*time.Minute) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } + assert.NoError(t, err) - if _, ok := kv.entries["state.abc123"]; !ok { - t.Fatal("expected key to be stored") - } + assert.Contains(t, kv.entries, "state.abc123") }🤖 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/adapters/core/service/store/nats_kv_state_store_test.go` around lines 50 - 103, Replace manual t.Fatal/t.Fatalf assertions in TestNATSKVStateStore_Store, TestNATSKVStateStore_Store_Duplicate, TestNATSKVStateStore_VerifyAndDelete and TestNATSKVStateStore_VerifyAndDelete_NotFound with testify/assert helpers: add import "github.com/stretchr/testify/assert", use assert.NoError(t, err) after Store/VerifyAndDelete calls, use assert.Contains(t, kv.entries, "state.abc123") to check stored key and assert.NotContains(t, kv.entries, "state.abc123") to check deletion, use assert.Equal(t, "session-data", value) for value equality, and use assert.Error(t, err) when expecting failures (duplicate or not found); reference the Store and VerifyAndDelete methods and the mockKV entries map when making replacements.Source: Coding guidelines
🧹 Nitpick comments (3)
internal/adapters/core/service/store/nats_kv_state_store.go (1)
47-48: 💤 Low valueConsider logging infrastructure errors before masking them.
When
Deletefails due to infrastructure issues (not revision mismatch), the error is silently converted to a genericBadRequestError("invalid state"). While this is secure (no information leakage), it may hinder debugging transient NATS failures.Consider logging the underlying error at debug/warn level before returning the masked error, or distinguishing revision conflicts from other failures if jetstream provides that granularity.
🤖 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/adapters/core/service/store/nats_kv_state_store.go` around lines 47 - 48, The Delete call in s.kv.Delete(ctx, key, jetstream.LastRevision(entry.Revision())) currently masks all errors by returning ungerr.BadRequestError("invalid state"); update the error handling in the method containing that call to log the underlying error (use process or package logger at debug/warn) before returning the generic BadRequestError, and if jetstream exposes a specific revision-conflict error type/value, branch to return the BadRequestError only for revision conflicts and log/return a different wrapper for infra errors; reference s.kv.Delete, jetstream.LastRevision(entry.Revision()) and the current return of ungerr.BadRequestError("invalid state") to locate where to add the logging and optional error-type check.internal/domain/service/oauth/provider_service.go (1)
19-37: 💤 Low valueNaming convention note: adapter vs service implementation.
The struct
gothProviderAdapterdoesn't follow the<name>ServiceImplnaming convention specified in coding guidelines forinternal/domain/service/**/*.go. However, this is intentionally an adapter pattern wrappinggoth.Provider, which is semantically different from a service implementation.If strict adherence is required, consider renaming to
providerServiceImpl. Otherwise, the current name clearly conveys intent.🤖 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/domain/service/oauth/provider_service.go` around lines 19 - 37, The type name gothProviderAdapter conflicts with the repository's <name>ServiceImpl convention; rename gothProviderAdapter to providerServiceImpl and gothProviderEntry to providerEntry, update NewProviderService to return &providerServiceImpl{...} and replace all references/usages of gothProviderAdapter and gothProviderEntry in this package (including any tests) to the new names so the adapter keeps its behavior but follows the ServiceImpl naming convention; ensure constructor signature (NewProviderService) and the map key ("google") initialization remain unchanged.Source: Coding guidelines
internal/domain/service/oauth_service.go (1)
36-42: ⚖️ Poor tradeoffConsider injecting
ProviderServicefor better testability.The
providerSvcis constructed directly fromconfig.Global.OAuthProvidersinside the constructor (line 37), making it difficult to mock in unit tests. Consider acceptingoauth.ProviderServiceas a parameter:func NewOAuthService( transactor crud.Transactor, + providerSvc oauth.ProviderService, oauthAccountRepo crud.Repository[users.OAuthAccount], stateStore store.StateStore, userSvc UserService, sessionSvc SessionService, ) OAuthService { return &oauthServiceImpl{ transactor: transactor, - providerSvc: oauth.NewProviderService(config.Global.OAuthProviders), + providerSvc: providerSvc, oauthAccountRepo: oauthAccountRepo, // ... } }This would require updating
service_provider.goto construct and inject the provider service.🤖 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/domain/service/oauth_service.go` around lines 36 - 42, Change the oauth service constructor to accept a oauth.ProviderService parameter instead of calling oauth.NewProviderService inside the constructor: replace the direct construction (NewProviderService(config.Global.OAuthProviders)) with a providerSvc parameter and assign it to the providerSvc field in the struct; then update service_provider.go so it constructs the ProviderService via oauth.NewProviderService(config.Global.OAuthProviders) and passes that instance into the oauth service factory when wiring dependencies. Also update any call sites/tests to pass a mock or real oauth.ProviderService accordingly.
🤖 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.
Outside diff comments:
In `@internal/adapters/core/service/store/nats_kv_state_store_test.go`:
- Around line 50-103: Replace manual t.Fatal/t.Fatalf assertions in
TestNATSKVStateStore_Store, TestNATSKVStateStore_Store_Duplicate,
TestNATSKVStateStore_VerifyAndDelete and
TestNATSKVStateStore_VerifyAndDelete_NotFound with testify/assert helpers: add
import "github.com/stretchr/testify/assert", use assert.NoError(t, err) after
Store/VerifyAndDelete calls, use assert.Contains(t, kv.entries, "state.abc123")
to check stored key and assert.NotContains(t, kv.entries, "state.abc123") to
check deletion, use assert.Equal(t, "session-data", value) for value equality,
and use assert.Error(t, err) when expecting failures (duplicate or not found);
reference the Store and VerifyAndDelete methods and the mockKV entries map when
making replacements.
---
Nitpick comments:
In `@internal/adapters/core/service/store/nats_kv_state_store.go`:
- Around line 47-48: The Delete call in s.kv.Delete(ctx, key,
jetstream.LastRevision(entry.Revision())) currently masks all errors by
returning ungerr.BadRequestError("invalid state"); update the error handling in
the method containing that call to log the underlying error (use process or
package logger at debug/warn) before returning the generic BadRequestError, and
if jetstream exposes a specific revision-conflict error type/value, branch to
return the BadRequestError only for revision conflicts and log/return a
different wrapper for infra errors; reference s.kv.Delete,
jetstream.LastRevision(entry.Revision()) and the current return of
ungerr.BadRequestError("invalid state") to locate where to add the logging and
optional error-type check.
In `@internal/domain/service/oauth_service.go`:
- Around line 36-42: Change the oauth service constructor to accept a
oauth.ProviderService parameter instead of calling oauth.NewProviderService
inside the constructor: replace the direct construction
(NewProviderService(config.Global.OAuthProviders)) with a providerSvc parameter
and assign it to the providerSvc field in the struct; then update
service_provider.go so it constructs the ProviderService via
oauth.NewProviderService(config.Global.OAuthProviders) and passes that instance
into the oauth service factory when wiring dependencies. Also update any call
sites/tests to pass a mock or real oauth.ProviderService accordingly.
In `@internal/domain/service/oauth/provider_service.go`:
- Around line 19-37: The type name gothProviderAdapter conflicts with the
repository's <name>ServiceImpl convention; rename gothProviderAdapter to
providerServiceImpl and gothProviderEntry to providerEntry, update
NewProviderService to return &providerServiceImpl{...} and replace all
references/usages of gothProviderAdapter and gothProviderEntry in this package
(including any tests) to the new names so the adapter keeps its behavior but
follows the ServiceImpl naming convention; ensure constructor signature
(NewProviderService) and the map key ("google") initialization remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 37aa19bd-c36d-4e42-bddd-bb3e0931402e
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
go.modinternal/adapters/core/service/store/in_memory_state_store.gointernal/adapters/core/service/store/nats_kv_state_store.gointernal/adapters/core/service/store/nats_kv_state_store_test.gointernal/core/service/store/state_store.gointernal/domain/service/oauth/google_provider_service.gointernal/domain/service/oauth/provider_service.gointernal/domain/service/oauth_service.gointernal/provider/service_provider.go
💤 Files with no reviewable changes (1)
- internal/domain/service/oauth/google_provider_service.go
- Add github.com/markbates/goth dependency - Implement gothProviderAdapter wrapping goth.Provider - Update StateStore interface to store/return session value - Update ProviderService to accept provider name parameter - Remove custom google_provider_service.go - Remove http.Client dependency from OAuthService
1 participant
Summary
Replace the custom
google_provider_service.go(~100 LOC of manual token exchange + userinfo fetch) with a goth-based provider adapter.Changes
github.com/markbates/gothdependencygothProviderAdapterwrappinggoth.Providerwith internal provider mapStateStoreinterface to store/return goth session value alongside stateProviderServiceinterface to accept provider name as parameterOAuthServiceto use singleProviderServiceinstead of manual map lookuphttp.Clientdependency fromOAuthServicegoogle_provider_service.goWhat was tested
make lint— 0 issuesmake build-all— all 3 binaries compilemake test— all tests passSummary by CodeRabbit