chore: remove dependency on meteor's oauth packages #35877
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 |
|
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:
|
![kodus-ai[bot]](https://avatars.githubusercontent.com/in/413034?s=60&v=4){kind=small}
| @@ -15,15 +15,15 @@ rocketchat:user-presence | |||
|
|
|||
There was a problem hiding this comment.
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.
| `?client_id=${config.clientId}` + | ||
| `&scope=${flatScope}` + | ||
| `&redirect_uri=${OAuth._redirectUri('github', config)}` + | ||
| `&state=${OAuth._stateParam(loginStyle, credentialToken, options.redirectUrl)}${allowSignup}`; |
There was a problem hiding this comment.
&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 accessredirectUrland prevent runtime errors.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| } catch (err) { | ||
| return []; | ||
| } |
There was a problem hiding this comment.
} catch (err) {
console.error('Failed to fetch GitHub emails:', err);
return [];
}The catch block in the getEmails function suppresses errors without logging, which makes debugging difficult.
This issue appears in multiple locations:
- apps/meteor/server/configuration/oauth/github.ts: Lines 70-72
- apps/meteor/server/configuration/oauth/github.ts: Lines 70-72
Please log errors in thecatchblock to aid debugging instead of silently returning an empty array.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| return _updateOAuthServices(); | ||
| }); | ||
|
|
||
| settings.watchByRegex(/^Accounts_OAuth_Custom-[a-z0-9_]+/, (key, value) => { | ||
| if (!value) { | ||
| return removeOAuthService(key); |
There was a problem hiding this comment.
settings.watchByRegex(/^Accounts_OAuth_.+/, async () => {
try {
return await _updateOAuthServices();
} catch (error) {
console.error('Error updating OAuth services:', error);
}
});
settings.watchByRegex(/^Accounts_OAuth_Custom-[a-z0-9_]+/, async (key, value) => {
if (!value) {
try {
return await removeOAuthService(key);
} catch (error) {
console.error(`Error removing OAuth service ${key}:`, error);
}
}
});The callback functions provided to settings.watchByRegex lack explicit error handling, which could lead to unhandled exceptions.
This issue appears in multiple locations:
- apps/meteor/server/configuration/oauth/index.ts: Lines 13-18
- apps/meteor/server/configuration/oauth/index.ts: Lines 13-18
Please wrap the calls within these callbacks intry...catchblocks to handle potential errors gracefully.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| }; | ||
|
|
||
| return registerOAuth1Service('twitter', urls, async (oauthBinding) => { | ||
| const response = await oauthBinding.getAsync('https://api.twitter.com/1.1/account/verify_credentials.json?include_email=true'); |
There was a problem hiding this comment.
try {
const response = await oauthBinding.getAsync('https://api.twitter.com/1.1/account/verify_credentials.json?include_email=true');
// Optional: Add checks for response status or structure before destructuring
if (!response || !response.data) {
// Or handle based on specific error structure if provided by getAsync
throw new Error('Failed to fetch user identity from Twitter or invalid response structure.');
}
const { data: identity } = response;
// ... (rest of the processing identity) ...
const fields = whitelistedFields.reduce(/* ... */);
const serviceData = { /* ... */ };
return {
serviceData,
options: {
profile: {
name: identity.name,
},
},
};
} catch (error) {
// Log the error and potentially re-throw or handle it as appropriate
// for the registerOAuth1Service callback context.
console.error('Error fetching Twitter user identity:', error);
throw new Error(`Failed to fetch user identity from Twitter. ${error instanceof Error ? error.message : String(error)}`);
}The API call to oauthBinding.getAsync lacks explicit error handling, which could lead to unhandled promise rejections or runtime errors.
This issue appears in multiple locations:
- apps/meteor/server/configuration/oauth/twitter.ts: Lines 24-52
- apps/meteor/server/configuration/oauth/twitter.ts: Lines 24-52
Please wrap the API call and subsequent processing in atry...catchblock to gracefully handle potential failures.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| settings.watchByRegex(/^Accounts_OAuth_.+/, () => { | ||
| return _updateOAuthServices(); | ||
| }); | ||
|
|
||
| settings.watchByRegex(/^Accounts_OAuth_Custom-[a-z0-9_]+/, (key, value) => { |
There was a problem hiding this comment.
settings.watchByRegex(/^Accounts_OAuth_(?!Custom-).+/, () => { // Exclude Custom OAuth
return _updateOAuthServices();
});
settings.watchByRegex(/^Accounts_OAuth_Custom-[a-z0-9_]+/, (key, value) => {The regular expression /^Accounts_OAuth_.+/ is too broad and overlaps with custom OAuth settings, potentially causing redundant operations.
This issue appears in multiple locations:
- apps/meteor/server/configuration/oauth/index.ts: Lines 12-16
- apps/meteor/server/configuration/oauth/index.ts: Lines 12-16
Please refine the regex to exclude custom settings using a negative lookahead (/^Accounts_OAuth_(?!Custom-).+/).
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| export const registerOAuth1Service = ( | ||
| serviceName: string, | ||
| urls: IOAuth1Urls, | ||
| handleOauthRequest: (binding: IOAuth1Binding, query?: Record<string, any>) => Promise<HandledOauthRequest>, |
There was a problem hiding this comment.
handleOauthRequest: (binding: IOAuth1Binding, query?: Record<string, unknown>) => Promise<HandledOauthRequest>,The query parameter in the handleOauthRequest function uses Record<string, any>, which bypasses type checking and can hide runtime errors.
This issue appears in multiple locations:
- apps/meteor/server/configuration/oauth/registerOAuth1Service.ts: Lines 7-7
- apps/meteor/server/configuration/oauth/registerOAuth1Service.ts: Lines 7-7
Please replaceanywithunknownor a more specific type for thequeryparameter to enhance type safety.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| [email protected] | ||
| [email protected] | ||
| # oauth1 is used only by twitter oauth |
There was a problem hiding this comment.
# oauth1 is used for OAuth 1.0 implementationsThe 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.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #35877 +/- ##
===========================================
+ Coverage 61.17% 61.21% +0.04%
===========================================
Files 3005 3009 +4
Lines 71381 71477 +96
Branches 16341 16343 +2
===========================================
+ Hits 43664 43753 +89
- Misses 24748 24754 +6
- Partials 2969 2970 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Proposed changes (including videos or screenshots)
We already overwrite the client code for all of OAuth providers from meteor packages. This PR is reorganizing it a bit and also bringing the server code from those package so that we may drop the dependencies to those packages.
Issue(s)
Steps to test or reproduce
Further comments
haven't tested anything yet.
Providers:
Extras, maybe: