Skip to content
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(twenty-server): add trusted domain - backend crud #10290

Merged
merged 21 commits into from
Feb 21, 2025

Conversation

AMoreaux
Copy link
Contributor

No description provided.

Introduced WorkspaceTrustedDomain entity, service, and module for managing trusted domains within workspaces. Updated Workspace entity to include a relation to trusted domains, enabling domain management tied to specific workspaces. This provides a foundation for validating and managing trusted domains.
Introduce functionality for managing workspace trusted domains, including creation, validation, and deletion. Added corresponding migrations, services, DTOs, resolvers, validations, and tests to support the feature.
@AMoreaux AMoreaux self-assigned this Feb 18, 2025
Removed validation token field and related methods, replacing it with a secure hash-based validation approach. Introduced email validation during trusted domain creation, updating the workflow and database constraints to ensure uniqueness. Cleaned up unused input classes and streamlined service methods for better maintainability.
Added support for input validation, resolver methods, and domain manager integration within the WorkspaceTrustedDomain module. Removed unused SSO IDP input and adjusted resolver method names for clarity and consistency. Updated the core engine module to include WorkspaceTrustedDomainModule.
Replaces the existing migration for workspace trusted domains with an updated version. Introduces a unique constraint on the domain and workspaceId combination to ensure data integrity. Other migration logic remains unchanged.
Introduce validation logic for workspace trusted domains, including a new mutation to validate domains using a token. Updated GraphQL schema, service, resolver, DTOs, and tests to support this functionality.
Introduced types, inputs, and mutations for managing and validating workspace trusted domains. Added health status types and queries for system and worker queue monitoring, along with updates to roles and permissions handling.
Added a new validation check to prevent re-validation of trusted domains. Updated exception codes and messages for better clarity and consistency. Enhanced test cases to cover these new scenarios.
@AMoreaux AMoreaux marked this pull request as ready for review February 19, 2025 16:45
…-allowed-domains

# Conflicts:
#	packages/twenty-front/src/generated-metadata/graphql.ts
Introduced types and mutations for managing workspace trusted domains, including creation, deletion, validation, and retrieval. This enhancement enables better control and flexibility for workspace domain management.
Copy link
Contributor

@greptile-apps greptile-apps bot left a 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 introduces a comprehensive workspace trusted domain management system, enabling domain validation and security controls for workspaces.

  • Added WorkspaceTrustedDomain entity with validation flow in /packages/twenty-server/src/engine/core-modules/workspace-trusted-domain/workspace-trusted-domain.entity.ts
  • Implemented secure domain validation via email with hash-based tokens in /packages/twenty-server/src/engine/core-modules/workspace-trusted-domain/services/workspace-trusted-domain.service.ts
  • Created new GraphQL mutations and queries for CRUD operations in /packages/twenty-front/src/generated/graphql.tsx
  • Added database migration 1739955345446-add-workspace-trusted-domain.ts with unique constraint on domain/workspace pairs
  • Removed potentially breaking FindAvailableSSOIDPInput class from SSO module without clear replacement

20 file(s) reviewed, 16 comment(s)
Edit PR Review Bot Settings | Greptile

Comment on lines 46 to 52
{capitalize(sender.firstName)} (
<Link
href={`mailto:${sender.email}`}
value={sender.email}
color={emailTheme.font.colors.blue}
/>
)<Trans>has added a trust domain: </Trans>
Copy link
Contributor

Choose a reason for hiding this comment

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

syntax: Missing space after closing parenthesis before the Trans component. This creates incorrect spacing in the rendered email.


public async up(queryRunner: QueryRunner): Promise<void> {
await queryRunner.query(
`CREATE TABLE "core"."workspaceTrustedDomain" ("id" uuid NOT NULL DEFAULT uuid_generate_v4(), "createdAt" TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT now(), "updatedAt" TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT now(), "domain" character varying NOT NULL, "isValidated" boolean NOT NULL DEFAULT false, "workspaceId" uuid NOT NULL, CONSTRAINT "IndexOnDomainAndWorkspaceId" UNIQUE ("domain", "workspaceId"), CONSTRAINT "PK_afa04c0f75f54a5e2c570c83cd6" PRIMARY KEY ("id"))`,
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider adding a length constraint to the 'domain' character varying field to prevent excessively long domains

Suggested change
`CREATE TABLE "core"."workspaceTrustedDomain" ("id" uuid NOT NULL DEFAULT uuid_generate_v4(), "createdAt" TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT now(), "updatedAt" TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT now(), "domain" character varying NOT NULL, "isValidated" boolean NOT NULL DEFAULT false, "workspaceId" uuid NOT NULL, CONSTRAINT "IndexOnDomainAndWorkspaceId" UNIQUE ("domain", "workspaceId"), CONSTRAINT "PK_afa04c0f75f54a5e2c570c83cd6" PRIMARY KEY ("id"))`,
`CREATE TABLE "core"."workspaceTrustedDomain" ("id" uuid NOT NULL DEFAULT uuid_generate_v4(), "createdAt" TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT now(), "updatedAt" TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT now(), "domain" character varying(253) NOT NULL, "isValidated" boolean NOT NULL DEFAULT false, "workspaceId" uuid NOT NULL, CONSTRAINT "IndexOnDomainAndWorkspaceId" UNIQUE ("domain", "workspaceId"), CONSTRAINT "PK_afa04c0f75f54a5e2c570c83cd6" PRIMARY KEY ("id"))`,

@Field(() => String)
@IsString()
@IsNotEmpty()
domain: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider adding @IsDomain() or custom domain validation to ensure proper domain format (e.g., example.com)

Suggested change
domain: string;
@IsDomain()
domain: string;

Copy link
Contributor

Choose a reason for hiding this comment

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

logic: This class's removal may break GraphQL schemas and queries that depend on FindAvailableSSOIDPInput type

Comment on lines 7 to 8
@Field()
@IsString()
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider adding () => String type to @field for consistency with other DTOs in the codebase

Comment on lines 256 to 259
await expect(
service.deleteTrustedDomain(workspace, trustedDomainId),
).rejects.toThrow();

Copy link
Contributor

Choose a reason for hiding this comment

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

logic: test expects generic throw() but should check for specific WorkspaceTrustedDomainException and code

Suggested change
await expect(
service.deleteTrustedDomain(workspace, trustedDomainId),
).rejects.toThrow();
await expect(
service.deleteTrustedDomain(workspace, trustedDomainId),
).rejects.toThrow(new WorkspaceTrustedDomainException(
'Trusted domain not found',
WorkspaceTrustedDomainExceptionCode.WORKSPACE_TRUSTED_DOMAIN_NOT_FOUND,
));


export enum WorkspaceTrustedDomainExceptionCode {
WORKSPACE_TRUSTED_DOMAIN_NOT_FOUND = 'WORKSPACE_TRUSTED_DOMAIN_NOT_FOUND',
WORKSPACE_TRUSTED_DOMAIN_ALREADY_VERIFIED = 'WORKSPACE_TRUSTED_DOMAIN_ALREADY_VERIFIED',
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: ALREADY_VERIFIED and ALREADY_VALIDATED (line 14) appear to be duplicate error codes for the same condition

Comment on lines 49 to 53
@Mutation(() => Boolean)
async validateWorkspaceTrustedDomain(
@Args('input')
{ validationToken, workspaceTrustedDomainId }: ValidateTrustedDomainInput,
): Promise<boolean> {
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: validateWorkspaceTrustedDomain mutation missing @AuthWorkspace guard which could allow unauthorized validation of domains

Comment on lines 66 to 68
return await this.workspaceTrustedDomainService.getAllTrustedDomainsByWorkspace(
currentWorkspace,
);
Copy link
Contributor

Choose a reason for hiding this comment

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

style: unnecessary await on return statement since the function is already async

Comment on lines 88 to 92
@OneToMany(
() => WorkspaceTrustedDomain,
(trustDomain) => trustDomain.workspace,
)
trustDomains: Relation<WorkspaceTrustedDomain[]>;
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: Missing onDelete cascade behavior for trusted domains relation. Other similar relations like workspaceUsers have this defined.

Copy link
Member

Choose a reason for hiding this comment

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

Yes need some for of cascading logic? (workspace deleted -> trustedDomains deleted)

Copy link
Member

@FelixMalfait FelixMalfait left a comment

Choose a reason for hiding this comment

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

Great work!!!

Changed mutation to return the validated domain instead of a boolean, streamlined domain validation logic, and clarified error messages. Removed redundant utility function and adjusted email template for better user guidance. These changes enhance clarity, reduce complexity, and improve user feedback when validating trusted domains.
Removed redundant tests for checkIsVerified functionality to simplify the test suite. Updated createTrustedDomain tests to reflect changes in domain validation logic and naming conventions. This improves maintainability and aligns with updated business logic.
…main

Replaced all instances of WorkspaceTrustedDomain with ApprovedAccessDomain across the application. Updated related entities, migrations, services, resolvers, and schemas to reflect the new naming convention, improving clarity and alignment with domain naming standards.
Renamed "TrustedDomain" to "ApprovedAccessDomain" for better clarity and consistency. Updated related types, inputs, and mutations accordingly to reflect the new naming convention. Removed redundant exports and adjusted queries and mutations to align with the changes.
Removed the process.env.NODE_TLS_REJECT_UNAUTHORIZED setting, which bypassed TLS verification. This improves security by preventing the acceptance of unauthorized or self-signed certificates.
Updated the formatting of the AddApprovedAccessDomain migration for better readability and consistency. No changes were made to the functionality or logic of the migration.
import { capitalize } from 'src/utils/capitalize';
import { APP_LOCALES, getImageAbsoluteURI } from 'twenty-shared';

type SendTrustDomainValidationProps = {
Copy link
Member

Choose a reason for hiding this comment

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

Still need to rename Trustxxx before we can merge?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it's in the next PR

@AMoreaux AMoreaux enabled auto-merge (squash) February 21, 2025 15:54
@AMoreaux AMoreaux merged commit bf92860 into main Feb 21, 2025
47 checks passed
@AMoreaux AMoreaux deleted the feat/add-allowed-domains branch February 21, 2025 16:02
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