ES-2961, MOSIP-44503, ES-3003 - Added test cases for JWE test cases & fixed the consent issue#1882
Merged
Merged
Conversation
Signed-off-by: Prathmesh Jadhav <prathmesh.j@cyberpwn.com>
Signed-off-by: prathmeshj12 <prathmesh.j@cyberpwn.com>
|
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 (1)
(Note: the hidden artifact above contains the complete, machine-parseable review stack for this PR.) WalkthroughThis PR adds comprehensive FAPI-JWE (Financial-grade API with JWE encryption) support to the eSignet test suite. It introduces Maven publishing configuration, extends EsignetUtil with dynamic FAPI key selection and new JWE-specific placeholders, adds FAPI-JWE test cases across all OAuth flows (authenticate, authorize, token, userinfo), introduces a new partial OIDC client update endpoint, and fixes consent flow test configurations to correctly reflect subsequent-authentication behavior. ChangesFAPI-JWE Support and Consent Flow Updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api-test/src/main/java/io/mosip/testrig/apirig/esignet/utils/EsignetUtil.java (1)
1060-1087:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard FAPI-JWE assertion signing and stop logging raw JWK content.
Line 1062 logs full key material, and Lines 1083-1084 can execute signing even if RSA parsing failed at Line 1064. That can produce null-key failures and stale token reuse.
🔧 Suggested fix
if (jsonString.contains("$CLIENT_ASSERTION_FAPI_JWE_JWT$")) { String oidcJWKKeyString = JWKKeyUtil.getJWKKey(OIDC_JWK_FOR_FAPI_JWE); - logger.info("oidcJWKKeyString =" + oidcJWKKeyString); + logger.debug("Loaded JWK for OIDC_JWK_FOR_FAPI_JWE"); + RSAKey fapiJweSigningKey = null; try { - oidc_JWK_Key_For_FAPI_JWE = RSAKey.parse(oidcJWKKeyString); - logger.info("oidc_JWK_Key_For_FAPI_JWE =" + oidc_JWK_Key_For_FAPI_JWE); + fapiJweSigningKey = RSAKey.parse(oidcJWKKeyString); + oidc_JWK_Key_For_FAPI_JWE = fapiJweSigningKey; + logger.debug("Parsed signing key for OIDC_JWK_FOR_FAPI_JWE"); } catch (java.text.ParseException e) { logger.error(e.getMessage()); } @@ - if (clientId != null) { + if (clientId != null && fapiJweSigningKey != null) { jsonString = replaceKeywordValue(jsonString, "$CLIENT_ASSERTION_FAPI_JWE_JWT$", - signJWKKey(clientId, oidc_JWK_Key_For_FAPI_JWE, tempUrl)); + signJWKKey(clientId, fapiJweSigningKey, tempUrl)); } else { - logger.error("Client ID not found in JSON for $CLIENT_ASSERTION_FAPI_JWE_JWT$."); + logger.error("Unable to generate $CLIENT_ASSERTION_FAPI_JWE_JWT$: missing client_id or signing key."); } }As per coding guidelines "SECRETS: Flag any credentials or API keys in code or config files."
🤖 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 `@api-test/src/main/java/io/mosip/testrig/apirig/esignet/utils/EsignetUtil.java` around lines 1060 - 1087, Remove logging of raw JWK material and ensure signing is only attempted when RSA parsing succeeded: stop calling logger.info on oidcJWKKeyString and remove logging of oidc_JWK_Key_For_FAPI_JWE in EsignetUtil; after calling RSAKey.parse (reference RSAKey.parse and variable oidc_JWK_Key_For_FAPI_JWE) check the parse result (or catch block) and only call signJWKKey(clientId, oidc_JWK_Key_For_FAPI_JWE, tempUrl) when oidc_JWK_Key_For_FAPI_JWE is non-null and valid; if parsing failed, log a safe error message (no key material) and skip replacing "$CLIENT_ASSERTION_FAPI_JWE_JWT$" to avoid null-key signing or stale tokens.
🤖 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
`@api-test/src/main/resources/esignet/FAPI/FAPIAuthorizationCode/FAPIAuthorizationCode.yml`:
- Line 29: The additionalDependencies entry currently references the old
producer ID TC_ESignet_FAPI_AuthenticateUser_forUserInfoJWE_01; update the
additionalDependencies value to the new producer ID
TC_ESignet_FAPI_AuthenticateUserV3_02 so the authenticate producer/consumer
sequencing and downstream $ID: resolution remains correct, and then scan other
YAMLs for any consumers or references still pointing at
TC_ESignet_FAPI_AuthenticateUser_forUserInfoJWE_01 and update them to
TC_ESignet_FAPI_AuthenticateUserV3_02 as well to keep dependency IDs in sync.
In
`@api-test/src/main/resources/esignet/FAPI/FAPIPartialUpdateOIDCClient/FAPIPartialUpdateOIDCClient.yml`:
- Around line 2-13: This test entry
ESignet_FAPIPartialUpdateOIDCClient_all_Valid_forUserInfoJWE_Smoke_sid consumes
the producer ID CreateOIDCClientFAPI_all_Valid_forUserInfoJWE_Smoke_sid_clientId
but lacks an explicit dependency; add an additionalDependencies entry (or update
the centralized interdependency config) referencing the producer test that
creates CreateOIDCClientFAPI_all_Valid_forUserInfoJWE_Smoke_sid (the producer
uniqueIdentifier/test name) so the producer runs before
ESignet_FAPIPartialUpdateOIDCClient_all_Valid_forUserInfoJWE_Smoke_sid and
prevents order-related failures.
In
`@api-test/src/main/resources/esignet/PartialUpdateOIDCClient/PartialUpdateOIDCClient.yml`:
- Around line 109-124: This duplicate-key negative test
(ESignet_PartialUpdateOIDCClient_MOCK_Duplicate_EncKey_forUserInfoUpdateJWE_Neg
/ uniqueIdentifier TC_ESignet_PatchOIDCClient_06) must explicitly depend on the
prior test that performs the initial key update so it cannot run first; add an
inline additionalDependencies entry referencing the prior key-update test (the
test that created/updated the same encPublicKey and/or the ID used in input,
e.g. the Create/Update test referenced by
ID:CreateOIDCClientV3_MOCK_all_Valid_forUserInfoToUpdateClient_Smoke or that
test's uniqueIdentifier) so the runner ensures the key is already set before
this duplicate-key scenario executes.
---
Outside diff comments:
In
`@api-test/src/main/java/io/mosip/testrig/apirig/esignet/utils/EsignetUtil.java`:
- Around line 1060-1087: Remove logging of raw JWK material and ensure signing
is only attempted when RSA parsing succeeded: stop calling logger.info on
oidcJWKKeyString and remove logging of oidc_JWK_Key_For_FAPI_JWE in EsignetUtil;
after calling RSAKey.parse (reference RSAKey.parse and variable
oidc_JWK_Key_For_FAPI_JWE) check the parse result (or catch block) and only call
signJWKKey(clientId, oidc_JWK_Key_For_FAPI_JWE, tempUrl) when
oidc_JWK_Key_For_FAPI_JWE is non-null and valid; if parsing failed, log a safe
error message (no key material) and skip replacing
"$CLIENT_ASSERTION_FAPI_JWE_JWT$" to avoid null-key signing or stale tokens.
🪄 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: ffbbaa16-1fd8-47cc-945a-ea354aaae2e6
📒 Files selected for processing (22)
api-test/pom.xmlapi-test/src/main/java/io/mosip/testrig/apirig/esignet/utils/EsignetUtil.javaapi-test/src/main/resources/esignet/EndToEndFlowWithV3MOCK/userinfo/GetOidcUserInfo.ymlapi-test/src/main/resources/esignet/FAPI/FAPIAuthenticateUser/FAPIAuthenticateUser.ymlapi-test/src/main/resources/esignet/FAPI/FAPIAuthorizationCode/FAPIAuthorizationCode.ymlapi-test/src/main/resources/esignet/FAPI/FAPICreateOIDCClientV3/FAPICreateOIDCClient.hbsapi-test/src/main/resources/esignet/FAPI/FAPICreateOIDCClientV3/FAPICreateOIDCClient.ymlapi-test/src/main/resources/esignet/FAPI/FAPIGenerateToken/FAPIGenerateToken.ymlapi-test/src/main/resources/esignet/FAPI/FAPIGetUserInfo/FAPIGetUserInfo.ymlapi-test/src/main/resources/esignet/FAPI/FAPIOauthDetails/FAPIOauthDetails.ymlapi-test/src/main/resources/esignet/FAPI/FAPIPartialUpdateOIDCClient/FAPIPartialUpdateOIDCClient.hbsapi-test/src/main/resources/esignet/FAPI/FAPIPartialUpdateOIDCClient/FAPIPartialUpdateOIDCClient.ymlapi-test/src/main/resources/esignet/FAPI/OauthFAPI/OauthFAPI.ymlapi-test/src/main/resources/esignet/OAuthDetailsRequest/OAuthDetailsRequest.ymlapi-test/src/main/resources/esignet/OTPAuthFactorFlow/ConsentNoCapture/AuthenticateConsentNoCapture/AuthenticateConsentNoCapture.ymlapi-test/src/main/resources/esignet/OTPAuthFactorFlow/ConsentNoCapture/OAuthConsentNoCapture/OAuthConsentNoCapture.ymlapi-test/src/main/resources/esignet/OTPAuthFactorFlow/VerifiedClaims/OAuthDetails/OAuthDetailsRequest.ymlapi-test/src/main/resources/esignet/OidcClient/OIDCClient.ymlapi-test/src/main/resources/esignet/OidcClient/OIDCClientFAPI.hbsapi-test/src/main/resources/esignet/PartialUpdateOIDCClient/PartialUpdateOIDCClient.ymlapi-test/src/main/resources/esignet/PartialUpdateOIDCClient/PartialUpdateOIDCClientNegSce.hbsapi-test/testNgXmlFiles/esignetSuite.xml
💤 Files with no reviewable changes (2)
- api-test/src/main/resources/esignet/OTPAuthFactorFlow/VerifiedClaims/OAuthDetails/OAuthDetailsRequest.yml
- api-test/src/main/resources/esignet/OTPAuthFactorFlow/ConsentNoCapture/OAuthConsentNoCapture/OAuthConsentNoCapture.yml
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
… fixed the consent issue Signed-off-by: prathmeshj12 <prathmesh.j@cyberpwn.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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
`@api-test/src/main/resources/esignet/PartialUpdateOIDCClient/PartialUpdateOIDCClient.yml`:
- Line 137: The inputTemplate value currently points to a template named
PartialUpdateOIDCClientNegSce which implies a negative scenario; update the test
to use a correctly named positive template or rename the template to something
neutral/accurate (e.g., PartialUpdateOIDCClientWithInlineJWK) and update the
inputTemplate reference accordingly, or swap it to the existing standard
positive template if one exists so the file's inputTemplate correctly reflects a
positive test case.
- Around line 113-129: Rename the test
ESignet_PartialUpdateOIDCClient_MOCK_Duplicate_EncKey_forUserInfoUpdateJWE_Neg
to reflect idempotency/success rather than a negative case (for example
ESignet_PartialUpdateOIDCClient_MOCK_Duplicate_EncKey_Idempotency_Pos), and
update any matching identifiers or references (uniqueIdentifier
TC_ESignet_PatchOIDCClient_06, inputTemplate
esignet/PartialUpdateOIDCClient/PartialUpdateOIDCClient, and any CI/test-suite
references) so the name and metadata consistently indicate this is a
positive/idempotency validation rather than a negative test.
- Around line 36-61: The negative test
ESignet_PartialUpdateOIDCClient_MOCK_Invlalid_alg_value_Neg mixes multiple
invalid fields (kty, n, alg, possibly kid/clientId) which obscures which
validation triggered the error; update the test inputTemplate/input so only the
primary field under test (alg = "" in PartialUpdateOIDCClientNegSce) is invalid
and make all other fields (kty, n, kid, clientId, e, use) valid or canonical
values, and split out separate negative tests for kty and n (and any other
cases) so each test validates a single failure mode.
🪄 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: 87d49183-65ba-4dbb-a8a6-f623a0c5301f
📒 Files selected for processing (9)
api-test/src/main/resources/config/testCaseInterDependency_mock.jsonapi-test/src/main/resources/config/testCaseInterDependency_mosip-id.jsonapi-test/src/main/resources/config/testCaseInterDependency_sunbirdrc.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/FAPIGenerateToken/FAPIGenerateToken.ymlapi-test/src/main/resources/esignet/FAPI/FAPIOauthDetails/FAPIOauthDetails.ymlapi-test/src/main/resources/esignet/FAPI/FAPIPartialUpdateOIDCClient/FAPIPartialUpdateOIDCClient.ymlapi-test/src/main/resources/esignet/PartialUpdateOIDCClient/PartialUpdateOIDCClient.yml
… fixed the consent issue Signed-off-by: prathmeshj12 <prathmesh.j@cyberpwn.com>
mohanachandran-s
approved these changes
May 20, 2026
zesu22
approved these changes
May 20, 2026
Contributor
zesu22
left a comment
zesu22
left a comment
There was a problem hiding this comment.
Approving because @mohanachandran-s already approved it
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
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ES-2961: Added test cases for JWE enc story.
ES-3003: Fixed the consent NOCAPTURE, Authentication test cases.
Summary by CodeRabbit