Conversation
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
…esponse Add idJagSettings and idJagEnabled to the Tenant struct so callers of LoadSettings / LoadAllSettings can read a tenant's Cross-App Access (XAA / ID-JAG) trust config. The backend LoadSSOSettingsResponse embeds the Tenant proto, which carries these two fields (JWTBearerSettings + bool); they were previously silently dropped on unmarshal. Also complete JWTBearerSettings with the jwt-bearer grant-type selector fields (audience/scope/custom-claims) that the backend proto carries, so the full trust config is readable. Read-only additions; no write-path (makeCreate*/makeUpdate*Request) changes. No int64 fields, so no json:",string" tags needed. Co-Authored-By: Claude <noreply@anthropic.com>
…tings Surfaces the Cross-App Access (ID-JAG) per-issuer JIT + attribute/group-role mapping (jitDisabled, attributeMapping, roleMappings, defaultSSORoles, groupsPriority, allowOverrideRoles) on the 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 with attribute and group mapping capabilities. The changes span from low-level type definitions to SDK interfaces and management logic. The review strategy prioritizes: (1) deep expertise in the core modified areas (group management logic and types), (2) SDK/interface design experience, and (3) SSO/tenant-related testing knowledge. Selected reviewers collectively cover all changed file categories with complementary expertise areas.
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
This PR adds the tenant Cross-App Access (ID-JAG) read model (idJagEnabled/idJagSettings) plus per-issuer JIT/attribute/group-role mapping fields, and (bundled in) SSO-config-scoped group loading.
No actionable issues survived verification — the new Tenant/IssuerSettings/JWTBearerSettings fields ride the existing generic JSON unmarshal path, the WithSSOID group methods are clean backward-compatible delegations (unfiltered callers still omit ssoId so older backends don't choke on unknown fields), and the shared JWTBearerSettings struct is passed whole into requests elsewhere so new fields flow through automatically. Good bones!
See inline comments for details (none this round). Woof!
Declared coverage: FULL — 6/6 changed files reviewed.
dorsha
changed the title
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
Since the last pass, the branch reworked XAA config into a proper per-SSO-config API: ConfigureXAASettings/LoadXAASettings/LoadAllXAASettings/DeleteXAASettings, dropping the per-issuer role/group mapping in favor of a config-level shared mapping (mirrors the SAML settings shape).
No new issues survived verification — the new endpoints, request building, and unmarshal helpers all follow the exact conventions of the existing ConfigureSAMLSettings/ConfigureOIDCSettings/DeleteSettings code, and test coverage for the new methods (success, ssoID variants, missing-arg errors) is solid. group.go/group_test.go are unchanged since the prior review and were skipped accordingly.
Good bones! Woof!
Declared coverage: PARTIAL — 7/9 changed files reviewed.
|
@shuni review |
|
🐕 Review complete — View session on Shuni Portal 🐾 |
There was a problem hiding this comment.
🐕 Shuni's Review
This PR adds the tenant Cross-App Access (ID-JAG) read model (idJagEnabled/idJagSettings), per-issuer JIT + attribute mapping fields, and a full per-SSO-config XAA API (ConfigureXAASettings/LoadXAASettings/LoadAllXAASettings/DeleteXAASettings), plus SSO-config-scoped group loading.
No actionable issues survived verification — every new method (ConfigureXAASettings, LoadXAASettings, LoadAllXAASettings, DeleteXAASettings, the WithSSOID group variants) is a byte-for-byte match of the existing ConfigureSAMLSettings/DeleteSettings/group request-building conventions, the new Tenant/IssuerSettings/JWTBearerSettings/SSOXAASettings* fields ride the generic JSON unmarshal path, and the unusual capitalized "XAASettings" JSON key mirrors the pre-existing SSOTenantAllSettingsResponse.SSOSettings precedent. Test coverage for the new surface (success, ssoID variants, missing-arg errors) is solid.
Good bones! Woof!
Declared coverage: FULL — 9/9 changed files reviewed.
Add providerID to SSOXAASettings / SSOXAASettingsResponse and send it on ConfigureXAASettings (only when set) so the chosen IdP round-trips, mirroring SSO SAML/OIDC. Matches the backend management API field name providerID. 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
Since the last pass, the only change is exposing a selected-IdP ProviderID on the XAA settings write/read types plus 3 lines of request wiring.
Actionable comments posted: 1
- 1 🟡 Minor:
providerIDJSON tag casing references a non-existent sibling field
Merge risk: 🟢 Low: isolated, well-tested addition; the one finding is a casing/verification concern, not a structural issue.
See inline comments for details.
Declared coverage: PARTIAL — 3/9 changed files reviewed.
|
🐕 Review complete — View session on Shuni Portal 🐾 |
…op tenant idJag - ConfigureXAASettings ssoID is now optional-last (tenantID, settings, ssoID) to match ConfigureSAMLSettings/ConfigureOIDCSettings. - Add dedicated XAAIssuerSettings/XAAJWTBearerSettings instead of widening the shared IssuerSettings/JWTBearerSettings (stops XAA fields leaking into SSO-app / third-party-app payloads). - Remove the read-only Tenant.IDJagSettings/IDJagEnabled fields; XAA is no longer surfaced on tenant load (read via the dedicated Load XAA settings API). 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
Since the last pass, XAA settings were refactored to dedicated XAAIssuerSettings/XAAJWTBearerSettings types, ConfigureXAASettings had its settings/ssoID params reordered, and the tenant-level IDJagEnabled/IDJagSettings read model was dropped.
Actionable comments posted: 1
- 1 🟠 Major: README's XAA example wasn't updated for this refactor and no longer compiles
Merge risk: 🟡 Moderate: the SDK code/tests are internally consistent, but the primary onboarding doc for this brand-new feature is broken.
See inline comments for details.
Declared coverage: FULL — 7/7 changed files reviewed.
…ctor - Pass settings before the optional ssoID to match the new signature ConfigureXAASettings(ctx, tenantID, settings, ssoID). - Use the dedicated XAAJWTBearerSettings / XAAIssuerSettings types. - Drop the tenant-load XAA snippet: idJagSettings/idJagEnabled are no longer exposed on Tenant().Load after the XAA/tenant decoupling (read via the dedicated LoadXAASettings API). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
🐕 Review complete — View session on Shuni Portal 🐾 |
…the XAA API 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) read model (idJagSettings/idJagEnabled) including the per-issuer JIT + attribute/group-role mapping fields.