MX-239: add confirmation flow for client-user endpoint#141
Merged
IOhacker merged 1 commit intoopenMF:developfrom Apr 13, 2026
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 42 minutes and 52 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThe PR refactors the self-service enrollment flow from single-step to two-step: Changes
Sequence DiagramsequenceDiagram
participant Client
participant API as SelfServiceRegistrationApiResource
participant Service as SelfServiceRegistrationWritePlatformService
participant DB as Database
participant Email as Email/SMS Service
Client->>API: POST /client-user (enrollment request)
API->>Service: selfEnroll(request)
Service->>DB: Create Client
Service->>DB: Create disabled AppSelfServiceUser
Service->>DB: Persist SelfServiceRegistration with token
Service->>Email: Deliver enrollment token
Note over Email: Failures logged & swallowed
Service-->>API: Return SelfServiceRegistration
API-->>Client: 200 (success message)
Client->>Client: Retrieve token from email/SMS
Client->>API: POST /client-user/confirm (token)
API->>Service: confirmEnrollment(request)
Service->>DB: Resolve SelfServiceRegistration by token
Service->>DB: Validate token state & expiry
Service->>DB: Enable AppSelfServiceUser
Service->>DB: Mark registration consumed
Service-->>API: Return enabled AppSelfServiceUser
API-->>Client: 200 (user ID)
Client->>Client: Authenticate with credentials
Client-->>API: Credentials now accepted (user enabled)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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)
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 |
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/test/java/org/apache/fineract/selfservice/registration/api/SelfServiceEnrollmentIntegrationTest.java (1)
75-82: Avoid formattingusernameinto the SQL text.Lines 79-81 still build the query with
String.format(...). Please route this through a parameterized helper so the test follows the same SQL-safety rule as the rest of the codebase.As per coding guidelines, "Always use prepared statements, parameterized queries, or Spring Data repositories."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/apache/fineract/selfservice/registration/api/SelfServiceEnrollmentIntegrationTest.java` around lines 75 - 82, The queryEnrollmentToken method currently injects the username via String.format into the SQL string; change it to use a parameterized query helper (e.g., the existing querySingleValueInPostgres that supports parameters) so the SQL becomes a constant with a placeholder and the username passed as a parameter (ensure proper escaping via the prepared statement, not manual replace). Update queryEnrollmentToken to build the SQL "SELECT external_authorization_token FROM request_audit_table WHERE username = ? AND request_type = 'ENROLLMENT' ORDER BY id DESC LIMIT 1" and call the parameterized variant of querySingleValueInPostgres (or wrap it with a new helper) passing username as the parameter. Ensure you reference the existing method name queryEnrollmentToken and the helper querySingleValueInPostgres when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/org/apache/fineract/selfservice/registration/api/SelfServiceRegistrationApiResource.java`:
- Around line 89-99: Update the OpenAPI `@ApiResponse` for the selfEnroll endpoint
to accurately describe the actual response: in
SelfServiceRegistrationApiResource.selfEnroll change the 200-response
description to indicate it returns a confirmation token (e.g. "200 - OK; returns
confirmation token string") and add a content/schema entry describing a string
response (or reference java.lang.String) so the generated docs match the method
return value (which currently returns
SelfServiceApiConstants.createRequestSuccessMessage).
- Around line 107-116: The POST confirmation endpoint in
SelfServiceRegistrationApiResource (path "client-user/confirm") lacks a
`@RequestBody` schema so OpenAPI clients can't tell whether to send
externalAuthenticationToken or the legacy requestId/authenticationToken pair;
add a documented request model (e.g., ConfirmSelfEnrollmentRequest DTO) or
inline `@RequestBody` with `@Schema` describing fields externalAuthenticationToken
(string) and legacy requestId and authenticationToken (strings), specify
required/oneOf semantics (externalAuthenticationToken OR the legacy pair),
annotate the controller method parameter (e.g.,
confirmEnrollment(ConfirmSelfEnrollmentRequest request)) with `@RequestBody` and
example values, and update the endpoint's Swagger annotations so the generated
OpenAPI spec clearly documents which payload to send.
In
`@src/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImpl.java`:
- Around line 485-488: Concurrent confirmation races can cause flush-time
optimistic locking exceptions and surface as 500s; wrap the critical section
around appUser.enable(), appSelfServiceUserRepository.saveAndFlush(appUser),
request.markConsumed(), and
selfServiceRegistrationRepository.saveAndFlush(request) in a try/catch that
catches the JPA/ Spring optimistic locking exception (e.g.,
OptimisticLockingFailureException / ObjectOptimisticLockingFailureException) and
translate it into the documented invalid/consumed-token response by throwing the
same domain/ API exception used for expired/consumed tokens in
SelfServiceRegistrationWritePlatformServiceImpl (or returning that error); this
ensures concurrent token replays map to the expected invalid-token result
instead of a 500.
- Around line 446-450: Remove logging of raw usernames in
SelfServiceRegistrationWritePlatformServiceImpl: update the log.error call that
currently logs username (failing delivery of enrollment confirmation token) and
the log.info call ("Self-enrollment created") and the other log at the
referenced location to avoid emitting PII. Replace the username field in those
messages with only the internal id (newClientId) or a one-way hashed identifier
(e.g., SHA-256 of username) if correlation is required, and keep the exception e
in the error log as before; ensure the changes are applied to the error and info
logging statements in the method handling self-enrollment delivery/creation so
no raw usernames are written to logs.
In
`@src/test/java/org/apache/fineract/selfservice/security/api/SelfForgotPasswordApiResourceIntegrationTest.java`:
- Around line 55-59: The test in SelfForgotPasswordApiResourceIntegrationTest
uses querySingleValue(...) to fetch enrollmentToken but only
assertsNotNull(enrollmentToken), which misses empty-string results; update the
assertion to ensure enrollmentToken is non-blank (e.g., replace or add an
assertion after retrieving enrollmentToken that checks it is not empty/blank
such as assertTrue(!enrollmentToken.isBlank()) or
assertFalse(enrollmentToken.trim().isEmpty())) so a missing-token query fails
the test immediately.
---
Nitpick comments:
In
`@src/test/java/org/apache/fineract/selfservice/registration/api/SelfServiceEnrollmentIntegrationTest.java`:
- Around line 75-82: The queryEnrollmentToken method currently injects the
username via String.format into the SQL string; change it to use a parameterized
query helper (e.g., the existing querySingleValueInPostgres that supports
parameters) so the SQL becomes a constant with a placeholder and the username
passed as a parameter (ensure proper escaping via the prepared statement, not
manual replace). Update queryEnrollmentToken to build the SQL "SELECT
external_authorization_token FROM request_audit_table WHERE username = ? AND
request_type = 'ENROLLMENT' ORDER BY id DESC LIMIT 1" and call the parameterized
variant of querySingleValueInPostgres (or wrap it with a new helper) passing
username as the parameter. Ensure you reference the existing method name
queryEnrollmentToken and the helper querySingleValueInPostgres when making the
change.
🪄 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
Run ID: 83a3609b-ad29-4487-a5a2-806662a0de83
📒 Files selected for processing (8)
src/main/java/org/apache/fineract/selfservice/registration/api/SelfServiceRegistrationApiResource.javasrc/main/java/org/apache/fineract/selfservice/registration/domain/SelfServiceRequestType.javasrc/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformService.javasrc/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImpl.javasrc/main/java/org/apache/fineract/selfservice/security/starter/SelfServiceSecurityConfiguration.javasrc/main/java/org/apache/fineract/selfservice/useradministration/domain/AppSelfServiceUser.javasrc/test/java/org/apache/fineract/selfservice/registration/api/SelfServiceEnrollmentIntegrationTest.javasrc/test/java/org/apache/fineract/selfservice/security/api/SelfForgotPasswordApiResourceIntegrationTest.java
Comment on lines
+89
to
+99
| @Operation(summary = "Self Enrollment Flow", description = "Creates a Fineract Client and a disabled Self Service User. Returns a confirmation token that must be validated to activate the user.") | ||
| @RequestBody(required = true, content = @Content(schema = @Schema(implementation = SelfServiceEnrollmentRequest.class))) | ||
| @ApiResponses({ | ||
| @ApiResponse(responseCode = "200", description = "OK"), | ||
| @ApiResponse(responseCode = "400", description = "Bad Request"), | ||
| @ApiResponse(responseCode = "409", description = "Conflict (Duplicate Username, Email, etc)") | ||
| }) | ||
| public String selfEnroll(final String apiRequestBodyAsJson) { | ||
| AppSelfServiceUser user = this.selfServiceRegistrationWritePlatformService.selfEnroll(apiRequestBodyAsJson); | ||
| this.selfServiceRegistrationWritePlatformService.selfEnroll(apiRequestBodyAsJson); | ||
| return SelfServiceApiConstants.createRequestSuccessMessage; | ||
| } |
There was a problem hiding this comment.
Fix the OpenAPI response description for self-enrollment.
Lines 96-99 return only SelfServiceApiConstants.createRequestSuccessMessage, but Line 89 says the endpoint returns a confirmation token. That will generate incorrect API docs for clients.
As per coding guidelines, "Maintain up-to-date API documentation via SpringDoc (OpenAPI 3) or REST Docs."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/apache/fineract/selfservice/registration/api/SelfServiceRegistrationApiResource.java`
around lines 89 - 99, Update the OpenAPI `@ApiResponse` for the selfEnroll
endpoint to accurately describe the actual response: in
SelfServiceRegistrationApiResource.selfEnroll change the 200-response
description to indicate it returns a confirmation token (e.g. "200 - OK; returns
confirmation token string") and add a content/schema entry describing a string
response (or reference java.lang.String) so the generated docs match the method
return value (which currently returns
SelfServiceApiConstants.createRequestSuccessMessage).
Comment on lines
+107
to
+116
| @POST | ||
| @Path("client-user/confirm") | ||
| @Consumes({MediaType.APPLICATION_JSON}) | ||
| @Produces({MediaType.APPLICATION_JSON}) | ||
| @Operation(summary = "Confirm Self Enrollment", description = "Validates the enrollment token and activates the self-service user.") | ||
| @ApiResponses({ | ||
| @ApiResponse(responseCode = "200", description = "OK"), | ||
| @ApiResponse(responseCode = "403", description = "Token expired or already consumed"), | ||
| @ApiResponse(responseCode = "404", description = "Token not found") | ||
| }) |
There was a problem hiding this comment.
Document the confirmation request payload.
The new public confirmation endpoint has response codes, but no @RequestBody schema for the token fields. The OpenAPI spec will not tell clients whether to send externalAuthenticationToken or the legacy requestId/authenticationToken pair.
As per coding guidelines, "Maintain up-to-date API documentation via SpringDoc (OpenAPI 3) or REST Docs."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/apache/fineract/selfservice/registration/api/SelfServiceRegistrationApiResource.java`
around lines 107 - 116, The POST confirmation endpoint in
SelfServiceRegistrationApiResource (path "client-user/confirm") lacks a
`@RequestBody` schema so OpenAPI clients can't tell whether to send
externalAuthenticationToken or the legacy requestId/authenticationToken pair;
add a documented request model (e.g., ConfirmSelfEnrollmentRequest DTO) or
inline `@RequestBody` with `@Schema` describing fields externalAuthenticationToken
(string) and legacy requestId and authenticationToken (strings), specify
required/oneOf semantics (externalAuthenticationToken OR the legacy pair),
annotate the controller method parameter (e.g.,
confirmEnrollment(ConfirmSelfEnrollmentRequest request)) with `@RequestBody` and
example values, and update the endpoint's Swagger annotations so the generated
OpenAPI spec clearly documents which payload to send.
- selfEnroll creates client + disabled user (enabled=false) - Enrollment token stored in request_audit_table with type ENROLLMENT - New POST /self/registration/client-user/confirm endpoint validates token and enables the user via AppSelfServiceUser.enable() - User cannot login until enrollment is confirmed - Token replay protection via markConsumed() + optimistic locking - Updated enrollment + forgot-password tests for 2-step flow - Wrapped token delivery in try-catch (swallowed for audit/retry) - Added BEGIN/COMMIT transaction wrapper to executeSqlInPostgres() All 30 tests pass (0 failures, 0 errors).
DeathGun44
force-pushed
the
MX-239-add-enrollment-confirmation-flow
branch
from
1d2cfcc to
4916c6a
Compare
IOhacker
approved these changes
Apr 13, 2026
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
2 participants
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.
Summary by CodeRabbit
Release Notes