-
Notifications
You must be signed in to change notification settings - Fork 68
feat: update send email verification API default parameter for sending email #1481
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: shitrerohit <[email protected]>
…ault Signed-off-by: sujitaw <[email protected]>
|
WalkthroughAdds an optional isDefaultVerified boolean to request DTO and interface. Updates user service flow: conditionally skip sending verification email when isDefaultVerified is true, immediately verify email post-creation, and proceed with username creation/persistence accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant API as API Gateway (DTO)
participant UserSvc as User Service
participant Email as Email Service
participant Verify as Verification Service
Client->>API: Create user request { email, ..., isDefaultVerified? }
API->>UserSvc: Validated payload (isDefaultVerified default false)
alt isDefaultVerified == true
UserSvc->>Verify: verifyEmail(user)
Verify-->>UserSvc: verified
note right of UserSvc: Skip sending verification email
else isDefaultVerified == false
UserSvc->>Email: sendVerificationEmail(user)
Email-->>UserSvc: queued/sent
end
UserSvc->>UserSvc: proceed to username creation & persistence
UserSvc-->>Client: user created (+verified if flagged)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/user/src/user.service.ts (1)
137-137: Initialize the variable for clarity.The variable
sendVerificationMailis declared but not initialized. While the logic works due to short-circuit evaluation on line 169, explicit initialization improves readability and makes the intent clearer.Apply this diff to initialize the variable:
- let sendVerificationMail: boolean; + let sendVerificationMail: boolean = false;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/api-gateway/src/user/dto/create-user.dto.ts(2 hunks)apps/user/src/user.service.ts(2 hunks)libs/common/src/interfaces/user.interface.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/user/src/user.service.ts (2)
apps/api-gateway/src/authz/authz.service.ts (1)
sendVerificationMail(44-47)apps/user/src/user.controller.ts (1)
sendVerificationMail(58-60)
🔇 Additional comments (6)
libs/common/src/interfaces/user.interface.ts (1)
23-23: LGTM! Interface extension is backward compatible.The optional
isDefaultVerifiedfield is properly typed and maintains backward compatibility with existing code.apps/api-gateway/src/user/dto/create-user.dto.ts (2)
1-2: LGTM! Import additions are appropriate.The new imports
ApiHidePropertyandIsBooleanare necessary for theisDefaultVerifiedfield implementation.
42-45: LGTM! Field is properly decorated and validated.The
isDefaultVerifiedfield is correctly configured with:
@ApiHideProperty()to hide it from Swagger documentation@IsOptional()to make it optional@IsBoolean()for type validation- Sensible default value of
falseapps/user/src/user.service.ts (3)
116-116: LGTM! Properly extracts the new field.The destructuring includes the new
isDefaultVerifiedfield from theuserEmailVerificationparameter.
153-164: Conditional email sending logic is correct.The email verification is correctly skipped when
isDefaultVerifiedis true, allowing for immediate verification in the subsequent flow.
169-169: No issues: sendEmailForVerification only returns true or throws, sosendVerificationMail || isDefaultVerifiedcovers all edge cases.
|
As discussed in the CREDEBL call dated 15th October. The fix provided here is not the best way to enable registering users without email verification. As having the flag responsible for making the user verified by default can open path to attacks like spamming of users on CREDEBL. @sujitaw mentioned he would be revisiting the requirements with @shitrerohit Some discussed suggestions to implement user registration without email verification were as follows:
We should be flexible with updating the verification email logic as it is our own, not dependent on Keycloak |
|
We can think of:
|
I believe the SMTP server would be a complete new feature regarding how someone sends email. For this specific issue I believe we can overlook that(even though a configurable email provider is what we want) for now. Let me know what you think. As this specific issue is mostly to avoid spamming of user accounts without verifying their email(no email required) with the suggested changes
Yes this was one of the options we discussed, however, as we don't have an admin portal, we can start with an API to add these users for the admin (CREDEBL) Also, I believe we'll also need to understand the use case for having this and if the base requirement is to add a large number of pre-approved users. This should help us get to the limit/number and if we want that in the first place.
Yess right |



What
Summary by CodeRabbit