-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(domain): allow to use custom domain #9811
base: main
Are you sure you want to change the base?
Conversation
Refactor subdomain generation logic to a dedicated utility while adding comprehensive tests for subdomain handling. Introduce Cloudflare-based custom domain management functions and exceptions for better domain lifecycle management. Simplify workspace domain updates and enhance overall validation processes.
Update terminology across services, entities, and exceptions from 'domain' to 'hostname' for consistency and clarity. Adjust related methods, fields, and exception codes accordingly. Ensure backward compatibility is maintained while implementing the new naming convention.
Refactor subdomain generation logic to a dedicated utility while adding comprehensive tests for subdomain handling. Introduce Cloudflare-based custom domain management functions and exceptions for better domain lifecycle management. Simplify workspace domain updates and enhance overall validation processes.
Update terminology across services, entities, and exceptions from 'domain' to 'hostname' for consistency and clarity. Adjust related methods, fields, and exception codes accordingly. Ensure backward compatibility is maintained while implementing the new naming convention.
… into feat/allow-to-use-custom-domain
…tom-domain # Conflicts: # packages/twenty-front/src/generated/graphql.tsx # packages/twenty-server/src/engine/core-modules/domain-manager/domain-manager.exception.ts # packages/twenty-server/src/engine/core-modules/domain-manager/service/domain-manager.service.ts # packages/twenty-server/src/engine/core-modules/workspace/services/workspace.service.ts # packages/twenty-server/src/engine/core-modules/workspace/workspace.resolver.ts
…tom-domain # Conflicts: # packages/twenty-server/src/engine/core-modules/auth/controllers/google-auth.controller.ts # packages/twenty-server/src/engine/core-modules/auth/services/sign-in-up.service.ts # packages/twenty-server/src/engine/core-modules/auth/strategies/google.auth.strategy.ts # packages/twenty-server/src/engine/core-modules/auth/strategies/microsoft.auth.strategy.ts
Refactored the auth flow to streamline invitation handling by replacing token-specific fields with a unified `invitation` object. Updated SSO logic to centralize logic for workspace determination, adjust redirect URI handling, and improve maintainability. Removed redundant dependencies and optimized multi-workspace support.
Eliminated the redundant "switch workspace" functionality from both server and front-end implementations. Updated related components and services to utilize hostname-based workspace handling instead. Ensured compatibility with existing workspace management flows.
Refactored domain settings to separate subdomain functionality into its own component for better modularity. Replaced inline logic with `SettingsSubdomain` to streamline `SettingsDomain` structure. Improved readability and maintainability of domain configuration flow.
Refactored domain settings to separate subdomain functionality into its own component for better modularity. Replaced inline logic with `SettingsSubdomain` to streamline `SettingsDomain` structure. Improved readability and maintainability of domain configuration flow.
…tom-domain # Conflicts: # packages/twenty-server/src/engine/core-modules/auth/services/sign-in-up.service.ts # packages/twenty-server/src/engine/core-modules/workspace-invitation/services/workspace-invitation.service.ts # packages/twenty-shared/src/index.ts
…tom-domain # Conflicts: # packages/twenty-front/src/modules/settings/admin-panel/hooks/useImpersonate.ts # packages/twenty-server/src/database/typeorm-seeds/core/feature-flags.ts
…tom-domain # Conflicts: # packages/twenty-front/src/generated/graphql.tsx # packages/twenty-front/src/modules/auth/hooks/useAuth.ts # packages/twenty-server/src/database/commands/upgrade-version/0-34/0-34-generate-subdomain.command.ts # packages/twenty-server/src/engine/core-modules/auth/auth.resolver.spec.ts # packages/twenty-server/src/engine/core-modules/auth/auth.resolver.ts # packages/twenty-server/src/engine/core-modules/auth/controllers/google-auth.controller.ts # packages/twenty-server/src/engine/core-modules/auth/controllers/microsoft-auth.controller.ts # packages/twenty-server/src/engine/core-modules/auth/controllers/sso-auth.controller.ts # packages/twenty-server/src/engine/core-modules/auth/services/auth.service.ts # packages/twenty-server/src/engine/core-modules/auth/services/sign-in-up.service.ts # packages/twenty-server/src/engine/core-modules/auth/services/social-sso.service.ts # packages/twenty-server/src/engine/core-modules/auth/strategies/google.auth.strategy.ts # packages/twenty-server/src/engine/core-modules/auth/strategies/microsoft.auth.strategy.ts # packages/twenty-server/src/engine/core-modules/domain-manager/service/domain-manager.service.ts # packages/twenty-website/src/content/developers/backend-development/server-commands.mdx
…tom-domain # Conflicts: # packages/twenty-shared/src/index.ts
…tom-domain # Conflicts: # packages/twenty-front/.eslintrc.cjs # packages/twenty-front/src/pages/settings/workspace/SettingsDomain.tsx # packages/twenty-server/src/engine/core-modules/feature-flag/enums/feature-flag-key.enum.ts
…tom-domain # Conflicts: # packages/twenty-front/src/generated-metadata/graphql.ts # packages/twenty-front/src/generated/graphql.tsx # packages/twenty-front/src/pages/settings/workspace/SettingsDomain.tsx # packages/twenty-server/src/engine/core-modules/workspace-invitation/services/workspace-invitation.service.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
This PR implements custom domain support through Cloudflare integration, allowing workspaces to use their own domains alongside the existing subdomain functionality.
- Added Cloudflare integration in
DomainManagerService
with methods for registering, updating, and verifying custom hostnames - Replaced
domainName
withhostname
field across the codebase through migration1734538670589-add-hostname-to-workspace.ts
- Added feature flag
IsCustomDomainEnabled
to control rollout of custom domain functionality - Implemented new
SettingsHostname
component with CNAME record verification and status polling - Security concern:
NODE_TLS_REJECT_UNAUTHORIZED=0
in codegen files disables SSL certificate validation, which is a significant security risk
73 file(s) reviewed, 45 comment(s)
Edit PR Review Bot Settings | Greptile
@@ -1,3 +1,5 @@ | |||
process.env.NODE_TLS_REJECT_UNAUTHORIZED = '0'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Setting NODE_TLS_REJECT_UNAUTHORIZED=0 disables SSL/TLS certificate validation. This is a critical security vulnerability and should never be used in production. Consider using proper certificates instead.
packages/twenty-front/codegen.cjs
Outdated
@@ -1,3 +1,5 @@ | |||
process.env.NODE_TLS_REJECT_UNAUTHORIZED = '0'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Setting NODE_TLS_REJECT_UNAUTHORIZED to '0' disables SSL certificate validation. This is a critical security risk and should not be used in production. Consider using proper SSL certificates instead.
onError: (error) => { | ||
// eslint-disable-next-line no-console | ||
console.error(error); | ||
redirectToDefaultDomain(); | ||
// redirectToDefaultDomain(); | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Error handling is incomplete. Users will be left in a broken state if workspace data cannot be retrieved. Either restore the redirect or implement alternative error handling.
if (!isDefined(hostname)) { | ||
url.hostname = `${subdomain}.${domainConfiguration.frontDomain}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: this condition should check subdomain.length !== 0 as well to maintain previous behavior and prevent empty subdomains
const url = hostname | ||
? // We assume that the protocol and port are the same as those of the current domain. | ||
new URL( | ||
`${currentLocation.protocol}//${hostname}${currentLocation.port ? `:${currentLocation.port}` : ''}`, | ||
) | ||
: new URL(window.location.href); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: creating a new URL object twice (line 14 and here) is inefficient - could reuse currentLocation
try { | ||
return this.workspaceRepository.save({ | ||
...workspace, | ||
...payload, | ||
}); | ||
} catch (e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: No error is being thrown or returned here. The catch block should either rethrow the error after cleanup or return a meaningful error response.
@@ -126,6 +122,10 @@ export class Workspace { | |||
@Column({ unique: true }) | |||
subdomain: string; | |||
|
|||
@Field({ nullable: true }) | |||
@Column({ unique: true, nullable: true }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: consider adding an index on the hostname column since it's used as a unique identifier and will likely be queried frequently
TypeORMModule, | ||
TypeORMModule, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: TypeORMModule is imported twice here, and it's already imported at the root level. Remove both of these lines.
WorkspaceExceptionCode.WORKSPACE_NOT_FOUND, | ||
), | ||
); | ||
workspaceValidator.assertIsDefinedOrThrow(workspace); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: assertIsDefinedOrThrow is called without an error parameter, which may result in a generic error instead of a specific workspace not found error
if (!hostname) return null; | ||
|
||
return this.domainManagerService.getCustomHostnameDetails(hostname); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: getCustomHostnameDetails call could throw an error but has no error handling - should be wrapped in try/catch
Introduced hostname support in domain handling to enhance URL generation and redirection logic. Updated related functions and state management to accommodate the hostname field. Removed unnecessary TLS environment variable from codegen files for a cleaner
...ages/twenty-server/src/engine/core-modules/domain-manager/utils/generate-random-subdomain.ts
Show resolved
Hide resolved
packages/twenty-front/src/modules/auth/sign-in-up/components/SignInUpGlobalScopeForm.tsx
Outdated
Show resolved
Hide resolved
@@ -7,12 +7,20 @@ export const useBuildWorkspaceUrl = () => { | |||
|
|||
const buildWorkspaceUrl = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we have tests on this function?
@@ -4,6 +4,7 @@ import { cookieStorageEffect } from '~/utils/recoil-effects'; | |||
export const lastAuthenticatedWorkspaceDomainState = createState< | |||
| { | |||
subdomain: string; | |||
hostname?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a big fan of having variables that can be underfined. Could we make them all required and return (the SERVER_URL domain all the way from the backend)
getPublicWorkspaceData?.hostname, | ||
); | ||
|
||
const isWorkspaceDefaultDomainWithLastAuthenticatedParamsExist = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a fan of this complexity! could we use the early return pattern instead in the effect?
if (! isMultiWorkspaceEnabled) { return}
...
query GetHostnameDetails { | ||
getHostnameDetails { | ||
hostname | ||
ownership_verification { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non standard case
formState: { isValid }, | ||
} = useForm<Form>({ | ||
const form = useForm<{ | ||
subdomain: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't hostname also part of the form?
if (isDefined(currentWorkspace?.hostname)) { | ||
pollIntervalFn = setInterval(async () => { | ||
try { | ||
const { data } = await getHostnameDetailsQuery({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need a useEffect here? can't we do it synchronously on click?
)} | ||
/> | ||
</StyledDomainFromWrapper> | ||
<Button onClick={handleSubmit(handleSave)} title={'save'}></Button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't we also translate button titles?
`ALTER TABLE "core"."workspace" RENAME COLUMN "domainName" TO "hostname"`, | ||
); | ||
await queryRunner.query( | ||
`ALTER TABLE "core"."workspace" ADD CONSTRAINT "UQ_e6fa363bdaf45cbf8ce97bcebf0" UNIQUE ("hostname")`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will break on production + How does it behave with NULLs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Postgres checks only nonnull values for unique constraints.
Why will it break the production?
@@ -60,7 +60,7 @@ export class CoreQueryBuilderFactory { | |||
throw new BadRequestException( | |||
`No object was found for the workspace associated with this API key. You may generate a new one here ${this.domainManagerService | |||
.buildWorkspaceURL({ | |||
subdomain: workspace.subdomain, | |||
workspaceSubdomainAndHostname: workspace, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not passing them as two arguments, hard to read!
@@ -37,6 +37,9 @@ export class AvailableWorkspaceOutput { | |||
@Field(() => String) | |||
subdomain: string; | |||
|
|||
@Field(() => String, { nullable: true }) | |||
hostname?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's make it mandatory? and return twenty.com
import { CustomHostname } from 'src/engine/core-modules/domain-manager/types/custom-hostname.type'; | ||
|
||
@ObjectType() | ||
export class OwnershipVerification { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's make one file per dto :)
@ObjectType() | ||
export class OwnershipVerificationHttp { | ||
@Field(() => String) | ||
http_body?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
case
@ObjectType() | ||
export class OwnershipVerification { | ||
@Field(() => String) | ||
name?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name is not nullable but can be undefined? looks inconsistent to me
|
||
const existingWorkspaceCount = await this.workspaceRepository.countBy({ | ||
subdomain, | ||
}); | ||
|
||
return `${subdomain}${existingWorkspaceCount > 0 ? `-${Math.random().toString(36).substring(2, 10)}` : ''}`; | ||
} | ||
|
||
private async getCustomHostname( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this file is too big and impossible to review :(
}); | ||
|
||
it('should return undefined if email is not a work email', () => { | ||
(isDefined as unknown as jest.Mock).mockReturnValue(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need these as casting? first time I see it!
@@ -0,0 +1,10 @@ | |||
import { isDefined } from 'src/utils/is-defined'; | |||
|
|||
export const getSubdomainNameFromDisplayName = (displayName?: string) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
DomainManagerExceptionCode, | ||
} from 'src/engine/core-modules/domain-manager/domain-manager.exception'; | ||
|
||
const isExist = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still have a problem with this naming :p isExist is not proper english
...workspace, | ||
...payload, | ||
}); | ||
} catch (e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e => error
Updated WorkspaceActivationStatus enum values to use uppercase casing for consistency with GraphQL best practices. This change ensures uniformity across the codebase and prevents potential discrepancies.
Updated redirectToWorkspaceDomain calls to use a single workspace object instead of separate subdomain and hostname parameters. This enhances code readability and ensures consistent function usage across components.
No description provided.