Skip to content

chore: remove dependency on meteor's oauth packages #35877

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.

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

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions apps/meteor/.meteor/packages
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,15 @@ rocketchat:user-presence

Copy link

Choose a reason for hiding this comment

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

kody code-review Documentation and Comments critical

The removal of accounts-github and accounts-twitter packages should be documented in the changelog or migration guide as it's a breaking change for users relying on these authentication methods.

This issue appears in multiple locations:

  • apps/meteor/.meteor/packages: Lines 15-15
  • apps/meteor/.meteor/packages: Lines 15-15
    Please document the removal of these packages in the changelog or migration guide to inform users of this breaking change.

Talk to Kody by mentioning @kody

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

[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]

[email protected]
[email protected]
# oauth1 is used only by twitter oauth
Copy link

Choose a reason for hiding this comment

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

kody code-review Documentation and Comments medium

# oauth1 is used for OAuth 1.0 implementations

The comment about oauth1 being used only by Twitter OAuth is misleading since accounts-twitter has been removed.

This issue appears in multiple locations:

  • apps/meteor/.meteor/packages: Lines 25-25
  • apps/meteor/.meteor/packages: Lines 25-25
    Please update the comment to reflect the general purpose of OAuth1 or remove it entirely.

Talk to Kody by mentioning @kody

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

[email protected]
[email protected]

[email protected]
Expand Down
19 changes: 19 additions & 0 deletions apps/meteor/client/lib/wrapLoginHandlerFn.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import type { Meteor } from 'meteor/meteor';

export type RequestCredentialOptions = Meteor.LoginWithExternalServiceOptions;
export type RequestCredentialCallback = (credentialTokenOrError?: string | Error) => void;

// Receives a function that accepts an options object and an optional callback
// Returns the same function but with the signature changed to also accept only the callback
// With this, you can make a function that accepts the arguments in any way that Meteor's login handlers may send them, without having to validate the params or mess with the signature types
export function wrapLoginHandlerFn(
loginHandlerFn: (options: RequestCredentialOptions, callback?: RequestCredentialCallback) => Promise<void> | void,
) {
return (options?: RequestCredentialOptions | RequestCredentialCallback, callback?: RequestCredentialCallback) => {
if (!callback && typeof options === 'function') {
return loginHandlerFn({}, options);
}

return loginHandlerFn(options as RequestCredentialOptions, callback);
};
}
19 changes: 4 additions & 15 deletions apps/meteor/client/lib/wrapRequestCredentialFn.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,9 @@
import type { OAuthConfiguration } from '@rocket.chat/core-typings';
import { Accounts } from 'meteor/accounts-base';
import type { Meteor } from 'meteor/meteor';
import { OAuth } from 'meteor/oauth';

import { loginServices } from './loginServices';

type RequestCredentialOptions = Meteor.LoginWithExternalServiceOptions;
type RequestCredentialCallback = (credentialTokenOrError?: string | Error) => void;
import { type RequestCredentialOptions, type RequestCredentialCallback, wrapLoginHandlerFn } from './wrapLoginHandlerFn';

type RequestCredentialConfig<T extends Partial<OAuthConfiguration>> = {
config: T;
Expand Down Expand Up @@ -38,15 +35,7 @@ export function wrapRequestCredentialFn<T extends Partial<OAuthConfiguration>>(
});
};

return (
options?: RequestCredentialOptions | RequestCredentialCallback,
credentialRequestCompleteCallback?: RequestCredentialCallback,
) => {
if (!credentialRequestCompleteCallback && typeof options === 'function') {
void wrapped({}, options);
return;
}

void wrapped(options as RequestCredentialOptions, credentialRequestCompleteCallback);
};
return wrapLoginHandlerFn((...args) => {
void wrapped(...args);
});
}
25 changes: 25 additions & 0 deletions apps/meteor/client/meteorOverrides/login/createOAuthLoginFn.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import { Accounts } from 'meteor/accounts-base';

import { createOAuthTotpLoginMethod } from './oauth';
import type { IOAuthProvider } from '../../definitions/IOAuthProvider';
import { overrideLoginMethod, type LoginCallback } from '../../lib/2fa/overrideLoginMethod';
import { wrapLoginHandlerFn } from '../../lib/wrapLoginHandlerFn';

export const createOAuthLoginFn = (provider: IOAuthProvider) => {
const loginHandler = wrapLoginHandlerFn((options, callback) => {
const credentialRequestCompleteCallback = Accounts.oauth.credentialRequestCompleteHandler(callback);
provider.requestCredential(options, credentialRequestCompleteCallback);
});

Accounts.oauth.registerService(provider.name);
Accounts.registerClientLoginFunction(provider.name, loginHandler);

const loginWithProvider = (options: Meteor.LoginWithExternalServiceOptions | undefined, cb: LoginCallback) =>
Accounts.applyLoginFunction(provider.name, [options, cb]);

const loginWithProviderAndTOTP = createOAuthTotpLoginMethod(provider);

return (options: Meteor.LoginWithExternalServiceOptions | undefined, callback?: LoginCallback) => {
overrideLoginMethod(loginWithProvider, [options], callback, loginWithProviderAndTOTP);
};
};
56 changes: 26 additions & 30 deletions apps/meteor/client/meteorOverrides/login/github.ts
Original file line number Diff line number Diff line change
@@ -1,42 +1,38 @@
import { Random } from '@rocket.chat/random';
import { Accounts } from 'meteor/accounts-base';
import { Github } from 'meteor/github-oauth';
import { Meteor } from 'meteor/meteor';
import { OAuth } from 'meteor/oauth';

import { createOAuthTotpLoginMethod } from './oauth';
import { overrideLoginMethod } from '../../lib/2fa/overrideLoginMethod';
import { createOAuthLoginFn } from './createOAuthLoginFn';
import { wrapRequestCredentialFn } from '../../lib/wrapRequestCredentialFn';

const { loginWithGithub } = Meteor;
const loginWithGithubAndTOTP = createOAuthTotpLoginMethod(Github);
Meteor.loginWithGithub = (options, callback) => {
overrideLoginMethod(loginWithGithub, [options], callback, loginWithGithubAndTOTP);
};
Meteor.loginWithGithub = createOAuthLoginFn({
name: 'github',

Github.requestCredential = wrapRequestCredentialFn('github', ({ config, loginStyle, options, credentialRequestCompleteCallback }) => {
const credentialToken = Random.secret();
const scope = options?.requestPermissions || ['user:email'];
const flatScope = scope.map(encodeURIComponent).join('+');
requestCredential: wrapRequestCredentialFn('github', ({ config, loginStyle, options, credentialRequestCompleteCallback }) => {
const credentialToken = Random.secret();
const scope = options?.requestPermissions || ['user:email'];
const flatScope = scope.map(encodeURIComponent).join('+');

let allowSignup = '';
if (Accounts._options?.forbidClientAccountCreation) {
allowSignup = '&allow_signup=false';
}
let allowSignup = '';
if (Accounts._options?.forbidClientAccountCreation) {
allowSignup = '&allow_signup=false';
}

const loginUrl =
`https://github.com/login/oauth/authorize` +
`?client_id=${config.clientId}` +
`&scope=${flatScope}` +
`&redirect_uri=${OAuth._redirectUri('github', config)}` +
`&state=${OAuth._stateParam(loginStyle, credentialToken, options.redirectUrl)}${allowSignup}`;
const loginUrl =
`https://github.com/login/oauth/authorize` +
`?client_id=${config.clientId}` +
`&scope=${flatScope}` +
`&redirect_uri=${OAuth._redirectUri('github', config)}` +
`&state=${OAuth._stateParam(loginStyle, credentialToken, options.redirectUrl)}${allowSignup}`;
Copy link

Choose a reason for hiding this comment

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

kody code-review Potential Issues high

&state=${OAuth._stateParam(loginStyle, credentialToken, options?.redirectUrl)}${allowSignup}`;

The code accesses options.redirectUrl directly without checking if options is null or undefined, which could lead to runtime errors.

This issue appears in multiple locations:

  • apps/meteor/client/meteorOverrides/login/github.ts: Lines 27-27
  • apps/meteor/client/meteorOverrides/login/github.ts: Lines 27-27
    Please use optional chaining (options?.redirectUrl) to safely access redirectUrl and prevent runtime errors.

Talk to Kody by mentioning @kody

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


OAuth.launchLogin({
loginService: 'github',
loginStyle,
loginUrl,
credentialRequestCompleteCallback,
credentialToken,
popupOptions: { width: 900, height: 450 },
});
OAuth.launchLogin({
loginService: 'github',
loginStyle,
loginUrl,
credentialRequestCompleteCallback,
credentialToken,
popupOptions: { width: 900, height: 450 },
});
}),
});
92 changes: 45 additions & 47 deletions apps/meteor/client/meteorOverrides/login/twitter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,55 +2,53 @@ import type { TwitterOAuthConfiguration } from '@rocket.chat/core-typings';
import { Random } from '@rocket.chat/random';
import { Meteor } from 'meteor/meteor';
import { OAuth } from 'meteor/oauth';
import { Twitter } from 'meteor/twitter-oauth';

import { createOAuthTotpLoginMethod } from './oauth';
import { overrideLoginMethod } from '../../lib/2fa/overrideLoginMethod';
import { createOAuthLoginFn } from './createOAuthLoginFn';
import { wrapRequestCredentialFn } from '../../lib/wrapRequestCredentialFn';

const { loginWithTwitter } = Meteor;
const loginWithTwitterAndTOTP = createOAuthTotpLoginMethod(Twitter);
Meteor.loginWithTwitter = (options, callback) => {
overrideLoginMethod(loginWithTwitter, [options], callback, loginWithTwitterAndTOTP);
};

Twitter.requestCredential = wrapRequestCredentialFn<TwitterOAuthConfiguration>(
'twitter',
({ loginStyle, options: requestOptions, credentialRequestCompleteCallback }) => {
const options = requestOptions as Record<string, string>;
const credentialToken = Random.secret();

let loginPath = `_oauth/twitter/?requestTokenAndRedirect=true&state=${OAuth._stateParam(
loginStyle,
credentialToken,
options?.redirectUrl,
)}`;

if (Meteor.isCordova) {
loginPath += '&cordova=true';
if (/Android/i.test(navigator.userAgent)) {
loginPath += '&android=true';
}
}

// Support additional, permitted parameters
if (options) {
const hasOwn = Object.prototype.hasOwnProperty;
Twitter.validParamsAuthenticate.forEach((param: string) => {
if (hasOwn.call(options, param)) {
loginPath += `&${param}=${encodeURIComponent(options[param])}`;
const validParamsAuthenticate = ['force_login', 'screen_name'];

Meteor.loginWithTwitter = createOAuthLoginFn({
name: 'twitter',

requestCredential: wrapRequestCredentialFn<TwitterOAuthConfiguration>(
'twitter',
({ loginStyle, options: requestOptions, credentialRequestCompleteCallback }) => {
const options = requestOptions as Record<string, string>;
const credentialToken = Random.secret();

let loginPath = `_oauth/twitter/?requestTokenAndRedirect=true&state=${OAuth._stateParam(
loginStyle,
credentialToken,
options?.redirectUrl,
)}`;

if (Meteor.isCordova) {
loginPath += '&cordova=true';
if (/Android/i.test(navigator.userAgent)) {
loginPath += '&android=true';
}
}

// Support additional, permitted parameters
if (options) {
const hasOwn = Object.prototype.hasOwnProperty;
validParamsAuthenticate.forEach((param: string) => {
if (hasOwn.call(options, param)) {
loginPath += `&${param}=${encodeURIComponent(options[param])}`;
}
});
}

const loginUrl = Meteor.absoluteUrl(loginPath);

OAuth.launchLogin({
loginService: 'twitter',
loginStyle,
loginUrl,
credentialRequestCompleteCallback,
credentialToken,
});
}

const loginUrl = Meteor.absoluteUrl(loginPath);

OAuth.launchLogin({
loginService: 'twitter',
loginStyle,
loginUrl,
credentialRequestCompleteCallback,
credentialToken,
});
},
);
},
),
});
7 changes: 7 additions & 0 deletions apps/meteor/definition/externals/meteor/accounts-base.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,13 @@ declare module 'meteor/accounts-base' {

function config(options: { clientStorage: 'session' | 'local' }): void;

function registerClientLoginFunction(
funcName: string,
func: (options?: RequestCredentialOptions | RequestCredentialCallback, callback?: RequestCredentialCallback) => void,
);

function applyLoginFunction(funcName: string, ...args: unknown[]): void;

class ConfigError extends Error {}

class LoginCancelledError extends Error {
Expand Down
65 changes: 65 additions & 0 deletions apps/meteor/definition/externals/meteor/oauth.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,44 @@ declare module 'meteor/oauth' {
| string;
}

interface IOAuth1BindingConstructor {
new (config, urls): IOAuth1Binding;
}

type HandledOauthRequest = { serviceData: Record<string, any>; options?: Record<string, any> } | null;

interface IOAuth1Binding {
accessToken: string | string[];
accessTokenSecret: string | string[];

prepareRequestToken(callbackUrl: string): void;
getAsync(
url: string,
params?: Record<string, any>,
callback?: unknown,
): Promise<{
content: string;
data: Record<string, any>;
headers: Record<string, string>;
redirected: boolean;
ok: boolean;
statusCode: number;
}>;
}

interface IOAuth1Urls {
requestToken: string | ((oauthBinding: IOAuth1Binding) => string);
authorize: string | ((oauthBinding: IOAuth1Binding) => string);
accessToken: string | ((oauthBinding: IOAuth1Binding) => string);
authenticate: string | ((oauthBinding: IOAuth1Binding, authParams: Record<string, any>) => string);
}

namespace OAuth {
function _retrievePendingCredential(key: string, ...args: string[]): Promise<string | Error | void>;
function openSecret(secret: string): string;
function sealSecret<T extends Record<string, any> | string | string[]>(
secret: T,
): T | { iv: string; ciphertext: string; algorithm: string; authTag: string };
function retrieveCredential(credentialToken: string, credentialSecret: string);
function _retrieveCredentialSecret(credentialToken: string): string | null;
const _pendingCredentials: Mongo.Collection<IOauthCredentials>;
Expand Down Expand Up @@ -42,5 +77,35 @@ declare module 'meteor/oauth' {
): string;

function _loginStyle(serviceName: string, config: { loginStyle?: string }, options?: Meteor.LoginWithExternalServiceOptions): string;

function registerService(
name: string,
version: 1,
urls: IOAuth1Urls,
handleOauthRequest: (binding: IOAuth1Binding, query?: Record<string, any>) => Promise<HandledOauthRequest>,
): void;
function registerService(
name: string,
version: 2,
urls: null,
handleOauthRequest: (query: Record<string, any>) => Promise<HandledOauthRequest>,
): void;
function registerService(
name: string,
version: 1 | 2,
urls: IOAuth1Urls | null,
handleOauthRequest:
| ((binding: IOAuth1Binding, query?: Record<string, any>) => Promise<HandledOauthRequest>)
| ((query: Record<string, any>) => Promise<HandledOauthRequest>),
): void;

function _queryParamsWithAuthTokenUrl(
authUrl: string,
oauthBinding: IOAuth1Binding,
params: Record<string, any>,
whitelistedQueryParams: string[],
): string;

function _fetch(url: string, method: 'GET' | 'POST', options: Record<string, any>): Promise<Response>;
}
}
Loading
Loading