Skip to content

feat(sso): allow disabling SAML AuthnRequest signing per SSO configuration - #842

Merged
dorsha merged 2 commits into
mainfrom
feat/saml-disable-request-signing
Sep 1, 2026
Merged

feat(sso): allow disabling SAML AuthnRequest signing per SSO configuration#842
dorsha merged 2 commits into
mainfrom
feat/saml-disable-request-signing

Conversation

@dorsha

@dorsha dorsha commented Aug 31, 2026

Copy link
Copy Markdown
Member

Description

Issue: https://github.com/descope/etc/issues/18144
node-sdk: descope/node-sdk#805 · python-sdk: descope/python-sdk#1684

Descope signs the SAML AuthnRequest it sends to a tenant's IdP. A few IdPs (NetIQ Access Manager among them) reject a signed request outright when their trusted-provider entry holds no signing certificate for Descope, and until now there was no way to opt out.

This exposes the new per-SSO-configuration flag:

  • DisableSignRequest on SSOSAMLSettings and SSOSAMLSettingsByMetadata.
  • DisableSignRequest on SSOSAMLSettingsResponse, so the stored value is readable.

The flag is always sent on configure, not omitted when false: the server treats the settings object as a full replacement, so omitting it on an update would silently turn signing back on.

Defaults to false, so existing callers keep signing exactly as they do today.

…ation

Descope always signs the SAML AuthnRequest it sends to a tenant's IdP. A few
IdPs (NetIQ Access Manager among them) reject a signed request outright when
their trusted-provider entry holds no signing certificate for Descope, and
until now there was no way to opt out.

Adds DisableSignRequest to SSOSAMLSettings and SSOSAMLSettingsByMetadata, and
exposes the stored value on SSOSAMLSettingsResponse. The flag is always sent on
configure - the server treats the settings object as a full replacement, so
omitting it on an update would silently turn signing back on.

Defaults to false, so every existing configuration keeps signing as it does
today.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@shuni-bot

shuni-bot Bot commented Aug 31, 2026

Copy link
Copy Markdown

🐕 Review complete — View session on Shuni Portal 🐾

@shuni-bot

shuni-bot Bot commented Aug 31, 2026

Copy link
Copy Markdown

🐕 Suggested Reviewers

This SSO SAML feature PR requires reviewers with deep expertise across three key areas: (1) type definitions and API contracts, (2) SSO management logic implementation, and (3) test coverage. Selected reviewers span all modified files with complementary expertise: aviadl brings broad knowledge of the entire change scope including documentation, types, and management code; omercnet provides type system and documentation review; yosiharan ensures implementation and test quality for the core logic.

Reviewer Reason
aviadl aviadl has the broadest coverage with 4 commits across all three critical areas (README.md, descope/internal/mgmt/sso.go, descope/types.go). Essential for reviewing the complete change including documentation, type definitions, and implementation logic.
omercnet omercnet has strong expertise in README.md and descope/types.go (4 commits). Critical for validating the new DisableSignRequest flag definition in types.go and ensuring documentation accurately reflects the feature.
yosiharan yosiharan has deep knowledge of descope/internal/mgmt/sso.go and descope/internal/mgmt/sso_test.go (2 commits each). Essential for reviewing the core implementation logic and ensuring comprehensive test coverage for the new SAML signing flag.

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
dorsha requested a review from aviadl August 31, 2026 17:49

@shuni-bot shuni-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐕 Shuni Review

Adds a DisableSignRequest flag to SSOSAMLSettings/SSOSAMLSettingsByMetadata/SSOSAMLSettingsResponse so per-tenant SAML AuthnRequest signing can be opted out.

Actionable comments posted: 0

Merge risk: 🟢 Low: the flag is unconditionally included in the request map (matching the "full replacement" semantics described in the PR), the response struct is unmarshaled via standard JSON tags, and both toggle directions (true/false) are exercised in new tests.


Review scope: Full review

Reviewed files (4)
  • README.md
  • descope/internal/mgmt/sso.go
  • descope/internal/mgmt/sso_test.go
  • descope/types.go

@dorsha
dorsha enabled auto-merge (squash) September 1, 2026 09:29
@shuni-bot

shuni-bot Bot commented Sep 1, 2026

Copy link
Copy Markdown

🐕 Review complete — View session on Shuni Portal 🐾

🤖 Model: claude-sonnet-5

@dorsha
dorsha merged commit 1d66ea5 into main Sep 1, 2026
16 checks passed

@shuni-bot shuni-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐕 Shuni Review

No new issues found.

🤖 Model: claude-sonnet-5


Review scope: Incremental re-review

Reviewed new changes (2)
  • README.md
  • descope/types.go
Previously reviewed and unchanged: 2 files.

@dorsha
dorsha deleted the feat/saml-disable-request-signing branch September 1, 2026 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants