fix: `Accounts_LoginExpiration` being used differently on codebase (#32527)

.changeset/empty-readers-teach.md

@@ -0,0 +1,8 @@
+---
+"@rocket.chat/meteor": patch
+"@rocket.chat/tools": patch
+"@rocket.chat/account-service": patch
+---
+
+Fixed an inconsistent evaluation of the `Accounts_LoginExpiration` setting over the codebase. In some places, it was being used as milliseconds while in others as days. Invalid values produced different results. A helper function was created to centralize the setting validation and the proper value being returned to avoid edge cases.
+Negative values may be saved on the settings UI panel but the code will interpret any negative, NaN or 0 value to the default expiration which is 90 days. 

apps/meteor/app/api/server/v1/users.ts

@@ -19,6 +19,7 @@ import {
 	isUsersCheckUsernameAvailabilityParamsGET,
 	isUsersSendConfirmationEmailParamsPOST,
 } from '@rocket.chat/rest-typings';
+import { getLoginExpirationInMs } from '@rocket.chat/tools';
 import { Accounts } from 'meteor/accounts-base';
 import { Match, check } from 'meteor/check';
 import { Meteor } from 'meteor/meteor';
@@ -1048,8 +1049,9 @@ API.v1.addRoute(
 
 			const token = me.services?.resume?.loginTokens?.find((token) => token.hashedToken === hashedToken);
 
-			const tokenExpires =
-				(token && 'when' in token && new Date(token.when.getTime() + settings.get<number>('Accounts_LoginExpiration') * 1000)) || undefined;
+			const loginExp = settings.get<number>('Accounts_LoginExpiration');
+
+			const tokenExpires = (token && 'when' in token && new Date(token.when.getTime() + getLoginExpirationInMs(loginExp))) || undefined;
 
 			return API.v1.success({
 				token: xAuthToken,

apps/meteor/app/authentication/server/startup/index.js

@@ -2,6 +2,7 @@ import { Apps, AppEvents } from '@rocket.chat/apps';
 import { User } from '@rocket.chat/core-services';
 import { Roles, Settings, Users } from '@rocket.chat/models';
 import { escapeRegExp, escapeHTML } from '@rocket.chat/string-helpers';
+import { getLoginExpirationInDays } from '@rocket.chat/tools';
 import { Accounts } from 'meteor/accounts-base';
 import { Match } from 'meteor/check';
 import { Meteor } from 'meteor/meteor';
@@ -31,7 +32,7 @@ Accounts.config({
 
 Meteor.startup(() => {
 	settings.watchMultiple(['Accounts_LoginExpiration', 'Site_Name', 'From_Email'], () => {
-		Accounts._options.loginExpirationInDays = settings.get('Accounts_LoginExpiration');
+		Accounts._options.loginExpirationInDays = getLoginExpirationInDays(settings.get('Accounts_LoginExpiration'));
 
 		Accounts.emailTemplates.siteName = settings.get('Site_Name');
 

ee/apps/account-service/Dockerfile

@@ -43,6 +43,9 @@ COPY ./ee/packages/license/dist packages/license/dist
 COPY ./packages/ui-kit/package.json packages/ui-kit/package.json
 COPY ./packages/ui-kit/dist packages/ui-kit/dist
 
+COPY ./packages/tools/package.json packages/tools/package.json
+COPY ./packages/tools/dist packages/tools/dist
+
 COPY ./ee/apps/${SERVICE}/dist .
 
 COPY ./package.json .

ee/apps/account-service/package.json

@@ -22,6 +22,7 @@
 		"@rocket.chat/models": "workspace:^",
 		"@rocket.chat/rest-typings": "workspace:^",
 		"@rocket.chat/string-helpers": "~0.31.25",
+		"@rocket.chat/tools": "workspace:^",
 		"@types/node": "^14.18.63",
 		"bcrypt": "^5.0.1",
 		"ejson": "^2.2.3",

ee/apps/account-service/src/Account.ts

@@ -1,6 +1,7 @@
 import { ServiceClass } from '@rocket.chat/core-services';
 import type { IAccount, ILoginResult } from '@rocket.chat/core-services';
 import { Settings } from '@rocket.chat/models';
+import { getLoginExpirationInDays } from '@rocket.chat/tools';
 
 import { loginViaResume } from './lib/loginViaResume';
 import { loginViaUsername } from './lib/loginViaUsername';
@@ -22,9 +23,8 @@ export class Account extends ServiceClass implements IAccount {
 			if (_id !== 'Accounts_LoginExpiration') {
 				return;
 			}
-			if (typeof value === 'number') {
-				this.loginExpiration = value;
-			}
+
+			this.loginExpiration = getLoginExpirationInDays(value as number);
 		});
 	}
 
@@ -46,8 +46,7 @@ export class Account extends ServiceClass implements IAccount {
 
 	async started(): Promise<void> {
 		const expiry = await Settings.findOne({ _id: 'Accounts_LoginExpiration' }, { projection: { value: 1 } });
-		if (expiry?.value) {
-			this.loginExpiration = expiry.value as number;
-		}
+
+		this.loginExpiration = getLoginExpirationInDays(expiry?.value as number);
 	}
 }

ee/apps/account-service/src/lib/utils.ts

@@ -1,5 +1,6 @@
 import crypto from 'crypto';
 
+import { convertFromDaysToMilliseconds } from '@rocket.chat/tools';
 import bcrypt from 'bcrypt';
 import { v4 as uuidv4 } from 'uuid';
 
@@ -60,4 +61,4 @@ export const validatePassword = (password: string, bcryptPassword: string): Prom
 	bcrypt.compare(getPassword(password), bcryptPassword);
 
 export const _tokenExpiration = (when: string | Date, expirationInDays: number): Date =>
-	new Date(new Date(when).getTime() + expirationInDays * 60 * 60 * 24 * 1000);
+	new Date(new Date(when).getTime() + convertFromDaysToMilliseconds(expirationInDays));

packages/tools/jest.config.ts

@@ -0,0 +1,3 @@
+export default {
+	preset: 'ts-jest',
+};

packages/tools/package.json

@@ -13,7 +13,9 @@
 		"lint": "eslint --ext .js,.jsx,.ts,.tsx .",
 		"lint:fix": "eslint --ext .js,.jsx,.ts,.tsx . --fix",
 		"test": "jest",
+		"test:cov": "jest --coverage",
 		"build": "rm -rf dist && tsc -p tsconfig.json",
+		"testunit": "jest",
 		"dev": "tsc -p tsconfig.json --watch --preserveWatchOutput"
 	},
 	"main": "./dist/index.js",

packages/tools/src/converter.spec.ts

@@ -0,0 +1,15 @@
+import { convertFromDaysToMilliseconds } from './converter';
+
+describe('convertFromDaysToMilliseconds', () => {
+	it('should throw an error when a non number is passed', () => {
+		// @ts-expect-error - Testing
+		expect(() => convertFromDaysToMilliseconds('90')).toThrow();
+	});
+	it('should return the value passed when its valid', () => {
+		expect(convertFromDaysToMilliseconds(85)).toBe(85 * 24 * 60 * 60 * 1000);
+	});
+	it('should fail if anything but an integer is passed', () => {
+		expect(() => convertFromDaysToMilliseconds(85.5)).toThrow();
+		expect(() => convertFromDaysToMilliseconds(-2.3)).toThrow();
+	});
+});

packages/tools/src/converter.ts

@@ -0,0 +1,7 @@
+export const convertFromDaysToMilliseconds = (days: number) => {
+	if (typeof days !== 'number' || !Number.isInteger(days)) {
+		throw new Error('days must be a number');
+	}
+
+	return days * 24 * 60 * 60 * 1000;
+};

packages/tools/src/getLoginExpiration.spec.ts

@@ -0,0 +1,35 @@
+import { getLoginExpirationInDays, getLoginExpirationInMs } from './getLoginExpiration';
+
+describe('getLoginExpirationInDays', () => {
+	it('should return 90 by default', () => {
+		expect(getLoginExpirationInDays()).toBe(90);
+	});
+	it('should return 90 when value is 0', () => {
+		expect(getLoginExpirationInDays(0)).toBe(90);
+	});
+	it('should return 90 when value is NaN', () => {
+		expect(getLoginExpirationInDays(NaN)).toBe(90);
+	});
+	it('should return 90 when value is negative', () => {
+		expect(getLoginExpirationInDays(-1)).toBe(90);
+	});
+	it('should return 90 when value is undefined', () => {
+		expect(getLoginExpirationInDays(undefined)).toBe(90);
+	});
+	it('should return 90 when value is not a number', () => {
+		// @ts-expect-error - Testing
+		expect(getLoginExpirationInDays('90')).toBe(90);
+	});
+	it('should return the value passed when its valid', () => {
+		expect(getLoginExpirationInDays(85)).toBe(85);
+	});
+});
+
+describe('getLoginExpirationInMs', () => {
+	it('should return 90 days in milliseconds when no value is passed', () => {
+		expect(getLoginExpirationInMs()).toBe(90 * 24 * 60 * 60 * 1000);
+	});
+	it('should return the value passed when its valid', () => {
+		expect(getLoginExpirationInMs(85)).toBe(85 * 24 * 60 * 60 * 1000);
+	});
+});

packages/tools/src/getLoginExpiration.ts

@@ -0,0 +1,16 @@
+import { convertFromDaysToMilliseconds } from './converter';
+
+const ACCOUNTS_DEFAULT_LOGIN_EXPIRATION_DAYS = 90;
+
+// Given a value, validates if it mets the conditions to be a valid login expiration.
+// Else, returns the default login expiration (which for Meteor is 90 days)
+export const getLoginExpirationInDays = (expiry?: number) => {
+	if (expiry && typeof expiry === 'number' && !Number.isNaN(expiry) && expiry > 0) {
+		return expiry;
+	}
+	return ACCOUNTS_DEFAULT_LOGIN_EXPIRATION_DAYS;
+};
+
+export const getLoginExpirationInMs = (expiry?: number) => {
+	return convertFromDaysToMilliseconds(getLoginExpirationInDays(expiry));
+};

packages/tools/src/index.ts

@@ -4,3 +4,5 @@ export * from './pick';
 export * from './stream';
 export * from './timezone';
 export * from './wrapExceptions';
+export * from './getLoginExpiration';
+export * from './converter';

yarn.lock

@@ -8432,6 +8432,7 @@ __metadata:
     "@rocket.chat/models": "workspace:^"
     "@rocket.chat/rest-typings": "workspace:^"
     "@rocket.chat/string-helpers": ~0.31.25
+    "@rocket.chat/tools": "workspace:^"
     "@types/bcrypt": ^5.0.1
     "@types/gc-stats": ^1.4.2
     "@types/node": ^14.18.63