refactor: remove authorization method calls (server) #34986
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #34986 +/- ##
===========================================
+ Coverage 59.46% 59.48% +0.02%
===========================================
Files 2830 2830
Lines 68579 68720 +141
Branches 15177 15204 +27
===========================================
+ Hits 40778 40877 +99
- Misses 25132 25171 +39
- Partials 2669 2672 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
@kody start-review |
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
![kodus-ai[bot]](https://avatars.githubusercontent.com/in/413034?s=60&v=4){kind=small}
| @@ -81,7 +82,7 @@ API.v1.addRoute( | |||
| throw new Meteor.Error('error-user-already-in-role', 'User already in role'); | |||
| } | |||
|
|
|||
| await Meteor.callAsync('authorization:addUserToRole', role._id, user.username, roomId); | |||
| await addUserToRole(this.userId, role._id, user.username, roomId); | |||
There was a problem hiding this comment.
try {
await addUserToRole(this.userId, role._id, user.username, roomId);
} catch (error) {
throw new Meteor.Error('error-role-assignment-failed', 'Failed to assign user to role', { error: error.message });
}Multiple functions lack proper error handling, which can lead to unhandled exceptions during role assignments and modifications.
This issue appears in multiple locations:
- apps/meteor/app/api/server/v1/roles.ts: Lines 85-85
- apps/meteor/app/authorization/server/methods/addUserToRole.ts: Lines 78-88
- apps/meteor/app/bot-helpers/server/index.ts: Lines 66-68
- apps/meteor/app/bot-helpers/server/index.ts: Lines 70-72
- apps/meteor/app/lib/server/methods/setAdminStatus.ts: Lines 39-43
Please add try-catch blocks around all relevant function calls to handle potential errors gracefully.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| throw new Meteor.Error('error-invalid-arguments', 'Invalid arguments', { | ||
| method: 'authorization:removeUserFromRole', | ||
| }); |
There was a problem hiding this comment.
throw new Meteor.Error('error-invalid-arguments', `Invalid arguments: roleId and username must be non-empty strings`, {The error message for invalid arguments should be more specific about which argument(s) failed validation to help with debugging and maintainability
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
https://rocketchat.atlassian.net/browse/CORE-935
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
This pull request refactors the authorization methods in the Rocket.Chat server codebase to enhance code modularity, maintainability, and reusability. The changes include:
roles.ts, a direct Meteor method call is replaced with a dedicated function call for adding a user to a role.addUserToRole.ts, the core logic for adding a user to a role is extracted into a separate async function, simplifying the Meteor method implementation.removeUserFromRole.ts, the logic for removing a user from a role is extracted into a separate function, updating the Meteor method to use this new function.index.tsof the bot helpers, bot helper methods are modified to use direct function calls instead ofMeteor.callAsync, auserIdparameter is added to the request method, and role management functions are updated with proper user authentication.setAdminStatus.ts, direct Meteor method calls are replaced with imported function calls for managing admin roles.These changes aim to improve the overall structure and efficiency of the authorization-related code in the Rocket.Chat server.