[MOSIP] SDK-based biometric quality evaluation with SBI fallback#784
Open
Hendrixx-RE wants to merge 1 commit into
Open
[MOSIP] SDK-based biometric quality evaluation with SBI fallback#784Hendrixx-RE wants to merge 1 commit into
Hendrixx-RE wants to merge 1 commit into
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughThis PR adds SDK quality evaluation capability to the MOSIP registration client. New audit events, configuration constants, and exception types support quality assessment. A new QualityEvaluationResult DTO encapsulates evaluation outcomes. BioServiceImpl refactored with async ExecutorService to evaluate quality via SDK with configurable timeout, falling back to SBI when SDK is disabled or unbound. ChangesSDK Quality Evaluation Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 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. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Introduces evaluateQuality() orchestrator in BioServiceImpl that routes quality scoring through the SDK when available, falling back to SBI when the provider is unbound, and blocking on configured SDK failures (timeout, null response, runtime exception). Fixes getCapturedBiometrics() which previously used SBI qualityScore for threshold gating even when sdkScore was populated. Changes: - BioServiceImpl: add evaluateQuality(), getSdkTimeoutMs(), sdkExecutor; refactor captureModality() to use evaluateQuality(); fix threshold gating in getCapturedBiometrics() to use effective score (SDK > SBI) - QualityEvaluationResult: new DTO carrying score, source, and fallback flag - BiometricsDto: add qualitySource field to track score origin end-to-end - RegistrationExceptionConstants: add REG-BQC-003 through REG-BQC-007 - RegistrationConstants: add SDK_QUALITY_TIMEOUT config key - AuditEvent: add SDK_QUALITY_SUCCESS/FAILED/FALLBACK/DISABLED and NO_VALID_QUALITY_SCORE Signed-off-by: Hendrixx-RE <athukuttan2004@gmail.com>
Hendrixx-RE
force-pushed
the
feature/sdk-biometric-quality-evaluation
branch
from
3984ff3 to
e0b3bda
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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
`@registration/registration-services/src/main/java/io/mosip/registration/dto/biometric/QualityEvaluationResult.java`:
- Line 10: The field "source" in QualityEvaluationResult is a raw String and
should be replaced with a type-safe enum (e.g., SourceType { SDK, SBI }) or
shared constants; add a new enum SourceType (or constants class), change
QualityEvaluationResult.source from String to SourceType and update its
constructor, getter, equals/hashCode/serialization annotations if present, and
then update all call sites in BioServiceImpl to pass SourceType.SDK or
SourceType.SBI instead of the string literals "SDK"/"SBI"; ensure any JSON/XML
mappers are adjusted to serialize the enum as the expected string if needed.
In
`@registration/registration-services/src/main/java/io/mosip/registration/exception/RegistrationExceptionConstants.java`:
- Line 98: The REG_NO_QUALITY_SOURCE enum constant in
RegistrationExceptionConstants is unused; remove the
REG_NO_QUALITY_SOURCE("REG-BQC-007", "Biometric quality evaluation is not
configured. Registration cannot proceed.") entry from the
RegistrationExceptionConstants definition (adjust surrounding commas/formatting
so the enum remains valid) and run a build/tests to ensure no references remain;
if you intended to use it instead, implement the exception throw/path that
references REG_NO_QUALITY_SOURCE in the biometric quality evaluation code rather
than leaving the dead constant.
In
`@registration/registration-services/src/main/java/io/mosip/registration/service/bio/impl/BioServiceImpl.java`:
- Line 73: BioServiceImpl creates a non-daemon ExecutorService (sdkExecutor)
that is never shut down, causing a thread/resource leak; add a lifecycle cleanup
method on the BioServiceImpl class annotated with `@PreDestroy` that shuts down
sdkExecutor (call shutdown(), optionally shutdownNow() on timeout, and
awaitTermination with a short timeout) and handle InterruptedException by
restoring the interrupt flag and logging; ensure the method references the
sdkExecutor field so the executor is properly terminated on Spring context
shutdown.
- Line 73: The current single-thread ExecutorService sdkExecutor (created via
Executors.newSingleThreadExecutor()) can be permanently blocked by an
unresponsive SDK call from evaluateQuality(); change the executor strategy so a
stuck SDK call won’t block all future evaluateQuality() invocations — e.g.,
replace sdkExecutor with a pooled or per-call executor such as
Executors.newCachedThreadPool() or create a fresh single-thread executor per
evaluateQuality() invocation, and limit growth using a bounded ThreadFactory,
Semaphore, or capped thread pool to prevent unbounded thread creation; update
usages that submit/ cancel tasks to use the new executor and ensure proper
shutdown of per-call executors if you choose that approach.
- Around line 235-255: The catch-all Exception block in BioServiceImpl is
swallowing InterruptedException from future.get(timeoutMs, MILLISECONDS) and
clearing the thread interrupt status; add an explicit catch
(InterruptedException e) immediately before the general Exception catch, restore
the interrupt flag with Thread.currentThread().interrupt(), log context (e.g.,
"Thread interrupted during SDK quality evaluation for {}" with
biometricsDto.getBioAttribute()), and rethrow or wrap as a
RegBaseCheckedException consistent with other handlers so interruption is not
lost.
- Around line 186-222: The code calls bioAPIFactory.getBioProvider(...) twice
(stored in a raw Object first and called again inside the lambda), causing a
TOCTOU and discarding the first result; fix by capturing the provider once into
a properly typed variable (replace Object provider with the concrete provider
interface returned by getBioProvider, e.g., final IYourProviderType
finalProvider = bioAPIFactory.getBioProvider(biometricType,
BiometricFunction.QUALITY_CHECK)), keep the existing null-check and SBI
fallback, and then use finalProvider inside the Future lambda instead of calling
bioAPIFactory.getBioProvider(...) again so the same provider instance is used
and the redundant allocation/call is removed.
- Around line 163-266: The evaluateQuality(...) method currently logs decisions
but never emits audit events; add an `@Autowired` AuditManagerService (or existing
auditFactory) field to the class and call auditFactory.audit(...) with the
appropriate AuditEvent at each branch: on SDK disabled emit
AuditEvent.SDK_QUALITY_DISABLED in the early !sdkEnabled branch; when provider
is null emit AuditEvent.SDK_FALLBACK_TO_SBI before falling back to SBI; on
successful SDK score return emit AuditEvent.SDK_QUALITY_SUCCESS (include score
and biometric attribute); and in timeout/null/exception/ExecutionException paths
emit AuditEvent.SDK_QUALITY_FAILED (include error details and biometric
attribute); follow the same audit payload pattern used in
LoginServiceImpl/PacketHandlerServiceImpl to construct event data.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cd167bf9-2819-4bc3-a68f-21988834ee83
📒 Files selected for processing (6)
registration/registration-services/src/main/java/io/mosip/registration/constants/AuditEvent.javaregistration/registration-services/src/main/java/io/mosip/registration/constants/RegistrationConstants.javaregistration/registration-services/src/main/java/io/mosip/registration/dto/biometric/QualityEvaluationResult.javaregistration/registration-services/src/main/java/io/mosip/registration/dto/packetmanager/BiometricsDto.javaregistration/registration-services/src/main/java/io/mosip/registration/exception/RegistrationExceptionConstants.javaregistration/registration-services/src/main/java/io/mosip/registration/service/bio/impl/BioServiceImpl.java
| public class QualityEvaluationResult { | ||
|
|
||
| private final double score; | ||
| private final String source; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
source should be an enum or constant — raw String literals "SDK" / "SBI" are not type-safe.
BioServiceImpl hardcodes "SDK" and "SBI" at call sites. A new source type, a typo, or a refactor won't be caught at compile time. Introduce a small enum or reuse constants:
♻️ Proposed refactor
+public enum QualitySource { SDK, SBI }
+
public class QualityEvaluationResult {
private final double score;
- private final String source;
+ private final QualitySource source;
private final boolean fallback;
- public QualityEvaluationResult(double score, String source, boolean fallback) {
+ public QualityEvaluationResult(double score, QualitySource source, boolean fallback) {Then in BioServiceImpl:
-return new QualityEvaluationResult(biometricsDto.getQualityScore(), "SBI", false);
+return new QualityEvaluationResult(biometricsDto.getQualityScore(), QualitySource.SBI, false);🤖 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
`@registration/registration-services/src/main/java/io/mosip/registration/dto/biometric/QualityEvaluationResult.java`
at line 10, The field "source" in QualityEvaluationResult is a raw String and
should be replaced with a type-safe enum (e.g., SourceType { SDK, SBI }) or
shared constants; add a new enum SourceType (or constants class), change
QualityEvaluationResult.source from String to SourceType and update its
constructor, getter, equals/hashCode/serialization annotations if present, and
then update all call sites in BioServiceImpl to pass SourceType.SDK or
SourceType.SBI instead of the string literals "SDK"/"SBI"; ensure any JSON/XML
mappers are adjusted to serialize the enum as the expected string if needed.
| REG_SDK_NULL_RESPONSE("REG-BQC-004", "Invalid biometric quality score received from SDK."), | ||
| REG_SDK_RUNTIME_ERROR("REG-BQC-005", "Error occurred during SDK-based quality evaluation. Please retry capture."), | ||
| REG_SBI_SCORE_MISSING("REG-BQC-006", "Default biometric quality score unavailable."), | ||
| REG_NO_QUALITY_SOURCE("REG-BQC-007", "Biometric quality evaluation is not configured. Registration cannot proceed."), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify REG_NO_QUALITY_SOURCE is never referenced outside its declaration
# Expect: only the declaration line in RegistrationExceptionConstants.java
rg -n --type=java "REG_NO_QUALITY_SOURCE"Repository: mosip/registration-client
Length of output: 308
Remove the unused REG_NO_QUALITY_SOURCE constant.
The constant REG_NO_QUALITY_SOURCE (line 98) is defined but never thrown or referenced anywhere in the codebase. Either remove it or implement the exception handling path it was intended for.
🤖 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
`@registration/registration-services/src/main/java/io/mosip/registration/exception/RegistrationExceptionConstants.java`
at line 98, The REG_NO_QUALITY_SOURCE enum constant in
RegistrationExceptionConstants is unused; remove the
REG_NO_QUALITY_SOURCE("REG-BQC-007", "Biometric quality evaluation is not
configured. Registration cannot proceed.") entry from the
RegistrationExceptionConstants definition (adjust surrounding commas/formatting
so the enum remains valid) and run a build/tests to ensure no references remain;
if you intended to use it instead, implement the exception throw/path that
references REG_NO_QUALITY_SOURCE in the biometric quality evaluation code rather
than leaving the dead constant.
| @Autowired | ||
| private BIRBuilder birBuilder; | ||
|
|
||
| private final ExecutorService sdkExecutor = Executors.newSingleThreadExecutor(); |
There was a problem hiding this comment.
sdkExecutor is never shut down — resource and thread leak in the Spring singleton.
Executors.newSingleThreadExecutor() creates a non-daemon thread. As a @Service singleton, BioServiceImpl lives for the application lifetime, but the executor is never torn down. On Spring context shutdown the thread stays alive, preventing clean JVM exit (critical for a JavaFX desktop app that expects clean process termination).
🛡️ Proposed fix — add `@PreDestroy`
+import javax.annotation.PreDestroy;
`@Service`
public class BioServiceImpl extends BaseService implements BioService {
private final ExecutorService sdkExecutor = Executors.newSingleThreadExecutor();
+
+ `@PreDestroy`
+ public void shutdownSdkExecutor() {
+ sdkExecutor.shutdownNow();
+ }As per coding guidelines: "Flag use of ThreadLocal without guaranteed removal (memory leak risk)" — the same principle applies to any managed resource that holds threads.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private final ExecutorService sdkExecutor = Executors.newSingleThreadExecutor(); | |
| import javax.annotation.PreDestroy; | |
| `@Service` | |
| public class BioServiceImpl extends BaseService implements BioService { | |
| private final ExecutorService sdkExecutor = Executors.newSingleThreadExecutor(); | |
| `@PreDestroy` | |
| public void shutdownSdkExecutor() { | |
| sdkExecutor.shutdownNow(); | |
| } |
🤖 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
`@registration/registration-services/src/main/java/io/mosip/registration/service/bio/impl/BioServiceImpl.java`
at line 73, BioServiceImpl creates a non-daemon ExecutorService (sdkExecutor)
that is never shut down, causing a thread/resource leak; add a lifecycle cleanup
method on the BioServiceImpl class annotated with `@PreDestroy` that shuts down
sdkExecutor (call shutdown(), optionally shutdownNow() on timeout, and
awaitTermination with a short timeout) and handle InterruptedException by
restoring the interrupt flag and logging; ensure the method references the
sdkExecutor field so the executor is properly terminated on Spring context
shutdown.
Single-thread executor can be permanently degraded by an unresponsive SDK.
future.cancel(true) on TimeoutException (line 236) only requests interruption of the running thread. If the SDK library does not check the interrupt flag, the thread remains blocked. Because sdkExecutor is a single-thread pool, every subsequent evaluateQuality() call enqueues behind the stuck task — all captures will time out serially until the JVM or SDK recovers. Under a misbehaving SDK this becomes a permanent throughput failure.
Consider replacing with a cached thread pool or a per-call thread, so each invocation gets a fresh thread and a stalled SDK call only affects its own biometric, not all future captures:
-private final ExecutorService sdkExecutor = Executors.newSingleThreadExecutor();
+// Each evaluation gets its own thread; stalled threads cannot block future evaluations.
+private final ExecutorService sdkExecutor = Executors.newCachedThreadPool();Note: With a cached pool, a consistently misbehaving SDK would keep spawning threads. Add a bounded semaphore or thread factory with a sensible cap if needed for a production hardening pass.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private final ExecutorService sdkExecutor = Executors.newSingleThreadExecutor(); | |
| // Each evaluation gets its own thread; stalled threads cannot block future evaluations. | |
| private final ExecutorService sdkExecutor = Executors.newCachedThreadPool(); |
🤖 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
`@registration/registration-services/src/main/java/io/mosip/registration/service/bio/impl/BioServiceImpl.java`
at line 73, The current single-thread ExecutorService sdkExecutor (created via
Executors.newSingleThreadExecutor()) can be permanently blocked by an
unresponsive SDK call from evaluateQuality(); change the executor strategy so a
stuck SDK call won’t block all future evaluateQuality() invocations — e.g.,
replace sdkExecutor with a pooled or per-call executor such as
Executors.newCachedThreadPool() or create a fresh single-thread executor per
evaluateQuality() invocation, and limit growth using a bounded ThreadFactory,
Semaphore, or capped thread pool to prevent unbounded thread creation; update
usages that submit/ cancel tasks to use the new executor and ensure proper
shutdown of per-call executors if you choose that approach.
Comment on lines
+163
to
+266
| /** | ||
| * Orchestrates biometric quality evaluation across SDK and SBI sources. | ||
| * | ||
| * Decision rules: | ||
| * - SDK disabled in config: use SBI score (AS-03) | ||
| * - SDK enabled but provider unbound: fall back to SBI score (AS-02) | ||
| * - SDK enabled, provider bound, call fails (timeout/null/exception): block, no fallback | ||
| */ | ||
| private QualityEvaluationResult evaluateQuality(BiometricsDto biometricsDto) throws RegBaseCheckedException { | ||
| boolean sdkEnabled = RegistrationConstants.ENABLE.equalsIgnoreCase( | ||
| (String) ApplicationContext.map().getOrDefault( | ||
| RegistrationConstants.QUALITY_CHECK_WITH_SDK, RegistrationConstants.DISABLE)); | ||
|
|
||
| if (!sdkEnabled) { | ||
| LOGGER.info("SDK quality evaluation disabled, using SBI score for {}", biometricsDto.getBioAttribute()); | ||
| return new QualityEvaluationResult(biometricsDto.getQualityScore(), "SBI", false); | ||
| } | ||
|
|
||
| BiometricType biometricType = BiometricType | ||
| .fromValue(io.mosip.commons.packet.constants.Biometric | ||
| .getSingleTypeByAttribute(biometricsDto.getBioAttribute()).name()); | ||
|
|
||
| // Check provider availability before committing to SDK path | ||
| Object provider = null; | ||
| try { | ||
| provider = bioAPIFactory.getBioProvider(biometricType, BiometricFunction.QUALITY_CHECK); | ||
| } catch (Exception e) { | ||
| LOGGER.warn("SDK provider not available for {}, falling back to SBI", biometricType); | ||
| } | ||
|
|
||
| if (provider == null) { | ||
| LOGGER.info("SDK provider unbound, falling back to SBI score for {}", biometricsDto.getBioAttribute()); | ||
| double sbiScore = biometricsDto.getQualityScore(); | ||
| if (sbiScore <= 0) { | ||
| LOGGER.error("SBI score also unavailable for {}", biometricsDto.getBioAttribute()); | ||
| throw new RegBaseCheckedException( | ||
| RegistrationExceptionConstants.REG_SBI_SCORE_MISSING.getErrorCode(), | ||
| RegistrationExceptionConstants.REG_SBI_SCORE_MISSING.getErrorMessage()); | ||
| } | ||
| return new QualityEvaluationResult(sbiScore, "SBI", true); | ||
| } | ||
|
|
||
| // SDK provider is bound - call with timeout. Any failure here blocks registration. | ||
| long timeoutMs = getSdkTimeoutMs(); | ||
| BIR bir = birBuilder.buildBir(biometricsDto, ProcessedLevelType.RAW); | ||
| BIR[] birList = new BIR[]{bir}; | ||
|
|
||
| Future<Double> future = sdkExecutor.submit(() -> { | ||
| try { | ||
| Map<BiometricType, Float> scoreMap = bioAPIFactory | ||
| .getBioProvider(biometricType, BiometricFunction.QUALITY_CHECK) | ||
| .getModalityQuality(birList, null); | ||
| return (scoreMap != null && scoreMap.get(biometricType) != null) | ||
| ? scoreMap.get(biometricType).doubleValue() | ||
| : null; | ||
| } catch (Exception e) { | ||
| LOGGER.error("SDK provider threw during quality check", e); | ||
| throw new RuntimeException(e); | ||
| } | ||
| }); | ||
|
|
||
| try { | ||
| Double score = future.get(timeoutMs, TimeUnit.MILLISECONDS); | ||
| if (score == null) { | ||
| LOGGER.error("SDK returned null quality score for {}", biometricsDto.getBioAttribute()); | ||
| throw new RegBaseCheckedException( | ||
| RegistrationExceptionConstants.REG_SDK_NULL_RESPONSE.getErrorCode(), | ||
| RegistrationExceptionConstants.REG_SDK_NULL_RESPONSE.getErrorMessage()); | ||
| } | ||
| LOGGER.info("SDK quality score {} received for {}", score, biometricsDto.getBioAttribute()); | ||
| return new QualityEvaluationResult(score, "SDK", false); | ||
|
|
||
| } catch (TimeoutException e) { | ||
| future.cancel(true); | ||
| LOGGER.error("SDK quality evaluation timed out after {}ms for {}", timeoutMs, biometricsDto.getBioAttribute()); | ||
| throw new RegBaseCheckedException( | ||
| RegistrationExceptionConstants.REG_SDK_TIMEOUT.getErrorCode(), | ||
| RegistrationExceptionConstants.REG_SDK_TIMEOUT.getErrorMessage()); | ||
|
|
||
| } catch (ExecutionException e) { | ||
| LOGGER.error("SDK runtime error during quality evaluation for {}", biometricsDto.getBioAttribute(), e); | ||
| throw new RegBaseCheckedException( | ||
| RegistrationExceptionConstants.REG_SDK_RUNTIME_ERROR.getErrorCode(), | ||
| RegistrationExceptionConstants.REG_SDK_RUNTIME_ERROR.getErrorMessage()); | ||
|
|
||
| } catch (RegBaseCheckedException e) { | ||
| throw e; | ||
|
|
||
| } catch (Exception e) { | ||
| LOGGER.error("Unexpected error during SDK quality evaluation", e); | ||
| throw new RegBaseCheckedException( | ||
| RegistrationExceptionConstants.REG_SDK_RUNTIME_ERROR.getErrorCode(), | ||
| RegistrationExceptionConstants.REG_SDK_RUNTIME_ERROR.getErrorMessage()); | ||
| } | ||
| } | ||
|
|
||
| private long getSdkTimeoutMs() { | ||
| String configured = getGlobalConfigValueOf(RegistrationConstants.SDK_QUALITY_TIMEOUT); | ||
| try { | ||
| return configured != null ? Long.parseLong(configured) : RegistrationConstants.SDK_QUALITY_TIMEOUT_DEFAULT_MS; | ||
| } catch (NumberFormatException e) { | ||
| return RegistrationConstants.SDK_QUALITY_TIMEOUT_DEFAULT_MS; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify whether any of the new SDK audit events are fired elsewhere,
# and confirm the audit helper method name on BaseService.
# Check for any usage of the new audit event constants
rg -n --type=java "SDK_QUALITY_SUCCESS|SDK_QUALITY_FAILED|SDK_FALLBACK_TO_SBI|SDK_QUALITY_DISABLED|NO_VALID_QUALITY_SCORE"
# Check what audit helper methods BaseService exposes
ast-grep --pattern 'class BaseService {
$$$
public $_ audit($$$) {
$$$
}
$$$
}'Repository: mosip/registration-client
Length of output: 1283
🏁 Script executed:
# Find BioServiceImpl class and its parent
rg -n --type=java "class BioServiceImpl" -A 3
# Search for audit() method calls in service classes to understand the pattern
rg -n --type=java "audit\(" | head -30
# Check if BioServiceImpl has access to auditFactory or similar
rg -n --type=java "auditFactory|AuditFactory" registration/registration-services/src/main/java/io/mosip/registration/service/bio/impl/BioServiceImpl.java
# Search for existing audit calls in BaseService or other bio service methods
fd BioServiceImpl.java --exec cat -n {} \;Repository: mosip/registration-client
Length of output: 27177
🏁 Script executed:
# Find BaseService and check for auditFactory
fd BaseService.java --exec head -100 {} \;
# Also search for how auditFactory is provided
rg -n --type=java "protected.*auditFactory|private.*auditFactory|@Autowired.*auditFactory" -A 2 | head -30Repository: mosip/registration-client
Length of output: 7770
🏁 Script executed:
# Search entire BaseService for auditFactory
rg -n "auditFactory" registration/registration-services/src/main/java/io/mosip/registration/service/BaseService.java
# Or find the file and check its full length
wc -l registration/registration-services/src/main/java/io/mosip/registration/service/BaseService.javaRepository: mosip/registration-client
Length of output: 169
🏁 Script executed:
# Search for auditFactory declaration in BaseService
grep -n "auditFactory" registration/registration-services/src/main/java/io/mosip/registration/service/BaseService.javaRepository: mosip/registration-client
Length of output: 51
🏁 Script executed:
# Search for how audit is done across service classes - look for audit pattern
rg -n "audit\(" registration/registration-services/src/main/java/io/mosip/registration/service/bio/ -B 2 -A 2
# Find where AuditEvent is imported and used
rg -n "import.*AuditEvent" registration/registration-services/src/main/java/io/mosip/registration/service/
# Check a service that uses auditFactory to see how it's imported/injected
rg -n "auditFactory" registration/registration-services/src/main/java/io/mosip/registration/service/login/impl/LoginServiceImpl.java -B 5 -A 2 | head -40Repository: mosip/registration-client
Length of output: 2693
🏁 Script executed:
# Check if BioServiceImpl imports AuditEvent
grep -n "import.*AuditEvent\|import.*AuditManagerService\|import.*Components" registration/registration-services/src/main/java/io/mosip/registration/service/bio/impl/BioServiceImpl.java
# Find BaseService's auditFactory field
sed -n '70,120p' registration/registration-services/src/main/java/io/mosip/registration/service/BaseService.javaRepository: mosip/registration-client
Length of output: 1815
🏁 Script executed:
# Search for auditFactory field in BaseService
sed -n '1,200p' registration/registration-services/src/main/java/io/mosip/registration/service/BaseService.java | grep -n "auditFactory" -A 2 -B 2
# Also check around line 100-150
sed -n '100,150p' registration/registration-services/src/main/java/io/mosip/registration/service/BaseService.javaRepository: mosip/registration-client
Length of output: 1455
🏁 Script executed:
# Continue searching for auditFactory in BaseService - it might be further down
grep -n "auditFactory\|AuditManagerService\|AuditEvent" registration/registration-services/src/main/java/io/mosip/registration/service/BaseService.java
# If not found, check if child classes need to define it themselves
rg -n "@Autowired.*auditFactory" registration/registration-services/src/main/java/io/mosip/registration/service/ -B 1Repository: mosip/registration-client
Length of output: 51
🏁 Script executed:
# Confirm BioServiceImpl has no auditFactory field or AuditEvent imports
head -50 registration/registration-services/src/main/java/io/mosip/registration/service/bio/impl/BioServiceImpl.java | grep -i "audit"
# Check if evaluateQuality is called from multiple places
rg -n "evaluateQuality" registration/registration-services/src/main/java/io/mosip/registration/service/bio/Repository: mosip/registration-client
Length of output: 473
Audit events defined but never emitted in quality evaluation
evaluateQuality() defines decision rules for SDK quality evaluation but uses only LOGGER calls. None of the five audit events (SDK_QUALITY_SUCCESS, SDK_QUALITY_FAILED, SDK_FALLBACK_TO_SBI, SDK_QUALITY_DISABLED, NO_VALID_QUALITY_SCORE) are fired in any code path. In MOSIP, biometric quality evaluation requires an auditable trail for compliance; the current implementation silently succeeds or fails with no audit record.
Add @Autowired for AuditManagerService and emit the corresponding audit event at each decision branch:
- SDK disabled:
auditFactory.audit(AuditEvent.SDK_QUALITY_DISABLED, ...) - Provider unbound fallback:
auditFactory.audit(AuditEvent.SDK_FALLBACK_TO_SBI, ...) - Success:
auditFactory.audit(AuditEvent.SDK_QUALITY_SUCCESS, ...) - Timeout/null/error paths:
auditFactory.audit(AuditEvent.SDK_QUALITY_FAILED, ...)
See how other service classes (e.g., LoginServiceImpl, PacketHandlerServiceImpl) emit audit events for reference on the correct pattern.
🤖 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
`@registration/registration-services/src/main/java/io/mosip/registration/service/bio/impl/BioServiceImpl.java`
around lines 163 - 266, The evaluateQuality(...) method currently logs decisions
but never emits audit events; add an `@Autowired` AuditManagerService (or existing
auditFactory) field to the class and call auditFactory.audit(...) with the
appropriate AuditEvent at each branch: on SDK disabled emit
AuditEvent.SDK_QUALITY_DISABLED in the early !sdkEnabled branch; when provider
is null emit AuditEvent.SDK_FALLBACK_TO_SBI before falling back to SBI; on
successful SDK score return emit AuditEvent.SDK_QUALITY_SUCCESS (include score
and biometric attribute); and in timeout/null/exception/ExecutionException paths
emit AuditEvent.SDK_QUALITY_FAILED (include error details and biometric
attribute); follow the same audit payload pattern used in
LoginServiceImpl/PacketHandlerServiceImpl to construct event data.
Comment on lines
+186
to
+222
| Object provider = null; | ||
| try { | ||
| provider = bioAPIFactory.getBioProvider(biometricType, BiometricFunction.QUALITY_CHECK); | ||
| } catch (Exception e) { | ||
| LOGGER.warn("SDK provider not available for {}, falling back to SBI", biometricType); | ||
| } | ||
|
|
||
| if (provider == null) { | ||
| LOGGER.info("SDK provider unbound, falling back to SBI score for {}", biometricsDto.getBioAttribute()); | ||
| double sbiScore = biometricsDto.getQualityScore(); | ||
| if (sbiScore <= 0) { | ||
| LOGGER.error("SBI score also unavailable for {}", biometricsDto.getBioAttribute()); | ||
| throw new RegBaseCheckedException( | ||
| RegistrationExceptionConstants.REG_SBI_SCORE_MISSING.getErrorCode(), | ||
| RegistrationExceptionConstants.REG_SBI_SCORE_MISSING.getErrorMessage()); | ||
| } | ||
| return new QualityEvaluationResult(sbiScore, "SBI", true); | ||
| } | ||
|
|
||
| // SDK provider is bound - call with timeout. Any failure here blocks registration. | ||
| long timeoutMs = getSdkTimeoutMs(); | ||
| BIR bir = birBuilder.buildBir(biometricsDto, ProcessedLevelType.RAW); | ||
| BIR[] birList = new BIR[]{bir}; | ||
|
|
||
| Future<Double> future = sdkExecutor.submit(() -> { | ||
| try { | ||
| Map<BiometricType, Float> scoreMap = bioAPIFactory | ||
| .getBioProvider(biometricType, BiometricFunction.QUALITY_CHECK) | ||
| .getModalityQuality(birList, null); | ||
| return (scoreMap != null && scoreMap.get(biometricType) != null) | ||
| ? scoreMap.get(biometricType).doubleValue() | ||
| : null; | ||
| } catch (Exception e) { | ||
| LOGGER.error("SDK provider threw during quality check", e); | ||
| throw new RuntimeException(e); | ||
| } | ||
| }); |
There was a problem hiding this comment.
TOCTOU: getBioProvider() is called twice, but the obtained provider reference is discarded.
At line 188 the provider is fetched and stored as Object solely to check for null. The actual lambda (lines 212–213) calls getBioProvider() a second time. This means:
- The availability check and the actual invocation are non-atomic (TOCTOU).
- The first
getBioProvider()result is always discarded — the allocation is wasted. - If the provider becomes unbound between the two calls, the exception is caught inside the lambda and re-wrapped as
REG_SDK_RUNTIME_ERRORrather than triggering the SBI fallback path.
Use the acquired reference in the lambda to eliminate both problems:
♻️ Proposed fix
- Object provider = null;
+ // Use var (Java 10+) or the concrete return type of getBioProvider
+ var provider = (Object) null;
try {
- provider = bioAPIFactory.getBioProvider(biometricType, BiometricFunction.QUALITY_CHECK);
+ provider = bioAPIFactory.getBioProvider(biometricType, BiometricFunction.QUALITY_CHECK);
} catch (Exception e) {
LOGGER.warn("SDK provider not available for {}, falling back to SBI", biometricType);
}
if (provider == null) { ... }
+ final var boundProvider = bioAPIFactory.getBioProvider(biometricType, BiometricFunction.QUALITY_CHECK);
Future<Double> future = sdkExecutor.submit(() -> {
try {
- Map<BiometricType, Float> scoreMap = bioAPIFactory
- .getBioProvider(biometricType, BiometricFunction.QUALITY_CHECK)
- .getModalityQuality(birList, null);
+ Map<BiometricType, Float> scoreMap = boundProvider.getModalityQuality(birList, null);Or, simpler — capture the result of the first getBioProvider() with the proper interface type and use finalProvider in the lambda, skipping the redundant second call entirely.
🤖 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
`@registration/registration-services/src/main/java/io/mosip/registration/service/bio/impl/BioServiceImpl.java`
around lines 186 - 222, The code calls bioAPIFactory.getBioProvider(...) twice
(stored in a raw Object first and called again inside the lambda), causing a
TOCTOU and discarding the first result; fix by capturing the provider once into
a properly typed variable (replace Object provider with the concrete provider
interface returned by getBioProvider, e.g., final IYourProviderType
finalProvider = bioAPIFactory.getBioProvider(biometricType,
BiometricFunction.QUALITY_CHECK)), keep the existing null-check and SBI
fallback, and then use finalProvider inside the Future lambda instead of calling
bioAPIFactory.getBioProvider(...) again so the same provider instance is used
and the redundant allocation/call is removed.
Comment on lines
+235
to
+255
| } catch (TimeoutException e) { | ||
| future.cancel(true); | ||
| LOGGER.error("SDK quality evaluation timed out after {}ms for {}", timeoutMs, biometricsDto.getBioAttribute()); | ||
| throw new RegBaseCheckedException( | ||
| RegistrationExceptionConstants.REG_SDK_TIMEOUT.getErrorCode(), | ||
| RegistrationExceptionConstants.REG_SDK_TIMEOUT.getErrorMessage()); | ||
|
|
||
| } catch (ExecutionException e) { | ||
| LOGGER.error("SDK runtime error during quality evaluation for {}", biometricsDto.getBioAttribute(), e); | ||
| throw new RegBaseCheckedException( | ||
| RegistrationExceptionConstants.REG_SDK_RUNTIME_ERROR.getErrorCode(), | ||
| RegistrationExceptionConstants.REG_SDK_RUNTIME_ERROR.getErrorMessage()); | ||
|
|
||
| } catch (RegBaseCheckedException e) { | ||
| throw e; | ||
|
|
||
| } catch (Exception e) { | ||
| LOGGER.error("Unexpected error during SDK quality evaluation", e); | ||
| throw new RegBaseCheckedException( | ||
| RegistrationExceptionConstants.REG_SDK_RUNTIME_ERROR.getErrorCode(), | ||
| RegistrationExceptionConstants.REG_SDK_RUNTIME_ERROR.getErrorMessage()); |
There was a problem hiding this comment.
InterruptedException is swallowed without restoring the interrupt flag.
future.get(timeoutMs, MILLISECONDS) can throw InterruptedException. It is caught by the final catch (Exception e) block (line 251) and silently converted to REG_SDK_RUNTIME_ERROR. This clears the calling thread's interrupt status, making the interruption invisible to any outer code that checks Thread.isInterrupted().
🛡️ Proposed fix — add an explicit `InterruptedException` catch
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt(); // restore interrupt status
+ future.cancel(true);
+ throw new RegBaseCheckedException(
+ RegistrationExceptionConstants.REG_SDK_RUNTIME_ERROR.getErrorCode(),
+ RegistrationExceptionConstants.REG_SDK_RUNTIME_ERROR.getErrorMessage());
} catch (TimeoutException e) {
future.cancel(true);🤖 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
`@registration/registration-services/src/main/java/io/mosip/registration/service/bio/impl/BioServiceImpl.java`
around lines 235 - 255, The catch-all Exception block in BioServiceImpl is
swallowing InterruptedException from future.get(timeoutMs, MILLISECONDS) and
clearing the thread interrupt status; add an explicit catch
(InterruptedException e) immediately before the general Exception catch, restore
the interrupt flag with Thread.currentThread().interrupt(), log context (e.g.,
"Thread interrupted during SDK quality evaluation for {}" with
biometricsDto.getBioAttribute()), and rethrow or wrap as a
RegBaseCheckedException consistent with other handlers so interruption is not
lost.
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.
Introduces evaluateQuality() orchestrator in BioServiceImpl that routes quality scoring through the SDK when available, falling back to SBI when the provider is unbound, and blocking on configured SDK failures (timeout, null response, runtime exception). Fixes getCapturedBiometrics() which previously used SBI qualityScore for threshold gating even when sdkScore was populated.
Changes:
This is a basic MVP of the DMP project described in #770
NOT TO BE MERGED UPSTREAM
Summary by CodeRabbit
New Features
Chores