From 16297ce76324e7a6aff59346d8fbfc02c8688ab2 Mon Sep 17 00:00:00 2001 From: Marcos Defendi Date: Fri, 11 Nov 2022 02:46:58 -0300 Subject: [PATCH 1/8] fix: disabling admin actions + do not count only federated and remote users as seat cap --- apps/meteor/app/models/server/models/Users.js | 1 + .../client/views/admin/users/AdminUserInfoActions.tsx | 10 +++++----- .../views/admin/users/SeatsCapUsage/SeatsCapUsage.tsx | 2 +- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/apps/meteor/app/models/server/models/Users.js b/apps/meteor/app/models/server/models/Users.js index e94a2458104b..6e01251dad6c 100644 --- a/apps/meteor/app/models/server/models/Users.js +++ b/apps/meteor/app/models/server/models/Users.js @@ -1006,6 +1006,7 @@ export class Users extends Base { { active: true, federated: true, + username: { $regex: /.*:.*/ }, }, options, ); diff --git a/apps/meteor/client/views/admin/users/AdminUserInfoActions.tsx b/apps/meteor/client/views/admin/users/AdminUserInfoActions.tsx index 124dcbac9fb3..6aaa83ea231b 100644 --- a/apps/meteor/client/views/admin/users/AdminUserInfoActions.tsx +++ b/apps/meteor/client/views/admin/users/AdminUserInfoActions.tsx @@ -79,11 +79,11 @@ const AdminUserInfoActions = ({ disabled: isAFederatedUser, }, }), - ...(changeAdminStatusAction && { makeAdmin: changeAdminStatusAction }), - ...(resetE2EKeyAction && { resetE2EKey: resetE2EKeyAction }), - ...(resetTOTPAction && { resetTOTP: resetTOTPAction }), - ...(deleteUserAction && { delete: deleteUserAction }), - ...(changeUserStatusAction && { changeActiveStatus: changeUserStatusAction }), + ...(changeAdminStatusAction && !isAFederatedUser && { makeAdmin: changeAdminStatusAction }), + ...(resetE2EKeyAction && !isAFederatedUser && { resetE2EKey: resetE2EKeyAction }), + ...(resetTOTPAction && !isAFederatedUser && { resetTOTP: resetTOTPAction }), + ...(deleteUserAction && !isAFederatedUser && { delete: deleteUserAction }), + ...(changeUserStatusAction && !isAFederatedUser && { changeActiveStatus: changeUserStatusAction }), }), [ t, diff --git a/apps/meteor/ee/client/views/admin/users/SeatsCapUsage/SeatsCapUsage.tsx b/apps/meteor/ee/client/views/admin/users/SeatsCapUsage/SeatsCapUsage.tsx index ee64fbffbdd0..aea22aad16bd 100644 --- a/apps/meteor/ee/client/views/admin/users/SeatsCapUsage/SeatsCapUsage.tsx +++ b/apps/meteor/ee/client/views/admin/users/SeatsCapUsage/SeatsCapUsage.tsx @@ -25,7 +25,7 @@ const SeatsCapUsage = ({ limit, members }: SeatsCapUsageProps): ReactElement => fontScale='c1' mb='x8' > -
{t('Seats_Available', { seatsLeft })}
+
{t('Seats_Available', { seatsLeft })}
{`${members}/${limit}`} From d49a9c497078f99d179f6b9ec25202151d489306 Mon Sep 17 00:00:00 2001 From: Marcos Defendi Date: Fri, 11 Nov 2022 03:11:34 -0300 Subject: [PATCH 2/8] fix: consider as federated users only remote users --- .../meteor/client/views/admin/users/AdminUserInfoWithData.tsx | 4 ++-- apps/meteor/client/views/admin/users/EditUserWithData.tsx | 2 +- packages/core-typings/src/IUser.ts | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/apps/meteor/client/views/admin/users/AdminUserInfoWithData.tsx b/apps/meteor/client/views/admin/users/AdminUserInfoWithData.tsx index 3cb2efebc811..f4032e4b02e6 100644 --- a/apps/meteor/client/views/admin/users/AdminUserInfoWithData.tsx +++ b/apps/meteor/client/views/admin/users/AdminUserInfoWithData.tsx @@ -1,4 +1,4 @@ -import { IUser } from '@rocket.chat/core-typings'; +import { isUserFederated, IUser } from '@rocket.chat/core-typings'; import { Callout } from '@rocket.chat/fuselage'; import { useMutableCallback } from '@rocket.chat/fuselage-hooks'; import { useSetting, useRolesDescription, useTranslation } from '@rocket.chat/ui-contexts'; @@ -109,7 +109,7 @@ const AdminUserInfoWithData = ({ uid, onReload }: AdminUserInfoWithDataProps): R isAdmin={data?.user.roles.includes('admin')} userId={data?.user._id} username={user.username} - isAFederatedUser={data?.user.federated} + isAFederatedUser={isUserFederated(data?.user as unknown as IUser)} onChange={onChange} onReload={onReload} /> diff --git a/apps/meteor/client/views/admin/users/EditUserWithData.tsx b/apps/meteor/client/views/admin/users/EditUserWithData.tsx index f42dd33bd697..c165870918c2 100644 --- a/apps/meteor/client/views/admin/users/EditUserWithData.tsx +++ b/apps/meteor/client/views/admin/users/EditUserWithData.tsx @@ -41,7 +41,7 @@ const EditUserWithData = ({ uid, onReload, ...props }: EditUserWithDataProps): R ); } - if (data?.user && isUserFederated({ federated: data?.user.federated })) { + if (data?.user && isUserFederated(data?.user as unknown as IUser)) { return ( {t('Edit_Federated_User_Not_Allowed')} diff --git a/packages/core-typings/src/IUser.ts b/packages/core-typings/src/IUser.ts index b3f03ad7e03c..e35bef04ae21 100644 --- a/packages/core-typings/src/IUser.ts +++ b/packages/core-typings/src/IUser.ts @@ -158,7 +158,7 @@ export interface IRegisterUser extends IUser { } export const isRegisterUser = (user: IUser): user is IRegisterUser => user.username !== undefined && user.name !== undefined; -export const isUserFederated = (user: Partial): user is IUser => 'federated' in user && user.federated === true; +export const isUserFederated = (user: Partial): user is IUser => 'federated' in user && user.federated === true && Boolean(user.username?.includes(':')); export type IUserDataEvent = { id: unknown; From db3dc7242fbad70a9342b0b0f31f80f83f725073 Mon Sep 17 00:00:00 2001 From: Marcos Defendi Date: Fri, 11 Nov 2022 03:14:49 -0300 Subject: [PATCH 3/8] chore: fix lint --- packages/core-typings/src/IUser.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/core-typings/src/IUser.ts b/packages/core-typings/src/IUser.ts index e35bef04ae21..e8a6d39da88d 100644 --- a/packages/core-typings/src/IUser.ts +++ b/packages/core-typings/src/IUser.ts @@ -158,7 +158,8 @@ export interface IRegisterUser extends IUser { } export const isRegisterUser = (user: IUser): user is IRegisterUser => user.username !== undefined && user.name !== undefined; -export const isUserFederated = (user: Partial): user is IUser => 'federated' in user && user.federated === true && Boolean(user.username?.includes(':')); +export const isUserFederated = (user: Partial): user is IUser => + 'federated' in user && user.federated === true && Boolean(user.username?.includes(':')); export type IUserDataEvent = { id: unknown; From d8ccfbc81f52464f293788188ceb3e99ac4b648f Mon Sep 17 00:00:00 2001 From: Marcos Defendi Date: Mon, 14 Nov 2022 11:37:05 -0300 Subject: [PATCH 4/8] fix: make sure everything is being validated on BE as well --- apps/meteor/app/2fa/server/functions/resetTOTP.ts | 6 ++++++ apps/meteor/app/lib/server/functions/deleteUser.ts | 11 ++++------- .../app/lib/server/functions/setUserActiveStatus.ts | 7 +++++++ apps/meteor/app/lib/server/methods/setAdminStatus.ts | 7 ++++++- apps/meteor/server/lib/resetUserE2EKey.ts | 6 ++++++ 5 files changed, 29 insertions(+), 8 deletions(-) diff --git a/apps/meteor/app/2fa/server/functions/resetTOTP.ts b/apps/meteor/app/2fa/server/functions/resetTOTP.ts index decce3432be5..a080573e0eae 100644 --- a/apps/meteor/app/2fa/server/functions/resetTOTP.ts +++ b/apps/meteor/app/2fa/server/functions/resetTOTP.ts @@ -1,6 +1,7 @@ import { Meteor } from 'meteor/meteor'; import { TAPi18n } from 'meteor/rocketchat:tap-i18n'; import type { IUser } from '@rocket.chat/core-typings'; +import { isUserFederated } from '@rocket.chat/core-typings'; import { Users } from '@rocket.chat/models'; import { settings } from '../../../settings/server'; @@ -60,6 +61,11 @@ export async function resetTOTP(userId: string, notifyUser = false): Promise { const user = Users.findOneById(userId, { - fields: { username: 1, avatarOrigin: 1, federation: 1, roles: 1 }, + fields: { username: 1, avatarOrigin: 1, roles: 1, federated: 1 }, }); if (!user) { return; } - if (user.federation) { - const existingSubscriptions = Subscriptions.find({ 'u._id': user._id }).count(); - - if (existingSubscriptions > 0) { - throw new Meteor.Error('FEDERATION_Error_user_is_federated_on_rooms'); - } + if (isUserFederated(user)) { + throw new Meteor.Error('error-user-is-federated', 'Cannot delete federated user'); } const subscribedRooms = getSubscribedRoomsForUserWithDetails(userId); diff --git a/apps/meteor/app/lib/server/functions/setUserActiveStatus.ts b/apps/meteor/app/lib/server/functions/setUserActiveStatus.ts index 998074d89e61..10b28845f567 100644 --- a/apps/meteor/app/lib/server/functions/setUserActiveStatus.ts +++ b/apps/meteor/app/lib/server/functions/setUserActiveStatus.ts @@ -2,6 +2,7 @@ import { Meteor } from 'meteor/meteor'; import { check } from 'meteor/check'; import { Accounts } from 'meteor/accounts-base'; import type { IUser, IUserEmail, IDirectMessageRoom } from '@rocket.chat/core-typings'; +import { isUserFederated } from '@rocket.chat/core-typings'; import * as Mailer from '../../../mailer'; import { Users, Subscriptions, Rooms } from '../../../models/server'; @@ -43,6 +44,12 @@ export function setUserActiveStatus(userId: string, active: boolean, confirmReli return false; } + if (isUserFederated(user)) { + throw new Meteor.Error('error-user-is-federated', 'Cannot change federated users status', { + method: 'setUserActiveStatus', + }); + } + // Users without username can't do anything, so there is no need to check for owned rooms if (user.username != null && !active) { const userAdmin = Users.findOneAdmin(userId); diff --git a/apps/meteor/app/lib/server/methods/setAdminStatus.ts b/apps/meteor/app/lib/server/methods/setAdminStatus.ts index 2f416fe8e6d7..78042a6a444d 100644 --- a/apps/meteor/app/lib/server/methods/setAdminStatus.ts +++ b/apps/meteor/app/lib/server/methods/setAdminStatus.ts @@ -1,5 +1,6 @@ import { Meteor } from 'meteor/meteor'; import { Match, check } from 'meteor/check'; +import { isUserFederated } from '@rocket.chat/core-typings'; import { hasPermission } from '../../../authorization/server'; @@ -18,7 +19,11 @@ Meteor.methods({ throw new Meteor.Error('error-not-allowed', 'Not allowed', { method: 'setAdminStatus' }); } - const user = Meteor.users.findOne({ _id: userId }, { fields: { username: 1 } }); + const user = Meteor.users.findOne({ _id: userId }, { fields: { username: 1, federated: 1 } }); + console.log({ user }); + if (!user || isUserFederated(user)) { + throw new Meteor.Error('error-not-allowed', 'Federated Users cant be admins', { method: 'setAdminStatus' }); + } if (admin) { return Meteor.call('authorization:addUserToRole', 'admin', user?.username); diff --git a/apps/meteor/server/lib/resetUserE2EKey.ts b/apps/meteor/server/lib/resetUserE2EKey.ts index 3897d18bbba1..7300c61724dd 100644 --- a/apps/meteor/server/lib/resetUserE2EKey.ts +++ b/apps/meteor/server/lib/resetUserE2EKey.ts @@ -1,6 +1,7 @@ import { Meteor } from 'meteor/meteor'; import { TAPi18n } from 'meteor/rocketchat:tap-i18n'; import type { IUser } from '@rocket.chat/core-typings'; +import { isUserFederated } from '@rocket.chat/core-typings'; import { Users, Subscriptions } from '../../app/models/server'; import { settings } from '../../app/settings/server'; @@ -61,6 +62,11 @@ export function resetUserE2EEncriptionKey(uid: string, notifyUser: boolean): boo sendResetNotitification(uid); } + const user = Users.findOneById(uid, { fields: { federated: 1, username: 1 } }); + if (isUserFederated(user)) { + throw new Meteor.Error('error-not-allowed', 'Federated Users cant have e2e encryption', { function: 'resetUserE2EEncriptionKey' }); + } + Users.resetE2EKey(uid); Subscriptions.resetUserE2EKey(uid); From 1a42dacbc34c14e55f7e057f2320fc15e319bb7f Mon Sep 17 00:00:00 2001 From: Marcos Defendi Date: Wed, 14 Dec 2022 22:45:38 -0300 Subject: [PATCH 5/8] fix: removing forgotten log --- apps/meteor/app/lib/server/methods/setAdminStatus.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/apps/meteor/app/lib/server/methods/setAdminStatus.ts b/apps/meteor/app/lib/server/methods/setAdminStatus.ts index 78042a6a444d..0a86000f0e68 100644 --- a/apps/meteor/app/lib/server/methods/setAdminStatus.ts +++ b/apps/meteor/app/lib/server/methods/setAdminStatus.ts @@ -20,7 +20,6 @@ Meteor.methods({ } const user = Meteor.users.findOne({ _id: userId }, { fields: { username: 1, federated: 1 } }); - console.log({ user }); if (!user || isUserFederated(user)) { throw new Meteor.Error('error-not-allowed', 'Federated Users cant be admins', { method: 'setAdminStatus' }); } From 09f1232f21b72e487f8d943157ae3005fa66d7ef Mon Sep 17 00:00:00 2001 From: Marcos Defendi Date: Wed, 4 Jan 2023 10:51:17 -0300 Subject: [PATCH 6/8] fix: apply suggestions from review --- apps/meteor/app/models/server/models/Users.js | 1 - 1 file changed, 1 deletion(-) diff --git a/apps/meteor/app/models/server/models/Users.js b/apps/meteor/app/models/server/models/Users.js index 80cbabec254f..181fbfdeb917 100644 --- a/apps/meteor/app/models/server/models/Users.js +++ b/apps/meteor/app/models/server/models/Users.js @@ -1006,7 +1006,6 @@ export class Users extends Base { { active: true, federated: true, - username: { $regex: /.*:.*/ }, }, options, ); From 380e309ddf06fc18c3eb9a873b2c00b6d624962e Mon Sep 17 00:00:00 2001 From: Marcos Defendi Date: Wed, 25 Jan 2023 09:49:17 -0300 Subject: [PATCH 7/8] fix: apply suggestions from review (WIP) --- .../admin/users/AdminUserInfoActions.tsx | 20 +++++++++---------- .../admin/users/AdminUserInfoWithData.tsx | 2 +- .../users/SeatsCapUsage/SeatsCapUsage.tsx | 2 +- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/apps/meteor/client/views/admin/users/AdminUserInfoActions.tsx b/apps/meteor/client/views/admin/users/AdminUserInfoActions.tsx index 3647e8849e13..440ef3dfdb06 100644 --- a/apps/meteor/client/views/admin/users/AdminUserInfoActions.tsx +++ b/apps/meteor/client/views/admin/users/AdminUserInfoActions.tsx @@ -15,7 +15,7 @@ import { useResetTOTPAction } from './hooks/useResetTOTPAction'; type AdminUserInfoActionsProps = { username: IUser['username']; userId: IUser['_id']; - isAFederatedUser: IUser['federated']; + isFederatedUser: IUser['federated']; isActive: boolean; isAdmin: boolean; onChange: () => void; @@ -25,7 +25,7 @@ type AdminUserInfoActionsProps = { const AdminUserInfoActions = ({ username, userId, - isAFederatedUser, + isFederatedUser, isActive, isAdmin, onChange, @@ -75,16 +75,16 @@ const AdminUserInfoActions = ({ editUser: { icon: 'edit', label: t('Edit'), - title: isAFederatedUser ? t('Edit_Federated_User_Not_Allowed') : t('Edit'), + title: isFederatedUser ? t('Edit_Federated_User_Not_Allowed') : t('Edit'), action: editUserClick, - disabled: isAFederatedUser, + disabled: isFederatedUser, }, }), - ...(changeAdminStatusAction && !isAFederatedUser && { makeAdmin: changeAdminStatusAction }), - ...(resetE2EKeyAction && !isAFederatedUser && { resetE2EKey: resetE2EKeyAction }), - ...(resetTOTPAction && !isAFederatedUser && { resetTOTP: resetTOTPAction }), - ...(deleteUserAction && !isAFederatedUser && { delete: deleteUserAction }), - ...(changeUserStatusAction && !isAFederatedUser && { changeActiveStatus: changeUserStatusAction }), + ...(changeAdminStatusAction && !isFederatedUser && { makeAdmin: changeAdminStatusAction }), + ...(resetE2EKeyAction && !isFederatedUser && { resetE2EKey: resetE2EKeyAction }), + ...(resetTOTPAction && !isFederatedUser && { resetTOTP: resetTOTPAction }), + ...(deleteUserAction && !isFederatedUser && { delete: deleteUserAction }), + ...(changeUserStatusAction && !isFederatedUser && { changeActiveStatus: changeUserStatusAction }), }), [ t, @@ -97,7 +97,7 @@ const AdminUserInfoActions = ({ deleteUserAction, resetE2EKeyAction, resetTOTPAction, - isAFederatedUser, + isFederatedUser, ], ); diff --git a/apps/meteor/client/views/admin/users/AdminUserInfoWithData.tsx b/apps/meteor/client/views/admin/users/AdminUserInfoWithData.tsx index 8f123a661329..5c875ee54a02 100644 --- a/apps/meteor/client/views/admin/users/AdminUserInfoWithData.tsx +++ b/apps/meteor/client/views/admin/users/AdminUserInfoWithData.tsx @@ -108,7 +108,7 @@ const AdminUserInfoWithData = ({ uid, onReload }: AdminUserInfoWithDataProps): R isAdmin={data?.user.roles.includes('admin')} userId={data?.user._id} username={user.username} - isAFederatedUser={isUserFederated(data?.user as unknown as IUser)} + isFederatedUser={isUserFederated(data?.user as unknown as IUser)} onChange={onChange} onReload={onReload} /> diff --git a/apps/meteor/ee/client/views/admin/users/SeatsCapUsage/SeatsCapUsage.tsx b/apps/meteor/ee/client/views/admin/users/SeatsCapUsage/SeatsCapUsage.tsx index 8c5a1b7c6046..0c4ca1c3b18f 100644 --- a/apps/meteor/ee/client/views/admin/users/SeatsCapUsage/SeatsCapUsage.tsx +++ b/apps/meteor/ee/client/views/admin/users/SeatsCapUsage/SeatsCapUsage.tsx @@ -26,7 +26,7 @@ const SeatsCapUsage = ({ limit, members }: SeatsCapUsageProps): ReactElement => fontScale='c1' mb='x8' > -
{t('Seats_Available', { seatsLeft })}
+
{t('Seats_Available', { seatsLeft })}
{`${members}/${limit}`} From f6a6e2eea56ff7870a046f5ffcffefd08ff9690b Mon Sep 17 00:00:00 2001 From: Marcos Defendi Date: Wed, 25 Jan 2023 12:52:34 -0300 Subject: [PATCH 8/8] fix: apply suggestions from review --- packages/core-typings/src/IUser.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/core-typings/src/IUser.ts b/packages/core-typings/src/IUser.ts index 21e9a4728bdd..2c0f611f2947 100644 --- a/packages/core-typings/src/IUser.ts +++ b/packages/core-typings/src/IUser.ts @@ -164,8 +164,7 @@ export interface IRegisterUser extends IUser { } export const isRegisterUser = (user: IUser): user is IRegisterUser => user.username !== undefined && user.name !== undefined; -export const isUserFederated = (user: Partial): user is IUser => - 'federated' in user && user.federated === true && Boolean(user.username?.includes(':')); +export const isUserFederated = (user: Partial): user is IUser => 'federated' in user && user.federated === true; export type IUserDataEvent = { id: unknown;