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.

Sign up for GitHub

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/2fa #9634

Merged
merged 6 commits into from
Jan 24, 2025
Merged

Feat/2fa #9634

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
198c54b
chore(otplib): add `otplib` package
nicolasrouanne Jan 10, 2025
a4a360c
feat(2fa): create `two-factor-method` entity and associated files
nicolasrouanne Jan 10, 2025
bc0359c
Merge branch 'main' into feat/2fa
FelixMalfait Jan 20, 2025
446b8ea
Merge branch 'main' into feat/2fa
FelixMalfait Jan 23, 2025
59f84ba
Fix: missing typeorm registration
FelixMalfait Jan 24, 2025
0d6291d
Not sure this is needed, maybe we can infer it from the underlying me…
FelixMalfait Jan 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/twenty-server/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
"monaco-editor": "^0.51.0",
"monaco-editor-auto-typings": "^0.4.5",
"openid-client": "^5.7.0",
"otplib": "^12.0.1",
"passport": "^0.7.0",
"psl": "^1.9.0",
"redis": "^4.7.0",
Expand Down
21 changes: 21 additions & 0 deletions ...nty-server/src/database/typeorm/core/migrations/common/1737033794408-two-factor-method.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import { MigrationInterface, QueryRunner } from 'typeorm';

export class TwoFactorMethod1737033794408 implements MigrationInterface {
name = 'TwoFactorMethod1737033794408';

public async up(queryRunner: QueryRunner): Promise<void> {
await queryRunner.query(
`CREATE TABLE "core"."twoFactorMethod" ("id" uuid NOT NULL DEFAULT uuid_generate_v4(), "userWorkspaceId" uuid NOT NULL, "createdAt" TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT now(), "updatedAt" TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT now(), "deletedAt" TIMESTAMP WITH TIME ZONE, CONSTRAINT "PK_752f0250dd6824289ceddd8b054" PRIMARY KEY ("id"))`,
);
await queryRunner.query(
`ALTER TABLE "core"."twoFactorMethod" ADD CONSTRAINT "FK_c1044145be65a4ee65c07e0a658" FOREIGN KEY ("userWorkspaceId") REFERENCES "core"."userWorkspace"("id") ON DELETE CASCADE ON UPDATE NO ACTION`,
);
}

public async down(queryRunner: QueryRunner): Promise<void> {
await queryRunner.query(
`ALTER TABLE "core"."twoFactorMethod" DROP CONSTRAINT "FK_c1044145be65a4ee65c07e0a658"`,
);
await queryRunner.query(`DROP TABLE "core"."twoFactorMethod"`);
}
}
2 changes: 2 additions & 0 deletions packages/twenty-server/src/database/typeorm/typeorm.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { FeatureFlagEntity } from 'src/engine/core-modules/feature-flag/feature-
import { KeyValuePair } from 'src/engine/core-modules/key-value-pair/key-value-pair.entity';
import { PostgresCredentials } from 'src/engine/core-modules/postgres-credentials/postgres-credentials.entity';
import { WorkspaceSSOIdentityProvider } from 'src/engine/core-modules/sso/workspace-sso-identity-provider.entity';
import { TwoFactorMethod } from 'src/engine/core-modules/two-factor-method/two-factor-method.entity';
import { UserWorkspace } from 'src/engine/core-modules/user-workspace/user-workspace.entity';
import { User } from 'src/engine/core-modules/user/user.entity';
import { Workspace } from 'src/engine/core-modules/workspace/workspace.entity';
Expand Down Expand Up @@ -47,6 +48,7 @@ export class TypeORMService implements OnModuleInit, OnModuleDestroy {
BillingEntitlement,
PostgresCredentials,
WorkspaceSSOIdentityProvider,
TwoFactorMethod,
],
metadataTableName: '_typeorm_generated_columns_and_materialized_views',
ssl: environmentService.get('PG_SSL_ALLOW_SELF_SIGNED')
Expand Down
49 changes: 49 additions & 0 deletions packages/twenty-server/src/engine/core-modules/two-factor-method/two-factor-method.entity.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import { Field, ObjectType } from '@nestjs/graphql';

import {
Column,
CreateDateColumn,
Entity,
JoinColumn,
ManyToOne,
PrimaryGeneratedColumn,
Relation,
UpdateDateColumn,
} from 'typeorm';

import { UserWorkspace } from 'src/engine/core-modules/user-workspace/user-workspace.entity';

@Entity({ name: 'twoFactorMethod', schema: 'core' })
@ObjectType('TwoFactorMethod')
export class TwoFactorMethod {
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit confused, this table does provide any information? no type, token, etc. why is it useful then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's because this PR is heavily WIP, nothing is really done. But:

  • the token will be derived from the Primary Key of this table
  • we only have one 2FA type which is TOTP, that's why we don't store it

Does it make sense?

@Field()
@PrimaryGeneratedColumn('uuid')
id: string;

@Field({ nullable: false })
@Column({ nullable: false, type: 'uuid' })
userWorkspaceId: string;

@Field(() => UserWorkspace)
@ManyToOne(
() => UserWorkspace,
(userWorkspace) => userWorkspace.twoFactorMethods,
{
onDelete: 'CASCADE',
},
)
@JoinColumn({ name: 'userWorkspaceId' })
userWorkspace: Relation<UserWorkspace>;

@Field()
@CreateDateColumn({ type: 'timestamptz' })
createdAt: Date;

@Field()
@UpdateDateColumn({ type: 'timestamptz' })
updatedAt: Date;

@Field({ nullable: true })
@Column({ nullable: true, type: 'timestamptz' })
deletedAt: Date;
}
14 changes: 14 additions & 0 deletions packages/twenty-server/src/engine/core-modules/two-factor-method/two-factor-method.module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import { Module } from '@nestjs/common';
import { TypeOrmModule } from '@nestjs/typeorm';

import { UserWorkspace } from 'src/engine/core-modules/user-workspace/user-workspace.entity';

import { TwoFactorMethod } from './two-factor-method.entity';
import { TwoFactorMethodService } from './two-factor-method.service';

@Module({
imports: [TypeOrmModule.forFeature([TwoFactorMethod, UserWorkspace], 'core')],
providers: [TwoFactorMethodService],
exports: [TwoFactorMethodService],
})
export class TwoFactorMethodModule {}
36 changes: 36 additions & 0 deletions ...ages/twenty-server/src/engine/core-modules/two-factor-method/two-factor-method.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import { Injectable } from '@nestjs/common';
import { InjectRepository } from '@nestjs/typeorm';

import { Repository } from 'typeorm';

import { TwoFactorMethod } from './two-factor-method.entity';
Copy link
Member

Choose a reason for hiding this comment

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

general about the naming: shouldn't we use two-factor-authentication instead of two-factor-method which seems more standard?

  • for data model: two-factor-method makes sense as a shortcut for two-factor-authentication-method
  • but for the module I would name it two-factor-authentication


@Injectable()
export class TwoFactorMethodService {
constructor(
@InjectRepository(TwoFactorMethod)
private readonly twoFactorMethodRepository: Repository<TwoFactorMethod>,
) {}

async createTwoFactorMethod(
userWorkspaceId: string,
): Promise<TwoFactorMethod> {
const twoFactorMethod = this.twoFactorMethodRepository.create({
userWorkspace: { id: userWorkspaceId },
});

return this.twoFactorMethodRepository.save(twoFactorMethod);
}

async findAll(): Promise<TwoFactorMethod[]> {
Copy link
Member

Choose a reason for hiding this comment

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

this shouldn't exist IMO:

  • if this endpoint is meant to be part of the "engine api" long term (usable by business modules), I don't think we should authorize such a getter (findAll)
  • if this endpoint is meant to be used within the engine, it's not useful as we can just call the ORM direclty and there is no additional business logic to wrap

return this.twoFactorMethodRepository.find();
}
Comment on lines +25 to +27
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: findAll() without any filters could expose all 2FA methods. Consider adding workspace-level filtering for security.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This whole service file was added as a proof of concept. It might be nice to clean it a bit. Can do it this afternoon.


async findOne(id: string): Promise<TwoFactorMethod | null> {
return this.twoFactorMethodRepository.findOne({ where: { id } });
Copy link
Member

Choose a reason for hiding this comment

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

same

}

async remove(id: string): Promise<void> {
await this.twoFactorMethodRepository.delete(id);
Copy link
Member

Choose a reason for hiding this comment

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

same

}
Comment on lines +33 to +35
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: No verification that the caller has permission to remove the 2FA method. Add authorization check.

}
10 changes: 9 additions & 1 deletion packages/twenty-server/src/engine/core-modules/user-workspace/user-workspace.entity.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,17 @@ import {
Entity,
JoinColumn,
ManyToOne,
OneToMany,
PrimaryGeneratedColumn,
Relation,
Unique,
UpdateDateColumn,
} from 'typeorm';

import { UUIDScalarType } from 'src/engine/api/graphql/workspace-schema-builder/graphql-types/scalars';
import { TwoFactorMethod } from 'src/engine/core-modules/two-factor-method/two-factor-method.entity';
import { User } from 'src/engine/core-modules/user/user.entity';
import { Workspace } from 'src/engine/core-modules/workspace/workspace.entity';
import { UUIDScalarType } from 'src/engine/api/graphql/workspace-schema-builder/graphql-types/scalars';

@Entity({ name: 'userWorkspace', schema: 'core' })
@ObjectType('UserWorkspace')
Expand Down Expand Up @@ -58,4 +60,10 @@ export class UserWorkspace {
@Field({ nullable: true })
@Column({ nullable: true, type: 'timestamptz' })
deletedAt: Date;

@OneToMany(
() => TwoFactorMethod,
(twoFactorMethod) => twoFactorMethod.userWorkspace,
)
twoFactorMethods: Relation<TwoFactorMethod[]>;
Comment on lines +64 to +68
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 @field decorator for GraphQL exposure of twoFactorMethods - this field won't be queryable via GraphQL

Comment on lines +64 to +68
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 cascade option to automatically handle deletion of associated TwoFactorMethod records

}
11 changes: 6 additions & 5 deletions packages/twenty-server/src/engine/core-modules/user-workspace/user-workspace.module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,22 +4,23 @@ import { NestjsQueryGraphQLModule } from '@ptc-org/nestjs-query-graphql';
import { NestjsQueryTypeOrmModule } from '@ptc-org/nestjs-query-typeorm';

import { TypeORMModule } from 'src/database/typeorm/typeorm.module';
import { TwoFactorMethod } from 'src/engine/core-modules/two-factor-method/two-factor-method.entity';
import { UserWorkspace } from 'src/engine/core-modules/user-workspace/user-workspace.entity';
import { UserWorkspaceResolver } from 'src/engine/core-modules/user-workspace/user-workspace.resolver';
import { UserWorkspaceService } from 'src/engine/core-modules/user-workspace/user-workspace.service';
import { DataSourceModule } from 'src/engine/metadata-modules/data-source/data-source.module';
import { WorkspaceDataSourceModule } from 'src/engine/workspace-datasource/workspace-datasource.module';
import { User } from 'src/engine/core-modules/user/user.entity';
import { WorkspaceInvitationModule } from 'src/engine/core-modules/workspace-invitation/workspace-invitation.module';
import { ObjectMetadataEntity } from 'src/engine/metadata-modules/object-metadata/object-metadata.entity';
import { UserWorkspaceResolver } from 'src/engine/core-modules/user-workspace/user-workspace.resolver';
import { Workspace } from 'src/engine/core-modules/workspace/workspace.entity';
import { DataSourceModule } from 'src/engine/metadata-modules/data-source/data-source.module';
import { ObjectMetadataEntity } from 'src/engine/metadata-modules/object-metadata/object-metadata.entity';
import { WorkspaceDataSourceModule } from 'src/engine/workspace-datasource/workspace-datasource.module';

@Module({
imports: [
NestjsQueryGraphQLModule.forFeature({
imports: [
NestjsQueryTypeOrmModule.forFeature(
[User, UserWorkspace, Workspace],
[User, UserWorkspace, Workspace, TwoFactorMethod],
'core',
),
NestjsQueryTypeOrmModule.forFeature([ObjectMetadataEntity], 'metadata'),
Expand Down
67 changes: 67 additions & 0 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9847,6 +9847,54 @@ __metadata:
languageName: node
linkType: hard

"@otplib/core@npm:^12.0.1":
version: 12.0.1
resolution: "@otplib/core@npm:12.0.1"
checksum: 10c0/efe703d92d50cf11b39e47fc6ccd10c01d8bfbd9423724e88aecf2470f740562b2422c10b779e0b7d1d29c09f5d3d00de69200f04c8250e2adeb13a15e5dee7f
languageName: node
linkType: hard

"@otplib/plugin-crypto@npm:^12.0.1":
version: 12.0.1
resolution: "@otplib/plugin-crypto@npm:12.0.1"
dependencies:
"@otplib/core": "npm:^12.0.1"
checksum: 10c0/7ad0cda9c643411a13a118bcf0d031ef61a9031f794a21b549fb3f1e38334f83c5481a9f706eb5b25066325759c1a598fbc1f9c05c4620dff5addccbe2ad23ad
languageName: node
linkType: hard

"@otplib/plugin-thirty-two@npm:^12.0.1":
version: 12.0.1
resolution: "@otplib/plugin-thirty-two@npm:12.0.1"
dependencies:
"@otplib/core": "npm:^12.0.1"
thirty-two: "npm:^1.0.2"
checksum: 10c0/971a6f592c0f33cb258dcb750f6f0c058cbfd45e0212ea6f29d0c75dc6c5fcb67dbc8f19c3e8ae710e93292319b8d3d15fd7281714deae9fadaab794f9d44544
languageName: node
linkType: hard

"@otplib/preset-default@npm:^12.0.1":
version: 12.0.1
resolution: "@otplib/preset-default@npm:12.0.1"
dependencies:
"@otplib/core": "npm:^12.0.1"
"@otplib/plugin-crypto": "npm:^12.0.1"
"@otplib/plugin-thirty-two": "npm:^12.0.1"
checksum: 10c0/cd683861b96d04b7581534169a5a130fc049bd3188a850a1642ba4cf2a53866dedafe5201763697e38a3f0726103783baf338448153752c464fff797948dc01c
languageName: node
linkType: hard

"@otplib/preset-v11@npm:^12.0.1":
version: 12.0.1
resolution: "@otplib/preset-v11@npm:12.0.1"
dependencies:
"@otplib/core": "npm:^12.0.1"
"@otplib/plugin-crypto": "npm:^12.0.1"
"@otplib/plugin-thirty-two": "npm:^12.0.1"
checksum: 10c0/5a1bf8198f169182e267ab6ae3115f2441190f34a212e8232ff4b9c2c8a7253fddf3aa185ab9a6246487e9c1052b676395e80b6e0b2825fa218eb8e29bd7b14b
languageName: node
linkType: hard

"@parcel/watcher-android-arm64@npm:2.4.1":
version: 2.4.1
resolution: "@parcel/watcher-android-arm64@npm:2.4.1"
Expand Down Expand Up @@ -38084,6 +38132,17 @@ __metadata:
languageName: node
linkType: hard

"otplib@npm:^12.0.1":
version: 12.0.1
resolution: "otplib@npm:12.0.1"
dependencies:
"@otplib/core": "npm:^12.0.1"
"@otplib/preset-default": "npm:^12.0.1"
"@otplib/preset-v11": "npm:^12.0.1"
checksum: 10c0/5a6cd332ed38f9fb6915407775bb9b4bf298d4d32c7c5691291add913dc1788064ab4ca353315f8add4ea704af2cb2c970d2f2d354725c6daa0c945cd5d09811
languageName: node
linkType: hard

"outvariant@npm:1.4.0":
version: 1.4.0
resolution: "outvariant@npm:1.4.0"
Expand Down Expand Up @@ -44756,6 +44815,13 @@ __metadata:
languageName: node
linkType: hard

"thirty-two@npm:^1.0.2":
version: 1.0.2
resolution: "thirty-two@npm:1.0.2"
checksum: 10c0/a6f8c69a153a1aa4d6948eabe004f5a38e45cb869d7d6c535df7460fc44c95c596a60efc56cce56b5e4f0988601db1298be5d1b6ae3fe12dcdeb461c9aeadd17
languageName: node
linkType: hard

"throttle-debounce@npm:^3.0.1":
version: 3.0.1
resolution: "throttle-debounce@npm:3.0.1"
Expand Down Expand Up @@ -45668,6 +45734,7 @@ __metadata:
monaco-editor: "npm:^0.51.0"
monaco-editor-auto-typings: "npm:^0.4.5"
openid-client: "npm:^5.7.0"
otplib: "npm:^12.0.1"
passport: "npm:^0.7.0"
psl: "npm:^1.9.0"
redis: "npm:^4.7.0"
Expand Down
Loading