Centralize OTP generation attempt validation into core OTP service#4334
Open
kavix wants to merge 5 commits into
Open
Centralize OTP generation attempt validation into core OTP service#4334kavix wants to merge 5 commits into
kavix wants to merge 5 commits into
Conversation
Move max-attempt tracking and enforcement from OTPExecutor into the core OTP notification service (notification/otp_service.go), so both flow executions and direct notification service calls share unified attempt limits. - Add MaxGenerationAttempts to OTPConfig in server configuration - Embed AttemptCount in JWT session token data - Update GenerateOTP interface to accept previousSessionToken and validate attempt limit - Add ErrorMaxOTPAttemptsExceeded to notification and authn/otp error constants - Remove attempt tracking, propertyKeyMaxOTPAttempts, and RuntimeKeyOTPAttemptCount from flow executor - Regenerate i18n default messages and mocks - Update unit tests across notification, authn/otp, and flow executor packages
Contributor
|
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 (3)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughOTP generation attempt tracking moves from the flow executor into the notification OTP service. Previous session tokens carry attempt counts, configuration validates the generation limit, and authentication and executor layers propagate and map max-attempt errors. ChangesOTP attempt configuration and notification enforcement
Authentication and flow integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant OTPExecutor
participant AuthenticationService
participant OTPAuthnService
participant NotificationOTPService
OTPExecutor->>AuthenticationService: SendOTP(priorSessionToken)
AuthenticationService->>OTPAuthnService: GenerateOTP(previousSessionToken)
OTPAuthnService->>NotificationOTPService: GenerateOTP(previousSessionToken)
NotificationOTPService->>NotificationOTPService: Verify token and increment AttemptCount
NotificationOTPService-->>OTPAuthnService: OTP session or max-attempt error
OTPAuthnService-->>AuthenticationService: Mapped result
AuthenticationService-->>OTPExecutor: OTP result or executor failure
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
docs/content/deployment/configuration.mdxESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/service.go`:
- Line 211: Update SendOTP and its HTTP request flow to accept and forward the
prior session token to otpService.GenerateOTP instead of always starting a new
session. Include the token in resend requests alongside senderId and recipient,
and preserve the returned session token for subsequent attempts so
ErrorMaxOTPAttemptsExceeded is enforced across retries.
In `@backend/internal/flow/executor/otp_executor_test.go`:
- Around line 297-312: The GenerateOTP expectation in the OTP executor test
currently accepts any session token, despite RuntimeData providing
"prev-session-tok". Replace the fourth mock.Anything matcher in
suite.mockOTPService.On("GenerateOTP", ...) with an assertion requiring the
exact previous session token while preserving the existing other arguments and
return values.
In `@backend/internal/notification/otp_service.go`:
- Around line 89-103: Update generateOTP around verifyAndDecodeSessionToken so
the previous token’s recipient identity and attributes are validated against the
current OTP request before reusing prevData.AttemptCount. Reject tokens
belonging to a different recipient or attribute set, and only increment
attemptCount after this binding check; preserve the existing maximum-attempt
enforcement.
In `@backend/internal/system/config/config.go`:
- Line 125: Document notification.otp.max_generation_attempts in the relevant
configuration content under docs/content/, including its default of 3, valid
range of 1–10, and impact on OTP regeneration; document session-token-based
generation attempt tracking and the outcome when the limit is reached in the
applicable authentication or flow guide under docs/content/guides/. The config
declaration in backend/internal/system/config/config.go and related test in
backend/internal/authn/service_test.go require no direct changes.
🪄 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: 2c4baee8-54e6-4e8f-a787-1e1ae4cadab1
⛔ Files ignored due to path filters (2)
backend/tests/mocks/authn/otpmock/OTPAuthnServiceInterface_mock.gois excluded by!**/*_mock.gobackend/tests/mocks/notification/notificationmock/OTPServiceInterface_mock.gois excluded by!**/*_mock.go
📒 Files selected for processing (17)
backend/internal/authn/otp/error_constants.gobackend/internal/authn/otp/service.gobackend/internal/authn/otp/service_test.gobackend/internal/authn/service.gobackend/internal/authn/service_test.gobackend/internal/flow/common/constants.gobackend/internal/flow/executor/constants.gobackend/internal/flow/executor/error_constants.gobackend/internal/flow/executor/otp_executor.gobackend/internal/flow/executor/otp_executor_test.gobackend/internal/notification/OTPServiceInterface_mock_test.gobackend/internal/notification/error_constants.gobackend/internal/notification/otp_service.gobackend/internal/notification/otp_service_test.gobackend/internal/system/config/config.gobackend/internal/system/config/config_test.gobackend/internal/system/i18n/core/defaults.go
💤 Files with no reviewable changes (3)
- backend/internal/flow/executor/constants.go
- backend/internal/flow/common/constants.go
- backend/internal/flow/executor/error_constants.go
kavix
force-pushed
the
centralize-otp-attempt
branch
from
July 24, 2026 15:01
311aab5 to
5aee19e
Compare
Add priorSessionToken parameter to SendOTP flow so that OTP generation attempts are tracked across retries. The OTP service increments the attempt count from the previous session, enabling ErrorMaxOTPAttemptsExceeded to be properly enforced on resend requests. Validate recipient binding in previous session token to ensure the token belongs to the same recipient and attribute before reusing attempt count. This prevents attempt count manipulation using tokens from different recipients. Also update OTP executor test to assert exact previous session token instead of mock.Anything to verify correct token forwarding from RuntimeData. Signed-off-by: kavindu kavix
kavix
force-pushed
the
centralize-otp-attempt
branch
from
July 24, 2026 15:05
5aee19e to
5357836
Compare
Add priorSessionToken parameter to SendOTP flow so that OTP generation attempts are tracked across retries. The OTP service increments the attempt count from the previous session, enabling ErrorMaxOTPAttemptsExceeded to be properly enforced on resend requests. Validate recipient binding in previous session token to ensure the token belongs to the same recipient and attribute before reusing attempt count. This prevents attempt count manipulation using tokens from different recipients. Document notification.otp.max_generation_attempts including default of 3, valid range of 1-10, and impact on OTP regeneration. Document session-token- based generation attempt tracking and the outcome when the limit is reached. Signed-off-by: kavindu kavix
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
backend/internal/authn/handler_test.go (1)
326-327: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the non-empty retry-token path.
The updated expectation only uses an empty
PriorSessionToken, so the test would still pass if the handler dropped or altered a real retry token. Set a non-empty token inotpRequestand assert that exact value reachesSendOTP.🤖 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/authn/handler_test.go` around lines 326 - 327, Update the test around the SendOTP expectation in the OTP handler test to assign a non-empty value to otpRequest.PriorSessionToken, then configure the mock and assertions to require that exact token in the SendOTP call. Preserve the existing session-token response and test flow.
🤖 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/handler.go`:
- Around line 96-97: Update the documentation for POST /authenticate/otp/send in
docs/content/apis.mdx or its endpoint API reference to describe the optional
priorSessionToken request field, how clients reuse the returned session token,
and the failure behavior when the maximum attempts are exceeded.
In `@backend/internal/authn/model.go`:
- Line 67: Update the PriorSessionToken handling in the OTP service to enforce
freshness before trusting its embedded AttemptCount: either consume each prior
token exactly once or compare it against a server-side latest-token/version
record for the recipient. Reject reused or non-latest tokens, while preserving
recipient binding and MaxGenerationAttempts enforcement.
In `@backend/internal/authn/service.go`:
- Around line 208-211: Update SendOTP and its request/service contract to accept
or obtain a configurable recipient attribute, then pass that value to
otpService.GenerateOTP instead of hardcoding "mobile_number". Preserve the
existing priorSessionToken forwarding and ensure callers provide the intended
attribute explicitly or through the established configuration/model.
In `@docs/content/guides/flows/advanced-configurations.mdx`:
- Line 991: Update the OTP generation attempts description and nearby property
table to remove references to the executor property maxAttempts, use the
configuration key notification.otp.max_generation_attempts, and link that
setting to the deployment configuration page.
---
Nitpick comments:
In `@backend/internal/authn/handler_test.go`:
- Around line 326-327: Update the test around the SendOTP expectation in the OTP
handler test to assign a non-empty value to otpRequest.PriorSessionToken, then
configure the mock and assertions to require that exact token in the SendOTP
call. Preserve the existing session-token response and test flow.
🪄 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: 076749a0-5714-48cc-b91e-782c03e9dd32
📒 Files selected for processing (11)
backend/internal/authn/AuthenticationServiceInterface_mock_test.gobackend/internal/authn/handler.gobackend/internal/authn/handler_test.gobackend/internal/authn/model.gobackend/internal/authn/service.gobackend/internal/authn/service_test.gobackend/internal/flow/executor/constants.gobackend/internal/flow/executor/otp_executor_test.gobackend/internal/notification/otp_service.godocs/content/deployment/configuration.mdxdocs/content/guides/flows/advanced-configurations.mdx
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/internal/authn/service_test.go
- backend/internal/notification/otp_service.go
- backend/internal/flow/executor/otp_executor_test.go
Comment on lines
96
to
+97
| sessionToken, svcErr := ah.authService.SendOTP(ctx, otpRequest.SenderID, notifcommon.ChannelTypeSMS, | ||
| otpRequest.Recipient) | ||
| otpRequest.Recipient, otpRequest.PriorSessionToken) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🔴 Documentation Required
This PR introduces user-facing changes that are not covered by documentation updates under docs/.
Please update the relevant documentation before merging.
Missing documentation:
backend/internal/authn/handler.go#L96-L97: Document the optionalpriorSessionTokenfield forPOST /authenticate/otp/send, how clients reuse the returned session token, and the max-attempt failure indocs/content/apis.mdxor the endpoint API reference.
As per path instructions, public REST request-schema changes require corresponding documentation under docs/.
🤖 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/authn/handler.go` around lines 96 - 97, Update the
documentation for POST /authenticate/otp/send in docs/content/apis.mdx or its
endpoint API reference to describe the optional priorSessionToken request field,
how clients reuse the returned session token, and the failure behavior when the
maximum attempts are exceeded.
Source: Path instructions
| Recipient string `json:"recipient" native:"required"` | ||
| SenderID string `json:"senderId" native:"required"` | ||
| Recipient string `json:"recipient" native:"required"` | ||
| PriorSessionToken string `json:"priorSessionToken,omitempty"` |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Prevent replay of older session tokens from bypassing the limit.
The new field accepts any still-valid OTP session JWT. The supplied backend/internal/notification/otp_service.go implementation verifies the token and recipient binding, then trusts its embedded AttemptCount; it does not establish that the token is the latest one. Reusing the first token therefore keeps requests at attempt 2 and bypasses MaxGenerationAttempts.
Enforce one-time use or a server-side latest-token/version check before accepting PriorSessionToken.
Based on the supplied OTP service implementation, token freshness is not enforced.
🤖 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/authn/model.go` at line 67, Update the PriorSessionToken
handling in the OTP service to enforce freshness before trusting its embedded
AttemptCount: either consume each prior token exactly once or compare it against
a server-side latest-token/version record for the recipient. Reject reused or
non-latest tokens, while preserving recipient binding and MaxGenerationAttempts
enforcement.
Comment on lines
+208
to
+211
| recipient, priorSessionToken string) (string, *tidcommon.ServiceError) { | ||
| logger := log.GetLogger().With(log.String(log.LoggerKeyComponentName, svcLoggerComponentName)) | ||
|
|
||
| sessionToken, otpValue, _, svcErr := as.otpService.GenerateOTP(ctx, recipient, "mobile_number") | ||
| sessionToken, otpValue, _, svcErr := as.otpService.GenerateOTP(ctx, recipient, "mobile_number", priorSessionToken) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Keep the recipient attribute configurable.
SendOTP now forwards the prior token, so recipientAttr participates in session-token binding, but this service still hardcodes "mobile_number". Add the recipient attribute to the request/service contract or source it from explicit configuration or a model.
Based on learnings, atomic OTP services must handle recipient attributes generically instead of hardcoding mobile_number in the service layer.
🤖 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/authn/service.go` around lines 208 - 211, Update SendOTP and
its request/service contract to accept or obtain a configurable recipient
attribute, then pass that value to otpService.GenerateOTP instead of hardcoding
"mobile_number". Preserve the existing priorSessionToken forwarding and ensure
callers provide the intended attribute explicitly or through the established
configuration/model.
Source: Learnings
| - User not found for the provided phone number | ||
| - User not identified and no identifying input declared | ||
| - Maximum OTP generation attempts exceeded (default: 3) | ||
| - Maximum OTP generation attempts exceeded: The executor tracks attempts per session using the session token. Each Generate OTP call without a prior session token starts fresh. When a prior session token is provided (via `runtimeData.otpSessionToken`), the server validates the recipient binding and increments the attempt count. If the count exceeds `maxAttempts` (default: 3), the flow fails. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Replace the removed executor property with the configuration key.
maxAttempts is no longer an executor property. Runtime enforcement reads notification.otp.max_generation_attempts, so this text and the nearby property table should be updated to reference the configuration setting and link to the deployment configuration page.
As per path instructions, technical claims in documentation must match the implementation.
🤖 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 `@docs/content/guides/flows/advanced-configurations.mdx` at line 991, Update
the OTP generation attempts description and nearby property table to remove
references to the executor property maxAttempts, use the configuration key
notification.otp.max_generation_attempts, and link that setting to the
deployment configuration page.
Source: Path instructions
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
Centralize OTP attempt-count validation and enforcement into the core OTP service (
notification/otp_service.go), ensuring both flow-based executions (OTPExecutor) and direct notification service calls share unified attempt limits backed by the JWT session token.Approach
MaxGenerationAttemptsconfiguration parameter toOTPConfigin server config (default 3, range [1, 10]).AttemptCountinotpSessionDataJWT claims struct.OTPServiceInterface.GenerateOTPandOTPAuthnServiceInterface.GenerateOTPto acceptpreviousSessionToken.previousSessionTokenis supplied,GenerateOTPdecodes it, incrementsAttemptCount, and returnsErrorMaxOTPAttemptsExceededwhen max generation attempts are reached.validateAttempts,getMaxOTPAttempts),propertyKeyMaxOTPAttempts, andRuntimeKeyOTPAttemptCountfromotp_executor.goandconstants.go.notification,authn/otp,authn, andflow/executor.Related Issues
Related PRs
Checklist
Security checks
Summary by CodeRabbit
notification.otp.max_generation_attemptsto limit OTP generation attempts per session (default 3; range 1–10).priorSessionTokenin OTP sending so attempt counting can continue across requests.