feat(backend): Deprecate domain field in favor of domains on SAML connections/account
#6186
Conversation
🦋 Changeset detectedLatest commit: b0f8cc3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 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 |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
… connection and account This commit deprecates the existing `domain` field, and introduces a new field on both models: - SAML connection - SAML account It also updates the create and update params to support the new fields
NicolasLopes7
force-pushed
the
nicolas/saml-connections-domains-field
branch
from
97982f0 to
653e04d
Compare
📝 WalkthroughWalkthroughThis change introduces support for multiple domains in SAML connections within the 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.changeset/lucky-spoons-wonder.md (1)
5-5: Fix grammatical issues in the changeset description.The static analysis tools have identified two grammatical issues that should be corrected for professional documentation:
- Add a comma before "but" when connecting two independent clauses
- Use "in" instead of "on" for the phrase "in a future API version"
-Deprecates `domain` field and introduce `domains`. Now, SAML connections support multiple domains, the `domain` field still supported but it's deprecated and will be removed on a future API version. +Deprecates `domain` field and introduce `domains`. Now, SAML connections support multiple domains, the `domain` field still supported, but it's deprecated and will be removed in a future API version.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.changeset/lucky-spoons-wonder.md(1 hunks)packages/backend/src/api/endpoints/SamlConnectionApi.ts(2 hunks)packages/backend/src/api/resources/JSON.ts(2 hunks)packages/backend/src/api/resources/SamlConnection.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
`**/*.{js,ts,tsx,jsx}`: All code must pass ESLint checks with the project's configuration. Use Prettier for consistent code formatting.
**/*.{js,ts,tsx,jsx}: All code must pass ESLint checks with the project's configuration.
Use Prettier for consistent code formatting.
packages/backend/src/api/resources/SamlConnection.tspackages/backend/src/api/resources/JSON.tspackages/backend/src/api/endpoints/SamlConnectionApi.ts
`**/*.{ts,tsx}`: Maintain comprehensive JSDoc comments for public APIs.
**/*.{ts,tsx}: Maintain comprehensive JSDoc comments for public APIs.
packages/backend/src/api/resources/SamlConnection.tspackages/backend/src/api/resources/JSON.tspackages/backend/src/api/endpoints/SamlConnectionApi.ts
`packages/**`: All publishable packages under the @clerk namespace must be located in the packages/ directory.
packages/**: All publishable packages under the @clerk namespace must be located in the packages/ directory.
packages/backend/src/api/resources/SamlConnection.tspackages/backend/src/api/resources/JSON.tspackages/backend/src/api/endpoints/SamlConnectionApi.ts
`**/*.ts`: Always define explicit return types for functions, especially public APIs. Use proper type annotations for variables and parameters where inference isn't clear. Avoid `a...
**/*.ts: Always define explicit return types for functions, especially public APIs.
Use proper type annotations for variables and parameters where inference isn't clear.
Avoidanytype; preferunknownwhen type is uncertain, then narrow with type guards.
Useinterfacefor object shapes that might be extended; usetypefor unions, primitives, and computed types.
Preferreadonlyproperties for immutable data structures.
Useprivatefor internal implementation details,protectedfor inheritance, andpublicexplicitly for clarity in public APIs.
Prefer composition and interfaces over deep inheritance chains; use mixins for shared behavior.
Use ES6 imports/exports consistently; avoid barrel files (index.ts re-exports) to prevent circular dependencies.
Use type-only imports (import type { ... }) where possible.
Useas constfor literal types and thesatisfiesoperator for type checking without widening.
Enable--incrementaland--tsBuildInfoFilefor faster builds.
Use ESLint with @typescript-eslint/recommended rules and Prettier for formatting.
Use lint-staged and Husky for pre-commit checks.
Use type-coverage to measure type safety.
packages/backend/src/api/resources/SamlConnection.tspackages/backend/src/api/resources/JSON.tspackages/backend/src/api/endpoints/SamlConnectionApi.ts
🪛 LanguageTool
.changeset/lucky-spoons-wonder.md
[uncategorized] ~5-~5: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...ains, the domain field still supported but it's deprecated and will be removed on ...
(COMMA_COMPOUND_SENTENCE)
[uncategorized] ~5-~5: The preposition “in” seems more likely in this position than the preposition “on”.
Context: ...but it's deprecated and will be removed on a future API version.
(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_ON_IN)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (8)
packages/backend/src/api/resources/JSON.ts (2)
634-634: LGTM! Proper addition of domains array support.The addition of the
domains: string[]property to theSamlConnectionJSONinterface correctly implements the new multiple domains feature while maintaining backward compatibility with the existingdomainproperty.
692-692: LGTM! Consistent implementation across SAML account connection interface.The addition of the
domains: string[]property to theSamlAccountConnectionJSONinterface is consistent with the changes made toSamlConnectionJSONand properly supports the new multiple domains functionality.packages/backend/src/api/endpoints/SamlConnectionApi.ts (2)
16-18: LGTM! Proper deprecation and new functionality implementation.The changes correctly implement the transition from single domain to multiple domains:
- The deprecation comment follows JSDoc standards and clearly communicates the migration path
- The new
domains: string[]property supports the enhanced functionality- Backward compatibility is maintained by keeping the deprecated
domainproperty
36-38: LGTM! Consistent implementation across update parameters.The changes to
UpdateSamlConnectionParamsare consistent with the create parameters and properly implement the same deprecation pattern and new functionality. The optional nature of the properties is appropriate for update operations.packages/backend/src/api/resources/SamlConnection.ts (4)
17-23: LGTM! Excellent implementation of deprecation and new functionality.The changes to the
SamlConnectionclass properly implement the migration from single domain to multiple domains:
- The deprecation comment follows JSDoc standards and is informative
- The new
domains: string[]property is well-documented- Backward compatibility is maintained
- The property ordering is logical and consistent
102-102: LGTM! Proper JSON deserialization support.The
fromJSONmethod correctly handles the newdomainsproperty from the JSON data, ensuring proper object construction with the updated interface.
129-133: LGTM! Consistent implementation across SAML account connection class.The changes to
SamlAccountConnectionare consistent with theSamlConnectionclass changes and properly implement the same deprecation pattern and new functionality.
147-147: LGTM! Complete JSON deserialization support.The
fromJSONmethod forSamlAccountConnectioncorrectly handles the newdomainsproperty, maintaining consistency with theSamlConnectionimplementation.
| '@clerk/backend': minor | ||
| --- | ||
|
|
||
| Deprecates `domain` field and introduce `domains`. Now, SAML connections support multiple domains, the `domain` field still supported but it's deprecated and will be removed on a future API version. |
There was a problem hiding this comment.
| Deprecates `domain` field and introduce `domains`. Now, SAML connections support multiple domains, the `domain` field still supported but it's deprecated and will be removed on a future API version. | |
| Introduces support for SAML connections with multiple domains, deprecating the `domain` field from `CreateSamlConnectionParams` and `UpdateSamlConnectionParams` in favor of `domains`. |
| @@ -13,7 +13,9 @@ type SamlConnectionListParams = { | |||
| type CreateSamlConnectionParams = { | |||
| name: string; | |||
| provider: SamlIdpSlug; | |||
| /** @deprecated Use `domains` array instead. This field will be removed in a future API version. */ | |||
There was a problem hiding this comment.
Nit: Multiple lines for JSDocs, but like a pattern than anything else.
| /** @deprecated Use `domains` array instead. This field will be removed in a future API version. */ | |
| /** | |
| * @deprecated Use `domains` array instead. This field will be removed in a future API version. | |
| */ |
| domain: string; | ||
| domains: string[]; |
There was a problem hiding this comment.
We should add deprecation comments here as well.
| @@ -14,9 +14,13 @@ export class SamlConnection { | |||
| */ | |||
| readonly name: string, | |||
| /** | |||
| * The domain of your organization. Sign in flows using an email with this domain will use the connection. | |||
| * @deprecated The domain of your organization. Sign in flows using an email with this domain will use the connection. | |||
There was a problem hiding this comment.
| * @deprecated The domain of your organization. Sign in flows using an email with this domain will use the connection. | |
| * @deprecated Use `domains` instead. |
LauraBeatris
changed the title
domain field in favor of domains on SAML connection and accountdomain field in favor of domains on SAML connections/account
Description
This commit deprecates the existing
domainfield, and introduces a newdomainsfield on both models:It also updates the create and update params to support the new fields
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
New Features
Deprecation Notice
Documentation