feat(shared,backend,clerk-js): Add SAML certificate validity fields#9077
Conversation
🦋 Changeset detectedLatest commit: dd5127a The changes in this PR will be included in the next version bump. This PR includes changesets to release 23 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
Disabled knowledge base sources:
📝 WalkthroughWalkthroughThis PR adds ChangesSAML IdP Certificate Timestamps
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/electron
@clerk/electron-passkeys
@clerk/eslint-plugin
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/hono
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
API Changes Report
Summary
🔴 Breaking changes index (2)Every breaking change, up front. Full diffs are in the package sections below.
@clerk/backendCurrent version: 3.10.0 🔴 Breaking Changes (2)Changed:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.changeset/little-lights-press.md (1)
7-8: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winChangeset summary is truncated mid-sentence.
Line 7 ends abruptly at "...exposing the IdP certificate validity window across" with no continuation — this will publish as an incomplete sentence in the CHANGELOG for
@clerk/clerk-js,@clerk/backend, and@clerk/shared.📝 Suggested fix
-Add `idpCertificateIssuedAt` and `idpCertificateExpiresAt` to SAML enterprise connections, exposing the IdP certificate validity window across +Add `idpCertificateIssuedAt` and `idpCertificateExpiresAt` to SAML enterprise connections, exposing the IdP certificate validity window across `@clerk/backend`, `@clerk/shared`, and `@clerk/clerk-js`.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.changeset/little-lights-press.md around lines 7 - 8, The changeset summary is cut off mid-sentence, so the generated changelog entry will publish incomplete text. Update the summary in the changeset markdown so it reads as a complete sentence describing the addition of idpCertificateIssuedAt and idpCertificateExpiresAt for SAML enterprise connections, and make sure the final wording is fully finished for the affected packages.
🧹 Nitpick comments (1)
packages/backend/src/api/__tests__/SamlConnectionApi.test.ts (1)
166-191: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd assertions for the new fields in the
updateSamlConnectiontest too.The mocked response for update also includes
idp_certificate_issued_at/idp_certificate_expires_at(via the...mockSamlConnectionResponsespread), but the test doesn't assertidpCertificateIssuedAt/idpCertificateExpiresAtlike the list/create tests do.✅ Suggested addition
expect(response.attributeMapping).toEqual({ userId: 'userId2', emailAddress: 'email2', firstName: 'firstName2', lastName: 'lastName2', }); + expect(response.idpCertificateIssuedAt).toBe(1672531200000); + expect(response.idpCertificateExpiresAt).toBe(1704067200000); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/backend/src/api/__tests__/SamlConnectionApi.test.ts` around lines 166 - 191, The updateSamlConnection test is missing coverage for the new certificate timestamp fields present in the mocked API response. In the SamlConnectionApi test suite, update the assertion block for apiClient.samlConnections.updateSamlConnection to also verify idpCertificateIssuedAt and idpCertificateExpiresAt, matching the expectations already used in the list/create tests. Keep the existing assertions for id, name, organizationId, and attributeMapping, and add the new field checks against the returned response.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.changeset/little-lights-press.md:
- Around line 7-8: The changeset summary is cut off mid-sentence, so the
generated changelog entry will publish incomplete text. Update the summary in
the changeset markdown so it reads as a complete sentence describing the
addition of idpCertificateIssuedAt and idpCertificateExpiresAt for SAML
enterprise connections, and make sure the final wording is fully finished for
the affected packages.
---
Nitpick comments:
In `@packages/backend/src/api/__tests__/SamlConnectionApi.test.ts`:
- Around line 166-191: The updateSamlConnection test is missing coverage for the
new certificate timestamp fields present in the mocked API response. In the
SamlConnectionApi test suite, update the assertion block for
apiClient.samlConnections.updateSamlConnection to also verify
idpCertificateIssuedAt and idpCertificateExpiresAt, matching the expectations
already used in the list/create tests. Keep the existing assertions for id,
name, organizationId, and attributeMapping, and add the new field checks against
the returned response.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: bc85d8e8-8cfb-41b8-9631-ce424109a13a
📒 Files selected for processing (10)
.changeset/little-lights-press.mdpackages/backend/src/api/__tests__/EnterpriseConnectionApi.test.tspackages/backend/src/api/__tests__/SamlConnectionApi.test.tspackages/backend/src/api/resources/EnterpriseConnection.tspackages/backend/src/api/resources/JSON.tspackages/backend/src/api/resources/SamlConnection.tspackages/clerk-js/src/core/resources/EnterpriseConnection.tspackages/clerk-js/src/core/resources/__tests__/Organization.test.tspackages/clerk-js/src/core/resources/__tests__/User.test.tspackages/shared/src/types/enterpriseConnection.ts
LauraBeatris
force-pushed
the
laura/add-saml-validity-fields
branch
from
636b322 to
801e7a8
Compare
LauraBeatris
force-pushed
the
laura/add-saml-validity-fields
branch
from
801e7a8 to
dd5127a
Compare
2 participants
Description
This PR updates the SAML connection resource to include SAML certificate validity fields recently introduced in FAPI/BAPI
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit