[MS-1214] Validate that Confirmation GUID was in Identification results

[BurningAXE]

JIRA ticket
Will be released in: 2025.3.1

Root cause analysis (for bugfixes only)

First known affected version: since forever

• Currently we don't check if the Confirmation GUID was in the Identification results which allows the data collector to pass any arbitrary GUID.

Notable changes

• Added a validation check that verifies the selected GUID was in the Identification callback list

Testing guidance

• Make an Identification and Confirm one of the returned GUIDs - should succeed
• Then try to Confirm a GUID that's not in the list - should not succeed

Additional work checklist

• Effect on other features and security has been considered
• Design document marked as "In development" (if applicable)
• External (Gitbook) and internal (Confluence) Documentation is up to date (or ticket created)
• Test cases in Testiny are up to date (or ticket created)
• Other teams notified about the changes (if applicable)

[Copilot]

Pull Request Overview

This PR adds validation to ensure that the GUID passed in a Confirmation request was actually present in the previous Identification results. This prevents data collectors from submitting arbitrary GUIDs that weren't part of the match results.

Key changes:

• Added validation logic to check that the selected GUID exists in the Identification callback event's scores
• Modified validators to be suspending functions to support async event repository operations
• Enhanced test coverage with new test cases for valid and invalid GUID scenarios

Reviewed Changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
ConfirmIdentityValidator.kt	Added validation logic to verify selected GUID exists in identification results using EventRepository
ConfirmIdentityValidatorTest.kt	Added comprehensive test cases for GUID validation scenarios
RequestActionValidator.kt	Changed validate() to suspend function to support async operations
VerifyValidator.kt	Updated to use suspend validate() function
EnrolLastBiometricsValidator.kt	Updated to use suspend validate() function
ActionRequestBuilder.kt	Changed build() to suspend function to call suspend validate()
IntentToActionMapper.kt	Added EventRepository dependency and passed to ConfirmIdentityValidator
ConfirmIdentityActionFactory.kt	Updated to create mock EventRepository with valid identification callback for tests
IntentToActionMapperTest.kt	Added EventRepository mock setup for test infrastructure
Various test files	Updated test functions to use runTest for suspending function calls

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The method name suggests it returns a single event, but .lastOrNull() implies there could be multiple IdentificationCallbackEvents in the session. Consider adding a comment explaining why the last event is selected, or rename the method to getLatestIdentificationCallbackEvent() to make the intent clearer.

[luhmirin-s]

Approving since this is technically a valid solution and I don't want to stall the hotfix/release flow.

But, IMO, in the long term, this check should be moved to a more appropriate place.

[luhmirin-s]

I am not a big fan of performing database queries in the validators, as it occurs in a somewhat nebulous UI state between screen loadings and is supposed to be a quick sanity check to ensure all required fields in the action are present.

And this is more of a business logic question rather than a parameter validity check. IMO, it would be better to simply check the presence of the selected GUID here and do the contains() check in the appropriate module down the line (i.e. SelectSubjectViewModel).

[alexandr-simprints]

Agree with Sergej's statement above. While fine as a hot fix, this check is definitely a piece of a business logic, and should be invoked within the targeted feature/module (Confirm Identity in this case).

@BurningAXE can we put a ticket in backlog to refactor this solution in future?

[BurningAXE]

Hm, I kind of agree with the general idea to keep validations short and quick and put business logic in a VM/use case. However, I also have two counter arguments:

1. We already do a DB call in sessionHasIdentificationCallback which checks for the presence of an IdentificationCallback. So, how is checking the contents of the IdentificationCallback any different - both in performance terms (how long it takes) and in terms of architecture (where we place this business logic)?
2. In general I'd prefer all the logic to be in one place so it's easier to reason about. If you want to understand why a particular Confirmation failed, it will be much easier if you need to go to just one file in stead of gathering scattered pieces of logic all around the codebase.

[luhmirin-s]

Hmm, sessionHasIdentificationCallback is definitely a weird and I forgot about it, if we are already doing a similar thing, then at least we should do them consistently - either have this check in a use case and provide a flag to the validator (as was done before) or move the identification callback use case into the validator directly to have it all in one place.

[BurningAXE]

Yeah, I didn't like the fact that with this implementation we'll read the IdentificationCallback twice. I guess the use case was extracted because it's used in two places (Confirmation and Enrol Last). I didn't extract the GUID validation because it happens in only one place and extracting it in a use case seems not worth it. But given that sessionHasIdentificationCallback is just a single line, I don't see an issue to duplicate it in both Confirmation and Enrol Last. This will reduce the DB reads from 2 to 1. What do you think?

[Copilot]

Pull Request Overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Using mutable state (var) in a validator class can lead to subtle bugs if the validator is reused. Consider passing identificationEvent as a parameter to validateSelectedGuid instead of storing it as mutable state. This would make the code more functional and thread-safe.

[Copilot]

The test uses a hardcoded string \"allowConfirmingGuidsNotInCallback\" instead of the constant ALLOW_CONFIRMING_GUIDS_NOT_IN_CALLBACK defined in ExperimentalProjectConfiguration. Use the constant to prevent issues if the key name changes: ExperimentalProjectConfiguration.ALLOW_CONFIRMING_GUIDS_NOT_IN_CALLBACK.

[Copilot]

The test uses a hardcoded string \"allowConfirmingGuidsNotInCallback\" instead of the constant ALLOW_CONFIRMING_GUIDS_NOT_IN_CALLBACK defined in ExperimentalProjectConfiguration. Use the constant to prevent issues if the key name changes: ExperimentalProjectConfiguration.ALLOW_CONFIRMING_GUIDS_NOT_IN_CALLBACK.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
80.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud