Add JWKS OKP key test and fix RSA validation#26882
Open
udeepa15 wants to merge 1 commit intowso2:masterfrom
Open
Add JWKS OKP key test and fix RSA validation#26882udeepa15 wants to merge 1 commit intowso2:masterfrom
udeepa15 wants to merge 1 commit intowso2:masterfrom
Conversation
Add OAuth2JWKSKeyTypesTestCase to validate OKP (Ed25519/EdDSA) keys in the JWKS endpoint and copy a test keystore containing OKP keys during test setup. Update Oauth2JWKSEndpointTestCase to locate the RSA key in the JWKS array before asserting values (instead of assuming the first entry), validate certificate thumbprint against x5c, and make validateKey operate on the provided JSON object. Also include updated test keystore (wso2carbon.p12) to support the new OKP key tests.
WalkthroughThese changes add a new OAuth2 JWKS integration test for OKP key types (Ed25519) and refactor an existing RSA key test. The new test validates OKP key presence and structure across super-tenant and tenant contexts using keystore swapping. The existing RSA test is refactored to search for RSA keys within the JWKS array instead of assuming the first key's type. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 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: 2
🧹 Nitpick comments (1)
modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2JWKSKeyTypesTestCase.java (1)
106-115: Match-and-stop on the expected OKP key to avoid future brittleness.This currently asserts on every OKP key. If additional OKP variants are added later, the test may fail even when Ed25519/EdDSA is correctly present.
Suggested refactor
- if ("OKP".equals(kty)) { - okpKeyFound = true; - validateKey(keyObj, CURVE, "The curve value"); - validateKey(keyObj, X_COORDINATE, "The x coordinate"); - validateKey(keyObj, THUMBPRINT, "The thumbprint"); - validateKey(keyObj, ALGORITHM, "The algorithm"); - Assert.assertEquals(keyObj.getString(CURVE), "Ed25519", "Incorrect curve for OKP key"); - Assert.assertEquals(keyObj.getString(ALGORITHM), "EdDSA", "Incorrect algorithm for OKP key"); + if ("OKP".equals(kty) + && "Ed25519".equals(keyObj.optString(CURVE)) + && "EdDSA".equals(keyObj.optString(ALGORITHM))) { + okpKeyFound = true; + validateKey(keyObj, CURVE, "The curve value"); + validateKey(keyObj, X_COORDINATE, "The x coordinate"); + validateKey(keyObj, THUMBPRINT, "The thumbprint"); + validateKey(keyObj, ALGORITHM, "The algorithm"); + break; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2JWKSKeyTypesTestCase.java` around lines 106 - 115, The test currently validates every OKP key found which can break when extra OKP variants are added; in the loop where you check if ("OKP".equals(kty)) and call validateKey(keyObj, CURVE/X_COORDINATE/THUMBPRINT/ALGORITHM) and the Assert.assertEquals checks, change the flow to stop after you find and validate the expected Ed25519/EdDSA key by setting okpKeyFound = true (if not already) and breaking out of the loop (or returning from the test) immediately after the assertions so only the intended OKP instance is checked; keep the same validateKey and Assert.assertEquals calls and ensure the break/return follows them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/Oauth2JWKSEndpointTestCase.java`:
- Around line 92-94: In Oauth2JWKSEndpointTestCase, guard against empty x5c
arrays before accessing index 0: when rsaKey.has(X5C) is true, fetch the
JSONArray via rsaKey.getJSONArray(X5C) and check its length() > 0 before calling
getString(0); if the array is empty, fail or skip the thumbprint assertion with
a clear message instead of letting a JSONException surface; update the code
around rsaKey, X5C, and getX509CertSHA256Thumbprint to use this length check and
handle the empty-case explicitly.
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2JWKSKeyTypesTestCase.java`:
- Around line 66-71: Add a precondition check for the test keystore file before
trying to apply configuration: verify that the File testKeystore exists (e.g.,
testKeystore.exists()) and fail fast with a clear assertion or
IllegalStateException including the keystorePath if it does not, immediately
before the serverConfigurationManager.applyConfiguration(...) call in the
OAuth2JWKSKeyTypesTestCase so setup failures are actionable.
---
Nitpick comments:
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2JWKSKeyTypesTestCase.java`:
- Around line 106-115: The test currently validates every OKP key found which
can break when extra OKP variants are added; in the loop where you check if
("OKP".equals(kty)) and call validateKey(keyObj,
CURVE/X_COORDINATE/THUMBPRINT/ALGORITHM) and the Assert.assertEquals checks,
change the flow to stop after you find and validate the expected Ed25519/EdDSA
key by setting okpKeyFound = true (if not already) and breaking out of the loop
(or returning from the test) immediately after the assertions so only the
intended OKP instance is checked; keep the same validateKey and
Assert.assertEquals calls and ensure the break/return follows them.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2JWKSKeyTypesTestCase.javamodules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/Oauth2JWKSEndpointTestCase.javamodules/integration/tests-integration/tests-backend/src/test/resources/keystores/products/wso2carbon.p12
Comment on lines
+92
to
94
| if (rsaKey.has(X5C) && rsaKey.has(X5T_SHA256)) { | ||
| String x5c = String.valueOf(rsaKey.getJSONArray(X5C).getString(0)); | ||
| String calculatedThumbPrint = getX509CertSHA256Thumbprint(x5c); |
There was a problem hiding this comment.
Guard against empty x5c arrays before reading index 0.
If x5c exists but is empty, this throws a JSONException and hides the real assertion intent.
Proposed fix
- if (rsaKey.has(X5C) && rsaKey.has(X5T_SHA256)) {
- String x5c = String.valueOf(rsaKey.getJSONArray(X5C).getString(0));
+ if (rsaKey.has(X5C) && rsaKey.has(X5T_SHA256)) {
+ Assert.assertTrue(rsaKey.getJSONArray(X5C).length() > 0,
+ "x5c must contain at least one certificate");
+ String x5c = rsaKey.getJSONArray(X5C).getString(0);
String calculatedThumbPrint = getX509CertSHA256Thumbprint(x5c);📝 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
| if (rsaKey.has(X5C) && rsaKey.has(X5T_SHA256)) { | |
| String x5c = String.valueOf(rsaKey.getJSONArray(X5C).getString(0)); | |
| String calculatedThumbPrint = getX509CertSHA256Thumbprint(x5c); | |
| if (rsaKey.has(X5C) && rsaKey.has(X5T_SHA256)) { | |
| Assert.assertTrue(rsaKey.getJSONArray(X5C).length() > 0, | |
| "x5c must contain at least one certificate"); | |
| String x5c = rsaKey.getJSONArray(X5C).getString(0); | |
| String calculatedThumbPrint = getX509CertSHA256Thumbprint(x5c); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/Oauth2JWKSEndpointTestCase.java`
around lines 92 - 94, In Oauth2JWKSEndpointTestCase, guard against empty x5c
arrays before accessing index 0: when rsaKey.has(X5C) is true, fetch the
JSONArray via rsaKey.getJSONArray(X5C) and check its length() > 0 before calling
getString(0); if the array is empty, fail or skip the thumbprint assertion with
a clear message instead of letting a JSONException surface; update the code
around rsaKey, X5C, and getX509CertSHA256Thumbprint to use this length check and
handle the empty-case explicitly.
Comment on lines
+66
to
+71
| File testKeystore = new File(keystorePath); | ||
|
|
||
| serverConfigurationManager.applyConfiguration(testKeystore, | ||
| new File(org.wso2.carbon.utils.CarbonUtils.getCarbonHome() + File.separator + "repository" + | ||
| File.separator + "resources" + File.separator + "security" + File.separator + "wso2carbon.p12"), | ||
| true, true); |
There was a problem hiding this comment.
Fail fast if the test keystore is missing.
Add an explicit existence assertion before applying configuration so setup failures are immediately actionable.
Proposed fix
File testKeystore = new File(keystorePath);
+ Assert.assertTrue(testKeystore.exists(),
+ "Test keystore not found at: " + testKeystore.getAbsolutePath());
serverConfigurationManager.applyConfiguration(testKeystore,
new File(org.wso2.carbon.utils.CarbonUtils.getCarbonHome() + File.separator + "repository" + 📝 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
| File testKeystore = new File(keystorePath); | |
| serverConfigurationManager.applyConfiguration(testKeystore, | |
| new File(org.wso2.carbon.utils.CarbonUtils.getCarbonHome() + File.separator + "repository" + | |
| File.separator + "resources" + File.separator + "security" + File.separator + "wso2carbon.p12"), | |
| true, true); | |
| File testKeystore = new File(keystorePath); | |
| Assert.assertTrue(testKeystore.exists(), | |
| "Test keystore not found at: " + testKeystore.getAbsolutePath()); | |
| serverConfigurationManager.applyConfiguration(testKeystore, | |
| new File(org.wso2.carbon.utils.CarbonUtils.getCarbonHome() + File.separator + "repository" + | |
| File.separator + "resources" + File.separator + "security" + File.separator + "wso2carbon.p12"), | |
| true, true); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2JWKSKeyTypesTestCase.java`
around lines 66 - 71, Add a precondition check for the test keystore file before
trying to apply configuration: verify that the File testKeystore exists (e.g.,
testKeystore.exists()) and fail fast with a clear assertion or
IllegalStateException including the keystorePath if it does not, immediately
before the serverConfigurationManager.applyConfiguration(...) call in the
OAuth2JWKSKeyTypesTestCase so setup failures are actionable.
Contributor
|
PR builder started |
Contributor
|
PR builder completed |
Contributor
|
Integration tests are failing due to inclusion of a binary file in the PR |
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.
Add OAuth2JWKSKeyTypesTestCase to validate OKP (Ed25519/EdDSA) keys in the JWKS endpoint and copy a test keystore containing OKP keys during test setup. Update Oauth2JWKSEndpointTestCase to locate the RSA key in the JWKS array before asserting values (instead of assuming the first entry), validate certificate thumbprint against x5c, and make validateKey operate on the provided JSON object. Also include updated test keystore (wso2carbon.p12) to support the new OKP key tests.
Summary by CodeRabbit
Release Notes