-
Notifications
You must be signed in to change notification settings - Fork 4
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
Changes from all commits
618480b
b906d91
3ab0d93
ccb2fd7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure proper error handling for The new security check in |
||
const community = await communityService.updateCommunityByFilter({ _id: req.params.communityId }, req.body); | ||
res.send(community); | ||
}); | ||
|
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'; | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Review implementation of The function |
||
export default { | ||
createCommunity, | ||
queryCommunities, | ||
|
@@ -157,4 +186,5 @@ export default { | |
deleteCommunityByFilter, | ||
addPlatformToCommunityById, | ||
populateRoles, | ||
validateRoleChanges, | ||
}; |
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.
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.