Skip to content
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

validate the community role changes to stop admins to revoke admin role from themeselvs #342

Merged
merged 4 commits into from
Apr 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 36 additions & 2 deletions __tests__/integration/community.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,7 @@
// identifierValues: [{
// discordId: discordGuildMember2.discordId,
// username: discordGuildMember2.username,
// ngu: discordGuildMember2.nickname,
// // ngu: discordGuildMember2.nickname,
// discriminator: discordGuildMember2.discriminator,
// nickname: discordGuildMember2.nickname,
// globalName: discordGuildMember2.globalName,
Expand All @@ -388,7 +388,7 @@
// identifierValues: [{
// discordId: discordGuildMember2.discordId,
// username: discordGuildMember2.username,
// ngu: discordGuildMember2.nickname,
// // ngu: discordGuildMember2.nickname,
// discriminator: discordGuildMember2.discriminator,
// nickname: discordGuildMember2.nickname,
// globalName: discordGuildMember2.globalName,
Expand Down Expand Up @@ -619,6 +619,40 @@
// .expect(httpStatus.FORBIDDEN);
// });

// 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);

Comment on lines +622 to +653
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// });

// test('should return 400 error if communityId is not a valid mongo id', async () => {
// await insertUsers([userOne]);

Expand Down
8 changes: 4 additions & 4 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
"@discordjs/rest": "^1.7.0",
"@notionhq/client": "^2.2.3",
"@sentry/node": "^7.50.0",
"@togethercrew.dev/db": "^3.0.42",
"@togethercrew.dev/db": "^3.0.51",
"@togethercrew.dev/tc-messagebroker": "^0.0.45",
"@types/express-session": "^1.17.7",
"@types/morgan": "^1.9.5",
Expand Down
3 changes: 3 additions & 0 deletions src/controllers/community.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ const getCommunity = catchAsync(async function (req: IAuthRequest, res: Response
res.send(community);
});
const updateCommunity = catchAsync(async function (req: IAuthRequest, res: Response) {
if (req.body.roles && req.community) {
await communityService.validateRoleChanges(req.user, req.community, req.body.roles);
}
Comment on lines +46 to +48
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const community = await communityService.updateCommunityByFilter({ _id: req.params.communityId }, req.body);
res.send(community);
});
Expand Down
34 changes: 32 additions & 2 deletions src/services/community.service.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,15 @@
import { HydratedDocument, Types } from 'mongoose';
import httpStatus from 'http-status';
import { Community, ICommunity, DatabaseManager, GuildMember, IRole } from '@togethercrew.dev/db';
import ApiError from '../utils/ApiError';
import {
Community,
ICommunity,
DatabaseManager,
GuildMember,
IRole,
IUser,
ICommunityRoles,
} from '@togethercrew.dev/db';
import { ApiError, roleUtil } from '../utils';
import guildMemberService from './discord/guildMember.service';
import roleService from './discord/role.service';
import platformService from './platform.service';
Expand Down Expand Up @@ -147,6 +155,27 @@ const populateRoles = async (community: HydratedDocument<ICommunity>): Promise<H
return community;
};

/**
* 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.');
}
};
Comment on lines +158 to +178
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

export default {
createCommunity,
queryCommunities,
Expand All @@ -157,4 +186,5 @@ export default {
deleteCommunityByFilter,
addPlatformToCommunityById,
populateRoles,
validateRoleChanges,
};
Loading