-
Notifications
You must be signed in to change notification settings - Fork 351
feat: connection longevity + one-click 'Make permanent' 2FA #3524
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
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
ea079fc
feat: surface connection longevity and a 'Make permanent' 2FA flow
tofikwest 50a7778
fix: address review on the connection-permanence changes
tofikwest 337d693
refactor(api): extract the 2FA (TOTP) endpoints into a focused contro…
tofikwest 69e0c4a
Merge branch 'main' into feat/browser-connection-make-permanent
tofikwest b5c1a73
fix(api): validate the authenticator setup key server-side + cover th…
tofikwest e682367
fix(api): keep the setup-key parser linear on untrusted input
tofikwest 72b3a2c
Merge branch 'main' into feat/browser-connection-make-permanent
tofikwest File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
121 changes: 121 additions & 0 deletions
121
apps/api/src/browserbase/browser-auth-totp.controller.spec.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,121 @@ | ||
| import { Reflector } from '@nestjs/core'; | ||
| import type { BrowserCredentialStorageService } from './browser-credential-storage.service'; | ||
| import { BrowserAuthTotpController } from './browser-auth-totp.controller'; | ||
|
|
||
| // The controller imports the auth guards, which pull in the real Prisma client | ||
| // and better-auth (ESM) at module load. Stub those so importing the controller | ||
| // doesn't open a DB connection or fail to parse — the guards aren't executed here | ||
| // (we test the controller methods + their RBAC metadata directly). | ||
| jest.mock('@db', () => ({ | ||
| db: {}, | ||
| // The DTO module (transitively imported) decorates fields with | ||
| // @IsEnum(TaskFrequency), so the enum must exist at module-eval time. | ||
| TaskFrequency: { | ||
| daily: 'daily', | ||
| weekly: 'weekly', | ||
| monthly: 'monthly', | ||
| quarterly: 'quarterly', | ||
| yearly: 'yearly', | ||
| }, | ||
| })); | ||
| jest.mock('../auth/auth.server', () => ({ | ||
| auth: { api: { getSession: jest.fn() } }, | ||
| })); | ||
| jest.mock('@trycompai/auth', () => ({ | ||
| statement: {}, | ||
| BUILT_IN_ROLE_PERMISSIONS: {}, | ||
| })); | ||
|
|
||
| // Mirrors PERMISSIONS_KEY in ../auth/permission.guard — imported by value rather | ||
| // than by reference so the test doesn't pull the guard's better-auth (ESM) chain | ||
| // into Jest. Keep in sync with the guard. | ||
| const PERMISSIONS_KEY = 'required_permissions'; | ||
|
|
||
| describe('BrowserAuthTotpController', () => { | ||
| const service = { | ||
| getOrgTotpStatuses: jest.fn(), | ||
| getProfileTotpStatus: jest.fn(), | ||
| setProfileTotp: jest.fn(), | ||
| clearProfileTotp: jest.fn(), | ||
| }; | ||
| let controller: BrowserAuthTotpController; | ||
|
|
||
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| controller = new BrowserAuthTotpController( | ||
| service as unknown as BrowserCredentialStorageService, | ||
| ); | ||
| }); | ||
|
|
||
| describe('delegation (organization + profile scoping reaches storage)', () => { | ||
| it('getOrgTotpStatuses passes the org id and wraps the map', async () => { | ||
| service.getOrgTotpStatuses.mockResolvedValue({ bap_1: true }); | ||
|
|
||
| const res = await controller.getOrgTotpStatuses('org_1'); | ||
|
|
||
| expect(service.getOrgTotpStatuses).toHaveBeenCalledWith('org_1'); | ||
| expect(res).toEqual({ statuses: { bap_1: true } }); | ||
| }); | ||
|
|
||
| it('getProfileTotp passes the org + profile ids', async () => { | ||
| service.getProfileTotpStatus.mockResolvedValue({ configured: true }); | ||
|
|
||
| const res = await controller.getProfileTotp('org_1', 'bap_1'); | ||
|
|
||
| expect(service.getProfileTotpStatus).toHaveBeenCalledWith({ | ||
| organizationId: 'org_1', | ||
| profileId: 'bap_1', | ||
| }); | ||
| expect(res).toEqual({ configured: true }); | ||
| }); | ||
|
|
||
| it('setProfileTotp forwards the seed with org + profile ids', async () => { | ||
| service.setProfileTotp.mockResolvedValue({ configured: true }); | ||
|
|
||
| await controller.setProfileTotp('org_1', 'bap_1', { | ||
| totpSeed: 'JBSWY3DPEHPK3PXP', | ||
| }); | ||
|
|
||
| expect(service.setProfileTotp).toHaveBeenCalledWith({ | ||
| organizationId: 'org_1', | ||
| profileId: 'bap_1', | ||
| totpSeed: 'JBSWY3DPEHPK3PXP', | ||
| }); | ||
| }); | ||
|
|
||
| it('clearProfileTotp passes the org + profile ids', async () => { | ||
| service.clearProfileTotp.mockResolvedValue({ configured: false }); | ||
|
|
||
| await controller.clearProfileTotp('org_1', 'bap_1'); | ||
|
|
||
| expect(service.clearProfileTotp).toHaveBeenCalledWith({ | ||
| organizationId: 'org_1', | ||
| profileId: 'bap_1', | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe('RBAC — reads require integration:read, writes require integration:update', () => { | ||
| const reflector = new Reflector(); | ||
| const permsOf = (method: object) => | ||
| reflector.get(PERMISSIONS_KEY, method as never); | ||
|
|
||
| it('gates read routes with integration:read', () => { | ||
| expect(permsOf(BrowserAuthTotpController.prototype.getOrgTotpStatuses)).toEqual( | ||
| [{ resource: 'integration', actions: ['read'] }], | ||
| ); | ||
| expect(permsOf(BrowserAuthTotpController.prototype.getProfileTotp)).toEqual([ | ||
| { resource: 'integration', actions: ['read'] }, | ||
| ]); | ||
| }); | ||
|
|
||
| it('gates write routes with integration:update', () => { | ||
| expect(permsOf(BrowserAuthTotpController.prototype.setProfileTotp)).toEqual([ | ||
| { resource: 'integration', actions: ['update'] }, | ||
| ]); | ||
| expect(permsOf(BrowserAuthTotpController.prototype.clearProfileTotp)).toEqual( | ||
| [{ resource: 'integration', actions: ['update'] }], | ||
| ); | ||
| }); | ||
| }); | ||
| }); |
116 changes: 116 additions & 0 deletions
116
apps/api/src/browserbase/browser-auth-totp.controller.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,116 @@ | ||
| import { | ||
| Body, | ||
| Controller, | ||
| Delete, | ||
| Get, | ||
| Param, | ||
| Post, | ||
| UseGuards, | ||
| } from '@nestjs/common'; | ||
| import { | ||
| ApiBody, | ||
| ApiOperation, | ||
| ApiParam, | ||
| ApiResponse, | ||
| ApiSecurity, | ||
| ApiTags, | ||
| } from '@nestjs/swagger'; | ||
| import { OrganizationId } from '../auth/auth-context.decorator'; | ||
| import { HybridAuthGuard } from '../auth/hybrid-auth.guard'; | ||
| import { PermissionGuard } from '../auth/permission.guard'; | ||
| import { RequirePermission } from '../auth/require-permission.decorator'; | ||
| import { BrowserCredentialStorageService } from './browser-credential-storage.service'; | ||
| import { SetAuthProfileTotpDto } from './dto/browserbase.dto'; | ||
|
|
||
| /** | ||
| * Automatic-2FA (TOTP) management for browser connections — kept in its own | ||
| * controller so the main profiles controller stays within the file-size limit and | ||
| * the 2FA routes are grouped. All routes share the `browserbase` v1 path prefix. | ||
| */ | ||
| @ApiTags('Browserbase') | ||
| @Controller({ path: 'browserbase', version: '1' }) | ||
| @UseGuards(HybridAuthGuard, PermissionGuard) | ||
| @ApiSecurity('apikey') | ||
| export class BrowserAuthTotpController { | ||
| constructor( | ||
| private readonly credentialStorageService: BrowserCredentialStorageService, | ||
| ) {} | ||
|
|
||
| // Declared before the `profiles/:profileId/...` routes so its static path is | ||
| // never captured as a profileId. | ||
| @Get('profiles/totp-statuses') | ||
| @RequirePermission('integration', 'read') | ||
| @ApiOperation({ | ||
| summary: 'Automatic-2FA status for all password connections', | ||
| description: | ||
| 'Reports, per connection, whether an authenticator setup key is stored — so the connections list can show which sign-ins keep running unattended and which may pause. Read live from the vault in one round-trip.', | ||
| }) | ||
| @ApiResponse({ status: 200 }) | ||
| async getOrgTotpStatuses( | ||
| @OrganizationId() organizationId: string, | ||
| ): Promise<{ statuses: Record<string, boolean> }> { | ||
| const statuses = | ||
| await this.credentialStorageService.getOrgTotpStatuses(organizationId); | ||
| return { statuses }; | ||
| } | ||
|
|
||
| @Get('profiles/:profileId/totp') | ||
| @RequirePermission('integration', 'read') | ||
| @ApiOperation({ | ||
| summary: 'Get automatic-2FA status for a connection', | ||
| description: | ||
| 'Reports whether an authenticator setup key (TOTP seed) is stored for this connection, read live from the vault, so scheduled sign-ins can generate 2FA codes unattended.', | ||
| }) | ||
| @ApiParam({ name: 'profileId', description: 'Browser auth profile ID' }) | ||
| @ApiResponse({ status: 200 }) | ||
| async getProfileTotp( | ||
| @OrganizationId() organizationId: string, | ||
| @Param('profileId') profileId: string, | ||
| ): Promise<{ configured: boolean }> { | ||
| return this.credentialStorageService.getProfileTotpStatus({ | ||
| organizationId, | ||
| profileId, | ||
| }); | ||
| } | ||
|
|
||
| @Post('profiles/:profileId/totp') | ||
| @RequirePermission('integration', 'update') | ||
| @ApiOperation({ | ||
| summary: 'Store an authenticator setup key for a connection', | ||
| description: | ||
| "Attach or replace the authenticator setup key (TOTP seed) on this connection's stored login, enabling unattended 2FA. Does not require re-entering the username or password.", | ||
| }) | ||
| @ApiParam({ name: 'profileId', description: 'Browser auth profile ID' }) | ||
| @ApiBody({ type: SetAuthProfileTotpDto }) | ||
| @ApiResponse({ status: 201 }) | ||
| async setProfileTotp( | ||
| @OrganizationId() organizationId: string, | ||
| @Param('profileId') profileId: string, | ||
| @Body() dto: SetAuthProfileTotpDto, | ||
|
tofikwest marked this conversation as resolved.
|
||
| ): Promise<{ configured: boolean }> { | ||
| return this.credentialStorageService.setProfileTotp({ | ||
| organizationId, | ||
| profileId, | ||
| totpSeed: dto.totpSeed, | ||
| }); | ||
| } | ||
|
|
||
| @Delete('profiles/:profileId/totp') | ||
| @RequirePermission('integration', 'update') | ||
| @ApiOperation({ | ||
| summary: 'Turn off automatic 2FA for a connection', | ||
| description: | ||
| 'Remove the stored authenticator setup key. Scheduled runs pause if the vendor then asks for a code.', | ||
| }) | ||
| @ApiParam({ name: 'profileId', description: 'Browser auth profile ID' }) | ||
| @ApiResponse({ status: 200 }) | ||
| async clearProfileTotp( | ||
| @OrganizationId() organizationId: string, | ||
| @Param('profileId') profileId: string, | ||
| ): Promise<{ configured: boolean }> { | ||
| return this.credentialStorageService.clearProfileTotp({ | ||
| organizationId, | ||
| profileId, | ||
| }); | ||
| } | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.