Route OTP registration, Magic Link registration, Magic Link authentication through authnprovider#3012
Open
ThumulaPerera wants to merge 2 commits into
Conversation
ThumulaPerera
requested review from
ThaminduDilshan and
darshanasbg
as code owners
Contributor
📝 WalkthroughWalkthroughThis PR refactors authentication services and flows to support a unified result model where verified identifiers are returned even when no local user is found. Magic link and OTP services now return result types that combine either an internal entity or verified identifiers. All authentication is delegated through the authn provider manager instead of direct service calls. ChangesAuthentication Result Model & Provider Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/internal/authnprovider/provider/default_authn_provider_test.go (1)
300-392: ⚡ Quick winAdd direct unit tests for the new
magiclinkcredential branch.This suite covers OTP well, but the newly added
authenticateWithMagicLinkpath is untested here (success, non-existing user, invalid payload, and service error mapping). Please add targeted tests forcredentials["magiclink"]routing and outcomes.🤖 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 `@backend/internal/authnprovider/provider/default_authn_provider_test.go` around lines 300 - 392, Add unit tests mirroring the existing OTP tests but targeting the magiclink credential branch: create tests in DefaultAuthnProviderTestSuite that call provider.Authenticate with credentials["magiclink"] payloads and assert correct behavior for (1) existing user (mock magiclink service to return InternalEntity with ID and ensure suite.mockService.GetEntity is called and result.IsExistingUser true), (2) non-existing user (magiclink returns nil InternalEntity plus VerifiedIdentifiers and ensure AttributesResponse populated and IsExistingUser false), (3) invalid payload (pass non-map magiclink value and expect ErrorCodeInvalidRequest), and (4) service error mapping (mock magiclink to return an error and assert Authenticate returns authnprovidercm.ErrorCodeAuthenticationFailed); reference newDefaultAuthnProvider, Authenticate, and authenticateWithMagicLink flow when adding these tests.
🤖 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.
Nitpick comments:
In `@backend/internal/authnprovider/provider/default_authn_provider_test.go`:
- Around line 300-392: Add unit tests mirroring the existing OTP tests but
targeting the magiclink credential branch: create tests in
DefaultAuthnProviderTestSuite that call provider.Authenticate with
credentials["magiclink"] payloads and assert correct behavior for (1) existing
user (mock magiclink service to return InternalEntity with ID and ensure
suite.mockService.GetEntity is called and result.IsExistingUser true), (2)
non-existing user (magiclink returns nil InternalEntity plus VerifiedIdentifiers
and ensure AttributesResponse populated and IsExistingUser false), (3) invalid
payload (pass non-map magiclink value and expect ErrorCodeInvalidRequest), and
(4) service error mapping (mock magiclink to return an error and assert
Authenticate returns authnprovidercm.ErrorCodeAuthenticationFailed); reference
newDefaultAuthnProvider, Authenticate, and authenticateWithMagicLink flow when
adding these tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: aa7b34a8-b525-4f90-9660-bf494d12bada
⛔ Files ignored due to path filters (2)
backend/tests/mocks/authn/magiclinkmock/MagicLinkAuthnServiceInterface_mock.gois excluded by!**/*_mock.gobackend/tests/mocks/authn/otpmock/OTPAuthnServiceInterface_mock.gois excluded by!**/*_mock.go
📒 Files selected for processing (16)
backend/cmd/server/servicemanager.gobackend/internal/authn/magiclink/service.gobackend/internal/authn/magiclink/service_test.gobackend/internal/authn/otp/service.gobackend/internal/authn/otp/service_test.gobackend/internal/authnprovider/manager/init.gobackend/internal/authnprovider/manager/manager.gobackend/internal/authnprovider/manager/manager_test.gobackend/internal/authnprovider/provider/default_authn_provider.gobackend/internal/authnprovider/provider/default_authn_provider_test.gobackend/internal/authnprovider/provider/init.gobackend/internal/flow/executor/init.gobackend/internal/flow/executor/magic_link_executor.gobackend/internal/flow/executor/magic_link_executor_test.gobackend/internal/flow/executor/sms_auth_executor.gobackend/internal/flow/executor/sms_auth_executor_test.go
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
ThumulaPerera
force-pushed
the
improve-magic-link-to-go-through-authnprovider
branch
from
f0a5a7d to
2dd0b98
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/internal/flow/executor/magic_link_executor_test.go (1)
536-544: ⚡ Quick winTighten
AuthenticateUserargument assertions in verify-mode testsUsing
mock.Anythingfor nearly allAuthenticateUserargs makes these tests pass even if credential type/payload wiring regresses. Prefermock.MatchedByfor the key fields (e.g., magic-link credential type/token/destination attribute) in at least one success and one failure-path test.🤖 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 `@backend/internal/flow/executor/magic_link_executor_test.go` around lines 536 - 544, The test currently stubs mockAuthnProvider.AuthenticateUser with mock.Anything for almost all args, allowing regressions in magic-link wiring; update the AuthenticateUser expectations in magic_link_executor_test.go to use mock.MatchedBy for the credential-related parameters (e.g., credential type equals magic-link, and the payload contains the expected token and destination) when setting up mockAuthnProvider.On(...).Return(...) for both a representative success case (the existing success stub that returns AuthnBasicResult) and at least one failure-path test so the mock asserts the magic-link credential type/token/destination are passed through correctly.
🤖 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 `@backend/internal/authn/magiclink/service.go`:
- Around line 146-156: The current handling in the block after
s.resolveUserFromSubject converts common.ErrorUserNotFound into a successful
MagicLinkAuthnResult even when subjectAttribute is empty; change the logic so
that if resolveErr != nil and resolveErr.Code == common.ErrorUserNotFound.Code,
you only return a successful MagicLinkAuthnResult when subjectAttribute is
non-empty (i.e., login-style identifier lookup), but when subjectAttribute is
empty (local user lookup) propagate/return resolveErr instead of building a
MagicLinkAuthnResult; update the branch around
resolveUserFromSubject/subjectAttribute/MagicLinkAuthnResult so executeVerify
receives an error for missing users rather than a nil UserID.
---
Nitpick comments:
In `@backend/internal/flow/executor/magic_link_executor_test.go`:
- Around line 536-544: The test currently stubs
mockAuthnProvider.AuthenticateUser with mock.Anything for almost all args,
allowing regressions in magic-link wiring; update the AuthenticateUser
expectations in magic_link_executor_test.go to use mock.MatchedBy for the
credential-related parameters (e.g., credential type equals magic-link, and the
payload contains the expected token and destination) when setting up
mockAuthnProvider.On(...).Return(...) for both a representative success case
(the existing success stub that returns AuthnBasicResult) and at least one
failure-path test so the mock asserts the magic-link credential
type/token/destination are passed through correctly.
🪄 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: a439af35-a0a7-4f3c-b0ba-44c67ea13d70
⛔ Files ignored due to path filters (1)
backend/tests/mocks/authn/magiclinkmock/MagicLinkAuthnServiceInterface_mock.gois excluded by!**/*_mock.go
📒 Files selected for processing (10)
backend/cmd/server/servicemanager.gobackend/internal/authn/magiclink/service.gobackend/internal/authn/magiclink/service_test.gobackend/internal/authnprovider/manager/init.gobackend/internal/authnprovider/provider/default_authn_provider.gobackend/internal/authnprovider/provider/default_authn_provider_test.gobackend/internal/authnprovider/provider/init.gobackend/internal/flow/executor/init.gobackend/internal/flow/executor/magic_link_executor.gobackend/internal/flow/executor/magic_link_executor_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
This PR incorporates 3 main changes
Approach
For 1 above :
Merged the separate VerifyOTP (verify-only) and Authenticate (verify + resolve user) methods into a single Authenticate that returns an OTPAuthnResult struct. When OTP verification succeeds but no local user exists, it returns {InternalEntity: nil, VerifiedIdentifiers: {"mobileNumber": recipient}} instead of an error. This lets callers distinguish "valid OTP, no user yet" from actual failures without inspecting error codes.
Updated authenticateWithOTP to handle the new result shape — nil entity triggers an early return with IsExistingUser: false and the verified identifiers, matching how federated auth already works. Also extracted a shared buildAttributesResponse helper and fixed the existing-user path to include attribute values (IsAttributeValuesIncluded: true).
Registration flow now calls authnProvider.AuthenticateUser instead of otpService.VerifyOTP directly, so registration and login both go through the same provider abstraction.
For 2 and 3 above :
Renamed VerifyMagicLink → Authenticate, introduced MagicLinkAuthnResult struct. User-not-found is now a successful result (InternalEntity: nil + VerifiedIdentifiers carrying the proven email/attribute) instead of an error. Token verification logic is extracted into a private verifyTokenAndExtractSubject helper.
Wired magicLinkService as a dependency through the initialization chain. Added authenticateWithMagicLink in the provider and registered the "magiclink" credential key in resolveCredentials — following the same entity-ID-or-early-return pattern as OTP and federated.
Verification now calls authnProvider.AuthenticateUser instead of magicLinkService.VerifyMagicLink directly. The old validateMagicLinkToken (which mixed auth concerns with flow concerns) is split: authentication moves to the provider, and flow-specific checks (execution-ID binding, JTI replay detection) stay in the executor as validateFlowClaims.
Related Issues
Related PRs
Checklist
breaking changelabel added.Security checks
Summary by CodeRabbit
New Features
Refactor