Signup page backend #2832
Signup page backend #2832
Conversation
|
WalkthroughThis pull request introduces significant changes across multiple files in the workflows service. Key modifications include alterations to database constraints and model definitions to allow nullable relationships, enhancements to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CollectionFlowSignupController
participant EndUserService
participant WorkflowTokenService
User->>CollectionFlowSignupController: POST /signup
CollectionFlowSignupController->>EndUserService: createEndUser(data)
EndUserService-->>CollectionFlowSignupController: endUserId
CollectionFlowSignupController->>WorkflowTokenService: updateToken(tokenScope, endUserId)
WorkflowTokenService-->>CollectionFlowSignupController: success
CollectionFlowSignupController-->>User: 201 Created
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Outside diff range and nitpick comments (12)
services/workflows-service/src/common/guards/token-guard-without-enduser/token-without-enduser-auth.decorator.ts (2)
6-7: Add JSDoc documentation for this security-critical decorator.Since this decorator plays a crucial role in authentication, it should be well-documented with:
- Purpose and usage
- Security implications
- Example usage
Add this documentation:
+/** + * Decorator that enables token-based authentication without requiring an end user, + * while explicitly disabling session-based authentication. + * + * @security This decorator should only be used for endpoints that specifically need + * to handle authentication without an end user context (e.g., initial signup flow). + * + * @example + * ```typescript + * @UseTokenWithoutEnduserAuthGuard() + * @Post('signup') + * async signup() { + * // Handler implementation + * } + * ``` + */ export const UseTokenWithoutEnduserAuthGuard = () => applyDecorators(UseGuards(TokenWithoutEnduserAuthGuard), disableSessionAuth());
6-7: Consider making the decorator more configurable.The current implementation is rigid with no configuration options. Consider making it more flexible for different use cases.
Consider this enhancement:
-export const UseTokenWithoutEnduserAuthGuard = () => +export interface TokenAuthOptions { + throwOnMissingToken?: boolean; + allowExpiredTokens?: boolean; +} + +export const UseTokenWithoutEnduserAuthGuard = (options?: TokenAuthOptions) => applyDecorators(UseGuards(TokenWithoutEnduserAuthGuard), disableSessionAuth());services/workflows-service/src/common/decorators/token-scope.decorator.ts (1)
Line range hint
11-15: Improve type safety in the TokenScope decoratorThe decorator uses a type assertion (
as ITokenScope) which bypasses TypeScript's type checking. Additionally, the return type isn't explicitly declared.Consider this safer implementation:
-export const TokenScope = createParamDecorator((_, ctx: ExecutionContext) => { +export const TokenScope = createParamDecorator<undefined, ExecutionContext, ITokenScope | null>((_, ctx: ExecutionContext): ITokenScope | null => { const request = ctx.switchToHttp().getRequest(); - return (request.tokenScope as ITokenScope) || null; + const tokenScope = request.tokenScope; + return tokenScope && typeof tokenScope === 'object' ? tokenScope : null; });services/workflows-service/src/collection-flow/dto/signup.dto.ts (2)
4-4: Add class-level API documentation.Consider adding
@ApiTags()and@ApiDescription()decorators to improve API documentation and provide more context about the signup DTO's purpose.+@ApiTags('signup') +@ApiDescription('Data transfer object for user signup information') export class SignupDto {
1-22: Consider additional security measures for signup process.While the DTO provides good data validation, consider implementing these security measures at the controller/service level:
- Rate limiting for signup attempts
- CAPTCHA or similar anti-bot measures
- Email domain validation/blocklist
- Password complexity requirements (if applicable)
Would you like me to provide implementation examples for these security measures?
services/workflows-service/src/common/guards/token-guard-without-enduser/token-without-enduser-auth.module.ts (1)
1-11: Consider renaming for consistency with NestJS conventions.The module name
TokenWithoutEnduserAuthModulecould be more concise. Consider renaming toTokenlessAuthModuleorAnonymousTokenModuleto better align with NestJS naming conventions while maintaining clarity of purpose.-export class TokenWithoutEnduserAuthModule {} +export class TokenlessAuthModule {}services/workflows-service/src/collection-flow/controllers/collection-flow.end-user.controller.ts (1)
25-28: Consider enhancing error handling and type safetyThe current implementation could benefit from:
- Explicit return type annotation
- Error handling for the service call
- Input validation beyond DTO
Consider applying these improvements:
@common.Post() @swagger.ApiCreatedResponse({ type: [EndUserModel] }) - getCompanyInfo( + async updateEndUserCompanyInfo( @TokenScope() tokenScope: ITokenScopeWithEndUserId, @common.Body() data: EndUserUpdateDto, - ) { - return this.endUserService.updateById(tokenScope.endUserId, { data: data }); + ): Promise<EndUserModel> { + try { + return await this.endUserService.updateById(tokenScope.endUserId, { data }); + } catch (error) { + throw new common.InternalServerErrorException('Failed to update end user company info'); + } }services/workflows-service/src/collection-flow/controllers/collection-flow.signup.ts (1)
15-20: Document the inheritance intention.The use of
protectedvisibility for injected services suggests this controller might be intended for inheritance. Consider adding a class-level JSDoc comment to explain this design decision, or change the visibility toprivateif inheritance is not intended.+/** + * Collection flow signup controller. + * @description Uses protected members to allow inheritance by specialized controllers. + */ @UseTokenWithoutEnduserAuthGuard() @ApiExcludeController()services/workflows-service/src/collection-flow/collection-flow.service.ts (1)
231-234: LGTM! Consider enhancing error handling.The conditional check aligns well with the PR objective of enabling token creation without end user. The change ensures that end user updates only occur when both the payload data and token scope contain valid end user information.
Consider adding error logging or debug information when the end user update is skipped, to help with troubleshooting:
if (payload.data.endUser && tokenScope.endUserId) { const { ballerineEntityId: _, ...endUserData } = payload.data.endUser; await this.endUserService.updateById(tokenScope.endUserId, { data: endUserData }); + } else if (payload.data.endUser) { + this.logger.debug('Skipping end user update: No endUserId in token scope', { + hasEndUserData: !!payload.data.endUser, + tokenScope: { endUserId: tokenScope.endUserId } + }); }services/workflows-service/src/common/guards/token-guard-without-enduser/token-without-enduser-auth.guard.ts (2)
32-32: Avoid using 'any' type when augmenting the request objectCasting
reqasanyto add thetokenScopeproperty bypasses TypeScript's type safety and can lead to maintenance issues.Extend the
Requestinterface to include thetokenScopeproperty for better type safety:interface TokenScopeRequest extends Request { tokenScope: typeof tokenEntity; } const req = context.switchToHttp().getRequest<TokenScopeRequest>(); // ... req.tokenScope = tokenEntity;
14-14: Specify the return type for the 'canActivate' methodExplicitly defining the return type enhances code readability and type safety. The
canActivatemethod should returnPromise<boolean>.Modify the method signature as follows:
-async canActivate(context: ExecutionContext) { +async canActivate(context: ExecutionContext): Promise<boolean> {services/workflows-service/src/common/guards/token-guard/token-auth.guard.ts (1)
22-22: Consider fetching only unexpired tokensCurrently,
findByTokenWithExpiredUnscopedretrieves tokens regardless of their expiration status, and expiration is checked manually afterwards. For efficiency and security, consider using a method that fetches only unexpired tokens, such asfindByToken, to minimize unnecessary processing of expired tokens.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (18)
services/workflows-service/prisma/migrations/20241112125400_remove_enduser_constraint_from_token/migration.sql(1 hunks)services/workflows-service/prisma/schema.prisma(2 hunks)services/workflows-service/src/auth/workflow-token/workflow-token.repository.ts(2 hunks)services/workflows-service/src/auth/workflow-token/workflow-token.service.ts(1 hunks)services/workflows-service/src/collection-flow/collection-flow.module.ts(2 hunks)services/workflows-service/src/collection-flow/collection-flow.service.ts(1 hunks)services/workflows-service/src/collection-flow/controllers/collection-flow.controller.ts(4 hunks)services/workflows-service/src/collection-flow/controllers/collection-flow.end-user.controller.ts(2 hunks)services/workflows-service/src/collection-flow/controllers/collection-flow.files.controller.ts(0 hunks)services/workflows-service/src/collection-flow/controllers/collection-flow.signup.ts(1 hunks)services/workflows-service/src/collection-flow/dto/signup.dto.ts(1 hunks)services/workflows-service/src/common/decorators/token-scope.decorator.ts(1 hunks)services/workflows-service/src/common/guards/token-guard-without-enduser/token-without-enduser-auth.decorator.ts(1 hunks)services/workflows-service/src/common/guards/token-guard-without-enduser/token-without-enduser-auth.guard.ts(1 hunks)services/workflows-service/src/common/guards/token-guard-without-enduser/token-without-enduser-auth.module.ts(1 hunks)services/workflows-service/src/common/guards/token-guard/token-auth.guard.ts(2 hunks)services/workflows-service/src/workflow/dtos/create-collection-flow-url.ts(1 hunks)services/workflows-service/src/workflow/workflow.service.ts(5 hunks)
💤 Files with no reviewable changes (1)
- services/workflows-service/src/collection-flow/controllers/collection-flow.files.controller.ts
✅ Files skipped from review due to trivial changes (1)
- services/workflows-service/prisma/migrations/20241112125400_remove_enduser_constraint_from_token/migration.sql
🔇 Additional comments (26)
services/workflows-service/src/common/guards/token-guard-without-enduser/token-without-enduser-auth.decorator.ts (1)
1-7: Verify the security implications of combining token auth with disabled sessions.
The decorator combines token-based authentication with session disabling, which is a security-critical configuration. Please ensure this aligns with the intended authentication flow.
Let's verify the guard's implementation and usage:
✅ Verification successful
Security implications verified. No issues found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Analyze the guard implementation and its usage
# Expected results:
# 1. Verify guard implements proper token validation
# 2. Identify routes using this decorator to ensure appropriate protection
# 3. Check for any session-related security implications
# Check guard implementation
echo "=== Guard Implementation ==="
ast-grep --pattern 'class TokenWithoutEnduserAuthGuard implements CanActivate {
$$$
}'
# Find usages of this decorator
echo "=== Decorator Usage ==="
rg -l "UseTokenWithoutEnduserAuthGuard"
# Check for session-related configurations
echo "=== Session Configurations ==="
rg -l "session" --type ts
Length of output: 11449
services/workflows-service/src/workflow/dtos/create-collection-flow-url.ts (2)
14-19: LGTM! Changes are well-implemented and consistent.
The modifications to make endUserId optional are implemented correctly with proper validation decorators and TypeScript types. This aligns well with the PR objective of enabling token creation without an end user.
14-19: Verify the impact on API consumers.
Since this is a breaking change in the API contract (making a previously required field optional), ensure that existing API consumers are aware of this change and can handle the optional endUserId.
services/workflows-service/src/common/guards/token-guard-without-enduser/token-without-enduser-auth.module.ts (1)
7-11: LGTM! Module structure follows NestJS best practices.
The module correctly provides and exports the necessary components for token authentication without end users.
services/workflows-service/src/collection-flow/controllers/collection-flow.end-user.controller.ts (2)
2-5: LGTM: Clean import restructuring
The import changes properly introduce the new token scope type while maintaining clean code organization.
25-28: Verify error handling in the service layer
Since this endpoint is part of the signup page backend implementation, we should ensure proper error handling exists in the service layer for cases like duplicate entries or validation failures.
services/workflows-service/src/auth/workflow-token/workflow-token.service.ts (1)
21-34: Verify repository implementation for null endUserId handling
These service methods support token management without end users, but we should verify the repository implementation.
services/workflows-service/src/auth/workflow-token/workflow-token.repository.ts (2)
56-66:
Add validation constraints to prevent unrestricted updates.
The current implementation allows updating any field without validation. This could lead to security issues if not properly constrained at the service layer.
Consider applying these changes:
async updateByToken(
token: string,
- data: Partial<Prisma.WorkflowRuntimeDataTokenUncheckedCreateInput>,
+ data: Pick<
+ Prisma.WorkflowRuntimeDataTokenUncheckedCreateInput,
+ 'endUserId' | 'expiresAt'
+ >,
) {
return await this.prisma.workflowRuntimeDataToken.update({
where: {
token,
+ deletedAt: null,
},
data,
});
}This change:
- Restricts which fields can be updated using
Pick - Adds a soft-delete check to prevent updating deleted tokens
Let's verify the current usage to ensure these constraints won't break existing code:
#!/bin/bash
# Description: Check what fields are being updated through this method
# Expected: Find update operations to understand which fields need to be included in Pick
rg -A 5 'updateByToken' 35-42: LGTM, but ensure proper handling of expired tokens.
The implementation correctly follows the repository pattern and maintains consistency with existing unscoped query methods. However, since this method can return expired tokens, ensure that the service layer properly validates the token expiration before performing sensitive operations.
Let's verify how this method is used in the service layer:
services/workflows-service/src/collection-flow/controllers/collection-flow.signup.ts (1)
1-14: LGTM! Imports and decorators are well structured.
The imports are organized properly, and the controller decorators appropriately handle authentication and API documentation exclusion.
services/workflows-service/src/collection-flow/collection-flow.module.ts (2)
42-42: LGTM! Clean import statement.
The import follows the existing pattern and is properly organized.
61-61: Verify required dependencies for signup functionality.
While the controller is properly added, let's verify that all necessary dependencies for the signup functionality are present in the module's providers.
services/workflows-service/src/collection-flow/controllers/collection-flow.controller.ts (4)
10-14: LGTM: Clean import organization
The imports are well-organized and the addition of ITokenScopeWithEndUserId type is properly structured.
30-31: LGTM: Improved dependency injection clarity
The reordering of dependencies and renaming to collectionFlowService improves code clarity and maintainability.
36-36: LGTM: Consistent service usage
The updates to use collectionFlowService are consistent across all methods and maintain the same functionality while being more explicit about the service being used.
Also applies to: 46-49, 72-72, 80-83, 89-89, 103-103, 108-108
40-41: Verify the impact of the type change
The parameter type change from ITokenScope to ITokenScopeWithEndUserId might affect existing clients of this endpoint. Please ensure all consumers of this endpoint are updated accordingly.
✅ Verification successful
Type Change Verified Successfully
The parameter type has been updated to ITokenScopeWithEndUserId with no remaining references to ITokenScope. Existing consumers, including apps/kyb-app/src/domains/collection-flow/collection-flow.api.ts, are compatible with this change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any direct usage of this endpoint in the frontend code
rg -l "collection-flow/user" --type ts --type tsx
# Search for any references to the old type usage
rg "ITokenScope.*getUser" --type ts
Length of output: 115
Script:
#!/bin/bash
# Search for any direct usage of this endpoint in the frontend code
rg -l "collection-flow/user" --type=typescript --type=typescriptreact
# Search for any references to the old type usage
rg "ITokenScope.*getUser" --type=typescript
Length of output: 185
Script:
#!/bin/bash
# Search for any direct usage of this endpoint in the frontend code
rg -l "collection-flow/user" -g "*.ts" -g "*.tsx"
# Search for any references to the old type usage
rg "ITokenScope.*getUser" -g "*.ts" -g "*.tsx"
Length of output: 162
services/workflows-service/prisma/schema.prisma (2)
818-818: Consider data migration needs for new Alert state field
The addition of state and decisionAt fields to the Alert model is well-structured. However, since state is non-optional, existing alerts in the database will need to be migrated.
#!/bin/bash
# Check if there's a migration file for handling existing alerts
echo "Searching for migration file..."
fd "alert.*state" --type f ./prisma/migrations
# Check for potential default value in migration
echo "Checking migration content for default value..."
rg "ALTER.*Alert.*state.*DEFAULT" ./prisma/migrations485-486: Verify handling of optional endUserId in existing code
The change to make endUserId optional aligns with the PR objective of enabling token creation without an end user. However, existing code might assume this field is always present.
services/workflows-service/src/common/guards/token-guard-without-enduser/token-without-enduser-auth.guard.ts (1)
28-30: Standardize error messages to prevent information disclosure
Providing specific error messages like 'Token has expired' can inadvertently expose sensitive information to potential attackers.
[security_issue]
Consider using a generic error message for all unauthorized access attempts:
if (tokenEntity.expiresAt < new Date()) {
- throw new UnauthorizedException('Token has expired');
+ throw new UnauthorizedException('Unauthorized');
}services/workflows-service/src/common/guards/token-guard/token-auth.guard.ts (1)
28-30:
Potential conflict with token creation without an end user
The guard throws an UnauthorizedException when tokenEntity.endUserId is not set. Since one of the PR objectives is to allow token creation without the involvement of an end user, this check might prevent those tokens from functioning as intended. Please verify if this aligns with the desired behavior.
Ensure that tokens without an endUserId are handled appropriately throughout the application.
services/workflows-service/src/workflow/workflow.service.ts (6)
1522-1524: Initialization and retrieval of endUserId and entityData are appropriate
The initialization of endUserId to null and the use of optional chaining to safely access entityData ensure that the code handles cases where mainRepresentative might be undefined.
1530-1530: Proper null check for entityData before proceeding
The condition else if (entityData) correctly checks for the existence of entityData before using it, preventing potential runtime errors due to undefined values.
1547-1547: Consistent null safety with entityData
The use of if (entityData) continues to ensure that any operations on entityData are safe and won't result in null reference errors.
1558-1558: Correct handling of nullable endUserId in workflow token creation
Assigning endUserId: endUserId ?? null ensures compatibility with the updated schema where endUserId can be null. This change aligns with the modifications allowing tokens to be created without an associated end user.
1697-1697:
Fix syntax error in function parameter definition
There's a syntax error in the function parameter definition at line 1697. In TypeScript, semicolons should not be used to separate parameters in a function. Replace the semicolon with a comma.
Apply this diff to fix the syntax error:
-private async __generateEndUserWithBusiness({
- entityType,
- workflowRuntimeData,
- entityData: { firstName: string; lastName: string };
- currentProjectId,
+private async __generateEndUserWithBusiness({
+ entityType,
+ workflowRuntimeData,
+ entityData: { firstName: string; lastName: string },
+ currentProjectId,Likely invalid or redundant comment.
2087-2087: Confirm the correctness of the new logic introduced
The changes around line 2087 need to be reviewed to ensure they align with the intended functionality, especially concerning event handling and token creation without an end user.
Run the following script to search for any potential issues in the event handling logic:
services/workflows-service/src/common/decorators/token-scope.decorator.ts
Show resolved
Hide resolved
services/workflows-service/src/collection-flow/dto/signup.dto.ts
Outdated
Show resolved
Hide resolved
services/workflows-service/src/collection-flow/dto/signup.dto.ts
Outdated
Show resolved
Hide resolved
...s-service/src/common/guards/token-guard-without-enduser/token-without-enduser-auth.module.ts
Show resolved
Hide resolved
...ces/workflows-service/src/collection-flow/controllers/collection-flow.end-user.controller.ts
Show resolved
Hide resolved
services/workflows-service/src/collection-flow/controllers/collection-flow.signup.ts
Outdated
Show resolved
Hide resolved
...ws-service/src/common/guards/token-guard-without-enduser/token-without-enduser-auth.guard.ts
Outdated
Show resolved
Hide resolved
...ws-service/src/common/guards/token-guard-without-enduser/token-without-enduser-auth.guard.ts
Show resolved
Hide resolved
services/workflows-service/src/common/guards/token-guard/token-auth.guard.ts
Show resolved
Hide resolved
services/workflows-service/src/common/guards/token-guard/token-auth.guard.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
services/workflows-service/src/collection-flow/controllers/types.ts (1)
1-8: Add JSDoc documentation for the SignupConfig typeThe type definition would benefit from documentation explaining its purpose, usage, and the implications of different configurations.
Add documentation like this:
+/** + * Configuration for the signup process. + * @property email.validation - When true, enables email validation during signup + * @example + * // Enable email validation + * const config: SignupConfig = { email: { validation: true } }; + */ export type SignupConfig = | { email?: { validation: boolean; }; } | null | undefined;services/workflows-service/src/collection-flow/dto/signup.dto.ts (1)
17-21: Consider security implications of validation messages.While the email validation is functionally correct, consider using a generic error message for failed email validation to prevent potential user enumeration attacks. Specific validation errors could reveal whether an email address exists in your system.
@ApiProperty({ required: true, type: String }) - @IsEmail() + @IsEmail({}, { + message: 'Invalid credentials provided', + }) @IsNotEmpty() @MaxLength(254) email!: string;services/workflows-service/src/end-user/end-user.service.ts (1)
16-17: Consider adding JSDoc documentation for the transaction parameterThe transaction parameter addition is well-implemented and maintains backward compatibility. However, it would be beneficial to add documentation explaining when and why a transaction should be passed.
Add documentation like this:
+ /** + * Creates a new end user + * @param args - The end user creation parameters + * @param transaction - Optional transaction for atomic operations (e.g., when creating user during signup) + * @returns The created end user + */ async create(args: Parameters<EndUserRepository['create']>[0], transaction?: PrismaTransaction) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
services/workflows-service/src/auth/workflow-token/workflow-token.repository.ts(3 hunks)services/workflows-service/src/auth/workflow-token/workflow-token.service.ts(1 hunks)services/workflows-service/src/collection-flow/controllers/collection-flow.signup.ts(1 hunks)services/workflows-service/src/collection-flow/controllers/types.ts(1 hunks)services/workflows-service/src/collection-flow/dto/signup.dto.ts(1 hunks)services/workflows-service/src/common/guards/token-guard-without-enduser/token-without-enduser-auth.guard.ts(1 hunks)services/workflows-service/src/end-user/end-user.repository.ts(5 hunks)services/workflows-service/src/end-user/end-user.service.ts(2 hunks)services/workflows-service/src/workflow/workflow.controller.external.ts(0 hunks)
💤 Files with no reviewable changes (1)
- services/workflows-service/src/workflow/workflow.controller.external.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- services/workflows-service/src/auth/workflow-token/workflow-token.repository.ts
- services/workflows-service/src/auth/workflow-token/workflow-token.service.ts
- services/workflows-service/src/collection-flow/controllers/collection-flow.signup.ts
- services/workflows-service/src/common/guards/token-guard-without-enduser/token-without-enduser-auth.guard.ts
🔇 Additional comments (10)
services/workflows-service/src/collection-flow/controllers/types.ts (1)
1-8: 🛠️ Refactor suggestion
Consider strengthening the type definition for better type safety
The current type definition has several potential issues:
- Allowing
null/undefinedmakes it harder to handle the configuration safely - Email validation being optional could lead to security gaps in the signup flow
Consider this stronger type definition:
-export type SignupConfig =
- | {
- email?: {
- validation: boolean;
- };
- }
- | null
- | undefined;
+export type SignupConfig = {
+ email: {
+ validation: boolean;
+ };
+};This change would:
- Force explicit configuration of email validation
- Remove the possibility of null/undefined configs
- Require consumers to make conscious decisions about validation
Let's verify the impact of this change:
services/workflows-service/src/collection-flow/dto/signup.dto.ts (3)
1-4: LGTM! Clean and focused imports.
The imports are minimal and the class declaration follows proper naming conventions.
5-9: LGTM with previous suggestions.
11-15: LGTM with previous suggestions.
services/workflows-service/src/end-user/end-user.service.ts (1)
4-4: LGTM! Clean type import addition
The import is properly grouped with other type imports and aligns with the transaction handling enhancement.
services/workflows-service/src/end-user/end-user.repository.ts (5)
11-11: LGTM! Improved naming convention.
The rename from prisma to prismaService enhances code clarity by making the dependency's purpose more explicit.
17-19: LGTM! Proper transaction handling.
The create method correctly handles transactions and maintains type safety with proper generic constraints.
26-28: LGTM! Consistent service usage across find methods.
The find methods consistently use prismaService and properly implement project scoping.
Also applies to: 35-37, 45-52, 74-77, 92-100
Line range hint 83-88: LGTM! Proper transaction and type handling.
The update method correctly implements transaction support and maintains proper type safety with explicit return type.
Line range hint 60-66: Investigate and fix the type issue instead of using @ts-ignore.
The presence of @ts-ignore suggests a potential type mismatch that should be addressed properly rather than suppressed.
Let's investigate the type issue:
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (6)
services/workflows-service/src/collection-flow/controllers/collection-flow.signup.controller.intg.test.ts (4)
57-71: Consider adding type information for mocked services.While the current mocking approach works, consider using TypeScript's
Partial<Type>or jest'sMock<Type>for better type safety and IDE support. This would help catch potential interface changes during refactoring.Example for one service:
-{ provide: AppLoggerService, useValue: noop }, +{ provide: AppLoggerService, useValue: noop as unknown as Partial<AppLoggerService> },
108-114: Consider extracting test data to constants.Move hardcoded test values to constants at the top of the file for better maintainability and reusability.
const TEST_CUSTOMER = { name: 'signup-test-customer', displayName: 'Signup Test Customer', logoImageUri: 'test', } as const;
136-138: Extract token expiration calculation to a helper function.The date calculation logic could be reused in other tests. Consider moving it to a helper function.
const getTokenExpiryDate = (daysFromNow: number) => { const now = new Date(); return new Date(now.setDate(now.getDate() + daysFromNow)); };
41-176: Consider architectural improvements for better test maintainability.The test implementation could benefit from:
Test Factory Pattern:
- Create a factory class for generating test entities
- Implement builder pattern for flexible test data creation
Custom Helpers:
- Add helper for token creation and management
- Create shared assertions for common validations
This would make the tests more maintainable and easier to extend.
Example test factory:
class TestDataFactory { static createCustomer(override = {}) { return { name: 'signup-test-customer', displayName: 'Signup Test Customer', logoImageUri: 'test', ...override, }; } static async createWorkflowToken(prisma: PrismaService, override = {}) { // Implementation } }services/workflows-service/src/collection-flow/controllers/collection-flow.signup.controller.ts (2)
27-27: Replaceconsole.logstatements with NestJSLoggerserviceUsing
console.logis not recommended for logging in production applications. Consider using the NestJSLoggerservice for better log management and to support different log levels.Apply this diff to use the
Loggerservice:+import { Logger } from '@nestjs/common'; export class CollectionFlowSignupController { + private readonly logger = new Logger(CollectionFlowSignupController.name); constructor( protected readonly prismaService: PrismaService, protected readonly endUserService: EndUserService, protected readonly workflowService: WorkflowService, protected readonly workflowTokenService: WorkflowTokenService, ) {} @common.Post() async signUp(@TokenScope() tokenScope: ITokenScope, @common.Body() payload: SignupDto) { try { - console.log('in signup'); + this.logger.log('in signup'); await this.prismaService.$transaction(async transaction => { const { config } = await this.workflowService.getWorkflowRuntimeDataById( tokenScope.workflowRuntimeDataId, {}, [tokenScope.projectId], ); - console.log('found workflow runtime'); + this.logger.log('found workflow runtime'); this.validateSignupInputByConfig(payload, config?.collectionFlow?.signup); - console.log('validated signup input'); + this.logger.log('validated signup input'); const endUser = await this.endUserService.create( { data: { ...payload, projectId: tokenScope.projectId }, }, transaction, ); - console.log('created end user'); + this.logger.log('created end user'); await this.workflowTokenService.updateByToken( tokenScope.token, { endUserId: endUser.id }, transaction, ); - console.log('updated token'); + this.logger.log('updated token'); }); } catch (error: unknown) { - console.log('error', error); + this.logger.error('Error in signUp method', error as Error); if (error instanceof common.BadRequestException) { throw error; } throw new common.InternalServerErrorException(error, 'Failed to process signup'); } }Also applies to: 35-35, 39-39, 48-48, 56-56, 59-59
82-85: Implement theisEmailValidfunctionThe
isEmailValidfunction currently always returnstrue, meaning no actual email validation is performed. Implement proper email validation to ensure the email addresses meet the required format.Do you want me to provide an implementation for the email validation function or open a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
services/workflows-service/src/collection-flow/collection-flow.module.ts(2 hunks)services/workflows-service/src/collection-flow/collection-flow.service.ts(2 hunks)services/workflows-service/src/collection-flow/controllers/collection-flow.signup.controller.intg.test.ts(1 hunks)services/workflows-service/src/collection-flow/controllers/collection-flow.signup.controller.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- services/workflows-service/src/collection-flow/collection-flow.module.ts
- services/workflows-service/src/collection-flow/collection-flow.service.ts
...flows-service/src/collection-flow/controllers/collection-flow.signup.controller.intg.test.ts
Show resolved
Hide resolved
...flows-service/src/collection-flow/controllers/collection-flow.signup.controller.intg.test.ts
Show resolved
Hide resolved
services/workflows-service/src/collection-flow/controllers/collection-flow.signup.controller.ts
Show resolved
Hide resolved
services/workflows-service/src/collection-flow/controllers/collection-flow.signup.controller.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
services/workflows-service/src/collection-flow/controllers/collection-flow.signup.controller.ts (2)
27-32: Add explicit typing for the transaction parameterThe transaction parameter should be explicitly typed for better maintainability and type safety.
- await this.prismaService.$transaction(async transaction => { + await this.prismaService.$transaction(async (transaction: Prisma.TransactionClient) => {
71-74: Implement email validation logicThe
isEmailValidfunction currently returnstruewithout any validation. This could allow invalid email addresses to pass through.Would you like me to:
- Generate a proper email validation implementation using regex or a validation library?
- Create a GitHub issue to track this TODO?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
services/workflows-service/src/collection-flow/controllers/collection-flow.signup.controller.ts(1 hunks)
🔇 Additional comments (2)
services/workflows-service/src/collection-flow/controllers/collection-flow.signup.controller.ts (2)
1-15: LGTM! Well-structured imports and appropriate decorators
The imports are well-organized and the decorators correctly implement the token-without-enduser requirement while properly hiding the controller from API documentation.
16-22: LGTM! Well-structured dependency injection
The constructor properly injects all required services with appropriate visibility modifiers.
services/workflows-service/src/collection-flow/controllers/collection-flow.signup.controller.ts
Show resolved
Hide resolved
services/workflows-service/src/collection-flow/controllers/collection-flow.signup.controller.ts
Outdated
Show resolved
Hide resolved
services/workflows-service/src/auth/workflow-token/workflow-token.repository.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
services/workflows-service/src/auth/workflow-token/workflow-token.repository.ts (3)
26-33: Document the purpose of "Unscoped" in the method name.While the implementation looks correct, the term "Unscoped" in the method name needs clarification. Consider adding JSDoc comments to explain:
- What security or business rules this method bypasses
- When it's appropriate to use this unscoped version
+/** + * Finds a token without applying standard security scopes. + * @param token - The token to search for + * @returns The token if it exists, is not expired, and not deleted + */ async findByTokenUnscoped(token: string) {
35-42: Document security implications of retrieving expired tokens.While this method is useful for token management, please add documentation to:
- Explain the "Unscoped" suffix
- Clarify when it's safe to retrieve expired tokens
- Document potential security implications
+/** + * Retrieves a token regardless of its expiration status. + * @param token - The token to search for + * @returns The token if it exists and is not deleted + * @security This method bypasses expiration checks - use with caution + */ async findByTokenWithExpiredUnscoped(token: string) {
Line range hint
44-54: Consider renaming method to reflect soft deletion behavior.The current implementation has some inconsistencies:
- The method name suggests deletion but it performs a soft delete via update
- Unlike
findByTokenWithExpiredUnscoped, this method won't handle expired tokensConsider either:
- Renaming to
softDeleteByTokenUnscopedfor clarity- Adding a separate method for handling expired tokens if needed
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
services/workflows-service/src/auth/workflow-token/workflow-token.repository.ts(3 hunks)services/workflows-service/src/collection-flow/controllers/collection-flow.signup.controller.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- services/workflows-service/src/collection-flow/controllers/collection-flow.signup.controller.ts
🔇 Additional comments (2)
services/workflows-service/src/auth/workflow-token/workflow-token.repository.ts (2)
8-8: LGTM! Improved naming clarity.
The parameter rename from prisma to prismaService better reflects its role as a service dependency.
Line range hint 10-24: LGTM! Consistent parameter naming.
The method maintains type safety while properly handling token creation with project context.
services/workflows-service/src/auth/workflow-token/workflow-token.repository.ts
Show resolved
Hide resolved
services/workflows-service/src/collection-flow/controllers/collection-flow.signup.controller.ts
Outdated
Show resolved
Hide resolved
services/workflows-service/src/collection-flow/controllers/collection-flow.signup.controller.ts
Outdated
Show resolved
Hide resolved
| async findByTokenWithExpiredUnscoped(token: string) { | ||
| return await this.prismaService.workflowRuntimeDataToken.findFirst({ | ||
| where: { | ||
| token, |
There was a problem hiding this comment.
Since this table will queried based on 'token' field in order to get related context, we might want to add an index on top of this field
| data: Parameters<typeof this.workflowTokenRepository.updateByToken>[1], | ||
| transaction?: PrismaTransaction, | ||
| ) { | ||
| return await this.workflowTokenRepository.updateByToken(token, data, transaction); |
There was a problem hiding this comment.
Since it’s important to avoid cross-tenant data exposure, we should enforce tenant isolation by ensuring that only end users related to the same projectId can connect. This helps prevent accidental data leakage across tenants.
Additionally, it’s preferable to use a single object parameter to encapsulate the data instead of passing a set of individual parameters.
async function updateWorkflowToken({ token, data, transaction })
Benefits:
- It’s clear what each value represents since they are labeled within the object.
- If new parameters are added, they can easily be added to the options object without changing the function signature, the refactor will be much easier.
...flows-service/src/collection-flow/controllers/collection-flow.signup.controller.intg.test.ts
Show resolved
Hide resolved
| it('should create a new EndUser and attach it to the WorkflowRuntimeDataToken', async () => { | ||
| const signupDto = { | ||
| firstName: 'John', | ||
| lastName: 'Doe', | ||
| email: 'email@email.com', | ||
| }; | ||
|
|
||
| expect(workflowRuntimeDataToken.endUserId).toBeNull(); | ||
|
|
||
| const response = await request(app.getHttpServer()) | ||
| .post('/collection-flow/signup') | ||
| .send(signupDto) | ||
| .set('authorization', `Bearer ${workflowRuntimeDataToken.token}`); | ||
|
|
||
| expect(response.status).toBe(201); | ||
|
|
||
| const workflowToken = await workflowTokenService.findByToken(workflowRuntimeDataToken.token); | ||
| expect(workflowToken?.endUserId).toBeDefined(); | ||
|
|
||
| const endUser = await endUserRepository.findById(workflowToken?.endUserId ?? '', {}, [ | ||
| project.id, | ||
| ]); | ||
| expect(endUser).toBeDefined(); | ||
| }); |
There was a problem hiding this comment.
Create a test with 2 different end users and try to connect a non related end user, currently the test will pass...
Summary by CodeRabbit
Release Notes
New Features
CollectionFlowSignupControllerfor handling user sign-up requests.WorkflowControllerExternal.endUserIdin various data models, enhancing flexibility in user associations.SignupDtofor validating user sign-up data.ITokenScopeWithEndUserIdto enhance token capabilities.Bug Fixes
Documentation
Tests
CollectionFlowSignupControllerto ensure proper functionality.