🔨 Maintenance: Bringing openid-client lib to latest version #544
Conversation
actual-github-bot
bot
changed the title
lelemm
changed the title
WalkthroughThe pull request introduces a significant update to the OpenID client implementation in the project. The primary change involves upgrading the The modifications include changing the import strategy for the OpenID client library, restructuring the client setup and authentication methods, and updating the approach to handling OpenID Connect flows. The changes impact how the OpenID client is instantiated, how authorization URLs are generated, how token exchanges are performed, and how user information is retrieved. The refactoring introduces a more modular and encapsulated approach to managing OpenID authentication, with updates to method signatures and the addition of utility functions for URL validation and server hostname detection. Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/accounts/openid.js (1)
10-17: Unnecessary imports and global assignments in Node.js 18+Since your
package.jsonspecifies Node.js version ">=18.0.0", thefetchAPI and thewebcryptomodule are globally available. Therefore, importingwebcryptofrom'node:crypto'andfetchfrom'node-fetch'is unnecessary. Additionally, the assignment ofwebcryptotoglobalThis.cryptois redundant.Apply this diff to remove the unnecessary imports and assignments:
- import { webcrypto } from 'node:crypto'; - import fetch from 'node-fetch'; /* eslint-disable-next-line no-undef */ - if (!globalThis.crypto) { - /* eslint-disable-next-line no-undef */ - globalThis.crypto = webcrypto; - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
upcoming-release-notes/544.mdis excluded by!**/*.mdyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
package.json(1 hunks)src/accounts/openid.js(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Docker image (alpine)
🔇 Additional comments (4)
src/accounts/openid.js (3)
Line range hint
263-268: Potential issue with user identificationThe logic for determining the
identitymay fail if none of the user information fields are present. This could lead to authentication failures.Please verify that the
userinforesponse will always contain at least one of the following fields:preferred_username,login,id, orname. If not, consider adding a default or handling this scenario appropriately.
127-138: Incorrect handling of GitHub user infoIn the
userinfomethod, the condition checks forconfig.authMethodto determine whether to fetch user info from GitHub. Ensure thatconfig.authMethodis set correctly and that this logic is intentional.Please verify that
config.authMethodcan only be'openid'or a value indicating GitHub authentication. If there are other authentication methods, the condition might skip necessary steps.
80-139: 🛠️ Refactor suggestionRedundant custom client methods
The
clientobject manually recreates methods (authorizationUrl,callback,grant,userinfo) that are already provided by theopenid-clientlibrary'sClientinstance. This can lead to maintenance challenges and potential bugs.Consider using the
Clientinstance's built-in methods instead of defining custom ones. Here's how you can refactor the code:- const client = { - redirect_uris: [ - new URL('/openid/callback', config.server_hostname).toString(), - ], - authorizationUrl(params) { - const urlObj = openIdClient.buildAuthorizationUrl(discovered, { - ...params, - redirect_uri: this.redirect_uris[0], - }); - return urlObj.href; - }, - async callback(redirectUri, params, checks) { - // Custom implementation - }, - async grant(grantParams) { - // Custom implementation - }, - async userinfo(accessToken, sub = '') { - // Custom implementation - }, - }; - return client; + // With the Client instance returned from setupOpenIdClient, use its methods directly. + return client;This refactor leverages the
openid-clientlibrary's tested and maintained methods, reducing custom code and potential errors.Likely invalid or redundant comment.
package.json (1)
41-41:⚠️ Potential issueEnsure compatibility with
openid-clientversion 6Upgrading
openid-clientfrom^5.4.2to^6.1.7introduces breaking changes. Version 6 includes significant updates and refactoring that may affect your codebase.Please consult the openid-client migration guide to ensure all changes are appropriately addressed. Verify that all usages of
openid-clientmethods in your code are updated to match the new API.
| @@ -256,12 +329,11 @@ export async function loginWithOpenIdFinalize(body) { | |||
| } else if (error.message === 'openid-grant-failed') { | |||
| return { error: 'openid-grant-failed' }; | |||
| } else { | |||
| throw error; // Re-throw other unexpected errors | |||
| throw error; | |||
There was a problem hiding this comment.
Unhandled exception may cause server crash
In the catch block, an error is re-thrown without proper handling. This could cause the server to crash if the exception is not caught elsewhere.
Consider handling the error gracefully and returning an appropriate response to the client.
} else {
- throw error;
+ console.error('An unexpected error occurred:', error);
+ return { error: 'unknown-error' };
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| throw error; | |
| } else { | |
| console.error('An unexpected error occurred:', error); | |
| return { error: 'unknown-error' }; | |
| } |
| const state = openIdClient.randomState(); | ||
| const code_verifier = openIdClient.randomPKCECodeVerifier(); | ||
| const code_challenge = await openIdClient.calculatePKCECodeChallenge( | ||
| code_verifier, | ||
| ); |
There was a problem hiding this comment.
Incorrect usage of PKCE generation methods
The methods openIdClient.randomState(), openIdClient.randomPKCECodeVerifier(), and openIdClient.calculatePKCECodeChallenge() are not part of the openid-client library's public API in version 6.x. Instead, PKCE and state generation are handled using the generators module.
Update the code to use the generators provided by the library:
+ const { generators } = openIdClient;
+ const code_verifier = generators.codeVerifier();
+ const code_challenge = generators.codeChallenge(code_verifier);
+ const state = generators.state();
- const state = openIdClient.randomState();
- const code_verifier = openIdClient.randomPKCECodeVerifier();
- const code_challenge = await openIdClient.calculatePKCECodeChallenge(
- code_verifier,
- );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const state = openIdClient.randomState(); | |
| const code_verifier = openIdClient.randomPKCECodeVerifier(); | |
| const code_challenge = await openIdClient.calculatePKCECodeChallenge( | |
| code_verifier, | |
| ); | |
| const { generators } = openIdClient; | |
| const code_verifier = generators.codeVerifier(); | |
| const code_challenge = generators.codeChallenge(code_verifier); | |
| const state = generators.state(); |
| state: body.state, | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
Incorrect parameters in token exchange
When using the client.grant() method with the authorization_code grant type, the redirect_uri must be included in the parameters. Additionally, the state parameter is not typically included in this context.
Adjust the parameters as follows:
tokenSet = await client.grant({
grant_type: 'authorization_code',
code: body.code,
+ redirect_uri: client.metadata.redirect_uris[0],
code_verifier,
- state: body.state,
});Ensure that the redirect_uri matches one of the registered URIs and that unnecessary parameters are removed.
Committable suggestion skipped: line range outside the PR's diff.
| let discovered; | ||
| if (typeof config.issuer === 'string') { | ||
| discovered = await openIdClient.discovery( | ||
| new URL(config.issuer), | ||
| config.client_id, | ||
| config.client_secret, | ||
| ); | ||
| } else { | ||
| const serverMetadata = { | ||
| issuer: config.issuer.name, | ||
| authorization_endpoint: config.issuer.authorization_endpoint, | ||
| token_endpoint: config.issuer.token_endpoint, | ||
| userinfo_endpoint: config.issuer.userinfo_endpoint, | ||
| }; | ||
| discovered = new openIdClient.Configuration( | ||
| serverMetadata, | ||
| config.client_id, | ||
| config.client_secret, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Potential misuse of openid-client methods
The methods openIdClient.discovery and new openIdClient.Configuration(...) do not align with the openid-client library's documented usage in version 6.x. Typically, you should use openIdClient.Issuer.discover() to discover the OpenID Provider's configuration and create a Client instance accordingly.
Consider revising the code to correctly utilize the openid-client library methods. Here's how you might adjust the implementation:
async function setupOpenIdClient(config) {
- let discovered;
- if (typeof config.issuer === 'string') {
- discovered = await openIdClient.discovery(
- new URL(config.issuer),
- config.client_id,
- config.client_secret,
- );
- } else {
- const serverMetadata = {
- issuer: config.issuer.name,
- authorization_endpoint: config.issuer.authorization_endpoint,
- token_endpoint: config.issuer.token_endpoint,
- userinfo_endpoint: config.issuer.userinfo_endpoint,
- };
- discovered = new openIdClient.Configuration(
- serverMetadata,
- config.client_id,
- config.client_secret,
- );
- }
+ let issuer;
+ if (typeof config.issuer === 'string') {
+ issuer = await openIdClient.Issuer.discover(config.issuer);
+ } else {
+ issuer = new openIdClient.Issuer({
+ issuer: config.issuer.name,
+ authorization_endpoint: config.issuer.authorization_endpoint,
+ token_endpoint: config.issuer.token_endpoint,
+ userinfo_endpoint: config.issuer.userinfo_endpoint,
+ });
+ }
+ const client = new issuer.Client({
+ client_id: config.client_id,
+ client_secret: config.client_secret,
+ redirect_uris: [
+ new URL('/openid/callback', config.server_hostname).toString(),
+ ],
+ response_types: ['code'],
+ });
// Replace the custom client object with the one provided by openid-client
+ return client;
}Committable suggestion skipped: line range outside the PR's diff.
| if (!config.authMethod || config.authMethod === 'openid') { | ||
| return openIdClient.fetchUserInfo(discovered, accessToken, sub); | ||
| } else { | ||
| const response = await fetch('https://api.github.com/user', { |
There was a problem hiding this comment.
❓ question: why are we connecting to GH api here? 🤔
I MAY ( 🤭 ) have forgotten to update the library since ever. This brings it to the latest version of the library