feat(xaa): add cross-app access (ID-JAG) tenant config types incl. mapping - #792
Conversation
Adds XAAIssuerSettings + JWTBearerSettings (per-issuer JIT + attribute/group-role mapping) and idJagSettings/idJagEnabled on the Tenant read model. Required for: descope/etc#17573 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
🐕 Review complete — View session on Shuni Portal 🐾 |
🐕 Suggested ReviewersThis PR introduces Cross-App Access (ID-JAG) tenant configuration types, including tenant settings, per-issuer JIT, and attribute/group-role mapping capabilities. Given the limited candidate pool, both contributors have relevant experience with the modified file (lib/management/types.ts). The assignment prioritizes reviewers with direct history in this file to ensure proper TypeScript type definitions and schema compatibility.
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. |
![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
Adds XAAIssuerSettings / JWTBearerSettings types plus idJagSettings/idJagEnabled on Tenant for Cross-App Access (ID-JAG) trust config — a purely additive, type-only change.
Sniffed out 0 issues. Types correctly reuse existing AttributeMapping/RoleMappings, are auto-exported via the existing export * from './management/types', and the "(read-only)" comment matches how Tenant is actually used elsewhere (only in load/loadAll/searchAll, never create/update).
Nothing to flag — good bones! 🐾
Declared coverage: FULL — 1/1 changed files reviewed.
dorsha
changed the title
The Renovate bumps of @typescript-eslint/eslint-plugin and parser to v8 (#788-#790) conflict with eslint-config-airbnb-typescript@17's peer requirement (^5.13.0), so npm install/ci fails with ERESOLVE - breaking Build, Lint, and Unit Tests across the repo. No airbnb-typescript release supports v8 (18 peers v7), so revert typescript- eslint to ^5 (matching airbnb@17) - the last known-working state. Lint and build pass. Co-Authored-By: Claude <noreply@anthropic.com>
|
🐕 Review complete — View session on Shuni Portal 🐾 |
![github-license-compliance[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
🐕 Shuni's Review
Re-review: since the last pass, the only new change is a devDependency revert (@typescript-eslint v8→v5) to unbreak install/lint; lib/management/types.ts is unchanged from the previously reviewed commit.
Sniffed out 0 issues. The lockfile revert is internally consistent — all @typescript-eslint/* packages pinned to 5.30.5, matching package.json's ^5.25.0/^5.27.0 ranges and satisfying eslint-config-airbnb-typescript's peer dependency on v5. No stray v8 references remain.
Nothing to flag — clean tail wag! 🐾
Declared coverage: PARTIAL — 2/3 changed files reviewed.
The jest v30 bump (#689) conflicts with ts-jest@29's peer (jest ^29), so npm ci fails ERESOLVE even after the typescript-eslint revert. Revert jest + @types/jest to ^29 to match ts-jest, restoring a clean npm ci. install, eslint, build, and all 453 tests pass. Co-Authored-By: Claude <noreply@anthropic.com>
|
🐕 Review complete — View session on Shuni Portal 🐾 |
There was a problem hiding this comment.
🐕 Shuni's Review
Re-review: the only new change since the last pass is another devDependency revert — jest/@types/jest v30→v29 — to match ts-jest's peer dependency range. lib/management/types.ts is unchanged.
Sniffed out 0 issues. Lockfile is internally consistent: jest@29.7.0, @types/jest@29.5.14, ts-jest@29.0.5 all align with package.json's ^29.0.0 ranges, and no orphaned v30 jest packages remain.
Nothing to flag — good bones, still! 🐾
Declared coverage: PARTIAL — 2/3 changed files reviewed.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
🐕 Uh oh! I ran into an error. Please check the logs for details. |
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
🐕 Shuni hit a problem and couldn't finish. |
|
🐕 Review complete — View session on Shuni Portal 🐾 |
There was a problem hiding this comment.
🐕 Shuni's Review
New since last pass: a per-ssoId XAA settings API (configureXAASettings/loadXAASettings/loadAllXAASettings/deleteXAASettings), dropping the per-issuer group mapping in favor of a config-level shared mapping.
Sniffed out 1 issue:
- 1 🟡 MEDIUM:
loadXAASettings/loadAllXAASettingsdon't normalizegroupsMappingthe way the sibling SAML load path does, creating a write/read shape mismatch.
See inline comment for details. Woof!
Declared coverage: PARTIAL — 5/7 changed files reviewed.
|
@shuni review |
|
🐕 Review complete — View session on Shuni Portal 🐾 |
There was a problem hiding this comment.
🐕 Shuni's Review
Re-review: HEAD is unchanged since the last pass (still at 5a108ce), so no new code to sniff.
Sniffed out 0 new issues. The previously flagged 🟡 MEDIUM (loadXAASettings/loadAllXAASettings not normalizing groupsMapping to match the roleMappings write shape, unlike the sibling SAML path) is still present and still unresolved in its existing thread — confirmed against the current code, not re-filed here to avoid a duplicate. Checked README, tests, paths, and types for anything new; all consistent with what's already been reviewed.
Nothing new to flag — good bones, still! 🐾
Declared coverage: PARTIAL — 6/7 changed files reviewed.
Mirror the SAML load transform so a loaded XAA config round-trips into configureXAASettings. Addresses shuni review. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
🐕 Review complete — View session on Shuni Portal 🐾 |
There was a problem hiding this comment.
🐕 Shuni's Review
The prior MEDIUM finding (groupsMapping write/read shape mismatch) is fixed — transformXAASettingsResponse now normalizes role.name→roleName like the SAML path. Good fix, but it surfaced two follow-ups.
Sniffed out 2 issues:
- 1 🟠 HIGH:
XAASettingsResponse.groupsMappingtype still declaresGroupsMapping[]in types.ts, now stale vs. the transformed runtime shape. - 1 🟡 MEDIUM: optional chaining on
rm.role?.namesilently dropsroleNameinstead of failing loudly, unlike the SAML sibling.
See inline comments. Woof!
⚠️ Review follow-up issue: Prior review threads were not verified after the latest updates.
Declared coverage: PARTIAL — 1/7 changed files reviewed.
…l chaining
Declare groupsMapping as RoleMappings ({roleName, groups}) to match the load
transform; use rm.role.name (fail-fast) like the SAML transform. Addresses shuni.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
🐕 Review complete — View session on Shuni Portal 🐾 |
There was a problem hiding this comment.
🐕 Shuni's Review
Both follow-up findings from the last round are fixed in 1ea0fb1: XAASettingsResponse.groupsMapping is now correctly typed as RoleMappings, and the transform's optional chaining was dropped to fail loudly like its SAML sibling.
Sniffed out 0 new issues. Verified both fixes directly against the diff — clean.
All caught up, tail's wagging! 🐾
⚠️ Review follow-up issue: Prior review threads were not verified after the latest updates.
Declared coverage: PARTIAL — 2/7 changed files reviewed.
Add providerID to XAASettings / XAASettingsResponse, send it on configureXAASettings when set, and carry it through the load transform. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
🐕 Review complete — View session on Shuni Portal 🐾 |
There was a problem hiding this comment.
🐕 Shuni Review
Adds a providerID field to the XAA settings read/write types and load transform, to expose the selected IdP for the SSO configuration.
Actionable comments posted: 0
🧹 Nitpick comments: 1
Merge risk: 🟢 Low: single dead-code line, no functional impact.
See inline comments for details.
Declared coverage: PARTIAL — 2/7 changed files reviewed.
…sform ready === setting, so ready.providerID = setting.providerID was a no-op; providerID already round-trips. Add a loadXAASettings assertion to lock it in. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
🐕 Review complete — View session on Shuni Portal 🐾 |
- configureXAASettings(tenantId, settings, ssoId?) - ssoId optional-last to
match configureSAMLSettings/configureOIDCSettings.
- Revert unrelated devDependency downgrades (jest/@types/jest 30, typescript-
eslint 8) and restore package-lock.json to main (kills ~10k-line churn).
- Remove the read-only Tenant.idJagSettings/idJagEnabled fields (XAA is read
via the dedicated Load XAA settings API, not tenant load).
- README: correct the shared-mapping load wording ({roleName, groups}).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
🐕 Review complete — View session on Shuni Portal 🐾 |
…o revert The prior "dependency downgrade revert" was based on a wrong premise: the branch never downgraded anything - its deps (jest, @typescript-eslint) already matched main; only the version field differed. Setting @typescript-eslint to ^8 broke the eslint-config-airbnb-typescript@17 peer (^5.13.0) -> npm ci ERESOLVE. Restore the branch's original package.json + package-lock.json (green). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
🐕 Review complete — View session on Shuni Portal 🐾 |
2 participants
Required for:
https://github.com/descope/etc/issues/17573
Adds the tenant Cross-App Access (ID-JAG) types (idJagSettings/idJagEnabled) including per-issuer JIT + attribute/group-role mapping.