feat: reject guard #231
feat: reject guard #231
Conversation
WalkthroughThe changes in this pull request introduce a new property to the Changes
Possibly related PRs
Suggested reviewers
Poem
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: 2
🧹 Outside diff range and nitpick comments (7)
packages/toolkits/pro/template/server/nestJs/src/public/reject.decorator.ts (2)
1-3: Consider extracting the metadata key as a constantTo avoid magic strings and improve maintainability, consider extracting the 'reject' key as a named constant.
import { SetMetadata } from '@nestjs/common'; +export const REJECT_KEY = 'reject'; -export const Reject = () => SetMetadata('reject', true); +export const Reject = () => SetMetadata(REJECT_KEY, true);
3-3: Consider making the decorator more flexibleThe decorator could accept an optional condition parameter to make it more versatile.
-export const Reject = () => SetMetadata('reject', true); +export const Reject = (condition: boolean = true) => SetMetadata('reject', condition);packages/toolkits/pro/template/server/nestJs/src/public/reject.guard.ts (2)
24-29: Enhance error handlingThe error handling could be improved by:
- Adding more context to the error
- Using a custom exception class
- Including the request path in the error message
+export class PreviewModeException extends HttpException { + constructor(message: string) { + super(message, HttpStatus.BAD_REQUEST); + } +} - throw new HttpException( + throw new PreviewModeException( i18n.t('exception.preview.reject-this-request', { lang: I18nContext.current().lang, + path: request.url, }), - HttpStatus.BAD_REQUEST );
1-31: Add unit tests for the guardPlease ensure comprehensive unit tests are added for the guard to cover:
- Successful pass-through when metadata is not present
- Rejection when metadata is present
- Error cases (missing i18n context, invalid context)
Would you like me to help generate the unit tests for this guard?
packages/toolkits/pro/template/server/nestJs/src/role/role.controller.ts (1)
Line range hint
23-27: Consider standardizing method naming conventionsWhile the @Reject() decorator implementation is correct, the method naming could be more consistent:
createvsupdateRolevsdeleteRoleConsider standardizing to either:
- All with prefix:
createRole,updateRole,deleteRole- All without prefix:
create,update,delete- create(@Body() createRoleDto: CreateRoleDto) { + createRole(@Body() createRoleDto: CreateRoleDto) {Or
- updateRole(@Body() dto: UpdateRoleDto) { + update(@Body() dto: UpdateRoleDto) { - deleteRole(@Param('id') id: number) { + delete(@Param('id') id: number) {Also applies to: 51-55, 58-62
packages/toolkits/pro/template/server/nestJs/src/i18/i18.controller.ts (1)
19-19: Consider standardizing decorator orderThe decorator order should follow NestJS best practices: method decorators (like
@Post()) should come first, followed by cross-cutting concerns (like@Reject()), and finally permission/auth decorators. Consider reordering as:- @Reject() - @Permission('i18n::add') - @Post() + @Post() + @Reject() + @Permission('i18n::add')Also applies to: 25-30
packages/toolkits/pro/template/server/nestJs/src/user/user.controller.ts (1)
55-61: Consider consolidating password update methodsThe controller has two separate password update methods with similar rejection handling. Consider consolidating them into a single method with role-based logic to reduce code duplication.
- @Reject() - @Patch('/admin/updatePwd') - @Permission('user::password::force-update') - async updatePwdAdmin(@Body() body: UpdatePwdAdminDto) { - return this.userService.updatePwdAdmin(body); - } - - @Reject() - @Patch('/updatePwd') - @Permission('user::update') - async updatePwdUser(@Body() body: UpdatePwdUserDto) { - return this.userService.updatePwdUser(body); - } + @Reject() + @Patch('/updatePwd') + async updatePassword( + @Body() body: UpdatePwdUserDto | UpdatePwdAdminDto, + @Req() req: Request & { user: { roles: string[] } } + ) { + const isAdmin = req.user.roles.includes('admin'); + return isAdmin + ? this.userService.updatePwdAdmin(body as UpdatePwdAdminDto) + : this.userService.updatePwdUser(body as UpdatePwdUserDto); + }Also applies to: 62-67, 79-84, 86-91
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
packages/toolkits/pro/template/server/nestJs/src/.generate/i18n.generated.ts(1 hunks)packages/toolkits/pro/template/server/nestJs/src/app.module.ts(2 hunks)packages/toolkits/pro/template/server/nestJs/src/i18/i18.controller.ts(3 hunks)packages/toolkits/pro/template/server/nestJs/src/i18/lang.controller.ts(3 hunks)packages/toolkits/pro/template/server/nestJs/src/i18n/enUS/exception.json(1 hunks)packages/toolkits/pro/template/server/nestJs/src/i18n/zhCN/exception.json(1 hunks)packages/toolkits/pro/template/server/nestJs/src/menu/menu.controller.ts(2 hunks)packages/toolkits/pro/template/server/nestJs/src/permission/permission.controller.ts(2 hunks)packages/toolkits/pro/template/server/nestJs/src/public/reject.decorator.ts(1 hunks)packages/toolkits/pro/template/server/nestJs/src/public/reject.guard.ts(1 hunks)packages/toolkits/pro/template/server/nestJs/src/role/role.controller.ts(2 hunks)packages/toolkits/pro/template/server/nestJs/src/user/user.controller.ts(3 hunks)
🔇 Additional comments (16)
packages/toolkits/pro/template/server/nestJs/src/i18n/zhCN/exception.json (1)
39-42: LGTM! Clear and well-structured message
The new preview section and rejection message are clear, informative, and follow the existing i18n structure.
packages/toolkits/pro/template/server/nestJs/src/public/reject.guard.ts (1)
1-31: Verify implementation consistency
Let's verify the decorator usage and guard registration across the codebase.
✅ Verification successful
Implementation verified and properly configured
The implementation is correctly set up with all necessary components in place:
- Guard is properly registered globally in
app.module.tsusingAPP_GUARD @Reject()decorator is consistently used across controllers for write operations- I18n translation key
preview.reject-this-requestis properly defined in both languages - Guard implementation correctly uses the reflector and i18n service
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent implementation of the reject guard feature
# Check for decorator usage
echo "Checking @Reject() decorator usage..."
rg "@Reject\(\)" -A 2
# Check for guard registration in module files
echo "Checking guard registration..."
rg "RejectRequestGuard" -A 5 "*.module.ts"
# Check for i18n key usage
echo "Checking i18n key usage..."
rg "preview\.reject-this-request" --type ts
Length of output: 6192
Script:
#!/bin/bash
# Let's verify the module registration and decorator definition
# Check for module registration with different file patterns
echo "Checking guard registration in app.module.ts..."
rg "RejectRequestGuard" -A 5 "app.module.ts"
# Check for the Reject decorator definition
echo "Checking Reject decorator definition..."
fd "reject.decorator.ts" --exec cat
# Check for i18n key definition
echo "Checking i18n key definition..."
fd "i18n.generated.ts" --exec rg "preview\.reject-this-request"
Length of output: 567
Script:
#!/bin/bash
# Let's check for app module with a broader search and verify i18n translations
# Search for app module in the nestjs template directory
echo "Searching for app module..."
fd "app.module.ts" --full-path
# Check providers in any module file that might register the guard
echo "Checking guard registration in all module files..."
fd "module.ts" --exec rg "providers.*RejectRequestGuard" -A 3
# Check for i18n translations in a broader scope
echo "Checking i18n translations..."
fd "en.json" --exec cat
Length of output: 1443
Script:
#!/bin/bash
# Let's check the specific app module content and i18n translations
# Check the content of the NestJS app module
echo "Checking NestJS app module content..."
cat packages/toolkits/pro/template/server/nestJs/src/app.module.ts
# Look for i18n translations in more specific locations
echo "Checking i18n translations in server directory..."
fd "exception.json" --exec cat
Length of output: 8617
packages/toolkits/pro/template/server/nestJs/src/i18/lang.controller.ts (2)
14-14: LGTM: Clean import addition
The Reject decorator import is properly added.
Line range hint 20-48: LGTM: Consistent rejection pattern
The @Reject() decorator is appropriately applied to all modification operations (create/update/remove) while leaving the read-only operation untouched. This aligns well with the PR's objective of protecting certain operations.
Let's verify this pattern is consistent across other controllers:
✅ Verification successful
The @Reject() pattern is consistently applied across controllers
The verification confirms that the @Reject() decorator is consistently applied across all controllers following the same pattern:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if other controllers follow the same pattern
ast-grep --pattern 'class $_Controller {
$$$
@Reject()
@Permission($_)
@$_()
$_($$) {
$$$
}
$$$
}'
Length of output: 118
Script:
#!/bin/bash
# Let's try a different approach to find controller files and check their content
# First, find all controller files
fd "controller.ts$" | while read -r file; do
echo "=== $file ==="
# Show content focusing on decorators and method signatures
rg "@(Reject|Permission|Post|Get|Patch|Delete)" "$file" -A 1
done
Length of output: 6101
packages/toolkits/pro/template/server/nestJs/src/i18n/enUS/exception.json (1)
40-42: Verify translations across all language files
The rejection message is clear and well-placed in a dedicated "preview" section. However, we should ensure consistency across all language files.
Let's check for the corresponding translations:
✅ Verification successful
Translations are properly implemented across language files
The verification shows that the rejection message exists in both English and Chinese translations with consistent meaning and context. The Chinese translation ("服务器拒绝了本次请求, 因为演示模式下不允许增加, 删除, 修改数据") accurately conveys the same message about request rejection in demonstration mode.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the rejection message exists in all language files
fd 'exception.json' | xargs jq -r 'if has("preview") then .preview["reject-this-request"] else "missing" end'
Length of output: 263
packages/toolkits/pro/template/server/nestJs/src/menu/menu.controller.ts (3)
15-15: LGTM: Clean import addition
The Reject decorator import is properly added.
32-45: LGTM: Consistent rejection pattern
The @Reject() decorator is appropriately applied to modification operations (create/update) while leaving read-only operations untouched.
46-52: LGTM: Improved formatting and consistent rejection pattern
The deleteMenu method benefits from improved parameter formatting while maintaining the consistent rejection pattern.
Let's verify the rejection guard is properly registered:
✅ Verification successful
LGTM: Rejection pattern is properly implemented and registered
The verification confirms that:
- The
RejectRequestGuardis properly registered as a global guard inapp.module.ts - The
Rejectdecorator is correctly implemented inreject.decorator.ts - The guard implementation in
reject.guard.tsproperly reads the decorator metadata
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if RejectGuard is properly registered in the application
ast-grep --pattern 'class AppModule {
$$$
providers: [
$$$
RejectGuard,
$$$
]
$$$
}'
Length of output: 106
Script:
#!/bin/bash
# Let's try a broader search for RejectGuard registration and usage
rg "RejectGuard" -l
# Check for Reject decorator implementation
ast-grep --pattern 'export function Reject() {
$$$
}'
# Check for Guard registration in main.ts or similar bootstrap files
rg "useGlobalGuards|APP_GUARD" -A 3
Length of output: 1541
Script:
#!/bin/bash
# Let's check for the RejectRequestGuard implementation and Reject decorator
rg "RejectRequestGuard" -A 5
# Also search for the Reject decorator implementation
rg "export.*Reject.*=" -A 3
Length of output: 1943
packages/toolkits/pro/template/server/nestJs/src/permission/permission.controller.ts (2)
17-17: LGTM: Clean import of the new decorator
The import statement is correctly placed with other decorators.
23-27: Verify the consistent application of @Reject() decorator
The @Reject() decorator is correctly applied to all write operations (create/update/delete) while read operations remain undecorated. This pattern aligns with the PR objective of protecting modification endpoints in demonstration mode.
Also applies to: 30-34, 47-51
✅ Verification successful
Let me gather more information to verify if @Reject() is consistently applied only to write operations (POST/PUT/DELETE) and not on read operations (GET).
Let me get one final piece of information to see the complete context of write operations with their decorators.
@Reject() decorator is consistently applied across write operations
Based on the verification results, the @Reject() decorator is correctly and consistently applied:
- All write operations (POST, DELETE) across controllers have the @Reject() decorator
- Read operations (GET endpoints) are correctly left undecorated
- The pattern is uniform across all controller files (permission, role, menu, i18)
This implementation aligns with the intended behavior of protecting modification endpoints in demonstration mode while allowing read operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify consistent application of @Reject() decorator across controllers
# Expect: @Reject() only on write operations
# Search for all controller write operations
ast-grep --pattern 'class $_Controller {
$$$
@Post()
$$$
}'
# Search for all @Reject() decorators
rg "@Reject\(\)"
Length of output: 1813
Script:
#!/bin/bash
# Search for all HTTP method decorators in controller files to verify @Reject() is only on write operations
rg -B1 "@(Post|Put|Delete|Get)\(\)" packages/toolkits/pro/template/server/nestJs/src/
Length of output: 1886
Script:
#!/bin/bash
# Search for write operations with their decorators
rg -B2 "@(Post|Put|Delete)\(\)" packages/toolkits/pro/template/server/nestJs/src/
Length of output: 1421
packages/toolkits/pro/template/server/nestJs/src/.generate/i18n.generated.ts (1)
47-49: LGTM: Well-structured i18n addition
The new "preview" section is correctly placed under the "exception" namespace and follows the established i18n structure.
packages/toolkits/pro/template/server/nestJs/src/role/role.controller.ts (1)
17-17: LGTM: Clean import of the reject decorator
The import statement is correctly placed with other decorators.
packages/toolkits/pro/template/server/nestJs/src/i18/i18.controller.ts (1)
Line range hint 63-71: Verify error handling in service layer
The @Reject() decorator is correctly applied to mutation operations. However, ensure that the underlying service methods (update and remove) properly handle and throw exceptions that can be caught by the RejectRequestGuard.
Also applies to: 73-78
✅ Verification successful
Let me check the service implementation for the specific methods mentioned in the review comment.
Let me try a different approach to find these methods.
Let me check the implementation of the findOne method since both update and remove depend on it.
Error handling is properly implemented in the service layer
The service methods properly handle and throw exceptions that can be caught by the RejectRequestGuard:
-
updatemethod:- Throws
HttpException(NOT_FOUND)if the i18n item doesn't exist (viafindOne) - Throws
HttpException(NOT_FOUND)if the language doesn't exist
- Throws
-
removemethod:- Throws
HttpException(NOT_FOUND)if the i18n item doesn't exist (viafindOne)
- Throws
All exceptions use proper HTTP status codes and internationalized error messages that can be caught and handled by the @Reject() decorator.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for error handling in i18.service.ts
rg -A 5 "throw new \w+Exception" "$(fd -t f "i18.service.ts")"
Length of output: 851
Script:
#!/bin/bash
# Search for update and remove method implementations in i18.service.ts
ast-grep --pattern 'update($_, $_) {
$$$
}'
ast-grep --pattern 'remove($_) {
$$$
}'
Length of output: 91
Script:
#!/bin/bash
# Search for update and remove methods with context
rg -A 10 "async update\(|update\(" "$(fd -t f "i18.service.ts")"
rg -A 10 "async remove\(|remove\(" "$(fd -t f "i18.service.ts")"
Length of output: 713
Script:
#!/bin/bash
# Search for findOne method implementation
rg -A 10 "async findOne\(|findOne\(" "$(fd -t f "i18.service.ts")"
Length of output: 1636
packages/toolkits/pro/template/server/nestJs/src/user/user.controller.ts (1)
Line range hint 33-39: Ensure consistent error handling in user creation
The register method correctly uses async/await and the @Reject() decorator. Verify that the service layer properly handles and throws exceptions for invalid registration attempts.
✅ Verification successful
Let me check the implementation of the create method in the user service to ensure comprehensive error handling.
Let me try a different approach to find the create method implementation.
Let me get more context of the create method implementation to verify the error handling.
Error handling for user registration is properly implemented
The service layer has comprehensive error handling for the user creation process:
- Checks for existing users and throws a BAD_REQUEST with localized message
- Has proper error handling for database operations (from previous search showing INTERNAL_SERVER_ERROR handling)
- Includes i18n support for error messages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for error handling in user creation
ast-grep --pattern 'throw new HttpException($$$)' "$(fd -t f "user.service.ts")"
Length of output: 2447
Script:
#!/bin/bash
# Search for the create method implementation
ast-grep --pattern 'async create($$$) {
$$$
}'
Length of output: 50
Script:
#!/bin/bash
# Search for the create method in user.service.ts with context
rg -A 10 "async create" "$(fd -t f "user.service.ts")"
Length of output: 364
Script:
#!/bin/bash
# Search for the create method with more context lines
rg -A 30 "async create" "$(fd -t f "user.service.ts")"
Length of output: 873
packages/toolkits/pro/template/server/nestJs/src/app.module.ts (2)
66-69: Review guard execution order
The RejectRequestGuard is positioned between AuthGuard and PermissionGuard. Consider whether rejection should happen:
- Before authentication (current: no)
- Before permission check (current: yes)
- After both (alternative approach)
The current order means rejected requests still need to be authenticated but don't reach the permission check.
✅ Verification successful
Let me analyze the implementation of these guards to better understand their dependencies and interactions.
Guard order is correctly positioned
The RejectRequestGuard is a simple metadata-based guard that checks for a 'reject' decorator and throws a BAD_REQUEST exception if present. It:
- Doesn't depend on authentication or user information
- Only uses Reflector to check metadata
- Returns early if no rejection is needed
The current order (Auth -> Reject -> Permission) is optimal because:
- Authentication should always happen first for security
- Rejection before permissions saves unnecessary permission checks
- No dependencies between guards that would require different ordering
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for guard dependencies and interactions
rg -l "RejectRequestGuard|AuthGuard|PermissionGuard" | xargs rg -A 5 "implements CanActivate"
Length of output: 3244
Script:
#!/bin/bash
# Check the implementation of RejectRequestGuard to understand its purpose and dependencies
cat packages/toolkits/pro/template/server/nestJs/src/public/reject.guard.ts
# Check if RejectRequestGuard needs user/auth information
rg -A 10 "class RejectRequestGuard" packages/toolkits/pro/template/server/nestJs/src/public/reject.guard.ts
Length of output: 1464
36-36: Verify guard initialization requirements
Ensure the RejectRequestGuard has access to all required dependencies (like i18n service) through proper injection.
Also applies to: 66-69
✅ Verification successful
Guard dependencies are properly initialized
The RejectRequestGuard has all required dependencies properly injected:
- It uses
Reflectorthrough constructor injection - It accesses i18n through
I18nContext.current()which is the correct way to access i18n in guards
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check guard constructor and dependencies
ast-grep --pattern 'class RejectRequestGuard {
constructor($$$) {
$$$
}
}'
Length of output: 84
Script:
#!/bin/bash
# Let's first find the file containing RejectRequestGuard
fd "reject.guard.ts"
# Then examine its content
rg -A 10 "class RejectRequestGuard" --type ts
Length of output: 1342
| const rejectRequest = this.reflector.getAllAndOverride('reject', [ | ||
| ctx.getHandler(), | ||
| ctx.getClass(), | ||
| ]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use constant for metadata key
The 'reject' string should be imported from the decorator file to maintain consistency and avoid duplication.
+import { REJECT_KEY } from './reject.decorator';
- const rejectRequest = this.reflector.getAllAndOverride('reject', [
+ const rejectRequest = this.reflector.getAllAndOverride(REJECT_KEY, [Committable suggestion skipped: line range outside the PR's diff.
| async canActivate(ctx: ExecutionContext): Promise<boolean> { | ||
| const i18n = I18nContext.current<I18nTranslations>(); | ||
| const rejectRequest = this.reflector.getAllAndOverride('reject', [ | ||
| ctx.getHandler(), | ||
| ctx.getClass(), | ||
| ]); | ||
| if (!rejectRequest) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add request context validation and logging
The guard should validate the request context and log rejected requests for monitoring purposes.
async canActivate(ctx: ExecutionContext): Promise<boolean> {
+ const request = ctx.switchToHttp().getRequest();
+ if (!request) {
+ throw new Error('Invalid execution context');
+ }
const i18n = I18nContext.current<I18nTranslations>();
+ if (!i18n) {
+ throw new Error('I18n context not available');
+ }
const rejectRequest = this.reflector.getAllAndOverride('reject', [
ctx.getHandler(),
ctx.getClass(),
]);
if (!rejectRequest) {
return true;
}
+ // Log rejected requests
+ console.log(`Request rejected: ${request.method} ${request.url}`);
throw new HttpException(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async canActivate(ctx: ExecutionContext): Promise<boolean> { | |
| const i18n = I18nContext.current<I18nTranslations>(); | |
| const rejectRequest = this.reflector.getAllAndOverride('reject', [ | |
| ctx.getHandler(), | |
| ctx.getClass(), | |
| ]); | |
| if (!rejectRequest) { | |
| return true; | |
| } | |
| async canActivate(ctx: ExecutionContext): Promise<boolean> { | |
| const request = ctx.switchToHttp().getRequest(); | |
| if (!request) { | |
| throw new Error('Invalid execution context'); | |
| } | |
| const i18n = I18nContext.current<I18nTranslations>(); | |
| if (!i18n) { | |
| throw new Error('I18n context not available'); | |
| } | |
| const rejectRequest = this.reflector.getAllAndOverride('reject', [ | |
| ctx.getHandler(), | |
| ctx.getClass(), | |
| ]); | |
| if (!rejectRequest) { | |
| return true; | |
| } | |
| // Log rejected requests | |
| console.log(`Request rejected: ${request.method} ${request.url}`); |
If the API is decorated with @ Reject(), RejectGurad will intercept the request and return a 400 error
Summary by CodeRabbit
New Features
previewsection in internationalization strings, including a rejection message for requests.Rejectdecorator to enhance error handling for various controller methods.RejectRequestGuardto manage access based on metadata.Bug Fixes
Documentation
@Reject()decorator across various controllers.