chore: remove other logins tokens method calls #35754
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #35754 +/- ##
========================================
Coverage 59.64% 59.64%
========================================
Files 2832 2832
Lines 68322 68322
Branches 15133 15133
========================================
Hits 40750 40750
Misses 24963 24963
Partials 2609 2609
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
![kody-ai[bot]](https://avatars.githubusercontent.com/in/413034?s=60&v=4){kind=small}
| removeNonLoginTokensExcept(userId: IUser['_id'], authToken: string) { | ||
| return this.col.updateOne( | ||
| { | ||
| _id: userId, | ||
| }, | ||
| { | ||
| $pull: { | ||
| 'services.resume.loginTokens': { | ||
| hashedToken: { $ne: authToken }, | ||
| }, | ||
| }, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
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.
| { | ||
| async post() { | ||
| return API.v1.success(await Meteor.callAsync('removeOtherTokens')); | ||
| return API.v1.success(await removeOtherTokens(this.userId, this.connection.id)); |
There was a problem hiding this comment.
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.
| export const removeOtherTokens = async function (userId: string, connectionId: string): Promise<void> { | ||
| const currentToken = Accounts._getLoginToken(connectionId); | ||
|
|
||
| await Users.removeNonLoginTokensExcept(userId, currentToken); |
There was a problem hiding this comment.
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.
|
|
||
| removeNonPATLoginTokensExcept(userId: any, authToken: any): any; | ||
|
|
||
| removeNonLoginTokensExcept(userId: any, authToken: any): any; |
There was a problem hiding this comment.
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.
|
|
||
| removeNonPATLoginTokensExcept(userId: any, authToken: any): any; | ||
|
|
||
| removeNonLoginTokensExcept(userId: any, authToken: any): any; |
There was a problem hiding this comment.
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.
ggazzo
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
https://rocketchat.atlassian.net/browse/ARCH-1565
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
This pull request focuses on improving the token management system within the Rocket.Chat application. The changes involve the removal of indirect method calls for token management, specifically targeting the removal of other login tokens. Key updates include:
Direct Function Call: In
apps/meteor/app/api/server/v1/users.ts, the code now directly calls theremoveOtherTokensfunction instead of usingMeteor.callAsync, enhancing code maintainability by reducing indirection.New Function Implementation: A new function is introduced in
apps/meteor/server/lib/removeOtherTokens.tsto remove non-login tokens, excluding the current one. However, this implementation currently lacks error handling and documentation.Security Enhancement: The
apps/meteor/server/methods/saveUserProfile.tsfile has been updated to import the new function and modify the token removal logic. This change improves security by directly removing other tokens rather than relying on a Meteor method.Interface Update: The
packages/model-typings/src/models/IUsersModel.tsfile now includes a new method,removeNonLoginTokensExcept, in theIUsersModelinterface.Model Update: In
packages/models/src/models/Users.ts, a new method has been added to remove non-login tokens except for a specified one, aligning with the interface update.These changes collectively aim to streamline the token management process, enhance security, and improve code clarity within the Rocket.Chat application.