feat(management): add enforceSSOExclusions to tenant create and update - #775
feat(management): add enforceSSOExclusions to tenant create and update#775hagaikali wants to merge 3 commits into
Conversation
The /v1/mgmt/tenant/create and /v1/mgmt/tenant/update endpoints accept enforceSSOExclusions and the Go SDK sends it on every write, but the Node SDK had no way to supply it. Since tenant update is a full replace, any Node SDK tenant write silently cleared an enforceSSOExclusions list set from the console or another SDK. Also adds enforceSSOExclusions and roleInheritance to the Tenant read type; both are returned by GET /v1/mgmt/tenant but were missing, which made roleInheritance write-only and unpreservable in a read-modify-write.
|
🐕 Review complete — View session on Shuni Portal 🐾 |
🐕 Suggested ReviewersThe review assignment balances breadth of coverage across core implementation, testing, and documentation, leveraging contributors with deep and focused experience on tenant management for an efficient and comprehensive review process.
Suggested by Shuni based on git history and PR context. Names are not @-mentioned to avoid notifying anyone — request a review from whoever fits best. |
|
@dorsha @itaihanski |
![shuni-bot[bot]](https://avatars.githubusercontent.com/in/2536750?s=60&v=4){kind=small}
There was a problem hiding this comment.
🐕 Shuni's Review
Threads an optional enforceSSOExclusions?: string[] through tenant create/createWithId/update and adds enforceSSOExclusions + roleInheritance to the read-side Tenant type.
The code is clean — I checked every positional slot in all three signatures against the README examples and the test call sites, and they line up exactly. Appending the param last keeps existing callers source-compatible. All findings below are in the README.
Sniffed out 4 issues:
- 1 🟡 MEDIUM:
enforceSSOExclusionslabel says "user IDs" but the example passes an email - 3 🟢 LOW: stale
createWithIdexample, overbroad omit-warning, hardcodedenforceSSO: truein copy-paste examples
See inline comments. Good bones! Woof!
| false, // disabled | ||
| '', // parent tenant ID | ||
| 'none', // roleInheritance | ||
| ['admin@domain.com'], // enforceSSOExclusions - user IDs excluded from SSO enforcement |
There was a problem hiding this comment.
🟡 MEDIUM: The label says user IDs, but the value is a login ID (admin@domain.com). This README treats those as distinct elsewhere (e.g. the batch-delete example explicitly calls for Descope user IDs like U2abc...).
Since the SDK does no client-side validation, whichever one is wrong produces a request that succeeds but silently doesn't match the intended user — the admin stays SSO-enforced with no error surfaced. Worth confirming against the API and making the label and the value agree (same on line 644).
| ); | ||
|
|
||
| // You can optionally set your own ID when creating a tenant | ||
| await descopeClient.management.tenant.createWithId('my-custom-id', 'My Tenant', ['domain.com'], { |
There was a problem hiding this comment.
🟢 LOW: createWithId takes the same enforceSSO/disabled/parent/roleInheritance/enforceSSOExclusions args, but this example was left at 4 args while create and update were expanded.
Reads like the new field isn't available on this path, which would push users toward create-then-update — exactly the full-replace call the note two lines below warns about.
| }); | ||
|
|
||
| // Update will override all fields as is. Use carefully. | ||
| // Any field you omit is cleared on the tenant, so read the tenant first and pass |
There was a problem hiding this comment.
🟢 LOW: "Any field you omit is cleared ... read the tenant first and pass back the values you want to keep" is broader than update actually is.
update has no parent parameter and never sends defaultRoles, and the Tenant read type has no parent field — so those two can't be "passed back" and aren't at risk. Scoping the sentence to update's own parameters avoids implying a read-modify-write is needed to protect them.
| 'My Tenant', | ||
| ['domain.com', 'another-domain.com'], | ||
| { customAttributeName: 'val' }, | ||
| true, // enforceSSO |
There was a problem hiding this comment.
🟢 LOW: This example hardcodes enforceSSO: true on the call the note above describes as a full replace. true is the coherent value for demonstrating exclusions, so I wouldn't flip it — but the existing warning only covers omitted fields, not the values the snippet hands you. Someone copy-pasting this to rename a tenant turns SSO enforcement on. A short // existing value — read it back before copying style caution would close that gap (same on line 621).
Use the README's user-ID placeholder instead of an email for enforceSSOExclusions, expand the createWithId example to show the same optional arguments, scope the full-replace warning to the parameters update actually takes, and note that the sample values are illustrative rather than defaults.
1 participant
The /v1/mgmt/tenant/create and /v1/mgmt/tenant/update endpoints accept enforceSSOExclusions and the Go SDK sends it on every write, but the Node SDK had no way to supply it. Since tenant update is a full replace, any Node SDK tenant write silently cleared an enforceSSOExclusions list set from the console or another SDK.
Also adds enforceSSOExclusions and roleInheritance to the Tenant read type; both are returned by GET /v1/mgmt/tenant but were missing, which made roleInheritance write-only and unpreservable in a read-modify-write.
Related Issues
No Issue submitted for it.
Noticed during day-to-day working and submitted a PR.
Description
management.tenant.create,createWithId, andupdatehad no way to sendenforceSSOExclusions, even though the endpoints they POST to accept it. Because tenantupdate is a full replace, this additionally made every Node SDK tenant write silently
erase an exclusions list configured from the console or another SDK.
This appends
enforceSSOExclusions?: string[]as the last parameter of all threemethods and sends it in the request body. With this change, the Node SDK's tenant
create/update request surface matches the Go SDK's field for field.
Must
create,createWithId, andupdateblocks inlib/management/tenant.test.tsto pass the new argument and assert it in the expectedPOST body. The existing parent-tenant test variants were intentionally left without the
new argument, so they continue to cover the omitted case. Full suite: 451/451 passing,
coverage above the configured thresholds (statements 97.89%, functions 98.41%,
lines 98.91% against a 97% gate).
createandupdateexamples now showenforceSSOExclusions, plus an explicit warning that fields omitted fromupdatearecleared.