MOSIP-44503 - Automated new user stories and added test cases for pending scenarios#1695
Open
prathmeshj12 wants to merge 2 commits intomosip:developfrom
Open
MOSIP-44503 - Automated new user stories and added test cases for pending scenarios#1695prathmeshj12 wants to merge 2 commits intomosip:developfrom
prathmeshj12 wants to merge 2 commits intomosip:developfrom
Conversation
…ding scenarios Signed-off-by: Prathmesh Jadhav <prathmesh.j@cyberpwn.com>
WalkthroughAdds FAPI test coverage and FAPI-aware utilities: PKCE caching/generation, DPoP/FAPI JWK and client-assertion keyword handling, and JWS/JWE userinfo decoding. Numerous FAPI test definitions, templates, and TestNG registrations were added; test-case dependency mappings were updated. Changes
Sequence Diagram(s)sequenceDiagram
participant TestNG as TestNG Runner
participant Script as GetWithParam
participant Util as EsignetUtil
participant KeyStore as RSA Key Store / JWK cache
participant Validator as OutputValidationUtil
TestNG->>Script: invoke GetUserInfo test (responseBody)
Script->>Script: extract jwtToken (idpAccessToken or raw)
Script->>Util: decodeUserInfoJWT(jwtToken)
Util->>KeyStore: if JWE => attempt RSA decrypt (iterate keys)
KeyStore-->>Util: decrypted payload / failure
Util-->>Script: return JSON {header?, payload}
Script->>Validator: doJsonOutputValidation(finalJson, expectedTemplate)
Validator-->>TestNG: validation result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 |
![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 (9)
api-test/src/main/java/io/mosip/testrig/apirig/esignet/testscripts/GetWithParam.java (2)
21-22: Unused import after refactoring.
DecodedJWTimport appears unused after the JWT handling was refactored to useEsignetUtil.decodeUserInfoJWT().♻️ Proposed fix
import com.auth0.jwt.JWT; -import com.auth0.jwt.interfaces.DecodedJWT;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api-test/src/main/java/io/mosip/testrig/apirig/esignet/testscripts/GetWithParam.java` around lines 21 - 22, Remove the now-unused DecodedJWT import from GetWithParam.java: the JWT handling is using EsignetUtil.decodeUserInfoJWT(), so delete the line importing com.auth0.jwt.interfaces.DecodedJWT to clean up the unused import and avoid compiler warnings.
210-210: Remove debugSystem.out.printlnstatement.This debug output should use the logger or be removed before merge.
♻️ Proposed fix
- System.out.println("JWT Parts Count: " + jwtToken.split("\\.").length); + logger.info("JWT Parts Count: " + jwtToken.split("\\.").length);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api-test/src/main/java/io/mosip/testrig/apirig/esignet/testscripts/GetWithParam.java` at line 210, Remove the debug System.out.println statement in GetWithParam (the line calling System.out.println("JWT Parts Count: " + jwtToken.split("\\.").length)); either delete it or replace it with the class logger (e.g., LOGGER.debug or logger.debug) so debug output uses the project's logging framework and not System.out.println.api-test/testNgXmlFiles/esignetSuite.xml (1)
1012-1090: Minor XML formatting inconsistencies.
- Some
<test>elements have extra leading tab indentation (lines 1012, 1021, 1047, 1056, 1073).- The
idKeyNameparameter is placed inside<classes>block forOauthFAPIandOauthFAPINegTC(lines 1027, 1079), while other entries place it as a sibling to<classes>.Consider aligning with the existing formatting pattern for consistency.
♻️ Example fix for OauthFAPI block
<test name="OauthFAPI"> <parameter name="ymlFile" value="esignet/FAPI/OauthFAPI/OauthFAPI.yml" /> + <parameter name="idKeyName" value="request_uri" /> <classes> <class name="io.mosip.testrig.apirig.esignet.testscripts.SimplePostForAutoGenIdForUrlEncoded" /> - <parameter name="idKeyName" value="request_uri" /> </classes> </test>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api-test/testNgXmlFiles/esignetSuite.xml` around lines 1012 - 1090, Fix XML formatting by removing the extra leading tab indentation for the <test> elements FAPICreateOIDCClient, OauthFAPI, FAPIOauthDetails, FAPIAuthenticateUser, and FAPIAuthorizationCode so they align with the other tests; and move the misplaced <parameter name="idKeyName" value="request_uri" /> entries currently inside the <classes> block of the OauthFAPI and OauthFAPINegTC tests (which reference class io.mosip.testrig.apirig.esignet.testscripts.SimplePostForAutoGenIdForUrlEncoded) to be a sibling of <classes> (same position as other tests that define idKeyName).api-test/src/main/java/io/mosip/testrig/apirig/esignet/utils/EsignetUtil.java (4)
299-300: PKCE cache is not isolated per test case.The static
pkceCachewith hardcoded key"GLOBAL_PKCE"means all FAPI tests share the same PKCE values. This could cause issues if:
- Tests run in parallel
- Different test flows need isolated PKCE values
Consider keying the cache by test case name or session identifier for better isolation.
Proposed fix
if (jsonString.contains("$CODE_CHALLENGE$") || jsonString.contains("$CODE_VERIFIER$") || jsonString.contains("$CODE_CHALLENGE_METHOD$")) { Map<String, String> pkce; - String key = "GLOBAL_PKCE"; + String key = testCaseName != null ? testCaseName + "_PKCE" : "GLOBAL_PKCE"; if (pkceCache.containsKey(key)) { pkce = pkceCache.get(key);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api-test/src/main/java/io/mosip/testrig/apirig/esignet/utils/EsignetUtil.java` around lines 299 - 300, The static pkceCache in EsignetUtil currently uses a single hardcoded key ("GLOBAL_PKCE") causing shared PKCE state across tests; change pkceCache usage so the outer Map is keyed by a session or test-case identifier (e.g., testName or sessionId) instead of a global constant, ensure methods that create/retrieve PKCE values (the code that references pkceCache and "GLOBAL_PKCE") accept or derive a sessionId parameter and use that key to store per-session maps, and update all callers to pass a unique test/session identifier to guarantee isolation for parallel or independent test flows.
2744-2746: Exception wrapping loses context.The
generatePKCEmethod wraps exceptions inRuntimeExceptionwithout logging the original error. This makes debugging difficult if PKCE generation fails.Proposed fix
} catch (Exception e) { + logger.error("Error generating PKCE: " + e.getMessage(), e); throw new RuntimeException("Error generating PKCE", e); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api-test/src/main/java/io/mosip/testrig/apirig/esignet/utils/EsignetUtil.java` around lines 2744 - 2746, The catch block in EsignetUtil.generatePKCE currently throws a RuntimeException without logging the underlying error; add a logging statement (e.g., LOGGER.error("Error generating PKCE", e) or use the existing logger instance) inside the catch to record the original exception and stack trace, then rethrow (preserving the exception as the cause) so failure context is retained for debugging.
2787-2788: Silent exception swallowing in JWE decryption loop.The catch block silently ignores all exceptions without any logging. While trying multiple keys is expected, completely swallowing exceptions makes debugging difficult when all keys fail for unexpected reasons.
Proposed fix to add debug logging
} catch (Exception e) { + logger.debug("Decryption failed with key " + keyName + ": " + e.getMessage()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api-test/src/main/java/io/mosip/testrig/apirig/esignet/utils/EsignetUtil.java` around lines 2787 - 2788, In EsignetUtil's JWE decryption loop (the catch block that currently swallows Exception), replace the empty catch with a logged message that includes contextual info (e.g., which key/index/recipient was tried and the exception message/stacktrace) at debug or warn level so failures while trying multiple keys are visible; use the class logger (e.g., LOGGER or logger used in EsignetUtil) and keep the loop behavior (continue to next key) after logging.
2795-2795: Avoid emoji in log/print statements.The emoji
✅inSystem.out.printlnmay cause encoding issues in some environments or log aggregators. Use plain text instead.Proposed fix
- System.out.println("✅ Decrypted payload: " + decrypted); + System.out.println("Decrypted payload: " + decrypted);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api-test/src/main/java/io/mosip/testrig/apirig/esignet/utils/EsignetUtil.java` at line 2795, Replace the System.out.println call that prints the decrypted payload with a plain-text log/print statement (no emoji) in EsignetUtil; locate the System.out.println("✅ Decrypted payload: " + decrypted) and change it to a non-emoji message like System.out.println("Decrypted payload: " + decrypted) or use the project logger if available to emit "Decrypted payload: " + decrypted.api-test/src/main/resources/esignet/FAPI/FAPICreateOIDCClientV3/FAPICreateOIDCClient.hbs (2)
35-46: Template ignores some YAML input fields.The YAML input provides
purpose_title,purpose_type, andpurpose_subTitle, but the template only uses a hardcoded"type": "verify"for the purpose object. Theuserinfo_response_typefrom input is also ignored in favor of the hardcoded"JWS"value.If these input fields should be configurable, update the template:
Proposed template fix
"additionalConfig": { - "userinfo_response_type": "JWS", - "purpose": { - "type": "verify" - }, + "userinfo_response_type": "{{userinfo_response_type}}", + "purpose": { + "type": "{{purpose_type}}", + "title": "{{purpose_title}}", + "subTitle": "{{purpose_subTitle}}" + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api-test/src/main/resources/esignet/FAPI/FAPICreateOIDCClientV3/FAPICreateOIDCClient.hbs` around lines 35 - 46, The template currently hardcodes "userinfo_response_type": "JWS" and purpose.type = "verify" inside the additionalConfig object; change those to use the incoming template variables (e.g., {{userinfo_response_type}} for userinfo_response_type and {{purpose_type}}, plus add {{purpose_title}} and {{purpose_subTitle}} into the purpose object) so the purpose object uses purpose_title, purpose_type, purpose_subTitle from the YAML and userinfo_response_type is rendered from the input instead of the hardcoded values; update the additionalConfig → purpose block and the userinfo_response_type entry accordingly.
1-48: Inconsistent indentation.The template has mixed indentation (tabs and spaces, varying levels). While functionally correct, this reduces readability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api-test/src/main/resources/esignet/FAPI/FAPICreateOIDCClientV3/FAPICreateOIDCClient.hbs` around lines 1 - 48, The template FAPICreateOIDCClient.hbs uses mixed tabs and spaces and inconsistent nesting; normalize indentation across the whole file (choose spaces, e.g., 2 or 4 spaces) and re-indent every block consistently—especially objects/arrays such as "request", "authContextRefs", "redirectUris", "clientNameLangMap", and "additionalConfig"—so opening braces and their child keys line up uniformly and each array element is indented the same amount; remove tabs, align the Handlebars {{`#each`}} block and its "{{acrValues}}" entries, and ensure nested properties (like "purpose" inside "additionalConfig") follow the same indentation level.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api-test/src/main/resources/esignet/AddIdentity/AddIdentity.yml`:
- Around line 1340-1367: The FAPIAuthenticateUser consumer test
ESignet_FAPIAuthenticateUser_AuthToken_Xsrf__uin_Otp_Valid_forUserInfo_Smoke is
using outputs from Dependent_Idrepo_AddIdentity_48 (via
$ID:AddIdentity_For_FAPI_smoke_Pos_UIN$ and
$ID:AddIdentity_For_FAPI_smoke_Pos_EMAIL$) but doesn't declare the producer
dependency; update the test case
ESignet_FAPIAuthenticateUser_AuthToken_Xsrf__uin_Otp_Valid_forUserInfo_Smoke to
include additionalDependencies: Dependent_Idrepo_AddIdentity_48 so the
AddIdentity producer (Dependent_Idrepo_AddIdentity_48) runs before the consumer
and provides the required UIN/EMAIL outputs.
In
`@api-test/src/main/resources/esignet/FAPI/FAPIAuthenticateUser/FAPIAuthenticateUser.yml`:
- Line 24: The otpChannels array contains objects with unquoted keys (e.g.,
{channel: "email"}) which is invalid JSON; update the entries so object keys are
quoted (e.g., {"channel": "email"} and {"channel": "phone"}) to produce valid
JSON for the otpChannels value.
In
`@api-test/src/main/resources/esignet/FAPI/FAPIAuthorizationCode/FAPIAuthorizationCode.yml`:
- Around line 4-10: The YAML has a dependency identifier mismatch:
uniqueIdentifier is "TC_ESignet_FAPIAuthorizationCode_01" but
additionalDependencies references "TC_ESignet_FAPI_AuthorizationCode_01" (extra
underscore). Update additionalDependencies to exactly match the producing test
case ID (e.g., change "TC_ESignet_FAPI_AuthorizationCode_01" to
"TC_ESignet_FAPIAuthorizationCode_01"), or remove the additionalDependencies
entry if no dependency is required; ensure consistency between the
uniqueIdentifier and any dependency entries so resolution succeeds.
In `@api-test/src/main/resources/esignet/FAPI/OauthFAPI/OauthFAPI.hbs`:
- Around line 17-18: The template unconditionally emits PKCE fields
"code_challenge" and "code_challenge_method" in OauthFAPI.hbs which breaks
fapi2.0 scenarios; update the Handlebars template to conditionally render those
two fields only when the active profile/scenario requires PKCE (i.e., not when
profile == "fapi2.0" or when scenario flag pkceEnabled is false). Locate the
"code_challenge" and "code_challenge_method" lines in OauthFAPI.hbs and wrap
them in a single conditional block that checks the profile/scenario PKCE flag so
PKCE is omitted for fapi2.0 tests (preserve existing variable names
{{code_challenge}} and {{code_challenge_method}}).
In `@api-test/src/main/resources/esignet/FAPI/OauthFAPINegTC/OauthFAPINegTC.yml`:
- Around line 63-93: Rename the test case key
ESignet_OauthFAPINegTC_Invaid_client_assertion_Neg to correct the typo
(ESignet_OauthFAPINegTC_Invalid_client_assertion_Neg) so the identifier is
spelled "Invalid"; update any references to this key (e.g., uniqueIdentifier
TC_ESignet_Oauth_FAPI_NegTC_03 and related input/output templates like
inputTemplate esignet/FAPI/OauthFAPI/OauthFAPI and outputTemplate
esignet/error2) to use the new key name to avoid broken references.
---
Nitpick comments:
In
`@api-test/src/main/java/io/mosip/testrig/apirig/esignet/testscripts/GetWithParam.java`:
- Around line 21-22: Remove the now-unused DecodedJWT import from
GetWithParam.java: the JWT handling is using EsignetUtil.decodeUserInfoJWT(), so
delete the line importing com.auth0.jwt.interfaces.DecodedJWT to clean up the
unused import and avoid compiler warnings.
- Line 210: Remove the debug System.out.println statement in GetWithParam (the
line calling System.out.println("JWT Parts Count: " +
jwtToken.split("\\.").length)); either delete it or replace it with the class
logger (e.g., LOGGER.debug or logger.debug) so debug output uses the project's
logging framework and not System.out.println.
In
`@api-test/src/main/java/io/mosip/testrig/apirig/esignet/utils/EsignetUtil.java`:
- Around line 299-300: The static pkceCache in EsignetUtil currently uses a
single hardcoded key ("GLOBAL_PKCE") causing shared PKCE state across tests;
change pkceCache usage so the outer Map is keyed by a session or test-case
identifier (e.g., testName or sessionId) instead of a global constant, ensure
methods that create/retrieve PKCE values (the code that references pkceCache and
"GLOBAL_PKCE") accept or derive a sessionId parameter and use that key to store
per-session maps, and update all callers to pass a unique test/session
identifier to guarantee isolation for parallel or independent test flows.
- Around line 2744-2746: The catch block in EsignetUtil.generatePKCE currently
throws a RuntimeException without logging the underlying error; add a logging
statement (e.g., LOGGER.error("Error generating PKCE", e) or use the existing
logger instance) inside the catch to record the original exception and stack
trace, then rethrow (preserving the exception as the cause) so failure context
is retained for debugging.
- Around line 2787-2788: In EsignetUtil's JWE decryption loop (the catch block
that currently swallows Exception), replace the empty catch with a logged
message that includes contextual info (e.g., which key/index/recipient was tried
and the exception message/stacktrace) at debug or warn level so failures while
trying multiple keys are visible; use the class logger (e.g., LOGGER or logger
used in EsignetUtil) and keep the loop behavior (continue to next key) after
logging.
- Line 2795: Replace the System.out.println call that prints the decrypted
payload with a plain-text log/print statement (no emoji) in EsignetUtil; locate
the System.out.println("✅ Decrypted payload: " + decrypted) and change it to a
non-emoji message like System.out.println("Decrypted payload: " + decrypted) or
use the project logger if available to emit "Decrypted payload: " + decrypted.
In
`@api-test/src/main/resources/esignet/FAPI/FAPICreateOIDCClientV3/FAPICreateOIDCClient.hbs`:
- Around line 35-46: The template currently hardcodes "userinfo_response_type":
"JWS" and purpose.type = "verify" inside the additionalConfig object; change
those to use the incoming template variables (e.g., {{userinfo_response_type}}
for userinfo_response_type and {{purpose_type}}, plus add {{purpose_title}} and
{{purpose_subTitle}} into the purpose object) so the purpose object uses
purpose_title, purpose_type, purpose_subTitle from the YAML and
userinfo_response_type is rendered from the input instead of the hardcoded
values; update the additionalConfig → purpose block and the
userinfo_response_type entry accordingly.
- Around line 1-48: The template FAPICreateOIDCClient.hbs uses mixed tabs and
spaces and inconsistent nesting; normalize indentation across the whole file
(choose spaces, e.g., 2 or 4 spaces) and re-indent every block
consistently—especially objects/arrays such as "request", "authContextRefs",
"redirectUris", "clientNameLangMap", and "additionalConfig"—so opening braces
and their child keys line up uniformly and each array element is indented the
same amount; remove tabs, align the Handlebars {{`#each`}} block and its
"{{acrValues}}" entries, and ensure nested properties (like "purpose" inside
"additionalConfig") follow the same indentation level.
In `@api-test/testNgXmlFiles/esignetSuite.xml`:
- Around line 1012-1090: Fix XML formatting by removing the extra leading tab
indentation for the <test> elements FAPICreateOIDCClient, OauthFAPI,
FAPIOauthDetails, FAPIAuthenticateUser, and FAPIAuthorizationCode so they align
with the other tests; and move the misplaced <parameter name="idKeyName"
value="request_uri" /> entries currently inside the <classes> block of the
OauthFAPI and OauthFAPINegTC tests (which reference class
io.mosip.testrig.apirig.esignet.testscripts.SimplePostForAutoGenIdForUrlEncoded)
to be a sibling of <classes> (same position as other tests that define
idKeyName).
🪄 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: CHILL
Plan: Pro
Run ID: 5d3ee552-d5dc-4c65-a954-5348f24c4371
📒 Files selected for processing (33)
api-test/src/main/java/io/mosip/testrig/apirig/esignet/testscripts/GetWithParam.javaapi-test/src/main/java/io/mosip/testrig/apirig/esignet/utils/EsignetUtil.javaapi-test/src/main/resources/esignet/AddIdentity/AddIdentity.ymlapi-test/src/main/resources/esignet/AddIdentityMock/AddIdentity.ymlapi-test/src/main/resources/esignet/EndToEndFlowWithV3MOCK/userinfo/GetOidcUserInfo.ymlapi-test/src/main/resources/esignet/EndToEndFlowWithV3MOCK/userinfo/GetOidcUserInfoDecryptResult.hbsapi-test/src/main/resources/esignet/FAPI/FAPIAuthenticateUser/FAPIAuthenticateUser.hbsapi-test/src/main/resources/esignet/FAPI/FAPIAuthenticateUser/FAPIAuthenticateUser.ymlapi-test/src/main/resources/esignet/FAPI/FAPIAuthenticateUser/FAPIAuthenticateUserResult.hbsapi-test/src/main/resources/esignet/FAPI/FAPIAuthorizationCode/FAPIAuthorizationCode.hbsapi-test/src/main/resources/esignet/FAPI/FAPIAuthorizationCode/FAPIAuthorizationCode.ymlapi-test/src/main/resources/esignet/FAPI/FAPIAuthorizationCode/FAPIAuthorizationCodeResult.hbsapi-test/src/main/resources/esignet/FAPI/FAPICreateOIDCClientV3/FAPICreateOIDCClient.hbsapi-test/src/main/resources/esignet/FAPI/FAPICreateOIDCClientV3/FAPICreateOIDCClient.ymlapi-test/src/main/resources/esignet/FAPI/FAPICreateOIDCClientV3/FAPICreateOIDCClientResult.hbsapi-test/src/main/resources/esignet/FAPI/FAPIGenerateToken/FAPIGenerateToken.hbsapi-test/src/main/resources/esignet/FAPI/FAPIGenerateToken/FAPIGenerateToken.ymlapi-test/src/main/resources/esignet/FAPI/FAPIGenerateToken/FAPIGenerateTokenResult.hbsapi-test/src/main/resources/esignet/FAPI/FAPIGetUserInfo/FAPIGetUserInfo.hbsapi-test/src/main/resources/esignet/FAPI/FAPIGetUserInfo/FAPIGetUserInfo.ymlapi-test/src/main/resources/esignet/FAPI/FAPIGetUserInfo/FAPIGetUserInfoResult.hbsapi-test/src/main/resources/esignet/FAPI/FAPIOauthDetails/FAPIOauthDetails.hbsapi-test/src/main/resources/esignet/FAPI/FAPIOauthDetails/FAPIOauthDetails.ymlapi-test/src/main/resources/esignet/FAPI/FAPIOauthDetails/FAPIOauthDetailsResult.hbsapi-test/src/main/resources/esignet/FAPI/FAPIOauthDetailsNegTC/FAPIOauthDetailsNegTC.ymlapi-test/src/main/resources/esignet/FAPI/OauthFAPI/OauthFAPI.hbsapi-test/src/main/resources/esignet/FAPI/OauthFAPI/OauthFAPI.ymlapi-test/src/main/resources/esignet/FAPI/OauthFAPI/OauthFAPIResult.hbsapi-test/src/main/resources/esignet/FAPI/OauthFAPINegTC/OauthFAPINegTC.ymlapi-test/src/main/resources/esignet/OidcClient/OIDCClient.ymlapi-test/src/main/resources/esignet/OidcClient/OIDCClientFAPI.hbsapi-test/src/main/resources/esignet/PAR/OauthParNegativeScenarios/OauthParNegativeScenarios.ymlapi-test/testNgXmlFiles/esignetSuite.xml
…ding scenarios Signed-off-by: Prathmesh Jadhav <prathmesh.j@cyberpwn.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@api-test/src/main/java/io/mosip/testrig/apirig/esignet/utils/EsignetUtil.java`:
- Around line 399-403: The code currently logs raw JWK strings and RSAKey
objects (e.g., jwkKeyString and dpopKey from RSAKey.parse(...)) and prints
decrypted userinfo payloads with System.out.println, exposing secrets/PII;
replace these logger.info/System.out.println calls with non-sensitive, redacted
debug logs (use logger.debug and only include safe identifiers such as
testCaseName and a masked key id or fingerprint), remove printing of full
jwkKeyString, dpopKey contents and decrypted userinfo, and ensure no
System.out.println calls remain—if you need to record that a key or payload was
processed, log a redacted value (e.g., "<REDACTED_JWK>" or key.kid) instead.
- Around line 1033-1061: If RSAKey.parse(oidcJWKKeyString) fails, we must fail
fast and avoid reusing the stale static oidc_JWK_Key_For_FAPI; on catch of
java.text.ParseException clear or null out the static oidc_JWK_Key_For_FAPI and
throw a runtime exception (e.g., IllegalStateException) with a descriptive
message so processing stops. Also add a null-check before calling
signJWKKey(clientId, oidc_JWK_Key_For_FAPI, tempUrl) and if
oidc_JWK_Key_For_FAPI is null, log and throw the same runtime exception to
prevent signing with a previous key. Use the symbols RSAKey.parse,
oidc_JWK_Key_For_FAPI, signJWKKey, and getValueFromEsignetWellKnownEndPoint to
locate the code.
In `@api-test/src/main/resources/config/testCaseInterDependency_mock.json`:
- Around line 788-790: The TC_ESignet_LinkedAuthenticationWla_03 entry is
missing a dependency on TC_ESignet_LinkTransaction_03 required for the
$ID:LinkTransaction_Consent_uin_User2_all_Valid_Smoke_sid_linkTransactionId$
output; add "TC_ESignet_LinkTransaction_03" to the dependency list for
TC_ESignet_LinkedAuthenticationWla_03 in the mock config (or alternatively add
it to the test's additionalDependencies in the YAML) so the LinkTransaction
output is available at runtime.
In `@api-test/src/main/resources/esignet/FAPI/OauthFAPINegTC/OauthFAPINegTC.yml`:
- Around line 8-32: The test currently only asserts the top-level "error" via
checkErrorsOnlyInResponse and outputTemplate "error2", which lets any
invalid_request satisfy the case; update this testcase (OauthFAPINegTC.yml) to
assert a branch-specific field (e.g., include "error_description" or an expected
"status" in the output JSON) that uniquely ties to the failing validation for
code_challenge_method and client_assertion (for example expect error_description
containing "unsupported_code_challenge_method" when code_challenge_method is
"S25", and expect an error_description about "invalid_client_assertion" for the
client_assertion case), and stop using the generic error2 template for these
inputs (or extend the template to include the specific field); apply the same
change to the other PAR-negative case referenced (the other block handling
code_challenge_method/client_assertion).
- Around line 39-62: The test YAML currently sets checkErrorsOnlyInResponse:
true with an empty output so it neither verifies PAR success nor the
PKCE-related error for missing code_challenge/code_challenge_method; update the
YAML to make the intended branch explicit and assert accordingly: if this is the
fapi2.0 branch, replace the empty output with expected PAR response fields
(e.g., request_uri, expires_in, client_id or other PAR success keys used by your
suite) so the absence of PKCE is validated as allowed/handled by the PAR; if
this is the nisdsp branch, change checkErrorsOnlyInResponse or the output to
assert the specific error response (error and error_description) that should be
returned when code_challenge/code_challenge_method are removed. Ensure the keys
referenced in the input (code_challenge, code_challenge_method) remain $REMOVE$
and that the test is linked as the producer for
TC_ESignet_FAPI_OauthDetails_NegTC_01 so downstream consumers get the explicit
expected behavior.
🪄 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: CHILL
Plan: Pro
Run ID: 9552ece8-b97e-475e-96ff-b5c779f203d3
📒 Files selected for processing (6)
api-test/src/main/java/io/mosip/testrig/apirig/esignet/utils/EsignetUtil.javaapi-test/src/main/resources/config/testCaseInterDependency_mock.jsonapi-test/src/main/resources/config/testCaseInterDependency_mosip-id.jsonapi-test/src/main/resources/esignet/FAPI/FAPIAuthenticateUser/FAPIAuthenticateUser.ymlapi-test/src/main/resources/esignet/FAPI/FAPIAuthorizationCode/FAPIAuthorizationCode.ymlapi-test/src/main/resources/esignet/FAPI/OauthFAPINegTC/OauthFAPINegTC.yml
✅ Files skipped from review due to trivial changes (1)
- api-test/src/main/resources/esignet/FAPI/FAPIAuthenticateUser/FAPIAuthenticateUser.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- api-test/src/main/resources/esignet/FAPI/FAPIAuthorizationCode/FAPIAuthorizationCode.yml
Comment on lines
+399
to
+403
| logger.info("Using JWK for testCase: " + testCaseName); | ||
| logger.info("oidcJWKKeyString = " + jwkKeyString); | ||
|
|
||
| String jkt = generateJKT(dpopKey); | ||
| logger.info("DPoP JKT: " + jkt); | ||
| RSAKey dpopKey = RSAKey.parse(jwkKeyString); | ||
| logger.info("dpopKey = " + dpopKey); |
There was a problem hiding this comment.
Remove secret and PII dumps from logs.
These additions print raw JWK strings / RSAKey objects and the decrypted userinfo payload. That exposes private-key material and end-user claims in CI logs, and the System.out.println(...) calls bypass the configured log level entirely. Keep only redacted identifiers behind debug logging.
Also applies to: 452-455, 2784-2795
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@api-test/src/main/java/io/mosip/testrig/apirig/esignet/utils/EsignetUtil.java`
around lines 399 - 403, The code currently logs raw JWK strings and RSAKey
objects (e.g., jwkKeyString and dpopKey from RSAKey.parse(...)) and prints
decrypted userinfo payloads with System.out.println, exposing secrets/PII;
replace these logger.info/System.out.println calls with non-sensitive, redacted
debug logs (use logger.debug and only include safe identifiers such as
testCaseName and a masked key id or fingerprint), remove printing of full
jwkKeyString, dpopKey contents and decrypted userinfo, and ensure no
System.out.println calls remain—if you need to record that a key or payload was
processed, log a redacted value (e.g., "<REDACTED_JWK>" or key.kid) instead.
Comment on lines
+1033
to
+1061
| if (jsonString.contains("$CLIENT_ASSERTION_FAPI_JWT$")) { | ||
| String oidcJWKKeyString = JWKKeyUtil.getJWKKey(OIDC_JWK_FOR_FAPI); | ||
| logger.info("oidcJWKKeyString =" + oidcJWKKeyString); | ||
| try { | ||
| oidc_JWK_Key_For_FAPI = RSAKey.parse(oidcJWKKeyString); | ||
| logger.info("oidc_JWK_Key_For_FAPI =" + oidc_JWK_Key_For_FAPI); | ||
| } catch (java.text.ParseException e) { | ||
| logger.error(e.getMessage()); | ||
| } | ||
|
|
||
| JSONObject root = new JSONObject(jsonString); | ||
| String clientId = root.optString("client_id", null); | ||
| String audKey = null; | ||
|
|
||
| if (root.has("aud_key")) { | ||
| audKey = root.optString("aud_key", null); | ||
| root.remove("aud_key"); | ||
| jsonString = root.toString(); | ||
| } | ||
|
|
||
| String tempUrl = getValueFromEsignetWellKnownEndPoint(audKey, EsignetConfigManager.getEsignetBaseUrl()); | ||
|
|
||
| if (clientId != null) { | ||
| jsonString = replaceKeywordValue(jsonString, "$CLIENT_ASSERTION_FAPI_JWT$", | ||
| signJWKKey(clientId, oidc_JWK_Key_For_FAPI, tempUrl)); | ||
| } else { | ||
| logger.error("Client ID not found in JSON for $CLIENT_ASSERTION_FAPI_JWT$."); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fail fast when the cached FAPI JWK cannot be parsed.
If RSAKey.parse(...) fails here, the code only logs and continues. Because oidc_JWK_Key_For_FAPI and clientAssertionToken are static fields, the next line can reuse a key/token from a previous testcase and sign the current request with stale credentials instead of failing this one.
🛠️ Proposed fix
if (jsonString.contains("$CLIENT_ASSERTION_FAPI_JWT$")) {
String oidcJWKKeyString = JWKKeyUtil.getJWKKey(OIDC_JWK_FOR_FAPI);
- logger.info("oidcJWKKeyString =" + oidcJWKKeyString);
- try {
- oidc_JWK_Key_For_FAPI = RSAKey.parse(oidcJWKKeyString);
- logger.info("oidc_JWK_Key_For_FAPI =" + oidc_JWK_Key_For_FAPI);
- } catch (java.text.ParseException e) {
- logger.error(e.getMessage());
- }
+ if (oidcJWKKeyString == null || oidcJWKKeyString.isBlank()) {
+ throw new SkipException("Missing cached FAPI JWK for $CLIENT_ASSERTION_FAPI_JWT$");
+ }
+
+ final RSAKey fapiJwk;
+ try {
+ fapiJwk = RSAKey.parse(oidcJWKKeyString);
+ } catch (java.text.ParseException e) {
+ throw new SkipException("Invalid cached FAPI JWK for $CLIENT_ASSERTION_FAPI_JWT$: " + e.getMessage());
+ }
JSONObject root = new JSONObject(jsonString);
String clientId = root.optString("client_id", null);
@@
String tempUrl = getValueFromEsignetWellKnownEndPoint(audKey, EsignetConfigManager.getEsignetBaseUrl());
if (clientId != null) {
jsonString = replaceKeywordValue(jsonString, "$CLIENT_ASSERTION_FAPI_JWT$",
- signJWKKey(clientId, oidc_JWK_Key_For_FAPI, tempUrl));
+ signJWKKey(clientId, fapiJwk, tempUrl));
} else {
logger.error("Client ID not found in JSON for $CLIENT_ASSERTION_FAPI_JWT$.");
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@api-test/src/main/java/io/mosip/testrig/apirig/esignet/utils/EsignetUtil.java`
around lines 1033 - 1061, If RSAKey.parse(oidcJWKKeyString) fails, we must fail
fast and avoid reusing the stale static oidc_JWK_Key_For_FAPI; on catch of
java.text.ParseException clear or null out the static oidc_JWK_Key_For_FAPI and
throw a runtime exception (e.g., IllegalStateException) with a descriptive
message so processing stops. Also add a null-check before calling
signJWKKey(clientId, oidc_JWK_Key_For_FAPI, tempUrl) and if
oidc_JWK_Key_For_FAPI is null, log and throw the same runtime exception to
prevent signing with a previous key. Use the symbols RSAKey.parse,
oidc_JWK_Key_For_FAPI, signJWKKey, and getValueFromEsignetWellKnownEndPoint to
locate the code.
Comment on lines
788
to
+790
| "TC_ESignet_LinkedAuthenticationWla_03": [ | ||
| "Dependent_Idrepo_AddIdentity_11", | ||
| "Dependent_Idrepo_AddIdentity_01", | ||
| "TC_ESignet_LinkTransaction_03" | ||
| "Dependent_Idrepo_AddIdentity_01" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if LinkTransaction_03 dependency is managed elsewhere
echo "=== Checking LinkedAuthenticationWla_03 YAML for inline dependencies ==="
rg -B2 -A10 "TC_ESignet_LinkedAuthenticationWla_03" --type yaml api-test/src/main/resources/esignet/Repository: mosip/esignet
Length of output: 3866
🏁 Script executed:
#!/bin/bash
# Search for LinkTransaction test cases in the LinkedAuthenticationWla YAML file
echo "=== Searching for LinkTransaction test case that produces the required ID ==="
rg "LinkTransaction_Consent_uin_User2_all_Valid_Smoke_sid_linkTransactionId" -B20 api-test/src/main/resources/esignet/ --type yaml
echo ""
echo "=== Looking for additionalDependencies in LinkedAuthenticationWla_03 test ==="
# Get the full test definition around uniqueIdentifier: TC_ESignet_LinkedAuthenticationWla_03
sed -n '/uniqueIdentifier: TC_ESignet_LinkedAuthenticationWla_03/,/^[[:space:]]*[a-zA-Z_][a-zA-Z0-9_]*:/p' api-test/src/main/resources/esignet/LinkedAuthenticationWla/LinkedAuthenticationWla.yml | head -40Repository: mosip/esignet
Length of output: 6122
🏁 Script executed:
#!/bin/bash
# Search for which LinkTransaction test produces the required ID
echo "=== Searching for LinkTransaction tests and their outputs ==="
rg "LinkTransaction_Consent_uin_User2_all_Valid_Smoke_sid" -B5 api-test/src/main/resources/esignet/ --type yaml | head -100
echo ""
echo "=== Check testCaseInterDependency_mock.json for original dependencies ==="
# Get lines around 788-790 to see the full context and original entry
sed -n '780,800p' api-test/src/main/resources/config/testCaseInterDependency_mock.jsonRepository: mosip/esignet
Length of output: 3337
🏁 Script executed:
#!/bin/bash
# Find the uniqueIdentifier for the LinkTransaction test that produces the required ID
echo "=== Find uniqueIdentifier of ESignet_LinkTransaction_Consent_uin_User2_all_Valid_Smoke_sid ==="
rg -B10 "ESignet_LinkTransaction_Consent_uin_User2_all_Valid_Smoke_sid:" api-test/src/main/resources/esignet/LinkTransaction/LinkTransaction.yml | grep -E "(uniqueIdentifier|ESignet_LinkTransaction_Consent_uin_User2)" | head -5
echo ""
echo "=== Check if TC_ESignet_LinkedAuthenticationWla_03 has additionalDependencies in YAML ==="
# Get full test definition for TC_ESignet_LinkedAuthenticationWla_03
rg -A50 "TC_ESignet_LinkedAuthenticationWla_03" api-test/src/main/resources/esignet/LinkedAuthenticationWla/LinkedAuthenticationWla.yml | grep -E "(uniqueIdentifier|additionalDependencies|linkTransactionId)" | head -10Repository: mosip/esignet
Length of output: 477
🏁 Script executed:
#!/bin/bash
# Get full definition of the LinkTransaction test to find its uniqueIdentifier
echo "=== Full definition of ESignet_LinkTransaction_Consent_uin_User2_all_Valid_Smoke_sid test ==="
sed -n '/ESignet_LinkTransaction_Consent_uin_User2_all_Valid_Smoke_sid:/,/^ [^ ]/p' api-test/src/main/resources/esignet/LinkTransaction/LinkTransaction.yml | head -30
echo ""
echo "=== Check testCaseInterDependency_mock.json for LinkTransaction entries ==="
# Search for LinkTransaction_03 in the config
rg "LinkTransaction_03" api-test/src/main/resources/config/testCaseInterDependency_mock.jsonRepository: mosip/esignet
Length of output: 1132
Add missing TC_ESignet_LinkTransaction_03 dependency.
TC_ESignet_LinkedAuthenticationWla_03 uses $ID:LinkTransaction_Consent_uin_User2_all_Valid_Smoke_sid_linkTransactionId$ from the LinkTransaction test output, but the test case lacks a declared dependency on TC_ESignet_LinkTransaction_03. This will cause the test to fail at runtime. The dependency must be restored either in the centralized config (lines 788-790) or as an additionalDependencies field in the YAML test definition.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api-test/src/main/resources/config/testCaseInterDependency_mock.json` around
lines 788 - 790, The TC_ESignet_LinkedAuthenticationWla_03 entry is missing a
dependency on TC_ESignet_LinkTransaction_03 required for the
$ID:LinkTransaction_Consent_uin_User2_all_Valid_Smoke_sid_linkTransactionId$
output; add "TC_ESignet_LinkTransaction_03" to the dependency list for
TC_ESignet_LinkedAuthenticationWla_03 in the mock config (or alternatively add
it to the test's additionalDependencies in the YAML) so the LinkTransaction
output is available at runtime.
Comment on lines
+8
to
+32
| checkErrorsOnlyInResponse: true | ||
| inputTemplate: esignet/FAPI/OauthFAPI/OauthFAPI | ||
| outputTemplate: esignet/error2 | ||
| input: '{ | ||
| "requestTime": "$TIMESTAMP$", | ||
| "client_id": "$ID:CreateOIDCClientFAPI_all_Valid_Smoke_sid_clientId$", | ||
| "scope": "openid profile", | ||
| "response_type": "code", | ||
| "redirect_uri": "$IDPREDIRECTURI$", | ||
| "display": "popup", | ||
| "prompt": "login", | ||
| "acr_values": "mosip:idp:acr:generated-code", | ||
| "nonce": "$UNIQUENONCEVALUEFORESIGNET$", | ||
| "state": "eree2311", | ||
| "claim_locales": "en", | ||
| "client_assertion_type": "urn:ietf:params:oauth:client-assertion-type:jwt-bearer", | ||
| "client_assertion": "$CLIENT_ASSERTION_FAPI_JWT$", | ||
| "aud_key": "pushed_authorization_request_endpoint", | ||
| "dpop_jkt": "$DPOP_JKT$", | ||
| "code_challenge": "$CODE_CHALLENGE$", | ||
| "code_challenge_method": "S25" | ||
| }' | ||
| output: '{ | ||
| "error": "invalid_request" | ||
| }' |
There was a problem hiding this comment.
Make these PAR negatives assert the validation branch, not just the top-level error.
Both cases use api-test/src/main/resources/esignet/error2.hbs:1-3, which only checks "error". With checkErrorsOnlyInResponse: true, any invalid_request/invalid_client response will satisfy the testcase, even if it comes from a different failure path. Please assert one branch-specific field here as well (for example error_description or status) so the suite actually pins code_challenge_method and client_assertion handling.
Also applies to: 69-93
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api-test/src/main/resources/esignet/FAPI/OauthFAPINegTC/OauthFAPINegTC.yml`
around lines 8 - 32, The test currently only asserts the top-level "error" via
checkErrorsOnlyInResponse and outputTemplate "error2", which lets any
invalid_request satisfy the case; update this testcase (OauthFAPINegTC.yml) to
assert a branch-specific field (e.g., include "error_description" or an expected
"status" in the output JSON) that uniquely ties to the failing validation for
code_challenge_method and client_assertion (for example expect error_description
containing "unsupported_code_challenge_method" when code_challenge_method is
"S25", and expect an error_description about "invalid_client_assertion" for the
client_assertion case), and stop using the generic error2 template for these
inputs (or extend the template to include the specific field); apply the same
change to the other PAR-negative case referenced (the other block handling
code_challenge_method/client_assertion).
Comment on lines
+39
to
+62
| checkErrorsOnlyInResponse: true | ||
| inputTemplate: esignet/FAPI/OauthFAPI/OauthFAPI | ||
| outputTemplate: esignet/FAPI/OauthFAPI/OauthFAPIResult | ||
| input: '{ | ||
| "requestTime": "$TIMESTAMP$", | ||
| "client_id": "$ID:CreateOIDCClientFAPI_all_Valid_Smoke_sid_clientId$", | ||
| "scope": "openid profile", | ||
| "response_type": "code", | ||
| "redirect_uri": "$IDPREDIRECTURI$", | ||
| "display": "popup", | ||
| "prompt": "login", | ||
| "acr_values": "mosip:idp:acr:generated-code", | ||
| "nonce": "$UNIQUENONCEVALUEFORESIGNET$", | ||
| "state": "eree2311", | ||
| "claim_locales": "en", | ||
| "client_assertion_type": "urn:ietf:params:oauth:client-assertion-type:jwt-bearer", | ||
| "client_assertion": "$CLIENT_ASSERTION_FAPI_JWT$", | ||
| "aud_key": "pushed_authorization_request_endpoint", | ||
| "dpop_jkt": "$DPOP_JKT$", | ||
| "code_challenge": "$REMOVE$", | ||
| "code_challenge_method": "$REMOVE$" | ||
| }' | ||
| output: '{ | ||
| }' |
There was a problem hiding this comment.
This case does not assert its intended PKCE behavior.
checkErrorsOnlyInResponse: true plus an empty expected body means this scenario validates neither the PAR success fields nor an error response. As written, it can pass without proving whether missing PKCE is allowed or rejected, and it is already used as a producer for TC_ESignet_FAPI_OauthDetails_NegTC_01 in api-test/src/main/resources/config/testCaseInterDependency_mosip-id.json. If this is the fapi2.0 branch, assert the returned PAR fields; if it is the nisdsp branch, assert the specific error here.
Based on learnings, PKCE differentiation between fapi2.0 and nisdsp is handled at the YAML test-data layer, so this YAML needs to make the intended branch explicit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api-test/src/main/resources/esignet/FAPI/OauthFAPINegTC/OauthFAPINegTC.yml`
around lines 39 - 62, The test YAML currently sets checkErrorsOnlyInResponse:
true with an empty output so it neither verifies PAR success nor the
PKCE-related error for missing code_challenge/code_challenge_method; update the
YAML to make the intended branch explicit and assert accordingly: if this is the
fapi2.0 branch, replace the empty output with expected PAR response fields
(e.g., request_uri, expires_in, client_id or other PAR success keys used by your
suite) so the absence of PKCE is validated as allowed/handled by the PAR; if
this is the nisdsp branch, change checkErrorsOnlyInResponse or the output to
assert the specific error response (error and error_description) that should be
returned when code_challenge/code_challenge_method are removed. Ensure the keys
referenced in the input (code_challenge, code_challenge_method) remain $REMOVE$
and that the test is linked as the producer for
TC_ESignet_FAPI_OauthDetails_NegTC_01 so downstream consumers get the explicit
expected behavior.
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.
Summary by CodeRabbit
New Features
Tests