Add Totp org config intergration tests #26545
Open
+775
−0
Conversation
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
WalkthroughA comprehensive integration test class is added to validate TOTP enrollment configuration, governance toggles, and inheritance behavior across multi-tenant organizations. The test covers Service Provider setup, adaptive authentication scripts, org-level enrollment settings, and inheritance scenarios in sub-organizations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
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
🤖 Fix all issues with AI agents
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/totp/TOTPOrgConfigurationTestCase.java`:
- Around line 735-743: The code calls Utils.sendPOSTMessage with sessionDataKey
returned from extractSessionDataKey without verifying it's non-null; add a guard
after calling extractSessionDataKey in TOTPOrgConfigurationTestCase to check if
sessionDataKey is null or empty and throw an informative exception (or fail the
test) before calling Utils.sendPOSTMessage; reference extractSessionDataKey, the
local variable sessionDataKey, and Utils.sendPOSTMessage so the null-check is
added in the block that currently prepares and posts credentials.
- Around line 650-689: The current code silently falls back to
getParentTOTPConfiguration() when the sub-org HTTP call fails or the property is
missing; instead, in the method that queries the sub-org TOTP config (the code
that builds the HttpGet, executes httpClient.execute(request) and inspects
response/responseString), change the control flow so that any non-200 response
or absence of the TOTP_ENROLL_CONFIG property causes the test to fail (throw an
AssertionError or use the test framework's fail/skip) rather than returning
getParentTOTPConfiguration(); specifically, after HttpResponse response =
httpClient.execute(request) check response.getStatusLine().getStatusCode() and
if it is not HttpStatus.SC_OK call fail with a clear message including the
status and responseString, and if connectors/properties are present but no prop
with name TOTP_ENROLL_CONFIG is found also fail with a clear message instead of
falling back to getParentTOTPConfiguration().
🧹 Nitpick comments (1)
modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/totp/TOTPOrgConfigurationTestCase.java (1)
320-321: Use unique sub-org names to avoid collisions across runs.If tests run in parallel or the environment is reused, fixed names can clash. Consider suffixing with a UUID.
♻️ Suggested change
+import java.util.UUID; @@ - String subOrgName = "sub-org-totp-test"; + String subOrgName = "sub-org-totp-test-" + UUID.randomUUID(); @@ - String subOrgName = "sub-org-totp-test-disabled"; + String subOrgName = "sub-org-totp-test-disabled-" + UUID.randomUUID();Also applies to: 362-363
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/totp/TOTPOrgConfigurationTestCase.java
🔇 Additional comments (5)
modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/totp/TOTPOrgConfigurationTestCase.java (5)
64-107: Concise constants and script builder.Clear constants plus centralized script generation make the scenarios easy to read and adjust.
147-313: Good coverage of org-level toggles and script overrides.The scenarios cover the key permutations cleanly.
398-449: Sub-org update helper is well-contained.Nice to have this encapsulated for future coverage expansion.
451-559: Setup/teardown helpers keep the test flow clean.Creation/update/delete steps are explicit and easy to follow.
561-633: Org CRUD & sharing helpers look solid.Encapsulation makes the sub-org tests easier to read.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Comment on lines
+650
to
+689
| String url = backendURL.replace("services/", "") + "o/" + subOrgId + | ||
| "/api/server/v1/identity-governance/TOTP"; | ||
|
|
||
| org.apache.http.client.methods.HttpGet request = new org.apache.http.client.methods.HttpGet(url); | ||
| request.addHeader(HttpHeaders.AUTHORIZATION, | ||
| "Basic " + Base64.getEncoder().encodeToString("admin:admin".getBytes(StandardCharsets.UTF_8))); | ||
| request.addHeader(HttpHeaders.ACCEPT, "application/json"); | ||
|
|
||
| HttpResponse response = httpClient.execute(request); | ||
| String responseString = EntityUtils.toString(response.getEntity()); | ||
|
|
||
| if (response.getStatusLine().getStatusCode() == HttpStatus.SC_OK) { | ||
| JSONObject responseJson = new JSONObject(responseString); | ||
| JSONArray connectors = responseJson.optJSONArray("connectors"); | ||
| if (connectors != null) { | ||
| for (int i = 0; i < connectors.length(); i++) { | ||
| JSONObject connector = connectors.getJSONObject(i); | ||
| JSONArray properties = connector.optJSONArray("properties"); | ||
| if (properties != null) { | ||
| for (int j = 0; j < properties.length(); j++) { | ||
| JSONObject prop = properties.getJSONObject(j); | ||
| if (TOTP_ENROLL_CONFIG.equals(prop.optString("name"))) { | ||
| return prop.optString("value", "true"); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // If we can't get the sub-org specific config, fall back to checking parent | ||
| // config | ||
| // This validates inheritance behavior - sub-org should use parent's | ||
| // configuration | ||
| log.info("Sub-org governance API returned status: " + response.getStatusLine().getStatusCode() + | ||
| ". Validating inheritance by confirming parent configuration is effective."); | ||
|
|
||
| // Return the parent org configuration as sub-orgs inherit from parent | ||
| return getParentTOTPConfiguration(); | ||
| } |
There was a problem hiding this comment.
Don’t silently fall back to parent config when sub-org API fails.
Returning the parent value on non-200 responses can mask sub-org API failures and yield false positives. Fail the test (or explicitly skip) when the sub-org endpoint can’t be validated or the property isn’t present.
🐛 Suggested change
- if (response.getStatusLine().getStatusCode() == HttpStatus.SC_OK) {
- JSONObject responseJson = new JSONObject(responseString);
- JSONArray connectors = responseJson.optJSONArray("connectors");
- if (connectors != null) {
- for (int i = 0; i < connectors.length(); i++) {
- JSONObject connector = connectors.getJSONObject(i);
- JSONArray properties = connector.optJSONArray("properties");
- if (properties != null) {
- for (int j = 0; j < properties.length(); j++) {
- JSONObject prop = properties.getJSONObject(j);
- if (TOTP_ENROLL_CONFIG.equals(prop.optString("name"))) {
- return prop.optString("value", "true");
- }
- }
- }
- }
- }
- }
-
- // If we can't get the sub-org specific config, fall back to checking parent
- // config
- // This validates inheritance behavior - sub-org should use parent's
- // configuration
- log.info("Sub-org governance API returned status: " + response.getStatusLine().getStatusCode() +
- ". Validating inheritance by confirming parent configuration is effective.");
-
- // Return the parent org configuration as sub-orgs inherit from parent
- return getParentTOTPConfiguration();
+ int status = response.getStatusLine().getStatusCode();
+ if (status != HttpStatus.SC_OK) {
+ Assert.fail("Sub-org governance API call failed. HTTP " + status + " Body: " + responseString);
+ }
+
+ JSONObject responseJson = new JSONObject(responseString);
+ JSONArray connectors = responseJson.optJSONArray("connectors");
+ if (connectors != null) {
+ for (int i = 0; i < connectors.length(); i++) {
+ JSONObject connector = connectors.getJSONObject(i);
+ JSONArray properties = connector.optJSONArray("properties");
+ if (properties != null) {
+ for (int j = 0; j < properties.length(); j++) {
+ JSONObject prop = properties.getJSONObject(j);
+ if (TOTP_ENROLL_CONFIG.equals(prop.optString("name"))) {
+ return prop.optString("value", "true");
+ }
+ }
+ }
+ }
+ }
+
+ Assert.fail("TOTP enrollment config not found in sub-org response. Body: " + responseString);
+ return null;📝 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
| String url = backendURL.replace("services/", "") + "o/" + subOrgId + | |
| "/api/server/v1/identity-governance/TOTP"; | |
| org.apache.http.client.methods.HttpGet request = new org.apache.http.client.methods.HttpGet(url); | |
| request.addHeader(HttpHeaders.AUTHORIZATION, | |
| "Basic " + Base64.getEncoder().encodeToString("admin:admin".getBytes(StandardCharsets.UTF_8))); | |
| request.addHeader(HttpHeaders.ACCEPT, "application/json"); | |
| HttpResponse response = httpClient.execute(request); | |
| String responseString = EntityUtils.toString(response.getEntity()); | |
| if (response.getStatusLine().getStatusCode() == HttpStatus.SC_OK) { | |
| JSONObject responseJson = new JSONObject(responseString); | |
| JSONArray connectors = responseJson.optJSONArray("connectors"); | |
| if (connectors != null) { | |
| for (int i = 0; i < connectors.length(); i++) { | |
| JSONObject connector = connectors.getJSONObject(i); | |
| JSONArray properties = connector.optJSONArray("properties"); | |
| if (properties != null) { | |
| for (int j = 0; j < properties.length(); j++) { | |
| JSONObject prop = properties.getJSONObject(j); | |
| if (TOTP_ENROLL_CONFIG.equals(prop.optString("name"))) { | |
| return prop.optString("value", "true"); | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| // If we can't get the sub-org specific config, fall back to checking parent | |
| // config | |
| // This validates inheritance behavior - sub-org should use parent's | |
| // configuration | |
| log.info("Sub-org governance API returned status: " + response.getStatusLine().getStatusCode() + | |
| ". Validating inheritance by confirming parent configuration is effective."); | |
| // Return the parent org configuration as sub-orgs inherit from parent | |
| return getParentTOTPConfiguration(); | |
| } | |
| String url = backendURL.replace("services/", "") + "o/" + subOrgId + | |
| "/api/server/v1/identity-governance/TOTP"; | |
| org.apache.http.client.methods.HttpGet request = new org.apache.http.client.methods.HttpGet(url); | |
| request.addHeader(HttpHeaders.AUTHORIZATION, | |
| "Basic " + Base64.getEncoder().encodeToString("admin:admin".getBytes(StandardCharsets.UTF_8))); | |
| request.addHeader(HttpHeaders.ACCEPT, "application/json"); | |
| HttpResponse response = httpClient.execute(request); | |
| String responseString = EntityUtils.toString(response.getEntity()); | |
| int status = response.getStatusLine().getStatusCode(); | |
| if (status != HttpStatus.SC_OK) { | |
| Assert.fail("Sub-org governance API call failed. HTTP " + status + " Body: " + responseString); | |
| } | |
| JSONObject responseJson = new JSONObject(responseString); | |
| JSONArray connectors = responseJson.optJSONArray("connectors"); | |
| if (connectors != null) { | |
| for (int i = 0; i < connectors.length(); i++) { | |
| JSONObject connector = connectors.getJSONObject(i); | |
| JSONArray properties = connector.optJSONArray("properties"); | |
| if (properties != null) { | |
| for (int j = 0; j < properties.length(); j++) { | |
| JSONObject prop = properties.getJSONObject(j); | |
| if (TOTP_ENROLL_CONFIG.equals(prop.optString("name"))) { | |
| return prop.optString("value", "true"); | |
| } | |
| } | |
| } | |
| } | |
| } | |
| Assert.fail("TOTP enrollment config not found in sub-org response. Body: " + responseString); | |
| return null; |
🤖 Prompt for AI Agents
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/totp/TOTPOrgConfigurationTestCase.java`
around lines 650 - 689, The current code silently falls back to
getParentTOTPConfiguration() when the sub-org HTTP call fails or the property is
missing; instead, in the method that queries the sub-org TOTP config (the code
that builds the HttpGet, executes httpClient.execute(request) and inspects
response/responseString), change the control flow so that any non-200 response
or absence of the TOTP_ENROLL_CONFIG property causes the test to fail (throw an
AssertionError or use the test framework's fail/skip) rather than returning
getParentTOTPConfiguration(); specifically, after HttpResponse response =
httpClient.execute(request) check response.getStatusLine().getStatusCode() and
if it is not HttpStatus.SC_OK call fail with a clear message including the
status and responseString, and if connectors/properties are present but no prop
with name TOTP_ENROLL_CONFIG is found also fail with a clear message instead of
falling back to getParentTOTPConfiguration().
Comment on lines
+735
to
+743
| String sessionDataKey = extractSessionDataKey(response); | ||
| // EntityUtils.consume(response.getEntity()); // Handled inside | ||
| // extractSessionDataKey | ||
|
|
||
| // 2. Post credentials to commonauth | ||
| response = Utils.sendPOSTMessage(sessionDataKey, commonAuthUrl, "User-Agent", | ||
| commonAuthUrl, SERVICE_PROVIDER_NAME, | ||
| USERNAME, PASSWORD, httpClient, samlSsoUrl); | ||
| return response; |
There was a problem hiding this comment.
Fail fast if sessionDataKey isn’t extracted.
Without a guard, the POST can go out with null, making failures opaque.
🔧 Suggested change
- String sessionDataKey = extractSessionDataKey(response);
+ String sessionDataKey = extractSessionDataKey(response);
+ Assert.assertNotNull(sessionDataKey, "sessionDataKey not found in login response");📝 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
| String sessionDataKey = extractSessionDataKey(response); | |
| // EntityUtils.consume(response.getEntity()); // Handled inside | |
| // extractSessionDataKey | |
| // 2. Post credentials to commonauth | |
| response = Utils.sendPOSTMessage(sessionDataKey, commonAuthUrl, "User-Agent", | |
| commonAuthUrl, SERVICE_PROVIDER_NAME, | |
| USERNAME, PASSWORD, httpClient, samlSsoUrl); | |
| return response; | |
| String sessionDataKey = extractSessionDataKey(response); | |
| Assert.assertNotNull(sessionDataKey, "sessionDataKey not found in login response"); | |
| // EntityUtils.consume(response.getEntity()); // Handled inside | |
| // extractSessionDataKey | |
| // 2. Post credentials to commonauth | |
| response = Utils.sendPOSTMessage(sessionDataKey, commonAuthUrl, "User-Agent", | |
| commonAuthUrl, SERVICE_PROVIDER_NAME, | |
| USERNAME, PASSWORD, httpClient, samlSsoUrl); | |
| return response; |
🤖 Prompt for AI Agents
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/totp/TOTPOrgConfigurationTestCase.java`
around lines 735 - 743, The code calls Utils.sendPOSTMessage with sessionDataKey
returned from extractSessionDataKey without verifying it's non-null; add a guard
after calling extractSessionDataKey in TOTPOrgConfigurationTestCase to check if
sessionDataKey is null or empty and throw an informative exception (or fail the
test) before calling Utils.sendPOSTMessage; reference extractSessionDataKey, the
local variable sessionDataKey, and Utils.sendPOSTMessage so the null-check is
added in the block that currently prepares and posts credentials.
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.
$subject
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.