SDKS-4694: Update PasswordCollector to handle nested Password policies.#190
Merged
Conversation
Contributor
📝 WalkthroughWalkthrough
Changes
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 docstrings
🧪 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
vibhorgoswami
temporarily deployed
to
github-pages
GitHub Actions
Inactive
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #190 +/- ##
=============================================
+ Coverage 43.98% 44.04% +0.05%
- Complexity 1231 1233 +2
=============================================
Files 306 306
Lines 9255 9250 -5
Branches 1326 1321 -5
=============================================
+ Hits 4071 4074 +3
Misses 4638 4638
+ Partials 546 538 -8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
davinci/src/test/kotlin/com/pingidentity/davinci/PasswordCollectorTest.kt (1)
183-216: Add a regression test for missingcontinueNodeinjection.Current fallback tests always set
collector.continueNode. Add one test where field policy is absent andcontinueNodeis never set, so this crash path is covered.🧪 Suggested test case
+ `@Test` + fun `no validation errors when no passwordPolicy and continueNode is not injected`() { + val collector = PasswordCollector() + collector.init(buildJsonObject { + put("type", "PASSWORD_VERIFY") + put("key", "user.password") + }) + collector.value = "any" + assertEquals(emptyList(), collector.validate()) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@davinci/src/test/kotlin/com/pingidentity/davinci/PasswordCollectorTest.kt` around lines 183 - 216, The tests assume continueNode is always injected which misses the crash path when PasswordCollector.continueNode is null; add a new unit test that initializes PasswordCollector via init(...) without adding a passwordPolicy to the field or global input and does NOT set collector.continueNode, then call collector.value = "any" and assert that collector.validate() returns an empty list (no exception). Reference PasswordCollector, init, continueNode, value and validate to locate where to add this regression test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@davinci/src/main/kotlin/com/pingidentity/davinci/collector/PasswordCollector.kt`:
- Around line 61-66: In the passwordPolicy() function update the JSON
deserialization call to use an explicit type parameter: change
json.decodeFromJsonElement(it) to json.decodeFromJsonElement<PasswordPolicy>(it)
so the PasswordPolicy type is enforced (reference: function passwordPolicy(),
symbol json.decodeFromJsonElement).
---
Nitpick comments:
In `@davinci/src/test/kotlin/com/pingidentity/davinci/PasswordCollectorTest.kt`:
- Around line 183-216: The tests assume continueNode is always injected which
misses the crash path when PasswordCollector.continueNode is null; add a new
unit test that initializes PasswordCollector via init(...) without adding a
passwordPolicy to the field or global input and does NOT set
collector.continueNode, then call collector.value = "any" and assert that
collector.validate() returns an empty list (no exception). Reference
PasswordCollector, init, continueNode, value and validate to locate where to add
this regression test.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: db76fa1b-d203-4594-9303-229f624deac8
📒 Files selected for processing (2)
davinci/src/main/kotlin/com/pingidentity/davinci/collector/PasswordCollector.ktdavinci/src/test/kotlin/com/pingidentity/davinci/PasswordCollectorTest.kt
vibhorgoswami
requested review from
george-bafaloukas-forgerock and
vahancouver
witrisna
approved these changes
Apr 28, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@davinci/src/androidTest/kotlin/com/pingidentity/davinci/Form-Field-Tests-Flow.json`:
- Around line 663-677: The fixture JSON is serializing user.password as a plain
string so Form.parse() passes formData.value[collector.id()] directly into
collector.init(...), never exercising PasswordCollector.init()'s nested
passwordPolicy or the global fallback; update at least one PASSWORD_VERIFY path
in the Form-Field-Tests-Flow.json to include a real passwordPolicy payload
(nested JSON structure matching what PasswordCollector.init() expects) for the
customForm node that maps to collector.id(), and add another case that omits the
nested policy to exercise the global-scope fallback so both branches of
PasswordCollector.init() are covered by Android tests.
- Around line 479-480: The Agreement Test button reuses id="btnErrorNode",
causing duplicate DOM ids; update the Agreement Test button's id to a unique
value (e.g., id="btnAgreementTest") in the customHTML block so it no longer
collides with the Error Node button; ensure the new id is only applied to the
Agreement Test button and update any test selectors that referenced the old id
if present (other related ids to locate in the diff: btnFormFields,
btnFormValidation, btnErrorNode).
- Around line 1257-1262: The JSON includes duplicate loopback branches that wire
multiValueSourceId "vgii63uk2p" back into node "nhsnrl9trd" multiple times (see
edge id "0rv5kwiqir" with source "nhsnrl9trd" and target "6ivnxxgt2j"); remove
the extra three loopback edges so "vgii63uk2p" only appears once on the true
"Form Fields" render path and not on eval-to-nhsnrl9trd loopbacks, and apply the
same cleanup for the repeated occurrences in the later block (around the
1489-1604 region) to make the selection graph deterministic.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d2a3f5d3-3071-43f3-b344-c76e488ce54e
📒 Files selected for processing (2)
davinci/src/androidTest/kotlin/com/pingidentity/davinci/Form-Field-Tests-Flow.jsondavinci/src/androidTest/kotlin/com/pingidentity/davinci/FormFieldsTest.kt
Comment on lines
+479
to
+480
| "customHTML": { | ||
| "value": "<div\n class=\"bg-light d-flex flex-column justify-content-center align-items-center position-absolute top-0 start-0 bottom-0 end-0 overflow-auto\">\n <div style=\"max-width: 400px; min-width: 400px; width: 100%\">\n <div class=\"card shadow mb-5\">\n <div class=\"card-body p-5 d-flex flex-column\">\n <img class=\"companyLogo align-self-center mb-5\" alt=\"\ncompanyName {{global.variables.companyName}}\n✕\" />\n <h1 class=\"text-center mb-4\">S</h1>\n <p class=\"text-muted text-center\">Select Test Form</p>\n <p class=\"text-danger mdi mdi-alert-circle\" data-id=\"feedback\" data-skcomponent=\"skerror\"></p>\n <form id=\"success\" data-id=\"success\">\n <div class=\"d-flex flex-column\">\n <button data-id=\"button\" type=\"submit\" class=\"btn btn-primary mb-3\" data-skcomponent=\"skbutton\"\n data-skbuttontype=\"form-submit\" data-skform=\"success\" id=\"btnFormFields\"\n data-skbuttonvalue=\"Form Fields\">\n Form Fields\n </button>\n <div class=\"d-flex flex-column\">\n <button data-id=\"button\" class=\"btn btn-link\" data-skcomponent=\"skbutton\"\n data-skbuttontype=\"form-submit\" data-skform=\"success\" id=\"btnFormValidation\"\n data-skbuttonvalue=\"Form Validation\">\n Form Validation\n </button>\n </div>\n <div class=\"d-flex flex-column\">\n <button data-id=\"button\" class=\"btn btn-link\" data-skcomponent=\"skbutton\"\n data-skbuttontype=\"form-submit\" data-skform=\"success\" id=\"btnErrorNode\"\n data-skbuttonvalue=\"Error Node\">\n Error Node\n </button>\n </div>\n <div class=\"d-flex flex-column\">\n <button data-id=\"button\" class=\"btn btn-link\" data-skcomponent=\"skbutton\"\n data-skbuttontype=\"form-submit\" data-skform=\"success\" id=\"btnErrorNode\"\n data-skbuttonvalue=\"Agreement Test\">\n Agreement Test\n </button>\n </div>\n </form>\n </div>\n </div>\n </div>\n</div>" |
Contributor
There was a problem hiding this comment.
Give the Agreement button a unique HTML id.
Both the "Error Node" and "Agreement Test" buttons use id="btnErrorNode". Duplicate ids make the template invalid DOM and can break selector-based UI automation against this page.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@davinci/src/androidTest/kotlin/com/pingidentity/davinci/Form-Field-Tests-Flow.json`
around lines 479 - 480, The Agreement Test button reuses id="btnErrorNode",
causing duplicate DOM ids; update the Agreement Test button's id to a unique
value (e.g., id="btnAgreementTest") in the customHTML block so it no longer
collides with the Error Node button; ensure the new id is only applied to the
Agreement Test button and update any test selectors that referenced the old id
if present (other related ids to locate in the diff: btnFormFields,
btnFormValidation, btnErrorNode).
Comment on lines
+663
to
+677
| "formData": { | ||
| "value": [ | ||
| { | ||
| "key": "user.username", | ||
| "value": "[\n {\n \"children\": [\n {\n \"text\": \"default username\"\n }\n ]\n }\n]" | ||
| }, | ||
| { | ||
| "key": "user.email", | ||
| "value": "[\n {\n \"children\": [\n {\n \"text\": \"default email\"\n }\n ]\n }\n]" | ||
| }, | ||
| { | ||
| "key": "user.password", | ||
| "value": "[\n {\n \"children\": [\n {\n \"text\": \"default password\"\n }\n ]\n }\n]" | ||
| } | ||
| ] |
Contributor
There was a problem hiding this comment.
Add a real passwordPolicy payload to at least one PASSWORD_VERIFY path.
Both customForm nodes still serialize user.password as a plain value. In davinci/src/main/kotlin/com/pingidentity/davinci/collector/Form.kt:29-60, Form.parse() passes formData.value[collector.id()] straight into collector.init(...), so this fixture never drives the new nested-policy path in PasswordCollector.init(). There is also no companion case here that exercises the global-scope fallback. As written, the PR’s core behavior can regress without any Android test catching it.
Also applies to: 901-915
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@davinci/src/androidTest/kotlin/com/pingidentity/davinci/Form-Field-Tests-Flow.json`
around lines 663 - 677, The fixture JSON is serializing user.password as a plain
string so Form.parse() passes formData.value[collector.id()] directly into
collector.init(...), never exercising PasswordCollector.init()'s nested
passwordPolicy or the global fallback; update at least one PASSWORD_VERIFY path
in the Form-Field-Tests-Flow.json to include a real passwordPolicy payload
(nested JSON structure matching what PasswordCollector.init() expects) for the
customForm node that maps to collector.id(), and add another case that omits the
nested policy to exercise the global-scope fallback so both branches of
PasswordCollector.init() are covered by Android tests.
Comment on lines
+1257
to
+1262
| "data": { | ||
| "id": "0rv5kwiqir", | ||
| "source": "nhsnrl9trd", | ||
| "target": "6ivnxxgt2j", | ||
| "multiValueSourceId": "vgii63uk2p" | ||
| }, |
Contributor
There was a problem hiding this comment.
Remove the extra "Form Fields" loopback branches.
multiValueSourceId: "vgii63uk2p" is wired four times: once to the actual "Form Fields" render path and three more times back into nhsnrl9trd through eval nodes. That makes the selection graph ambiguous and potentially order-dependent instead of deterministic.
Also applies to: 1489-1604
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@davinci/src/androidTest/kotlin/com/pingidentity/davinci/Form-Field-Tests-Flow.json`
around lines 1257 - 1262, The JSON includes duplicate loopback branches that
wire multiValueSourceId "vgii63uk2p" back into node "nhsnrl9trd" multiple times
(see edge id "0rv5kwiqir" with source "nhsnrl9trd" and target "6ivnxxgt2j");
remove the extra three loopback edges so "vgii63uk2p" only appears once on the
true "Form Fields" render path and not on eval-to-nhsnrl9trd loopbacks, and
apply the same cleanup for the repeated occurrences in the later block (around
the 1489-1604 region) to make the selection graph deterministic.
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.
JIRA Ticket
SDKS-4694
Description
Matches the iOS implementation of PasswordPolicy verify component for retrieving the password policy. Also adds a backup option to retrieve the password policy from the global scope if the internal password_verify component is not available.
Summary by CodeRabbit
Refactor
New Features