[LFXV2-1250] Validate id_token_social.sub before linking social identity#28
Conversation
… tests Jira Ticket: https://linuxfoundation.atlassian.net/browse/LFXV2-1250 Assisted by [Claude Code](https://claude.ai/code) Signed-off-by: Mauricio Zanetti Salomao <mauriciozanetti86@gmail.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
Disabled knowledge base sources:
WalkthroughAdds a pre-validation step to identity linking: Changes
Sequence DiagramsequenceDiagram
participant Client
participant MsgHandler as Message Handler
participant IdentityLinker as Identity Linker
participant TokenExtractor as Token Extractor
participant DB as Database
Client->>MsgHandler: LinkIdentity(request)
MsgHandler->>IdentityLinker: ValidateLinkRequest(ctx, request)
alt Validation fails
IdentityLinker->>TokenExtractor: Extract subject from identity token
TokenExtractor-->>IdentityLinker: Error or subject
IdentityLinker-->>MsgHandler: validation error
MsgHandler-->>Client: Validation error response
else Validation succeeds
IdentityLinker-->>MsgHandler: nil (validation passed)
MsgHandler->>IdentityLinker: LinkIdentity(ctx, request)
IdentityLinker->>DB: Link identity record
DB-->>IdentityLinker: Success
IdentityLinker-->>MsgHandler: Link result
MsgHandler-->>Client: Success response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/service/message_handler_test.go`:
- Around line 1207-1215: The table-driven tests define expectLinkerCalled but
never assert it, causing false-positive passes; update the test loop that
iterates over tests to assert that the mockIdentityLinker observed a
LinkIdentity invocation matches expectLinkerCalled (e.g., check a boolean field
or call count on mockIdentityLinker) after the handler runs and before finishing
the subtest; apply the same assertion to the second similar table-driven test
block (the one around the other occurrence) so both use expectLinkerCalled to
verify LinkIdentity was actually invoked.
🪄 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
Run ID: ec1c2f4f-c291-4d1b-9922-253ccb9ade74
📒 Files selected for processing (7)
internal/domain/port/user.gointernal/infrastructure/auth0/identity_test.gointernal/infrastructure/auth0/user.gointernal/infrastructure/authelia/user.gointernal/infrastructure/mock/user.gointernal/service/message_handler.gointernal/service/message_handler_test.go
There was a problem hiding this comment.
Pull request overview
This PR adds a validation step to the identity-linking flow to prevent linking when the provided identity token represents an existing LFID (Auth0 database) account, and updates the IdentityLinker port + implementations accordingly.
Changes:
- Add
ValidateLinkRequesttoport.IdentityLinkerand call it frommessageHandlerOrchestrator.LinkIdentitybefore user metadata lookup/linking. - Implement
ValidateLinkRequestfor Auth0 (rejectsubvalues prefixed withauth0|) and no-op implementations for Authelia and mock. - Add tests covering the new validation behavior (Auth0 unit test + service orchestrator test).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/service/message_handler.go | Calls ValidateLinkRequest before proceeding with metadata lookup and linking. |
| internal/service/message_handler_test.go | Adds an orchestrator-level test harness with a mock identity linker for validation gating behavior. |
| internal/infrastructure/mock/user.go | Adds a no-op ValidateLinkRequest to satisfy the updated IdentityLinker interface. |
| internal/infrastructure/authelia/user.go | Adds a no-op ValidateLinkRequest to satisfy the updated IdentityLinker interface. |
| internal/infrastructure/auth0/user.go | Adds Auth0-specific ValidateLinkRequest logic based on extracted JWT subject. |
| internal/infrastructure/auth0/identity_test.go | Adds unit tests for Auth0 ValidateLinkRequest. |
| internal/domain/port/user.go | Extends IdentityLinker with the new ValidateLinkRequest method. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
…sage handler tests Jira Ticket: https://linuxfoundation.atlassian.net/browse/LFXV2-1250 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Mauricio Zanetti Salomao <mauriciozanetti86@gmail.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/service/message_handler_test.go (1)
1207-1276:⚠️ Potential issue | 🟡 MinorAdd explicit
LinkIdentityinvocation assertions in table tests.These cases validate response payloads, but they still don’t assert that
LinkIdentitywas actually invoked on success. This can allow false-positive passes if orchestration regresses. Please track/verify call state per case.Suggested test hardening
tests := []struct { name string messageData []byte linker *mockIdentityLinker reader *mockUserServiceReader expectSuccess bool expectError string + expectLinkerCalled bool }{ { name: "validate guard blocks database user token", @@ expectSuccess: false, expectError: "the provided identity token belongs to an existing LFID account and cannot be linked", + expectLinkerCalled: false, }, { name: "validate guard passes for social token", @@ expectSuccess: true, + expectLinkerCalled: true, }, { name: "invalid json returns error", @@ expectSuccess: false, + expectLinkerCalled: false, }, } @@ for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + linkerCalled := false + origLink := tt.linker.linkIdentityFunc + tt.linker.linkIdentityFunc = func(ctx context.Context, req *model.LinkIdentity) error { + linkerCalled = true + if origLink != nil { + return origLink(ctx, req) + } + return nil + } + orchestrator := NewMessageHandlerOrchestrator( WithIdentityLinkerForMessageHandler(tt.linker), WithUserReaderForMessageHandler(tt.reader), ) @@ if tt.expectError != "" && response.Error != tt.expectError { t.Errorf("error = %q, want %q", response.Error, tt.expectError) } + if linkerCalled != tt.expectLinkerCalled { + t.Errorf("LinkIdentity called = %v, want %v", linkerCalled, tt.expectLinkerCalled) + } }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/message_handler_test.go` around lines 1207 - 1276, The table tests currently verify the response payload but don't assert that the identity linking path was executed; update the mockIdentityLinker instances in each test case to track invocation (e.g., add a called flag or counter in the mock and set it inside linkIdentityFunc), then after calling orchestrator.LinkIdentity(...) assert that linkIdentity was invoked when expectSuccess is true and not invoked when the validateLinkRequestFunc returns a validation error; use the existing mockIdentityLinker and its linkIdentityFunc/validateLinkRequestFunc hooks to set and check the flag immediately after unmarshalling the UserDataResponse.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/service/message_handler_test.go`:
- Around line 1207-1276: The table tests currently verify the response payload
but don't assert that the identity linking path was executed; update the
mockIdentityLinker instances in each test case to track invocation (e.g., add a
called flag or counter in the mock and set it inside linkIdentityFunc), then
after calling orchestrator.LinkIdentity(...) assert that linkIdentity was
invoked when expectSuccess is true and not invoked when the
validateLinkRequestFunc returns a validation error; use the existing
mockIdentityLinker and its linkIdentityFunc/validateLinkRequestFunc hooks to set
and check the flag immediately after unmarshalling the UserDataResponse.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3a8de7e8-877d-4256-8798-1557b60a2689
📒 Files selected for processing (2)
internal/infrastructure/auth0/identity_test.gointernal/service/message_handler_test.go
…counts Jira Ticket: https://linuxfoundation.atlassian.net/browse/LFXV2-1250 Assisted by [Claude Code](https://claude.ai/code) Signed-off-by: Mauricio Zanetti Salomao <mauriciozanetti86@gmail.com>
d291c44
into
linuxfoundation:main
Overview
Jira Ticket: https://linuxfoundation.atlassian.net/browse/LFXV2-1250
Adds a defense-in-depth guard against merging two LFID database users during social identity linking (LFXV2-1250).
When a social provider is already connected to a different LFID, Auth0 returns the
subof the existing database user (auth0|...) instead of the social identity's ownsub(e.g.google-oauth2|...). If the service proceeds with the link in this state, it merges two LFID database users — effectively locking the secondary account's owner out with no current recovery path.The solution adds
ValidateLinkRequestto theIdentityLinkerport. The Auth0 implementation rejects the request early ifid_token_social.substarts withauth0|. Mock and Authelia implementations are no-op stubs to be completed in follow-up tickets.How it works
The guard is inserted in the
LinkIdentityhandler before any Auth0 Management API call:ValidateLinkRequestextracts thesubclaim fromIdentityTokenwithout signature verification (the Management API rejects tampered tokens at link time) and rejects if the prefix isauth0|. Valid social prefixes (google-oauth2|,github|,linkedin|, etc.) pass through.Changes
internal/domain/port/user.go— addsValidateLinkRequesttoIdentityLinkerinterfaceinternal/infrastructure/auth0/user.go— Auth0 implementation: extractssub, rejectsauth0|prefixinternal/service/message_handler.go— callsValidateLinkRequestbeforeMetadataLookupinternal/infrastructure/mock/user.go— no-op stubinternal/infrastructure/authelia/user.go— no-op stubinternal/infrastructure/auth0/identity_test.go— unit tests forValidateLinkRequestinternal/service/message_handler_test.go— unit tests for the guard in the handlerTest Evidence
Error path —
auth0|sub rejectedSocial token, link proceeds
{"time":"2026-03-17T14:24:45.526Z","level":"DEBUG","msg":"API call successful","method":"POST","status_code":201,"description":"link identity to user","subject":"lfx.auth-service.user_identity.link"} {"time":"2026-03-17T14:24:45.526Z","level":"DEBUG","msg":"identity linked successfully","user_id":"aut****","status_code":201,"subject":"lfx.auth-service.user_identity.link"}