Skip to content

chore: remove other logins tokens method calls #35754

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 2 commits into from
Apr 29, 2025
Merged

chore: remove other logins tokens method calls #35754

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3b2179d
chore: remove other logins tokens method calls
Apr 9, 2025
66dcf2b
Merge branch 'develop' into chore/remove-other-login-tokens-method
ggazzo Apr 17, 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
3 changes: 2 additions & 1 deletion apps/meteor/app/api/server/v1/users.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import { regeneratePersonalAccessTokenOfUser } from '../../../../imports/persona
import { removePersonalAccessTokenOfUser } from '../../../../imports/personal-access-tokens/server/api/methods/removeToken';
import { UserChangedAuditStore } from '../../../../server/lib/auditServerEvents/userChanged';
import { i18n } from '../../../../server/lib/i18n';
import { removeOtherTokens } from '../../../../server/lib/removeOtherTokens';
import { resetUserE2EEncriptionKey } from '../../../../server/lib/resetUserE2EKey';
import { sendWelcomeEmail } from '../../../../server/lib/sendWelcomeEmail';
import { registerUser } from '../../../../server/methods/registerUser';
Expand Down Expand Up @@ -1135,7 +1136,7 @@ API.v1.addRoute(
{ authRequired: true },
{
async post() {
return API.v1.success(await Meteor.callAsync('removeOtherTokens'));
return API.v1.success(await removeOtherTokens(this.userId, this.connection.id));
Copy link

@kody-ai kody-ai bot Apr 17, 2025

Choose a reason for hiding this comment

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

kody code-review Error Handling high

try {
  const result = await removeOtherTokens(this.userId, this.connection.id);
  return API.v1.success(result);
} catch (error) {
  return API.v1.failure(error instanceof Error ? error.message : 'Unknown error occurred');
}

The removeOtherTokens function call lacks proper error handling, which could lead to unhandled exceptions and poor user experience.

This issue appears in multiple locations:

  • apps/meteor/app/api/server/v1/users.ts: Lines 1139-1139
    Please wrap the removeOtherTokens function call in a try-catch block to handle potential errors gracefully and provide meaningful error messages.

Talk to Kody by mentioning @kody

Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.

​

​

},
},
);
Expand Down
8 changes: 8 additions & 0 deletions apps/meteor/server/lib/removeOtherTokens.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { Users } from '@rocket.chat/models';
import { Accounts } from 'meteor/accounts-base';

export const removeOtherTokens = async function (userId: string, connectionId: string): Promise<void> {
const currentToken = Accounts._getLoginToken(connectionId);

await Users.removeNonLoginTokensExcept(userId, currentToken);
Comment on lines +4 to +7
Copy link

@kody-ai kody-ai bot Apr 17, 2025

Choose a reason for hiding this comment

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

kody code-review Error Handling high

export const removeOtherTokens = async function (userId: string, connectionId: string): Promise<void> {
	try {
		const currentToken = Accounts._getLoginToken(connectionId);
		if (!currentToken) {
			throw new Error('No valid login token found');
		}
		await Users.removeNonLoginTokensExcept(userId, currentToken);
	} catch (error) {
		console.error(`Failed to remove tokens for user ${userId}:`, error);
		throw error;
	}
};

The removeOtherTokens function lacks error handling for token retrieval and removal operations, which could result in silent failures.

This issue appears in multiple locations:

  • apps/meteor/server/lib/removeOtherTokens.ts: Lines 4-7
    Please add try-catch blocks within the removeOtherTokens function to handle potential errors during token operations and log them appropriately.

Talk to Kody by mentioning @kody

Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.

​

​

};
3 changes: 2 additions & 1 deletion apps/meteor/server/methods/saveUserProfile.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { settings as rcSettings } from '../../app/settings/server';
import { setUserStatusMethod } from '../../app/user-status/server/methods/setUserStatus';
import { compareUserPassword } from '../lib/compareUserPassword';
import { compareUserPasswordHistory } from '../lib/compareUserPasswordHistory';
import { removeOtherTokens } from '../lib/removeOtherTokens';

const MAX_BIO_LENGTH = 260;
const MAX_NICKNAME_LENGTH = 120;
Expand Down Expand Up @@ -142,7 +143,7 @@ async function saveUserProfile(
);

try {
await Meteor.callAsync('removeOtherTokens');
await removeOtherTokens(this.userId, this.connection?.id || '');
} catch (e) {
Accounts._clearAllLoginTokens(this.userId);
}
Expand Down
2 changes: 2 additions & 0 deletions packages/model-typings/src/models/IUsersModel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,8 @@ export interface IUsersModel extends IBaseModel<IUser> {

removeNonPATLoginTokensExcept(userId: any, authToken: any): any;

removeNonLoginTokensExcept(userId: any, authToken: any): any;
Copy link

@kody-ai kody-ai bot Apr 17, 2025

Choose a reason for hiding this comment

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

kody code-review Error Handling medium

removeNonLoginTokensExcept(userId: string, authToken: string): Promise<UpdateResult>;

The removeNonLoginTokensExcept method uses 'any' as the return type, which reduces type safety and makes error handling more difficult.

This issue appears in multiple locations:

  • packages/model-typings/src/models/IUsersModel.ts: Lines 184-184
    Please specify a more precise return type (Promise) for the removeNonLoginTokensExcept method to ensure better type safety and error handling.

Talk to Kody by mentioning @kody

Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.

​

​

Copy link

@kody-ai kody-ai bot Apr 17, 2025

Choose a reason for hiding this comment

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

kody code-review Maintainability medium

removeNonLoginTokensExcept(userId: string, authToken: string): Promise<UpdateResult>;

The removeNonLoginTokensExcept method uses 'any' type for parameters, reducing type safety and potentially leading to runtime errors.

This issue appears in multiple locations:

  • packages/model-typings/src/models/IUsersModel.ts: Lines 184-184
    Please replace the 'any' type with specific types (string for userId and authToken) in the removeNonLoginTokensExcept method to improve type safety.

Talk to Kody by mentioning @kody

Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.

​

​


removeRoomsByRoomIdsAndUserId(rids: any, userId: any): any;

removeRolesByUserId(uid: IUser['_id'], roles: IRole['_id'][]): Promise<UpdateResult>;
Expand Down
15 changes: 15 additions & 0 deletions packages/models/src/models/Users.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1324,6 +1324,21 @@ export class UsersRaw extends BaseRaw<IUser, DefaultFields<IUser>> implements IU
);
}

removeNonLoginTokensExcept(userId: IUser['_id'], authToken: string) {
return this.col.updateOne(
{
_id: userId,
},
{
$pull: {
'services.resume.loginTokens': {
hashedToken: { $ne: authToken },
},
},
},
);
Comment on lines +1327 to +1339
Copy link

@kody-ai kody-ai bot Apr 17, 2025

Choose a reason for hiding this comment

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

kody code-review Security high

removeNonLoginTokensExcept(userId: IUser['_id'], authToken: string) {
		if (!userId || !authToken || typeof authToken !== 'string' || authToken.trim() === '') {
			throw new Error('Invalid userId or authToken parameters');
		}
		return this.col.updateOne(
			{
				_id: userId,
			},
			{
				$pull: {
					'services.resume.loginTokens': {
						hashedToken: { $ne: authToken },
					},
				},
			},
		);
	}

The removeNonLoginTokensExcept method lacks validation for userId and authToken parameters, which could lead to security issues.

This issue appears in multiple locations:

  • packages/models/src/models/Users.ts: Lines 1327-1339
    Please add parameter validation to the removeNonLoginTokensExcept method to ensure userId and authToken are valid and non-empty strings.

Talk to Kody by mentioning @kody

Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.

​

​

}

removeRoomsByRoomIdsAndUserId(rids: IRoom['_id'][], userId: IUser['_id']) {
return this.updateMany(
{
Expand Down
Loading