diff --git a/.changeset/empty-readers-teach.md b/.changeset/empty-readers-teach.md new file mode 100644 index 00000000000..b4bd075ef65 --- /dev/null +++ b/.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. diff --git a/apps/meteor/app/api/server/v1/users.ts b/apps/meteor/app/api/server/v1/users.ts index c26957fa199..041ef0410df 100644 --- a/apps/meteor/app/api/server/v1/users.ts +++ b/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('Accounts_LoginExpiration') * 1000)) || undefined; + const loginExp = settings.get('Accounts_LoginExpiration'); + + const tokenExpires = (token && 'when' in token && new Date(token.when.getTime() + getLoginExpirationInMs(loginExp))) || undefined; return API.v1.success({ token: xAuthToken, diff --git a/apps/meteor/app/authentication/server/startup/index.js b/apps/meteor/app/authentication/server/startup/index.js index bffbe1f9876..2e4c599ce55 100644 --- a/apps/meteor/app/authentication/server/startup/index.js +++ b/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'); diff --git a/ee/apps/account-service/Dockerfile b/ee/apps/account-service/Dockerfile index c662d876530..acbc5b0371d 100644 --- a/ee/apps/account-service/Dockerfile +++ b/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 . diff --git a/ee/apps/account-service/package.json b/ee/apps/account-service/package.json index c41932159e0..194a3e1760e 100644 --- a/ee/apps/account-service/package.json +++ b/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", diff --git a/ee/apps/account-service/src/Account.ts b/ee/apps/account-service/src/Account.ts index 4521574c12c..c71376d301f 100644 --- a/ee/apps/account-service/src/Account.ts +++ b/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 { 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); } } diff --git a/ee/apps/account-service/src/lib/utils.ts b/ee/apps/account-service/src/lib/utils.ts index 1d4a3a6ad58..f52f323606d 100644 --- a/ee/apps/account-service/src/lib/utils.ts +++ b/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)); diff --git a/packages/tools/jest.config.ts b/packages/tools/jest.config.ts new file mode 100644 index 00000000000..959a31a7c6b --- /dev/null +++ b/packages/tools/jest.config.ts @@ -0,0 +1,3 @@ +export default { + preset: 'ts-jest', +}; diff --git a/packages/tools/package.json b/packages/tools/package.json index e6bbae9b5be..ac79955314b 100644 --- a/packages/tools/package.json +++ b/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", diff --git a/packages/tools/src/converter.spec.ts b/packages/tools/src/converter.spec.ts new file mode 100644 index 00000000000..5a27b6a97b9 --- /dev/null +++ b/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(); + }); +}); diff --git a/packages/tools/src/converter.ts b/packages/tools/src/converter.ts new file mode 100644 index 00000000000..e71c264857d --- /dev/null +++ b/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; +}; diff --git a/packages/tools/src/getLoginExpiration.spec.ts b/packages/tools/src/getLoginExpiration.spec.ts new file mode 100644 index 00000000000..edd652172a5 --- /dev/null +++ b/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); + }); +}); diff --git a/packages/tools/src/getLoginExpiration.ts b/packages/tools/src/getLoginExpiration.ts new file mode 100644 index 00000000000..de21fdce21a --- /dev/null +++ b/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)); +}; diff --git a/packages/tools/src/index.ts b/packages/tools/src/index.ts index b1b53ab71a9..96faa4d5596 100644 --- a/packages/tools/src/index.ts +++ b/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'; diff --git a/yarn.lock b/yarn.lock index 3b68dcde380..add9a5e1946 100644 --- a/yarn.lock +++ b/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