feat(python): add full PAM connection settings fields to CLI helper schema (KSM-1140)

[stas-schaller]

Summary

Expands the Python CLI helper's v3/field_type.py to carry the complete set of PAM connection settings fields, matching the Java SDK reference implementation and cross-checked against vault TypeScript type definitions.

Changes

New Features

• KSM-1140: PamSettings.connection.connection expanded from 8 fields to the full PamSettingsConnection schema — RDP (30 fields), SSH/Terminal (11 fields), VNC (8 fields), database (4 fields), Telnet (4 fields), Kubernetes (7 fields), plus a nested sftp object (SFTPConnection: 6 fields)
• KSM-1140: PamSettings.connection.portForward gains useSpecifiedLocalPort, localPort, and allowKeeperDBProxy (vault TS source)
• KSM-1140: allowSupplyHost moved to the PamSetting wrapper level (matches vault TypeScript PamSettingsFieldData; Java duplicates it at both levels — vault TS is the authoritative source)
• KSM-1140: PamRemoteBrowserSettings.connection gains recordingIncludeKeys, ignoreInitialSslCert, audio control fields (disableAudio, disableCopy, disablePaste, audioChannels, audioBps, audioSampleRate), and browser session fields (sessionPersistence, allowFileUploads, allowFileDownloads)

Testing

cd sdk/python/helper && python -m pytest -q
cd sdk/python/core  && python -m pytest -q

Breaking Changes

None. All additions are new keys in existing schema dicts; existing field names and types are unchanged.

Related Issues

• Closes KSM-1140
• Jira: KSM-1140, KSM-1138

[mgallego-keeper]

Thorough pass on this one. The field level work is solid: I AST diffed the new schema against Java's PamSettingsConnection (sdk/java/core/src/main/kotlin/com/keepersecurity/secretsManager/core/RecordData.kt:663) and all 76 shared fields match on both name and type, including the ones that are easy to get wrong (scrollback, serverAliveInterval, width/height/dpi/colorDepth as int while fontSize/port/destPort stay str). SFTPConnection's six fields are exact and there are no duplicate keys. Nice.

Three things I think need to change before this merges, plus a few questions. Details inline; the big one is that most of the new keys are never read at runtime.

1. The changelog bullet is in the wrong file.

The code change is entirely in sdk/python/helper, but the bullet went into sdk/python/core/README.md under ### 17.4.0, where the other ten entries are all keeper_secrets_manager_core changes. The helper's changelog is the ## Recent Changes section of sdk/python/helper/README.md, which on this branch already has a ### Version 1.1.3 heading holding KSM-1119 and KSM-1127. 5da81237 (KSM-1127) is the closest precedent: same file, same class, six days ago, logged there. More in the inline comment.

2. sdk/python/helper/setup.py still says version="1.1.2", and 1.1.2 is already on PyPI.

The helper README documents 1.1.3 but setup.py was never bumped. publish.pypi.sdk.yml has a validate-versions job that hard aborts the whole Python publish when the helper version already exists on PyPI, so the 17.4.0 release will fail as things stand. This predates your PR (it arrived with KSM-1119 and KSM-1127), but since this adds a third 1.1.3 eligible change it looks like the right moment to bump it.

3. Most of the new keys are inert.

See the inline comment on the connection list node. Short version: value_type: list is a terminal leaf for every consumer of these schema dicts, so the roughly 71 keys added under portForward and the inner connection are never validated, never filtered, and never reach template output. allowSupplyHost and the pamRemoteBrowserSettings block are live, and the latter fixes real silent data loss, so there is genuine value here; it is just concentrated in about 12 of the 82 new keys.

Tests and CI

There is no coverage for pamSettings or pamRemoteBrowserSettings today (neither class is even imported in tests/v3/v3_field_type_all_fields_test.py), and .github/workflows/test.python.helper.yml triggers on branches: [ master ] only, so Test-Python-Helper will not run on a PR into a release branch. A test asserting the new pamRemoteBrowserSettings keys survive to_dict() would cover the part that actually changes behavior. Widening that workflow's branches: to include release/sdk/python/** might be worth its own ticket.

Smaller notes

• Java groups these fields with // RDP specific settings style comment headers. 79 keys in one flat block will be painful to re-diff against Java next time; copying the headers over would help a lot.
• The 11 live pamRemoteBrowserSettings keys render as <#ADD: Insert a bool> in ksm secret template field because they have no desc. Consistent with their neighbours, but since these are the ones users will actually see, a short desc on each would be a real win.
• resizeMethod and colorScheme got reordered, which adds churn to the diff without changing anything.
• Description counts are slightly off: Java's VNC group is 7 fields, not 8, and "database (4 fields)" counts database and dbConnectionMethod, which already shipped in KSM-1127, so this PR adds 2 there.
• Adjacent gaps if full PAM coverage is the goal, both of which would take effect immediately since they sit under dict nodes: PamHostname is missing allowSupplyUser and allowSupplyHost (Java's Host at RecordData.kt:369 has both, and pamHostname is used by the built in pamDatabase, pamDirectory and pamMachine record types, so those two keys are being dropped from user input today), and PamResources is missing adminCredentialRef (Java's PamResource at RecordData.kt:426).

[mgallego-keeper]

Re-reviewed at ef1fd54. All three asks from round 1 are done, and done correctly. Before the new findings, I owe you corrections: I got the field sourcing wrong last round, and your PR description was right.

I found the vault TypeScript locally (vault/js/lib/record-types/field-data-pam-settings.ts plus field-data.ts), and it confirms every name I questioned.

Round-1 doubt	Verdict	Vault source
sessionPersistence, allowFileUploads, allowFileDownloads	all real, correctly named	ConnectionSettingsHTTP :156-171. sessionPersistence is the SessionPersistence enum (none/user/resource), audioChannels is 1 | 2, audioBps is 8 | 16
allowKeeperDBProxy	real	PamPortForwardSettings :285-291
rbiSettings: {sessionPersistence}	real, and not a duplicate	KeeperDbRbiSettings / KeeperDBSettings :235-242, reached via DatabaseConnectionSettings :244

Two corrections that cut the other way:

• I said Java "would choke" on the helper's extra keys. It will not. Record data is decoded with nonStrictJson, which sets ignoreUnknownKeys = true (SecretsManager.kt:1698, used for KeeperRecordData at :1385). Unknown keys are silently ignored, which is harder to debug than an exception but is not a crash.
• I said pamHostname is missing allowSupplyUser and allowSupplyHost. Drop that one: the vault's HostFieldData is {hostName, port} only (field-data.ts:132), and Java's Host is the outlier. In the vault, allowSupplyUser lives on the connection settings and allowSupplyHost on the field data, not on the host field. The PamResources note does stand: PamResourcesFieldData has adminCredentialRef (field-data.ts:156) with a dedicated UI component (AdminCredentialField.tsx), and this schema drops it.

So the names in this PR are sound. Two things still need to change, both one-liners, details inline.

1. allowSupplyHost is one level too deep, so the only live pamSettings addition in this PR is inert. The vault reads value[0].allowSupplyHost (record-types.ts:378, api-dag-pam-link.ts:233); this schema emits value[0].connection.allowSupplyHost, and no vault connection-settings type has that member. Your description names the right target (PamSettingsFieldData), the code just sits one level below it. Moving the key to the field's top-level schema fixes it; I patched and ran it to confirm.

2. The commit removes database and dbConnectionMethod, reverting KSM-1127, while this PR's own README hunk keeps the KSM-1127 line saying both fields are present. Behavior-neutral either way, but 1.1.3 cannot ship a changelog that its code contradicts. Restore the lines or retract the entry, either is fine.

What is confirmed working

The pamRemoteBrowserSettings block is the real content of this PR and it delivers. Base versus ef1fd54:

PamRemoteBrowserSettings(value={"connection": {
    "protocol": "http", "disableAudio": True, "disableCopy": False,
    "allowFileDownloads": False, "sessionPersistence": "persistent", "audioChannels": 2}}).to_dict()

# base (release/sdk/python/core/v17.4.0)
# value[0].connection == {"protocol": "http"}                       <- 5 keys silently destroyed
# ef1fd54
# value[0].connection == {"protocol": "http", "disableAudio": true, "disableCopy": false,
#                         "allowFileDownloads": false, "sessionPersistence": "persistent",
#                         "audioChannels": 2}

False, 0 and "" survive as well, since build_value only drops None. And it is not just the Python API: the same 11 keys now survive Parser plus Record via f.pamRemoteBrowserSettings={...}, and they appear in both ksm secret template field pamRemoteBrowserSettings and the record template for any record type carrying the field. Junk keys are still filtered. That is the silent-data-loss fix, and it is why this should land.

Release plumbing also checks out: helper 1.1.3 is not on PyPI (latest 1.1.2, 2026-06-15), validate-versions reads python3 setup.py --version so it sees the bump, the changelog is in the right file, and every claim in the new changelog line matches a key the code actually emits. Helper suite: 63 passed. git diff --check clean.

Not asking for these, but they are the interesting follow-ups

• portForward and connection should be value_type: dict, not list. This is the actual fix for KSM-1140's remaining fields, and it is a sourcing bug rather than an engine limitation: the vault and Java both model these as single objects (PamSettingsFieldData, PamSetting), and only the JS, .NET and Rust SDKs use arrays. Flip the two nodes to dict and the whole nested field set becomes live, validated and filtered with no change to build_value, is_value_valid or _expand_value_type. It is a behavior change to the emitted shape, so it wants its own ticket and tests, not a release branch. Worth deciding whether the three array-typed SDKs or the vault is wrong before anyone writes it.
• KSM-1140 is not closed by this PR. The ticket is "Python SDK: Add PAM connection settings fields (KSM-738 parity)" and its body is about the Python SDK having no typed PAM classes at all, 0 of 61 fields, with the CLI helper schema as an additional site. sdk/python/core still has no PAM types (dtos.py:798 lists the field type names and nothing more), and this PR now adds 12 helper keys. Also tracking/features/KSM-738-SDK-TRACKING.md, cited by the ticket, is not in the repo on any branch. The PR description still describes the pre-strip change (RDP 30 fields, SSH/Terminal 11, VNC 8, nested sftp, the portForward trio) and says "Breaking Changes: None. All additions are new keys", which is no longer accurate now that the diff also removes two. Worth updating the description and splitting the ticket.
• pamRemoteBrowserSettings.connection is still missing live fields: allowSupplyUser, plus acceptLanguage and suppressJsDialogs from the current vault model. Details inline.
• Still no test coverage for either class, and .github/workflows/test.python.helper.yml triggers on branches: [ master ], so Test-Python-Helper did not run on this PR: only the two Socket Security checks did. A single test asserting the 11 RBI keys survive to_dict() would pin exactly the regression class this PR fixes. Widening that workflow to release/sdk/python/** deserves its own ticket.
• Unrelated pre-existing bug, worth its own ticket, since it will bite anyone using the fields this PR adds: the documented sub-key syntax silently produces an empty field when the value is a JSON object. Field.__init__ runs the value setter first, which JSON-decodes the string into a dict, and then skips the {value_key: value} wrapping because of the isinstance(self.value, dict) is False guard (field.py:30). So c.pamRemoteBrowserSettings.connection={"disableAudio":true} yields {"type": "pamRemoteBrowserSettings", "value": []} with no error, on both base and ef1fd54. The field-level form c.pamRemoteBrowserSettings={"connection":{...}} works fine.

Happy to file the tickets (value_type: dict change, KSM-1140 split, workflow branches, Field value-key bug) if useful.

[mgallego-keeper]

Round 3. Both blockers from round 2 are fixed in dfd4f720, and I verified both by running the code rather than reading the schema. One phrase in the changelog is left over from the fix itself, and it is the only thing standing between this and an approval.

Both blockers, confirmed fixed

allowSupplyHost depth. Now a sibling of connection at the field level, so it emits value[0].allowSupplyHost, which is what vault TS reads. Confirmed through to_dict(), through template generation, and through the full Parser -> Record -> get_record_create_obj() path that ksm secret add actually runs. False survives as well, so no falsy-drop.

KSM-1127 revert. database and dbConnectionMethod are restored byte identical to 5da81237. They no longer appear in the PR diff at all, so the revert is fully undone and that changelog line is true again.

What the PR is now

The net diff against release/sdk/python/core/v17.4.0 is 3 files, +15/-2, and every added key is live and name-verified against vault TS field-data-pam-settings.ts:

Where	Keys	Live?
PamRemoteBrowserSettings.connection	recordingIncludeKeys, ignoreInitialSslCert, disableAudio, disableCopy, disablePaste, audioChannels, audioBps, audioSampleRate, sessionPersistence, allowFileUploads, allowFileDownloads	yes, dict node
PamSettings (field level)	allowSupplyHost	yes, dict node

That is 12 keys, all of which survive serialization, down from the 82 claimed in round 1 of which roughly 71 were inert. This is a much better PR than the one I first reviewed. Helper suite: 63 passed.

The one remaining item

The KSM-1140 changelog line still says allowSupplyHost was added to PamSettings.connection. After dfd4f720 that is the wrong location, and it is the location where the value is silently discarded (value collapses to [], no error). It is a one-phrase fix and I left a suggestion block on the line with exact replacement text.

I am requesting changes only to keep the changelog from shipping guidance that loses user data on PyPI. Push that one line and I will approve immediately; nothing else is outstanding on my side.

Not blocking, for the tracking ticket

Unchanged from round 2, all still unfiled and none of them belong in this PR:

1. portForward and connection should be value_type: dict, not list. The vault and Java both model them as single objects. This is the real KSM-1140 fix and it is a breaking change, so it needs its own ticket.
2. The outer connection wrapper means value[0].connection is {portForward, connection} rather than vault's PamConnectionSettings. Same root cause as 1.
3. KSM-1140's actual scope is typed PAM classes in sdk/python/core, still 0 of 61 fields, so this PR does not close the ticket.
4. Test-Python-Helper is branches: [ master ], so it did not run on this release-branch PR. I ran the suite locally instead.
5. Field.__init__ dropping the value_key wrapper for a JSON-object sub-key (field.py:30) is what turns a wrong sub-key into a silent empty field, which is exactly the failure mode in the changelog comment above.

One small pre-existing thing I noticed and am deliberately not asking you to change: no built-in record type in default_record_types.yml carries pamSettings or pamRemoteBrowserSettings, so these schemas are only reachable as custom fields or via a server-supplied record type. Related, the KSM-1127 line's "present in the pamDatabase connection schema" is loose, since the pamDatabase record type has no pamSettings field. Both predate this PR.

[mgallego-keeper]

Round 4: approved

73cbe807 takes the one blocker left from round 3, and that was the only thing between this PR and an approval. Promise kept.

Verified at this head rather than from the diff:

• allowSupplyHost is at the field level (field_type.py:880, sibling of connection at 881) and serializes to value[0].allowSupplyHost. The location the previous wording pointed at still returns {"value": []}, silently, so this correction was worth making and it now reads correctly.
• All 11 PamRemoteBrowserSettings.connection fields named in the entry exist (field_type.py:853-868) and serialize under connection.
• database and dbConnectionMethod are still inside the inner connection.schema (field_type.py:903-904), so the KSM-1127 entry above this one also still holds.
• Helper suite: 63 passed, 0 failed at 73cbe807.
• Clean merge against release/sdk/python/core/v17.4.0. The base's STE pass (c10a925e) touched sdk/python/core/README.md and the examples but not the helper README, so this line lands exactly as written.

All 8 review threads are resolved. Nothing further from me on the code.