Feat/2fa #9634

nicolasrouanne · 2025-01-15T10:32:19Z

Description

Closes #7003
Implements 2FA with TOTP.

Warning

This is a draft PR, with only partial changes, made as a mean of discussion about #7003 (it's easier to reason about real code)

Behaviour

a totpSecret is stored for each user
use otplib to create a QR code and to validate an otp against an totpSecret (great demo website by otplib)
OTP is asked upon each login attempt

Source

Inspired by:

RFC 6238
Cal.com's implementation of 2FA, namely
- raising a 401 when missing OTP and 2FA is enabled, with a specific error code
- catching the 401 in the frontend and displaying the OTP input

Remaining

encrypt totpSecret at rest using a symetric algorithm

- https://www.npmjs.com/package/otplib#references

packages/twenty-server/src/engine/core-modules/auth/auth.resolver.ts

nicolasrouanne · 2025-01-16T13:19:18Z

packages/twenty-server/src/engine/core-modules/user-workspace/user-workspace.entity.ts

+  @Field(() => [TwoFactorMethod], { nullable: true })
+  @OneToMany(
+    () => TwoFactorMethod,
+    (twoFactorMethod) => twoFactorMethod.userWorkspace,
+  )
+  twoFactorMethods: Relation<TwoFactorMethod[]>;


I added the twoFactorMethods relation in UserWorkspace, but for some reason, typeorm can't seem to find the association

/Users/nicolasrouanne/dev/twenty/src/metadata-builder/EntityMetadataBuilder.ts:1111 entityMetadata.relations.forEach((relation) => { ^ TypeORMError: Entity metadata for UserWorkspace#twoFactorMethods was not found. Check if you specified a correct entity object and if it's connected in the connection options. at /Users/nicolasrouanne/dev/twenty/src/metadata-builder/EntityMetadataBuilder.ts:1121:23

After a quick chat with @FelixMalfait on Discord, it's probably a module-wiring issue. We are going to add the TwoFactorModule to AuthModule, to ensure it's properly loaded

Actually fixed by @FelixMalfait on 59f84ba 🙏🏻

- also add a `is2FAEnabled` column to `userWorkspace` entity

…thods? TBC but removing for now to merge

FelixMalfait

Good first step! Thank you!!!

greptile-apps

PR Summary

This PR introduces the initial database and service layer for Two-Factor Authentication (2FA) using TOTP (Time-based One-Time Password).

Missing critical totpSecret column in TwoFactorMethod entity and migration for storing TOTP secrets
TwoFactorMethodService needs additional methods for TOTP validation and secret generation using the otplib package
Consider adding status/type fields to TwoFactorMethod entity to track enabled/disabled state and authentication method type
Security concern: Need to implement encryption for TOTP secrets at rest as noted in PR description
UserWorkspace to TwoFactorMethod relationship allows multiple 2FA methods per user, but business logic for handling multiple methods isn't implemented yet

_{💡 (3/5) Reply to the bot's comments like "Can you suggest a fix for this @greptileai?" or ask follow-up questions!}

_{8 file(s) reviewed, 8 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

packages/twenty-server/package.json

...twenty-server/src/database/typeorm/core/migrations/common/1737033794408-two-factor-method.ts

packages/twenty-server/src/engine/core-modules/two-factor-method/two-factor-method.entity.ts

packages/twenty-server/src/engine/core-modules/two-factor-method/two-factor-method.service.ts

greptile-apps · 2025-01-24T09:45:31Z

packages/twenty-server/src/engine/core-modules/two-factor-method/two-factor-method.service.ts

+  async findAll(): Promise<TwoFactorMethod[]> {
+    return this.twoFactorMethodRepository.find();
+  }


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

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.

greptile-apps · 2025-01-24T09:45:32Z

packages/twenty-server/src/engine/core-modules/two-factor-method/two-factor-method.service.ts

+  async remove(id: string): Promise<void> {
+    await this.twoFactorMethodRepository.delete(id);
+  }


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

greptile-apps · 2025-01-24T09:45:32Z

packages/twenty-server/src/engine/core-modules/user-workspace/user-workspace.entity.ts

+  @OneToMany(
+    () => TwoFactorMethod,
+    (twoFactorMethod) => twoFactorMethod.userWorkspace,
+  )
+  twoFactorMethods: Relation<TwoFactorMethod[]>;


logic: missing @field decorator for GraphQL exposure of twoFactorMethods - this field won't be queryable via GraphQL

greptile-apps · 2025-01-24T09:45:32Z

packages/twenty-server/src/engine/core-modules/user-workspace/user-workspace.entity.ts

+  @OneToMany(
+    () => TwoFactorMethod,
+    (twoFactorMethod) => twoFactorMethod.userWorkspace,
+  )
+  twoFactorMethods: Relation<TwoFactorMethod[]>;


style: consider adding cascade option to automatically handle deletion of associated TwoFactorMethod records

charlesBochet · 2025-01-24T14:07:47Z

packages/twenty-server/src/engine/core-modules/two-factor-method/two-factor-method.entity.ts

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


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

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?

charlesBochet · 2025-01-24T14:10:05Z

packages/twenty-server/src/engine/core-modules/two-factor-method/two-factor-method.service.ts

+
+import { Repository } from 'typeorm';
+
+import { TwoFactorMethod } from './two-factor-method.entity';


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

charlesBochet · 2025-01-24T14:12:00Z

packages/twenty-server/src/engine/core-modules/two-factor-method/two-factor-method.service.ts

+    return this.twoFactorMethodRepository.save(twoFactorMethod);
+  }
+
+  async findAll(): Promise<TwoFactorMethod[]> {


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

charlesBochet · 2025-01-24T14:12:08Z

packages/twenty-server/src/engine/core-modules/two-factor-method/two-factor-method.service.ts

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


charlesBochet · 2025-01-24T14:12:23Z

packages/twenty-server/src/engine/core-modules/two-factor-method/two-factor-method.service.ts

+  }
+
+  async remove(id: string): Promise<void> {
+    await this.twoFactorMethodRepository.delete(id);


nicolasrouanne · 2025-01-24T14:17:53Z

@charlesBochet I see your review and I'm sorry about a few things:

this PR wasn't really meant for review, apart from the general direction of creating a TwoFactorMethod entity and link it to the UserWorkspace
I haven't pushed it through because I was blocked by a module-wiring issue, and @FelixMalfait fixed it in 59f84ba
I added some dummy generated service methods and will only leave the relevant ones: create; Maybe findOne is relevant, or you prefer calling the ORM directly?

Now that I'm unblocked, I can add unit tests, auth logic in the resolvers, and remove all the cruft before merging. Frontend can be done in a separate task, but this is probably too raw right now to merge as is.

FelixMalfait · 2025-01-24T15:13:05Z

Sorry @nicolasrouanne I think there was a miscommunication between me and @charlesBochet - agree with your points and keep the discussed approach which is to do partial merges along the way

charlesBochet · 2025-01-24T17:23:52Z

Ok, let's merge it, my commentaries are non blocking. More than happy with the iterative paths as long as the comments are addressed or discussed in upcoming PRs

github-actions · 2025-01-24T17:27:15Z

	Fails
🚫	`node` failed.

Log

�[31mError: �[39m SyntaxError: Unexpected token C in JSON at position 0
    at JSON.parse (<anonymous>)
�[90m    at parseJSONFromBytes (node:internal/deps/undici/undici:5584:19)�[39m
�[90m    at successSteps (node:internal/deps/undici/undici:5555:27)�[39m
�[90m    at fullyReadBody (node:internal/deps/undici/undici:1665:9)�[39m
�[90m    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)�[39m
�[90m    at async specConsumeBody (node:internal/deps/undici/undici:5564:7)�[39m
danger-results://tmp/danger-results-717cd744.json

Generated by 🚫 dangerJS against 0d6291d

# Description Closes twentyhq#7003 Implements 2FA with TOTP. >[!WARNING] > This is a draft PR, with only partial changes, made as a mean of discussion about twentyhq#7003 (it's easier to reason about real code) ## Behaviour - a `totpSecret` is stored for each user - use [`otplib`](https://github.com/yeojz/otplib/tree/master) to create a QR code and to validate an `otp` against an `totpSecret` (great [demo website](https://otplib.yeojz.dev/) by `otplib`) - OTP is asked upon each login attempt ## Source Inspired by: - [RFC 6238](https://datatracker.ietf.org/doc/html/rfc6238) - Cal.com's implementation of 2FA, namely - [raising a 401](https://github.com/calcom/cal.com/blob/c21ba636d2bec4ed55775f0b058f70fdc371c410/packages/features/auth/lib/next-auth-options.ts#L188-L190) when missing OTP and 2FA is enabled, with a [specific error code](https://github.com/calcom/cal.com/blob/c21ba636d2bec4ed55775f0b058f70fdc371c410/packages/features/auth/lib/ErrorCode.ts#L9) - [catching the 401](https://github.com/calcom/cal.com/blob/c21ba636d2bec4ed55775f0b058f70fdc371c410/apps/web/modules/auth/login-view.tsx#L160) in the frontend and [displaying](https://github.com/calcom/cal.com/blob/c21ba636d2bec4ed55775f0b058f70fdc371c410/apps/web/modules/auth/login-view.tsx#L276) the OTP input ## Remaining - [ ] encrypt `totpSecret` at rest using a symetric algorithm --------- Co-authored-by: Félix Malfait <felix.malfait@gmail.com> Co-authored-by: Félix Malfait <felix@twenty.com>

chore(otplib): add otplib package

198c54b

- https://www.npmjs.com/package/otplib#references

charlesBochet added -PR: awaiting author -PR: wip labels Jan 15, 2025

charlesBochet assigned FelixMalfait Jan 15, 2025

nicolasrouanne commented Jan 15, 2025

View reviewed changes

packages/twenty-server/src/engine/core-modules/auth/auth.resolver.ts Outdated Show resolved Hide resolved

FelixMalfait reviewed Jan 15, 2025

View reviewed changes

packages/twenty-server/src/engine/core-modules/auth/auth.resolver.ts Outdated Show resolved Hide resolved

nicolasrouanne force-pushed the feat/2fa branch from b24e435 to e876cc3 Compare January 16, 2025 13:13

nicolasrouanne commented Jan 16, 2025

View reviewed changes

feat(2fa): create two-factor-method entity and associated files

a4a360c

- also add a `is2FAEnabled` column to `userWorkspace` entity

nicolasrouanne force-pushed the feat/2fa branch from e876cc3 to a4a360c Compare January 17, 2025 13:33

FelixMalfait and others added 4 commits January 20, 2025 14:09

Merge branch 'main' into feat/2fa

bc0359c

Merge branch 'main' into feat/2fa

446b8ea

Fix: missing typeorm registration

59f84ba

Not sure this is needed, maybe we can infer it from the underlying me…

0d6291d

…thods? TBC but removing for now to merge

FelixMalfait marked this pull request as ready for review January 24, 2025 09:43

FelixMalfait approved these changes Jan 24, 2025

View reviewed changes

greptile-apps bot reviewed Jan 24, 2025

View reviewed changes

charlesBochet reviewed Jan 24, 2025

View reviewed changes

charlesBochet merged commit 17def22 into twentyhq:main Jan 24, 2025
30 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/2fa #9634

Feat/2fa #9634

nicolasrouanne commented Jan 15, 2025

nicolasrouanne Jan 16, 2025

nicolasrouanne Jan 24, 2025

nicolasrouanne Jan 24, 2025 •

edited

Loading

FelixMalfait left a comment

greptile-apps bot left a comment

greptile-apps bot Jan 24, 2025

nicolasrouanne Jan 24, 2025

greptile-apps bot Jan 24, 2025

greptile-apps bot Jan 24, 2025

greptile-apps bot Jan 24, 2025

charlesBochet Jan 24, 2025

nicolasrouanne Jan 24, 2025

charlesBochet Jan 24, 2025

charlesBochet Jan 24, 2025

charlesBochet Jan 24, 2025

charlesBochet Jan 24, 2025

nicolasrouanne commented Jan 24, 2025

FelixMalfait commented Jan 24, 2025

charlesBochet commented Jan 24, 2025

github-actions bot commented Jan 24, 2025


		import { Repository } from 'typeorm';

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

Feat/2fa #9634

Feat/2fa #9634

Conversation

nicolasrouanne commented Jan 15, 2025

Description

Behaviour

Source

Remaining

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolasrouanne Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

FelixMalfait left a comment

Choose a reason for hiding this comment

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Jan 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greptile-apps bot Jan 24, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 24, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolasrouanne commented Jan 24, 2025

FelixMalfait commented Jan 24, 2025

charlesBochet commented Jan 24, 2025

github-actions bot commented Jan 24, 2025

Log

nicolasrouanne Jan 24, 2025 •

edited

Loading