Skip to content

chore: deprecate getUserRoles saveCustomFields setReaction setUsername saveUserProfile setUserPassword methods #36146

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.

Sign up for GitHub

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

Merged
ggazzo merged 13 commits into from
Aug 4, 2025
Merged

chore: deprecate getUserRoles saveCustomFields setReaction setUsername saveUserProfile setUserPassword methods #36146

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
e9c76e2
fix: Update username change restriction to allow editing only if the …
ggazzo Jul 31, 2025
ddeb1da
fix enforce 2fa only if requirePasswordChange is not set
ggazzo Jul 31, 2025
e901b64
fix: Add logic to unset requirePasswordChange for users after profile…
ggazzo Aug 1, 2025
74bcb81
feat: Add `rooms.getRoles` endpoint and update related methods and hooks
ggazzo Jul 10, 2025
8443d63
feat: Implement `users.getPublicRoles` endpoint and update related au…
ggazzo Jul 10, 2025
d6b0b9f
refactor: Update import path for metrics in deprecationWarningLogger.ts
ggazzo Jun 4, 2025
44a8d90
chore: remove useMethod saveUserProfile setUsername saveCustomFields …
ggazzo Jul 11, 2025
d67b6e8
chore: remove useMethod setReaction
ggazzo Jun 6, 2025
9daac84
chore: depreacte getRoomRoles method and use new endpoint
ggazzo Jul 10, 2025
aaf8bbc
refactor: Replace `getUserRoles` method
ggazzo Jul 11, 2025
f8a1f49
refactor: Update ResetPasswordPage to use new `users.updateOwnBasicIn…
ggazzo Jul 31, 2025
4b512e4
refactor `findByRoomIdAndRoles` to return the projection properly
ggazzo Aug 1, 2025
fc4b2d2
review
ggazzo Aug 4, 2025
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
10 changes: 5 additions & 5 deletions apps/meteor/app/api/server/v1/channels.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -466,11 +466,11 @@ API.v1.addRoute(
return API.v1.forbidden();
}

const moderators = (
await Subscriptions.findByRoomIdAndRoles(findResult._id, ['moderator'], {
projection: { u: 1 },
}).toArray()
).map((sub: ISubscription) => sub.u);
const moderators = await Subscriptions.findByRoomIdAndRoles(findResult._id, ['moderator'], {
projection: { u: 1, _id: 0 },
})
.map((sub) => sub.u)
.toArray();

return API.v1.success({
moderators,
Expand Down
10 changes: 5 additions & 5 deletions apps/meteor/app/api/server/v1/groups.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1215,11 +1215,11 @@ API.v1.addRoute(
userId: this.userId,
});

const moderators = (
await Subscriptions.findByRoomIdAndRoles(findResult.rid, ['moderator'], {
projection: { u: 1 },
}).toArray()
).map((sub: any) => sub.u);
const moderators = await Subscriptions.findByRoomIdAndRoles(findResult.rid, ['moderator'], {
projection: { u: 1, _id: 0 },
})
.map((sub) => sub.u)
.toArray();

return API.v1.success({
moderators,
Expand Down
45 changes: 43 additions & 2 deletions apps/meteor/app/api/server/v1/roles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { api } from '@rocket.chat/core-services';
import { api, Authorization } from '@rocket.chat/core-services';
import type { IRole } from '@rocket.chat/core-typings';
import { Roles, Users } from '@rocket.chat/models';
import { isRoleAddUserToRoleProps, isRoleDeleteProps, isRoleRemoveUserFromRoleProps } from '@rocket.chat/rest-typings';
import { ajv, isRoleAddUserToRoleProps, isRoleDeleteProps, isRoleRemoveUserFromRoleProps } from '@rocket.chat/rest-typings';
import { check, Match } from 'meteor/check';
import { Meteor } from 'meteor/meteor';

Expand All @@ -13,6 +13,7 @@ import { addUserToRole } from '../../../authorization/server/methods/addUserToRo
import { apiDeprecationLogger } from '../../../lib/server/lib/deprecationWarningLogger';
import { notifyOnRoleChanged } from '../../../lib/server/lib/notifyListener';
import { settings } from '../../../settings/server/index';
import type { ExtractRoutesFromAPI } from '../ApiClass';
import { API } from '../api';
import { getPaginationItems } from '../helpers/getPaginationItems';
import { getUserFromParams } from '../helpers/getUserFromParams';
Expand Down Expand Up @@ -243,3 +244,43 @@ API.v1.addRoute(
},
},
);

const rolesRoutes = API.v1.get(
'roles.getUsersInPublicRoles',
{
authRequired: true,
response: {
200: ajv.compile<{
users: {
_id: string;
username: string;
roles: string[];
}[];
}>({
type: 'object',
properties: {
users: {
type: 'array',
items: {
type: 'object',
properties: { _id: { type: 'string' }, username: { type: 'string' }, roles: { type: 'array', items: { type: 'string' } } },
},
},
},
}),
},
},

async () => {
return API.v1.success({
users: await Authorization.getUsersFromPublicRoles(),
});
},
);

type RolesEndpoints = ExtractRoutesFromAPI<typeof rolesRoutes>;

declare module '@rocket.chat/rest-typings' {
// eslint-disable-next-line @typescript-eslint/naming-convention, @typescript-eslint/no-empty-interface
interface Endpoints extends RolesEndpoints {}
}
63 changes: 63 additions & 0 deletions apps/meteor/app/api/server/v1/rooms.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { isPrivateRoom, isPublicRoom } from '@rocket.chat/core-typings';
import { Messages, Rooms, Users, Uploads, Subscriptions } from '@rocket.chat/models';
import type { Notifications } from '@rocket.chat/rest-typings';
import {
ajv,
isGETRoomsNameExists,
isRoomsImagesProps,
isRoomsMuteUnmuteUserProps,
Expand All @@ -23,6 +24,7 @@ import * as dataExport from '../../../../server/lib/dataExport';
import { eraseRoom } from '../../../../server/lib/eraseRoom';
import { findUsersOfRoomOrderedByRole } from '../../../../server/lib/findUsersOfRoomOrderedByRole';
import { openRoom } from '../../../../server/lib/openRoom';
import type { RoomRoles } from '../../../../server/lib/roles/getRoomRoles';
import { hideRoomMethod } from '../../../../server/methods/hideRoom';
import { muteUserInRoom } from '../../../../server/methods/muteUserInRoom';
import { toggleFavoriteMethod } from '../../../../server/methods/toggleFavorite';
Expand All @@ -37,12 +39,14 @@ import { sendFileMessage } from '../../../file-upload/server/methods/sendFileMes
import { syncRolePrioritiesForRoomIfRequired } from '../../../lib/server/functions/syncRolePrioritiesForRoomIfRequired';
import { executeArchiveRoom } from '../../../lib/server/methods/archiveRoom';
import { cleanRoomHistoryMethod } from '../../../lib/server/methods/cleanRoomHistory';
import { executeGetRoomRoles } from '../../../lib/server/methods/getRoomRoles';
import { leaveRoomMethod } from '../../../lib/server/methods/leaveRoom';
import { executeUnarchiveRoom } from '../../../lib/server/methods/unarchiveRoom';
import { applyAirGappedRestrictionsValidation } from '../../../license/server/airGappedRestrictionsWrapper';
import type { NotificationFieldType } from '../../../push-notifications/server/methods/saveNotificationSettings';
import { saveNotificationSettingsMethod } from '../../../push-notifications/server/methods/saveNotificationSettings';
import { settings } from '../../../settings/server';
import type { ExtractRoutesFromAPI } from '../ApiClass';
import { API } from '../api';
import { composeRoomWithLastMessage } from '../helpers/composeRoomWithLastMessage';
import { getPaginationItems } from '../helpers/getPaginationItems';
Expand Down Expand Up @@ -1005,3 +1009,62 @@ API.v1.addRoute(
},
},
);

const isRoomGetRolesPropsSchema = {
type: 'object',
properties: {
rid: { type: 'string' },
},
additionalProperties: false,
Copy link
Contributor

@pierre-lehnen-rc pierre-lehnen-rc Aug 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
additionalProperties: false,
additionalProperties: true,

Avoid blocking additional properties on endpoints unless you have a specific reason to (like passing the whole params object along to something else).
with additionalProperties: false, if we need to add an additional param in the future, it would need to be flagged as a breaking change, while with additionalProperties: true we can still add new params without necessarily breaking compatibility.

Copy link
Member Author

@ggazzo ggazzo Aug 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

additional parameters will never be considered breaking change, removing or saying that extra is no longer supported may be the case.

the default is to not accept extra fields, and it wouldn't be the first case where people pass extra fields believing that something different happens works while it doesn't

Copy link
Contributor

@pierre-lehnen-rc pierre-lehnen-rc Aug 1, 2025

Choose a reason for hiding this comment

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

I'm just following what was decided on the backend chapter meeting. If we don't accept extra fields and then add a new one on a minor release, clients will need to pass a different set of params for different server versions, even though they are in the same major.

Copy link
Member Author

@ggazzo ggazzo Aug 1, 2025

Choose a reason for hiding this comment

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

if a new field is added as required, yes, that is a breaking change, if you add an optional one, thats fine.
but how accepting infinite fields with no use because the client may already be passing it is a bit pointless, how will the client already be passing a field with coherent data of something that does not exist and not need to modify it? How would the client already be passing something in advanced without knowing what should be passed?

Copy link
Member Author

@ggazzo ggazzo Aug 1, 2025

Choose a reason for hiding this comment

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

I think what happened in the past was our client (Rocket.Chat frontend) was passing extra information, pure laziness ours

'We have an object here, send it all something will be used the rest must be discarded-or no', but for me this is a lazy and not recommended reason, this is legacy of the lack of type and anyusage, but it should not happen anymore

Copy link
Contributor

@tassoevan tassoevan Aug 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If a client application uses an extra parameter introduced in a minor version, that means the client is not backward-compatible with versions prior that minor. If it aims to be compatible across a whole set under a major X.* version, it has to use the interface/contract of the initial minor (X.0), without extra parameters. It's normal to assume an extra parameter will be consumed and impact the response; breaking this expectation by swallowing the extra parameter is bad.

Copy link
Member

@sampaiodiego sampaiodiego Aug 4, 2025

Choose a reason for hiding this comment

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

I'm ok by having additionalProperties: false for this particular endpoint since it is a new one, but we need to be careful if we ever change it.. I agree being more permissive (by additionalProperties: true) is something that makes the whole development cycle easier since it doesn't bring any concerns regarding breaking changes and legacy clients, I also see value on being more restrictive as this ensures a tighter contract and removes the risk of having code using a non-documented field.

Copy link
Member

@sampaiodiego sampaiodiego Aug 4, 2025

Choose a reason for hiding this comment

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

and I think we should all agree with whatever definition we come up with and document it so people understand the reasoning behind the decision and also provides a guide for us all when defining the schemas for our endpoints

required: ['rid'],
};
export const roomEndpoints = API.v1.get(
'rooms.roles',
{
authRequired: true,
query: ajv.compile<{
rid: string;
}>(isRoomGetRolesPropsSchema),
response: {
200: ajv.compile<{
roles: RoomRoles[];
}>({
type: 'object',
properties: {
roles: {
type: 'array',
items: {
type: 'object',
properties: {
rid: { type: 'string' },
u: {
type: 'object',
properties: { _id: { type: 'string' }, username: { type: 'string' } },
required: ['_id', 'username'],
},
roles: { type: 'array', items: { type: 'string' } },
},
required: ['rid', 'u', 'roles'],
},
},
},
required: ['roles'],
}),
},
},
async function () {
const { rid } = this.queryParams;
const roles = await executeGetRoomRoles(rid, this.userId);

return API.v1.success({
roles,
});
},
);

type RoomEndpoints = ExtractRoutesFromAPI<typeof roomEndpoints>;

declare module '@rocket.chat/rest-typings' {
// eslint-disable-next-line @typescript-eslint/naming-convention, @typescript-eslint/no-empty-interface
interface Endpoints extends RoomEndpoints {}
}
10 changes: 8 additions & 2 deletions apps/meteor/app/api/server/v1/users.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { MeteorError, Team, api, Calendar } from '@rocket.chat/core-services';
import type { IExportOperation, ILoginToken, IPersonalAccessToken, IUser, UserStatus } from '@rocket.chat/core-typings';
import { type IExportOperation, type ILoginToken, type IPersonalAccessToken, type IUser, type UserStatus } from '@rocket.chat/core-typings';
import { Users, Subscriptions } from '@rocket.chat/models';
import {
isUserCreateParamsPOST,
Expand Down Expand Up @@ -176,7 +176,13 @@ API.v1.addRoute(
twoFactorMethod: 'password',
};

await executeSaveUserProfile.call(this as unknown as Meteor.MethodThisType, userData, this.bodyParams.customFields, twoFactorOptions);
await executeSaveUserProfile.call(
this as unknown as Meteor.MethodThisType,
this.user,
userData,
this.bodyParams.customFields,
twoFactorOptions,
);

return API.v1.success({
user: await Users.findOneById(this.userId, { projection: API.v1.defaultFieldsToExclude }),
Expand Down
6 changes: 2 additions & 4 deletions apps/meteor/app/apps/server/bridges/rooms.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,11 +237,9 @@ export class AppRoomBridge extends RoomBridge {
}

private async getUsersByRoomIdAndSubscriptionRole(roomId: string, role: string): Promise<IUser[]> {
const subs = (await Subscriptions.findByRoomIdAndRoles(roomId, [role], {
const subs = await Subscriptions.findByRoomIdAndRoles<{ uid: string }>(roomId, [role], {
projection: { uid: '$u._id', _id: 0 },
}).toArray()) as unknown as {
uid: string;
}[];
}).toArray();
// Was this a bug?
const users = await Users.findByIds(subs.map((user: { uid: string }) => user.uid)).toArray();
const userConverter = this.orch.getConverters().get('users');
Expand Down
2 changes: 1 addition & 1 deletion apps/meteor/app/lib/server/functions/saveUser/validateUserEditing.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ export async function validateUserEditing(userId: IUser['_id'], userData: Update
if (
isEditingField(user.username, userData.username) &&
!settings.get('Accounts_AllowUsernameChange') &&
(!canEditOtherUserInfo || editingMyself)
(!canEditOtherUserInfo || (editingMyself && user.username))
) {
throw new MeteorError('error-action-not-allowed', 'Edit username is not allowed', {
method: 'insertOrUpdateUser',
Expand Down
2 changes: 1 addition & 1 deletion apps/meteor/app/lib/server/lib/deprecationWarningLogger.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { Logger } from '@rocket.chat/logger';
import semver from 'semver';

import { metrics } from '../../../metrics/server';
import { metrics } from '../../../metrics/server/lib/metrics';

const deprecationLogger = new Logger('DeprecationWarning');

Expand Down
7 changes: 5 additions & 2 deletions apps/meteor/app/lib/server/methods/getRoomRoles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,19 @@
import type { IRoom, ISubscription } from '@rocket.chat/core-typings';
import type { IRoom } from '@rocket.chat/core-typings';
import type { ServerMethods } from '@rocket.chat/ddp-client';
import { Rooms } from '@rocket.chat/models';
import { check } from 'meteor/check';
import { Meteor } from 'meteor/meteor';

import type { RoomRoles } from '../../../../server/lib/roles/getRoomRoles';
import { getRoomRoles } from '../../../../server/lib/roles/getRoomRoles';
import { canAccessRoomAsync } from '../../../authorization/server';
import { settings } from '../../../settings/server';
import { methodDeprecationLogger } from '../lib/deprecationWarningLogger';

declare module '@rocket.chat/ddp-client' {
// eslint-disable-next-line @typescript-eslint/naming-convention
interface ServerMethods {
getRoomRoles(rid: IRoom['_id']): ISubscription[];
getRoomRoles(rid: IRoom['_id']): RoomRoles[];
}
}

Expand All @@ -36,6 +38,7 @@ export const executeGetRoomRoles = async (rid: IRoom['_id'], fromUserId?: string

Meteor.methods<ServerMethods>({
async getRoomRoles(rid) {
methodDeprecationLogger.method('getRoomRoles', '8.0.0', 'Use the /v1/room.getRoles endpoint instead');
const fromUserId = Meteor.userId();

return executeGetRoomRoles(rid, fromUserId);
Expand Down
12 changes: 10 additions & 2 deletions apps/meteor/app/lib/server/methods/getUserRoles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,25 @@
import { Authorization } from '@rocket.chat/core-services';
import type { IUser, IRocketChatRecord } from '@rocket.chat/core-typings';
import type { IUser } from '@rocket.chat/core-typings';
import type { ServerMethods } from '@rocket.chat/ddp-client';
import { Meteor } from 'meteor/meteor';

import { methodDeprecationLogger } from '../lib/deprecationWarningLogger';

declare module '@rocket.chat/ddp-client' {
// eslint-disable-next-line @typescript-eslint/naming-convention
interface ServerMethods {
getUserRoles(): (IRocketChatRecord & Pick<IUser, '_id' | 'roles' | 'username'>)[];
getUserRoles(): Pick<IUser, '_id' | 'roles' | 'username'>[];
}
}

Meteor.methods<ServerMethods>({
async getUserRoles() {
methodDeprecationLogger.method(
'getUserRoles',
'8.0.0',
'This method is deprecated and will be removed in the future. Use the /v1/roles.getUsersInPublicRoles endpoint instead.',
);

if (!Meteor.userId()) {
throw new Meteor.Error('error-invalid-user', 'Invalid user', { method: 'getUserRoles' });
}
Expand Down
2 changes: 2 additions & 0 deletions apps/meteor/app/lib/server/methods/saveCustomFields.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { Meteor } from 'meteor/meteor';

import { saveCustomFields } from '../functions/saveCustomFields';
import { RateLimiter } from '../lib';
import { methodDeprecationLogger } from '../lib/deprecationWarningLogger';

declare module '@rocket.chat/ddp-client' {
// eslint-disable-next-line @typescript-eslint/naming-convention
Expand All @@ -14,6 +15,7 @@ declare module '@rocket.chat/ddp-client' {

Meteor.methods<ServerMethods>({
async saveCustomFields(fields = {}) {
methodDeprecationLogger.method('saveCustomFields', '8.0.0', 'Use the endpoint /v1/users.updateOwnBasicInfo instead');
const uid = Meteor.userId();
if (!uid) {
throw new Meteor.Error('error-invalid-user', 'Invalid user', { method: 'saveCustomFields' });
Expand Down
2 changes: 2 additions & 0 deletions apps/meteor/app/lib/server/methods/setUsername.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { Meteor } from 'meteor/meteor';

import { setUsernameWithValidation } from '../functions/setUsername';
import { RateLimiter } from '../lib';
import { methodDeprecationLogger } from '../lib/deprecationWarningLogger';

declare module '@rocket.chat/ddp-client' {
// eslint-disable-next-line @typescript-eslint/naming-convention
Expand All @@ -14,6 +15,7 @@ declare module '@rocket.chat/ddp-client' {

Meteor.methods<ServerMethods>({
async setUsername(username, param = {}) {
methodDeprecationLogger.method('setUsername', '8.0.0', 'Use the endpoint /v1/users.updateOwnBasicInfo instead');
check(username, String);

const userId = Meteor.userId();
Expand Down
2 changes: 2 additions & 0 deletions apps/meteor/app/reactions/server/setReaction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { canAccessRoomAsync } from '../../authorization/server';
import { hasPermissionAsync } from '../../authorization/server/functions/hasPermission';
import { emoji } from '../../emoji/server';
import { isTheLastMessage } from '../../lib/server/functions/isTheLastMessage';
import { methodDeprecationLogger } from '../../lib/server/lib/deprecationWarningLogger';
import { notifyOnMessageChange } from '../../lib/server/lib/notifyListener';

export const removeUserReaction = (message: IMessage, reaction: string, username: string) => {
Expand Down Expand Up @@ -161,6 +162,7 @@ declare module '@rocket.chat/ddp-client' {

Meteor.methods<ServerMethods>({
async setReaction(reaction, messageId, shouldReact) {
methodDeprecationLogger.method('setReaction', '8.0.0', 'Use the endpoint /v1/chat.react instead');
const uid = Meteor.userId();
if (!uid) {
throw new Meteor.Error('error-invalid-user', 'Invalid user', { method: 'setReaction' });
Expand Down
9 changes: 6 additions & 3 deletions apps/meteor/client/components/message/toolbar/items/actions/ReactionMessageAction.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { isOmnichannelRoom, type IMessage, type IRoom, type ISubscription } from '@rocket.chat/core-typings';
import { useFeaturePreview } from '@rocket.chat/ui-client';
import { useUser, useMethod } from '@rocket.chat/ui-contexts';
import { useUser, useEndpoint } from '@rocket.chat/ui-contexts';
import { useCallback } from 'react';
import { useTranslation } from 'react-i18next';

Expand All @@ -20,7 +20,7 @@ type ReactionMessageActionProps = {
const ReactionMessageAction = ({ message, room, subscription }: ReactionMessageActionProps) => {
const chat = useChat();
const user = useUser();
const setReaction = useMethod('setReaction');
const setReaction = useEndpoint('POST', '/v1/chat.react');
const quickReactionsEnabled = useFeaturePreview('quickReactions');
const { quickReactions, addRecentEmoji } = useEmojiPickerData();
const { t } = useTranslation();
Expand All @@ -44,7 +44,10 @@ const ReactionMessageAction = ({ message, room, subscription }: ReactionMessageA
}

const toggleReaction = (emoji: string) => {
setReaction(`:${emoji}:`, message._id);
setReaction({
emoji: `:${emoji}:`,
messageId: message._id,
});
addRecentEmoji(emoji);
};

Expand Down
Loading
Loading