Skip to content

Commit

Permalink
Merge branch 'develop' into test/decouple-presence
Browse files Browse the repository at this point in the history
  • Loading branch information
kodiakhq[bot] authored Mar 28, 2024
2 parents 5d22683 + 466de64 commit fca7dfe
Show file tree
Hide file tree
Showing 12 changed files with 74 additions and 56 deletions.
7 changes: 7 additions & 0 deletions .changeset/fuzzy-cherries-buy.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"@rocket.chat/meteor": major
---

Api login should not suggest which credential is wrong (password/username)

Failed login attemps will always return `Unauthorized` instead of the internal fail reason
14 changes: 0 additions & 14 deletions apps/meteor/app/lib/server/lib/loginErrorMessageOverride.js

This file was deleted.

16 changes: 16 additions & 0 deletions apps/meteor/app/lib/server/lib/loginErrorMessageOverride.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// Do not disclose if user exists when password is invalid
import { Accounts } from 'meteor/accounts-base';
import { Meteor } from 'meteor/meteor';

const { _runLoginHandlers } = Accounts;

Accounts._options.ambiguousErrorMessages = true;
Accounts._runLoginHandlers = async function (methodInvocation, options) {
const result = await _runLoginHandlers.call(Accounts, methodInvocation, options);

if (result.error instanceof Meteor.Error) {
result.error = new Meteor.Error(401, 'User not found');
}

return result;
};
2 changes: 1 addition & 1 deletion apps/meteor/app/utils/rocketchat.info
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
{
"version": "6.7.0-develop"
"version": "7.0.0-develop"
}
10 changes: 0 additions & 10 deletions apps/meteor/client/meteorOverrides/login/google.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,6 @@ import { overrideLoginMethod, type LoginCallback } from '../../lib/2fa/overrideL
import { wrapRequestCredentialFn } from '../../lib/wrapRequestCredentialFn';
import { createOAuthTotpLoginMethod } from './oauth';

declare module 'meteor/accounts-base' {
// eslint-disable-next-line @typescript-eslint/no-namespace
namespace Accounts {
export const _options: {
restrictCreationByEmailDomain?: string | (() => string);
forbidClientAccountCreation?: boolean | undefined;
};
}
}

declare module 'meteor/meteor' {
// eslint-disable-next-line @typescript-eslint/no-namespace
namespace Meteor {
Expand Down
10 changes: 9 additions & 1 deletion apps/meteor/definition/externals/meteor/accounts-base.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ declare module 'meteor/accounts-base' {

function _insertLoginToken(userId: string, token: { token: string; when: Date }): void;

function _runLoginHandlers<T>(methodInvocation: T, loginRequest: Record<string, any>): LoginMethodResult | undefined;
function _runLoginHandlers<T>(methodInvocation: T, loginRequest: Record<string, any>): Promise<LoginMethodResult>;

function registerLoginHandler(name: string, handler: (options: any) => undefined | object): void;

Expand Down Expand Up @@ -54,6 +54,14 @@ declare module 'meteor/accounts-base' {

const _accountData: Record<string, any>;

interface AccountsServerOptions {
ambiguousErrorMessages?: boolean;
restrictCreationByEmailDomain?: string | (() => string);
forbidClientAccountCreation?: boolean | undefined;
}

export const _options: AccountsServerOptions;

// eslint-disable-next-line @typescript-eslint/no-namespace
namespace oauth {
function credentialRequestCompleteHandler(
Expand Down
2 changes: 1 addition & 1 deletion apps/meteor/package.json
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"name": "@rocket.chat/meteor",
"description": "The Ultimate Open Source WebChat Platform",
"version": "6.7.0-develop",
"version": "7.0.0-develop",
"private": true,
"author": {
"name": "Rocket.Chat",
Expand Down
11 changes: 10 additions & 1 deletion apps/meteor/tests/end-to-end/api/28-roles.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { expect } from 'chai';
import { before, describe, it } from 'mocha';
import { after, before, describe, it } from 'mocha';
import type { Response } from 'supertest';

import { getCredentials, api, request, credentials } from '../../data/api-data.js';
Expand Down Expand Up @@ -84,6 +84,15 @@ describe('[Roles]', function () {
});
});

after(async () => {
if (!isEnterprise) {
return;
}
await request.post(api('roles.delete')).set(credentials).send({
roleId: testRoleId,
});
});

it('should throw an error when not running EE to update a role', async function () {
// TODO this is not the right way to do it. We're doing this way for now just because we have separate CI jobs for EE and CE,
// ideally we should have a single CI job that adds a license and runs both CE and EE tests.
Expand Down
52 changes: 27 additions & 25 deletions apps/meteor/tests/end-to-end/api/31-failed-login-attempts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,25 +17,31 @@ describe('[Failed Login Attempts]', function () {

before((done) => getCredentials(done));

before(async () => {
await updateSetting('Block_Multiple_Failed_Logins_Enabled', true);
await updateSetting('Block_Multiple_Failed_Logins_By_Ip', true);
await updateSetting('Block_Multiple_Failed_Logins_By_User', true);
await updateSetting('Block_Multiple_Failed_Logins_Attempts_Until_Block_by_User', maxAttemptsByUser);
await updateSetting('Block_Multiple_Failed_Logins_Time_To_Unblock_By_User_In_Minutes', userBlockSeconds / 60);
await updateSetting('Block_Multiple_Failed_Logins_Attempts_Until_Block_By_Ip', maxAttemptsByIp);
await updateSetting('Block_Multiple_Failed_Logins_Time_To_Unblock_By_Ip_In_Minutes', ipBlockSeconds / 60);

await updatePermission('logout-other-user', ['admin']);
});

after(async () => {
await updateSetting('Block_Multiple_Failed_Logins_Attempts_Until_Block_by_User', 10);
await updateSetting('Block_Multiple_Failed_Logins_Time_To_Unblock_By_User_In_Minutes', 5);
await updateSetting('Block_Multiple_Failed_Logins_Attempts_Until_Block_By_Ip', 50);
await updateSetting('Block_Multiple_Failed_Logins_Time_To_Unblock_By_Ip_In_Minutes', 5);
await updateSetting('Block_Multiple_Failed_Logins_Enabled', false);
});
before(() =>
Promise.all([
updateSetting('Block_Multiple_Failed_Logins_Enabled', true),
updateSetting('Block_Multiple_Failed_Logins_By_Ip', true),
updateSetting('Block_Multiple_Failed_Logins_By_User', true),
updateSetting('Block_Multiple_Failed_Logins_Attempts_Until_Block_by_User', maxAttemptsByUser),
updateSetting('Block_Multiple_Failed_Logins_Time_To_Unblock_By_User_In_Minutes', userBlockSeconds / 60),
updateSetting('Block_Multiple_Failed_Logins_Attempts_Until_Block_By_Ip', maxAttemptsByIp),
updateSetting('Block_Multiple_Failed_Logins_Time_To_Unblock_By_Ip_In_Minutes', ipBlockSeconds / 60),
updatePermission('logout-other-user', ['admin']),
]),
);

after(() =>
Promise.all([
updateSetting('Block_Multiple_Failed_Logins_Attempts_Until_Block_by_User', 10),
updateSetting('Block_Multiple_Failed_Logins_Time_To_Unblock_By_User_In_Minutes', 5),
updateSetting('Block_Multiple_Failed_Logins_Attempts_Until_Block_By_Ip', 50),
updateSetting('Block_Multiple_Failed_Logins_Time_To_Unblock_By_Ip_In_Minutes', 5),
updateSetting('Block_Multiple_Failed_Logins_Enabled', true),
updateSetting('Block_Multiple_Failed_Logins_By_Ip', true),
updateSetting('Block_Multiple_Failed_Logins_By_User', true),
updatePermission('logout-other-user', ['admin']),
]),
);

async function shouldFailLoginWithUser(username: string, password: string) {
await request
Expand All @@ -48,7 +54,7 @@ describe('[Failed Login Attempts]', function () {
.expect(401)
.expect((res) => {
expect(res.body).to.have.property('status', 'error');
expect(res.body).to.have.property('message', 'Incorrect password');
expect(res.body).to.have.property('message', 'Unauthorized');
});
}

Expand Down Expand Up @@ -163,11 +169,7 @@ describe('[Failed Login Attempts]', function () {
userLogin = await createUser();
});

afterEach(async () => {
await deleteUser(user);
await deleteUser(user2);
await deleteUser(userLogin);
});
afterEach(() => Promise.all([deleteUser(user), deleteUser(user2), deleteUser(userLogin)]));

afterEach(async () => {
// reset counter
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "rocket.chat",
"version": "6.7.0-develop",
"version": "7.0.0-develop",
"description": "Rocket.Chat Monorepo",
"main": "index.js",
"private": true,
Expand Down
2 changes: 1 addition & 1 deletion packages/core-typings/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
"$schema": "https://json.schemastore.org/package",
"name": "@rocket.chat/core-typings",
"private": true,
"version": "6.7.0-develop",
"version": "7.0.0-develop",
"devDependencies": {
"@rocket.chat/eslint-config": "workspace:^",
"eslint": "~8.45.0",
Expand Down
2 changes: 1 addition & 1 deletion packages/rest-typings/package.json
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"name": "@rocket.chat/rest-typings",
"private": true,
"version": "6.7.0-develop",
"version": "7.0.0-develop",
"devDependencies": {
"@rocket.chat/eslint-config": "workspace:^",
"@types/jest": "~29.5.7",
Expand Down

0 comments on commit fca7dfe

Please sign in to comment.