From ddfc5e6aa8fc636931f495d6f23d56367466e3b5 Mon Sep 17 00:00:00 2001 From: sct Date: Tue, 2 Mar 2021 08:18:31 +0000 Subject: [PATCH] fix: add correct permission checks to modifying user password/permissions --- server/routes/user/index.ts | 5 +++- server/routes/user/usersettings.ts | 29 ++++++++++++++++--- .../UserSettings/UserPasswordChange/index.tsx | 26 +++++++++++++++-- src/i18n/locale/en.json | 2 ++ 4 files changed, 54 insertions(+), 8 deletions(-) diff --git a/server/routes/user/index.ts b/server/routes/user/index.ts index 803aed7c..d29567a6 100644 --- a/server/routes/user/index.ts +++ b/server/routes/user/index.ts @@ -167,7 +167,10 @@ router.get<{ id: string }, UserRequestsResponse>( } ); -const canMakePermissionsChange = (permissions: number, user?: User) => +export const canMakePermissionsChange = ( + permissions: number, + user?: User +): boolean => // Only let the owner grant admin privileges !(hasPermission(Permission.ADMIN, permissions) && user?.id !== 1) || // Only let users with the manage settings permission, grant the same permission diff --git a/server/routes/user/usersettings.ts b/server/routes/user/usersettings.ts index c2e07511..e102e2e2 100644 --- a/server/routes/user/usersettings.ts +++ b/server/routes/user/usersettings.ts @@ -1,5 +1,6 @@ import { Router } from 'express'; import { getRepository } from 'typeorm'; +import { canMakePermissionsChange } from '.'; import { User } from '../../entity/User'; import { UserSettings } from '../../entity/UserSettings'; import { @@ -21,6 +22,7 @@ const isOwnProfileOrAdmin = (): Middleware => { message: "You do not have permission to view this user's settings.", }); } + next(); }; return authMiddleware; @@ -137,7 +139,19 @@ userSettingsRoutes.post< if (req.body.newPassword.length < 8) { return next({ status: 400, - message: 'Password must be at least 8 characters', + message: 'Password must be at least 8 characters.', + }); + } + + if ( + (user.id === 1 && req.user?.id !== 1) || + (user.hasPermission(Permission.ADMIN) && + user.id !== req.user?.id && + req.user?.id !== 1) + ) { + return next({ + status: 403, + message: "You do not have permission to modify this user's password.", }); } @@ -283,13 +297,20 @@ userSettingsRoutes.post< return next({ status: 404, message: 'User not found.' }); } - if (user.id === 1) { + // Only let the owner user modify themselves + if (user.id === 1 && req.user?.id !== 1) { return next({ - status: 500, - message: 'Permissions for user with ID 1 cannot be modified', + status: 403, + message: 'You do not have permission to modify this user', }); } + if (!canMakePermissionsChange(req.body.permissions, req.user)) { + return next({ + status: 403, + message: 'You do not have permission to grant this level of access', + }); + } user.permissions = req.body.permissions; await userRepository.save(user); diff --git a/src/components/UserProfile/UserSettings/UserPasswordChange/index.tsx b/src/components/UserProfile/UserSettings/UserPasswordChange/index.tsx index cc973aa0..545cf539 100644 --- a/src/components/UserProfile/UserSettings/UserPasswordChange/index.tsx +++ b/src/components/UserProfile/UserSettings/UserPasswordChange/index.tsx @@ -5,7 +5,7 @@ import React from 'react'; import { defineMessages, useIntl } from 'react-intl'; import { useToasts } from 'react-toast-notifications'; import useSWR from 'swr'; -import { useUser } from '../../../../hooks/useUser'; +import { Permission, useUser } from '../../../../hooks/useUser'; import Error from '../../../../pages/_error'; import Alert from '../../../Common/Alert'; import Button from '../../../Common/Button'; @@ -33,6 +33,9 @@ const messages = defineMessages({ nopasswordsetDescription: 'This user account currently does not have a password specifically for {applicationTitle}.\ Configure a password below to enable this account to sign in as a "local user."', + nopermission: 'No Permission', + nopermissionDescription: + "You do not have permission to modify this user's password.", }); const UserPasswordChange: React.FC = () => { @@ -41,14 +44,14 @@ const UserPasswordChange: React.FC = () => { const { addToast } = useToasts(); const router = useRouter(); const { user: currentUser } = useUser(); - const { user } = useUser({ id: Number(router.query.userId) }); + const { user, hasPermission } = useUser({ id: Number(router.query.userId) }); const { data, error, revalidate } = useSWR<{ hasPassword: boolean }>( user ? `/api/v1/user/${user?.id}/settings/password` : null ); const PasswordChangeSchema = Yup.object().shape({ currentPassword: Yup.lazy(() => - data?.hasPassword + data?.hasPassword && currentUser?.id === user?.id ? Yup.string().required( intl.formatMessage(messages.validationCurrentPassword) ) @@ -73,6 +76,23 @@ const UserPasswordChange: React.FC = () => { return ; } + if ( + currentUser?.id !== user?.id && + hasPermission(Permission.ADMIN) && + currentUser?.id !== 1 + ) { + return ( + <> +
+

{intl.formatMessage(messages.password)}

+
+ + {intl.formatMessage(messages.nopermissionDescription)} + + + ); + } + return ( <>
diff --git a/src/i18n/locale/en.json b/src/i18n/locale/en.json index a7ad2a04..1a4315a8 100644 --- a/src/i18n/locale/en.json +++ b/src/i18n/locale/en.json @@ -708,6 +708,8 @@ "components.UserProfile.UserSettings.UserPasswordChange.newpassword": "New Password", "components.UserProfile.UserSettings.UserPasswordChange.nopasswordset": "No Password Set", "components.UserProfile.UserSettings.UserPasswordChange.nopasswordsetDescription": "This user account currently does not have a password specifically for {applicationTitle}. Configure a password below to enable this account to sign in as a \"local user.\"", + "components.UserProfile.UserSettings.UserPasswordChange.nopermission": "No Permission", + "components.UserProfile.UserSettings.UserPasswordChange.nopermissionDescription": "You do not have permission to modify this user's password.", "components.UserProfile.UserSettings.UserPasswordChange.password": "Password", "components.UserProfile.UserSettings.UserPasswordChange.save": "Save Changes", "components.UserProfile.UserSettings.UserPasswordChange.saving": "Saving…",