validate the community role changes to stop admins to revoke admin role from themeselvs #342
Conversation
WalkthroughThe recent updates focus on enhancing the security and integrity of the community management system. Key changes include preventing administrators from removing their own admin roles, updating dependencies for better performance and stability, and refining integration tests to cover new scenarios involving role management. Changes
Recent Review DetailsConfiguration used: CodeRabbit UI Files ignored due to path filters (1)
Files selected for processing (4)
Additional comments not posted (1)
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
@coderabbitai review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| if (req.body.roles && req.community) { | ||
| await communityService.validateRoleChanges(req.user, req.community, req.body.roles); | ||
| } |
There was a problem hiding this comment.
Ensure proper error handling for validateRoleChanges.
The new security check in updateCommunity correctly calls validateRoleChanges to prevent admins from revoking their own roles. However, it's crucial to ensure that any exceptions thrown by validateRoleChanges are caught and handled appropriately to avoid unhandled promise rejections and to provide meaningful feedback to the client.
| /** | ||
| * Validates role changes to ensure an admin cannot revoke their own admin role | ||
| * @param {HydratedDocument<IUser>} user - The user object representing the current user | ||
| * @param {HydratedDocument<ICommunity>} community - The community document | ||
| * @param {string[]} newRoles - The new roles to be assigned to the community | ||
| * @throws {ApiError} If an admin tries to revoke their own admin role | ||
| */ | ||
| const validateRoleChanges = async ( | ||
| user: HydratedDocument<IUser>, | ||
| community: HydratedDocument<ICommunity>, | ||
| newRoles: ICommunityRoles[], | ||
| ): Promise<void> => { | ||
| const initialUserRoles: string[] = await roleUtil.getUserRolesForCommunity(user, community); | ||
| const originalRoles = community.roles; | ||
| community.roles = newRoles; | ||
| const updatedUserRoles: string[] = await roleUtil.getUserRolesForCommunity(user, community); | ||
| community.roles = originalRoles; | ||
| if (initialUserRoles.includes('admin') && !updatedUserRoles.includes('admin')) { | ||
| throw new ApiError(httpStatus.BAD_REQUEST, 'Admin role cannot be revoked by the user themselves.'); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Review implementation of validateRoleChanges.
The function validateRoleChanges is critical for ensuring that admins cannot revoke their own admin roles. It's important to ensure that the logic correctly identifies the roles before and after the update, and that it properly uses the roles to determine if an admin role is being revoked. Additionally, consider edge cases such as changes in role names or IDs that might not directly match 'admin' but still carry administrative privileges.
| // test('should return 400 when admin users trys to revoke admin role from themselves', async () => { | ||
| // await insertCommunities([communityOne, communityTwo, communityThree]); | ||
| // await insertUsers([userOne, userTwo]); | ||
| // await insertPlatforms([platformOne, platformTwo, platformThree]); | ||
| // await insertGuildMembers( | ||
| // [discordGuildMember1, discordGuildMember2, discordGuildMember3, discordGuildMember4], | ||
| // connection, | ||
| // ); | ||
| // await insertRoles([discordRole1, discordRole2, discordRole3, discordRole4], connection); | ||
|
|
||
| // const res1 = await request(app) | ||
| // .patch(`/api/v1/communities/${communityOne._id}`) | ||
| // .set('Authorization', `Bearer ${userTwoAccessToken}`) | ||
| // .send({ roles: [] }) | ||
| // .expect(httpStatus.BAD_REQUEST); | ||
|
|
||
| // const res2 = await request(app) | ||
| // .patch(`/api/v1/communities/${communityOne._id}`) | ||
| // .set('Authorization', `Bearer ${userTwoAccessToken}`) | ||
| // .send({ | ||
| // roles: [{ | ||
| // roleType: 'admin', | ||
| // source: { | ||
| // platform: 'discord', | ||
| // identifierType: 'member', | ||
| // identifierValues: [userOne.discordId], | ||
| // platformId: platformOne._id, | ||
| // }, | ||
| // },] | ||
| // }) | ||
| // .expect(httpStatus.BAD_REQUEST); | ||
|
|
There was a problem hiding this comment.
Add assertions to the new test scenario.
The new test scenario designed to trigger a BAD_REQUEST when an admin tries to revoke their own admin role is a good start. However, it's important to include assertions that verify the contents of the response to ensure that it contains the expected error message and that no changes were made to the user's roles.
Summary by CodeRabbit