refactor(multirespondent): make MRF Singpass/MyInfo validation step-aware#9537
Open
kimyungju wants to merge 1 commit into
Open
refactor(multirespondent): make MRF Singpass/MyInfo validation step-aware#9537kimyungju wants to merge 1 commit into
kimyungju wants to merge 1 commit into
Conversation
…ware The decision "must SPCP/MyInfo be re-verified on this MRF step?" was hardcoded as step === 1 (and "skip if previousSubmissionId") in two independent backend locations, kept in sync by hand. Extract it into a single shared, tested helper isSingpassEnforcedOnStep(form, stepNumber) consumed by both _handleGenerateOtp and handleNdiResponses, and remove the two stale TODOs. Behaviour-preserving: with no per-step Singpass config in the schema, the helper returns true only for step 1 of an auth-enabled form, so every form configuration that exists today validates exactly as before. The helper centralises the step rule only; each call site keeps its own authType handling, which legitimately differs. Enabling Singpass on step 2+ (schema plus enforcement) is a separate follow-up; the helper's JSDoc documents what it must wire (including the controller's step-2+ path) so it does not silently fail open.
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR centralises the “Singpass enforcement by MRF step” gating into a shared helper and reuses it in both the OTP generation controller and the MRF NDI middleware, with unit tests added for the new helper.
Changes:
- Introduced
isSingpassEnforcedOnStephelper to encapsulate current “step 1 only” Singpass enforcement logic. - Updated OTP generation flow and NDI response middleware to use the shared step gate.
- Added unit tests covering
isSingpassEnforcedOnStep.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| apps/backend/src/app/modules/verification/verification.controller.ts | Uses the new helper to gate auth verification in the “no previousSubmissionId” path. |
| apps/backend/src/app/modules/submission/multirespondent-submission/multirespondent-submission.utils.ts | Adds isSingpassEnforcedOnStep helper and documentation. |
| apps/backend/src/app/modules/submission/multirespondent-submission/multirespondent-submission.middleware.ts | Replaces inline stepNumber === 1 gate with isSingpassEnforcedOnStep. |
| apps/backend/src/app/modules/submission/multirespondent-submission/tests/multirespondent-submission.utils.spec.ts | Adds tests for isSingpassEnforcedOnStep. |
Comment on lines
+586
to
+589
| export const isSingpassEnforcedOnStep = ( | ||
| form: { authType: FormAuthType }, | ||
| stepNumber: number, | ||
| ): boolean => form.authType !== FormAuthType.NIL && stepNumber === 1 |
Comment on lines
201
to
+204
| setFormTags(form) | ||
| if (!isSingpassEnforcedOnStep(form, 1)) { | ||
| return okAsync(form) | ||
| } |
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
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.
Problem
The MRF Singpass/MyInfo (SPCP) re-verification decision is hardcoded as a
step === 1assumption in two independent backend locations:verification.controller.ts:_handleGenerateOtptreats the presence ofpreviousSubmissionId(any step 2+ request) as a signal to skip SPCP/MyInfo.multirespondent-submission.middleware.ts:handleNdiResponsesgates verified-content generation behindstepNumber === 1.Both carried
// TODOcomments pointing at the same future fix. The behaviour is correct for how MRF forms are configured today; the issue is maintainability. The same rule lives in two hand-synced places rather than one named location.Closes #9513
Solution
Extract the duplicated step rule into a single shared, tested helper
isSingpassEnforcedOnStep(form, stepNumber)and have both call sites consume it. This is a behaviour-preserving refactor: for every form configuration that exists today, the validation outcome is unchanged.Scope is intentionally limited to centralising the step rule. There is no per-step Singpass config in the form schema today, so the helper reads
authTypeand the step number only (authType !== NIL && stepNumber === 1); it does not unify the per-authType policy, which legitimately differs between the two call sites. Enabling Singpass on step 2+ (schema plus enforcement) is left as a follow-up rather than widening this PR, per the issue's Non-goals. The helper's JSDoc documents what that follow-up must wire (including the controller's step-2+ path) so it does not silently fail open.Breaking Changes
Improvements:
isSingpassEnforcedOnStep(form, stepNumber)helper inmultirespondent-submission.utils.tsas the single source of truth for the step rule, with unit tests._handleGenerateOtpandhandleNdiResponsesnow call the helper instead of inliningstep === 1/previousSubmissionIdassumptions.// TODOcomments; replace with a documented extension point for the per-step-config follow-up.Tests
pnpm --filter formsg-backend test -- multirespondent-submission.utils.spec: new unit tests for the helper (everyauthTypeon step 1 is enforced,NILis not, steps 0/2/3/10 are not).pnpm --filter formsg-backend test -- verification.controller.spec: existing_handleGenerateOtpsuite stays green (no behaviour change for SPCP-on-step-1 and no-SPCP forms).pnpm --filter formsg-backend test -- multirespondent-submission.middleware.spec: existinghandleNdiResponsessuite stays green (step-1 enforce, step-2+ skip, NIL no-op).Deploy Notes
No new environment variables, scripts, or dependencies.