Feature oauth account linking#202
Conversation
|
All contributors have signed the CLA. ✅ Thank you! |
📝 WalkthroughWalkthroughThe PR introduces ChangesOAuth Account Linking and Unlinking
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler
participant AuthService
participant OAuthRepo
rect rgba(100, 149, 237, 0.5)
Note over Client,OAuthRepo: Link OAuth Provider
Client->>Handler: POST /api/auth/link/:provider
Handler->>Handler: Extract userID from context
alt userID missing
Handler-->>Client: 401 Unauthorized
else userID present
Handler->>AuthService: LinkOAuthProvider(userID, provider, providerUserID)
AuthService->>OAuthRepo: Create(UserOAuthAccount)
alt creation error
OAuthRepo-->>AuthService: error
AuthService-->>Handler: error
Handler-->>Client: 500 Internal Server Error
else creation success
OAuthRepo-->>AuthService: nil
AuthService-->>Handler: nil
Handler-->>Client: 200 OK with success message
end
end
end
rect rgba(144, 238, 144, 0.5)
Note over Client,OAuthRepo: Unlink OAuth Provider
Client->>Handler: DELETE /api/auth/unlink/:provider
Handler->>Handler: Extract userID from context
alt userID missing
Handler-->>Client: 401 Unauthorized
else userID present
Handler->>AuthService: UnlinkOAuthProvider(userID, provider)
AuthService->>OAuthRepo: FindByUserID(userID)
OAuthRepo-->>AuthService: list of UserOAuthAccount
alt deletion blocked
AuthService-->>Handler: error (last login method)
Handler-->>Client: 400 Bad Request
else deletion allowed
AuthService->>OAuthRepo: Delete(userID, provider)
alt deletion error
OAuthRepo-->>AuthService: error
AuthService-->>Handler: error
Handler-->>Client: 500 Internal Server Error
else deletion success
OAuthRepo-->>AuthService: nil
AuthService-->>Handler: nil
Handler-->>Client: 200 OK with success message
end
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 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.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/testutils/setup.go (1)
44-51:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
UserOAuthAccountto integration-test migrations.
oauthAccountRepois now wired intoAuthService, but the in-memory schema setup does not createuser_oauth_accounts, which can break OAuth login/link test paths with missing-table errors.Suggested fix
err = db.AutoMigrate( &models.User{}, + &models.UserOAuthAccount{}, &models.RefreshToken{}, &models.VerificationToken{}, &models.PasswordResetToken{}, &models.AuditLog{}, &models.OAuthAccessToken{}, )Also applies to: 106-126
🤖 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/testutils/setup.go` around lines 44 - 51, The AutoMigrate call in setup.go is missing the UserOAuthAccount model, which causes OAuth-related test paths to fail when the oauthAccountRepo tries to access the user_oauth_accounts table. Add &models.UserOAuthAccount{} to the list of models being passed to the db.AutoMigrate function, ensuring it is included alongside the other models like User, RefreshToken, VerificationToken, PasswordResetToken, AuditLog, and OAuthAccessToken. The comment also mentions this applies to lines 106-126, so verify all AutoMigrate calls in the file include this model.
🤖 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/handler/auth_handler.go`:
- Around line 1017-1024: The error handling in the provider link operation
(around line 1017) currently returns HTTP 500 for all errors from the service
layer. Instead of always returning StatusInternalServerError, use errors.Is() to
perform sentinel checks against custom error types exported from
service/errors.go and map them to appropriate HTTP status codes. For example,
check for conflict errors (return 409), not found errors (return 404), and
validation errors (return 400) before defaulting to 500 for unexpected internal
errors. Apply the same fix to the unlink operation mentioned in the comment
(around line 1056).
- Around line 997-1015: The LinkOAuthProvider handler method is accepting
providerUserID directly from a query parameter without verification, which
allows authenticated users to link arbitrary OAuth identities without proving
ownership. Remove the direct c.Query("providerUserID") input and instead
implement a proper OAuth callback flow where providerUserID is derived from the
verified OAuth provider response. The LinkOAuthProvider call should only execute
after the application has completed the OAuth token exchange with the provider
and extracted the providerUserID from the verified provider credentials.
- Around line 1011-1013: In both the LinkProvider and UnlinkProvider methods,
replace the unsafe direct type assertion `userID.(string)` with a checked
assertion pattern using `uid, ok := userID.(string)`. If the assertion fails (ok
is false), return an HTTP 401 Unauthorized response to the client before
attempting to use the userID value. This prevents a panic if the context
contract changes and userID is not a string type, consistent with the pattern
already used in other handler methods like GetAuditLogs and DisableMFA.
In `@internal/models/user_oauth_account.go`:
- Around line 8-15: Add unique constraints to the UserOAuthAccount struct by
including uniqueness specifications in the gorm tags for the Provider and
ProviderUserID fields (either as individual unique constraints or as a composite
unique constraint combining both fields) to prevent duplicate external account
mappings, and then add &models.UserOAuthAccount{} to the AutoMigrate call in the
database initialization code to ensure the table is created during production
deployments.
In `@internal/service/auth_service.go`:
- Around line 706-713: The s.userRepo.Update() call is not checking for errors,
which means database failures when updating the email_verified flag are silently
ignored. Capture the error returned from s.userRepo.Update() and handle it
appropriately by either returning an error, logging it, or taking other
corrective action to prevent the login from succeeding with an inconsistent
persisted state.
- Around line 631-645: The code currently treats any error from FindByProvider
as a missing account and falls through to email lookup or create logic, but this
masks transient database errors. Instead of checking if err is nil, use
errors.Is() to specifically check if the error is the not-found sentinel error.
Only proceed with the fallback email lookup in FindByEmail when the error is
specifically a not-found error; return immediately for any other error type.
Apply this same pattern to the second location mentioned in lines 689-704 that
has similar FindByProvider error handling.
- Around line 1130-1154: The LinkOAuthProvider and UnlinkOAuthProvider methods
currently return raw repository errors directly, which forces handlers into
generic error responses. Define custom error types in service/errors.go for
OAuth-related scenarios such as OAuthAccountAlreadyExists, OAuthAccountNotFound,
and InvalidOAuthProvider, then modify both methods to catch the errors returned
from s.oauthAccountRepo.Create() and s.oauthAccountRepo.Delete() respectively,
map them to the appropriate service error types based on the underlying error
details, and return the typed service errors instead of raw repository errors.
This allows handlers to check for specific error types and respond with
appropriate HTTP status codes without being tightly coupled to repository
implementation details.
---
Outside diff comments:
In `@internal/testutils/setup.go`:
- Around line 44-51: The AutoMigrate call in setup.go is missing the
UserOAuthAccount model, which causes OAuth-related test paths to fail when the
oauthAccountRepo tries to access the user_oauth_accounts table. Add
&models.UserOAuthAccount{} to the list of models being passed to the
db.AutoMigrate function, ensuring it is included alongside the other models like
User, RefreshToken, VerificationToken, PasswordResetToken, AuditLog, and
OAuthAccessToken. The comment also mentions this applies to lines 106-126, so
verify all AutoMigrate calls in the file include this model.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9a1c173d-e3b5-44b0-990d-851781aa3fc7
📒 Files selected for processing (6)
internal/handler/auth_handler.gointernal/models/user_oauth_account.gointernal/repository/user_oauth_account_repository.gointernal/routes/routes.gointernal/service/auth_service.gointernal/testutils/setup.go
|
I have read the CLA and agree to its terms |
|
recheck |
|
I have read the CLA and agree to its terms. |
|
recheck |
|
@pari-28 could you resolev the 5 issues that are flagged by sonar |
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/auth_service.go`:
- Around line 701-713: The userRepo.Create and oauthAccountRepo.Create
operations are not atomic, risking orphaned users if the OAuth account creation
fails after the user is created. Wrap both operations in a database transaction
so that if either the user creation or the OAuth account creation fails, the
entire transaction is rolled back. This ensures the system remains in a
consistent state and both operations either succeed together or fail together.
You'll need to obtain a transaction handle from your database connection and
pass it to both the userRepo.Create and oauthAccountRepo.Create methods, then
use transaction commit or rollback based on whether any errors occur.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ea5bc399-f8ee-4dbf-852d-2052bb2292c0
📒 Files selected for processing (4)
cmd/server/main.gointernal/models/user_oauth_account.gointernal/repository/user_oauth_account_repository.gointernal/service/auth_service.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/models/user_oauth_account.go
- internal/repository/user_oauth_account_repository.go
|
I have addressed the SonarQube issues and pushed the corresponding fixes. The project builds successfully and all tests are passing. Please let me know if any further changes are needed. Thank you! |
|
@pari-28 you can add some test for it in followup pr |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/handler/auth_handler.go`:
- Around line 1001-1010: The LinkProvider handler unconditionally returns a 400
Bad Request response with an "not yet supported" message, which prevents any
provider linking from functioning. Remove this hardcoded error response and
implement the actual account linking logic instead. The section that calls
c.JSON with http.StatusBadRequest and the error message about provider token
verification needs to be replaced with the real implementation that processes
the provider account linking request based on the provider parameter.
- Around line 992-1009: The error responses returned in this endpoint use
utils.UnauthorizedResponse and utils.ErrorResponse which do not conform to the
required error schema of {"error":"message","code":"ERROR_CODE"}. Replace both
error response calls (the utils.UnauthorizedResponse call returning
http.StatusUnauthorized and the utils.ErrorResponse call returning
http.StatusBadRequest) with responses that include both an "error" field with
the message and a "code" field with an appropriate error code (such as
"UNAUTHORIZED" for the first case and "UNSUPPORTED_OPERATION" or similar for the
account linking case) to match the handler guidelines.
- Line 1012: Remove the redundant return statement at the end of the function in
auth_handler.go. The return statement after the c.JSON(...) call is unnecessary
since it is the final statement in the function and does not require an explicit
return. Simply delete the return statement on line 1012 to resolve the
staticcheck S1023 warning.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 65d51b60-36d3-4f82-8072-93ed33c0c893
📒 Files selected for processing (2)
internal/handler/auth_handler.gointernal/service/auth_service.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/service/auth_service.go
| c.JSON( | ||
| http.StatusUnauthorized, | ||
| utils.UnauthorizedResponse("Unauthorized"), | ||
| ) | ||
| return | ||
| } | ||
|
|
||
| provider := c.Param("provider") | ||
|
|
||
| c.JSON( | ||
| http.StatusBadRequest, | ||
| utils.ErrorResponse( | ||
| fmt.Sprintf( | ||
| "linking %s accounts requires provider token verification and is not yet supported", | ||
| provider, | ||
| ), | ||
| nil, | ||
| ), |
There was a problem hiding this comment.
Error payload shape does not follow the handler guideline contract.
These branches return utils.UnauthorizedResponse / utils.ErrorResponse, which do not match the required {"error":"message","code":"ERROR_CODE"} shape for handler errors. Please align both error responses in this endpoint to the mandated schema (or route through a shared helper that emits that schema).
As per coding guidelines, "internal/handler/**/*.go: Return JSON error responses in format {\"error\": \"message\", \"code\": \"ERROR_CODE\"} from HTTP handlers".
🤖 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/handler/auth_handler.go` around lines 992 - 1009, The error
responses returned in this endpoint use utils.UnauthorizedResponse and
utils.ErrorResponse which do not conform to the required error schema of
{"error":"message","code":"ERROR_CODE"}. Replace both error response calls (the
utils.UnauthorizedResponse call returning http.StatusUnauthorized and the
utils.ErrorResponse call returning http.StatusBadRequest) with responses that
include both an "error" field with the message and a "code" field with an
appropriate error code (such as "UNAUTHORIZED" for the first case and
"UNSUPPORTED_OPERATION" or similar for the account linking case) to match the
handler guidelines.
Source: Coding guidelines
|
I've addressed the issues you pointed out:
The project builds successfully and all tests are passing. CodeRabbit still flags LinkProvider because the endpoint now intentionally returns an error until proper provider-side token verification is implemented. I preferred disabling the insecure flow rather than allowing arbitrary account claiming. Please let me know if you would like provider token verification to be implemented in this PR or handled in a follow-up PR. Thanks! |
|
@pari-28 implement this in the follow up pr and i will review the another one then i will merge the both together |
|
I've opened a follow-up PR (#218) to address the remaining review feedback from this PR. Included in #218
All checks are passing ( Thank you! |
|
@pari-28 Just a few things to sort out before we merge: The LinkProvider route is wired up but always returns a 400 which will confuse anyone hitting the API. wire the handler up properly The last login method guard has a small bug where PasswordHash == "" will never actually be true since we always hash a random password for OAuth users. So right now someone could unlink their only provider and get locked out. Probably easiest to just check len(accounts) == 1 alone and block from there. Also in linkOAuthAccountIfNeeded, the FindByProvider check doesn't verify the account actually belongs to the current user. If that oauthID is already tied to someone else we'd silently let it through which is a bit of a security hole. Worth adding a user ID check there. Smaller things: would be good to add a LinkedAt timestamp to UserOAuthAccount for audit purposes, and the Delete in the repo returns nil even when nothing was deleted so the caller gets a 200 for unlinking a provider they never had. Overall this is in really good shape though, just these bits to tighten up! |
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/handler/auth_handler.go (1)
1013-1017:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard the
userIDtype assertion to prevent handler panic.Line 1014 uses
userID.(string)without validating the type. If the middleware/context contract drifts anduserIDis not a string, the handler will panic. Use a checked assertion:- err := h.authService.LinkOAuthProvider( - userID.(string), + uid, ok := userID.(string) + if !ok { + c.JSON(http.StatusUnauthorized, utils.UnauthorizedResponse("Unauthorized")) + return + } + + err := h.authService.LinkOAuthProvider( + uid, provider, providerUserID, )🤖 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/handler/auth_handler.go` around lines 1013 - 1017, The LinkOAuthProvider method call uses an unchecked type assertion on userID with userID.(string) which will cause a panic if userID is not a string type. Replace the unchecked assertion with a checked assertion that validates the type before use, handling the case where userID is not a string by returning an appropriate error response to the caller rather than allowing the handler to panic.
🤖 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/auth_service.go`:
- Around line 1219-1222: The check in the last-login-method guard is too
restrictive because it only counts the number of accounts without considering
whether the user has a password fallback. Modify the condition to prevent
unlinking only when the user has exactly one login method AND no password hash
is set. Instead of just checking if len(accounts) equals 1, add an additional
condition to verify that the user also lacks a valid PasswordHash (or equivalent
password field) before returning the error. This allows users who have both a
password and a single OAuth provider to unlink the OAuth provider while
retaining password-based login access.
---
Duplicate comments:
In `@internal/handler/auth_handler.go`:
- Around line 1013-1017: The LinkOAuthProvider method call uses an unchecked
type assertion on userID with userID.(string) which will cause a panic if userID
is not a string type. Replace the unchecked assertion with a checked assertion
that validates the type before use, handling the case where userID is not a
string by returning an appropriate error response to the caller rather than
allowing the handler to panic.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ab0cbd81-de6d-4bc9-b085-8070cab1d78b
📒 Files selected for processing (5)
internal/handler/auth_handler.gointernal/models/user_oauth_account.gointernal/repository/user_oauth_account_repository.gointernal/repository/user_repository.gointernal/service/auth_service.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/repository/user_oauth_account_repository.go
- internal/models/user_oauth_account.go
| // Prevent removing the last login method | ||
| if len(accounts) == 1 { | ||
| return errors.New("cannot unlink the last login method") | ||
| } |
There was a problem hiding this comment.
Last-login-method guard blocks users with password fallback from unlinking.
The check len(accounts) == 1 prevents unlinking when the user has exactly one OAuth account, regardless of whether they have a usable password. A user who registered with email/password and later linked one OAuth provider will be blocked from unlinking that provider even though they can still log in with their password.
Consider checking whether the user has an alternative login method (e.g., a non-empty PasswordHash set via normal registration) before blocking:
+ user, err := s.userRepo.FindByID(userID)
+ if err != nil {
+ return err
+ }
+
+ hasPasswordFallback := user.PasswordHash != ""
+
// Prevent removing the last login method
- if len(accounts) == 1 {
+ if len(accounts) == 1 && !hasPasswordFallback {
return errors.New("cannot unlink the last login method")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Prevent removing the last login method | |
| if len(accounts) == 1 { | |
| return errors.New("cannot unlink the last login method") | |
| } | |
| user, err := s.userRepo.FindByID(userID) | |
| if err != nil { | |
| return err | |
| } | |
| hasPasswordFallback := user.PasswordHash != "" | |
| // Prevent removing the last login method | |
| if len(accounts) == 1 && !hasPasswordFallback { | |
| return errors.New("cannot unlink the last login method") | |
| } |
🤖 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.go` around lines 1219 - 1222, The check in the
last-login-method guard is too restrictive because it only counts the number of
accounts without considering whether the user has a password fallback. Modify
the condition to prevent unlinking only when the user has exactly one login
method AND no password hash is set. Instead of just checking if len(accounts)
equals 1, add an additional condition to verify that the user also lacks a valid
PasswordHash (or equivalent password field) before returning the error. This
allows users who have both a password and a single OAuth provider to unlink the
OAuth provider while retaining password-based login access.
|
Thanks for the feedback. I've addressed the remaining review comments. Changes made
Verificationgo build ./...
go test ./... |
2 participants
Fixes #89
Checklist
I have read the CLA and agree to its terms.on this PR.Summary by CodeRabbit
Summary by CodeRabbit