Skip to content
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.

Sign up for GitHub

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

[WIP] 🔨 Maintenance: Bringing openid-client lib to latest version #544

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
+123 −64
Open

[WIP] 🔨 Maintenance: Bringing openid-client lib to latest version #544

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
76bfb7e
🔨 Maintenance: Bringing openid-client lib to latest version
lelemm Jan 8, 2025
dba9376
added md
lelemm Jan 8, 2025
a460732
ops
lelemm Jan 8, 2025
fd2d309
Merge branch 'master' into enhancement/openid-client-latest-version
lelemm Jan 24, 2025
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
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
"jws": "^4.0.0",
"migrate": "^2.0.1",
"nordigen-node": "^1.4.0",
"openid-client": "^5.4.2",
"openid-client": "^6.1.7",
"uuid": "^9.0.0",
"winston": "^3.14.2"
},
Expand Down
125 changes: 98 additions & 27 deletions src/accounts/openid.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,20 @@
import getAccountDb, { clearExpiredSessions } from '../account-db.js';
import * as uuid from 'uuid';
import { generators, Issuer } from 'openid-client';
import * as openIdClient from 'openid-client';
import finalConfig from '../load-config.js';
import { TOKEN_EXPIRATION_NEVER } from '../util/validate-user.js';
import {
getUserByUsername,
transferAllFilesFromUser,
} from '../services/user-service.js';
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;
}

export async function bootstrapOpenId(config) {
if (!('issuer' in config)) {
Expand Down Expand Up @@ -48,25 +56,87 @@ export async function bootstrapOpenId(config) {
}

async function setupOpenIdClient(config) {
let issuer =
typeof config.issuer === 'string'
? await Issuer.discover(config.issuer)
: new Issuer({
issuer: config.issuer.name,
authorization_endpoint: config.issuer.authorization_endpoint,
token_endpoint: config.issuer.token_endpoint,
userinfo_endpoint: config.issuer.userinfo_endpoint,
});
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,
);
}
Comment on lines +59 to +78
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.


const client = new issuer.Client({
client_id: config.client_id,
client_secret: config.client_secret,
redirect_uri: new URL(
'/openid/callback',
config.server_hostname,
).toString(),
validate_id_token: true,
});
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) {
const currentUrl = new URL(redirectUri);
currentUrl.searchParams.set('code', params.code);
const tokens = await openIdClient.authorizationCodeGrant(
discovered,
currentUrl,
{
pkceCodeVerifier: checks.code_verifier,
idTokenExpected: true,
},
);
return {
access_token: tokens.access_token,
expires_at: tokens.expires_at,
claims: () => tokens.claims?.(),
};
},
async grant(grantParams) {
const currentUrl = new URL(this.redirect_uris[0]);
currentUrl.searchParams.set('code', grantParams.code);
currentUrl.searchParams.set('state', grantParams.state);
const tokens = await openIdClient.authorizationCodeGrant(
discovered,
currentUrl,
{
pkceCodeVerifier: grantParams.code_verifier,
expectedState: grantParams.state,
idTokenExpected: false,
},
);
return {
access_token: tokens.access_token,
expires_at: tokens.expires_at,
claims: () => tokens.claims?.(),
};
},
async userinfo(accessToken, sub = '') {
if (!config.authMethod || config.authMethod === 'openid') {
return openIdClient.fetchUserInfo(discovered, accessToken, sub);
} else {
const response = await fetch('https://api.github.com/user', {
Copy link
Member

Choose a reason for hiding this comment

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

❓ question: ‏why are we connecting to GH api here? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol, that was a test. I need to change to the userinfo endpoint

headers: {
Authorization: `Bearer ${accessToken}`,
},
});
return response.json();
}
},
};

return client;
}
Expand Down Expand Up @@ -102,9 +172,11 @@ export async function loginWithOpenIdSetup(returnUrl) {
return { error: 'openid-setup-failed' };
}

const state = generators.state();
const code_verifier = generators.codeVerifier();
const code_challenge = generators.codeChallenge(code_verifier);
const state = openIdClient.randomState();
const code_verifier = openIdClient.randomPKCECodeVerifier();
const code_challenge = await openIdClient.calculatePKCECodeChallenge(
code_verifier,
);
Comment on lines +175 to +179
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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();


const now_time = Date.now();
const expiry_time = now_time + 300 * 1000;
Expand Down Expand Up @@ -171,7 +243,6 @@ export async function loginWithOpenIdFinalize(body) {

try {
let tokenSet = null;

if (!config.authMethod || config.authMethod === 'openid') {
const params = { code: body.code, state: body.state };
tokenSet = await client.callback(client.redirect_uris[0], params, {
Expand All @@ -184,9 +255,12 @@ export async function loginWithOpenIdFinalize(body) {
code: body.code,
redirect_uri: client.redirect_uris[0],
code_verifier,
state: body.state,
});
}

Comment on lines +258 to +261
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

const userInfo = await client.userinfo(tokenSet.access_token);

const identity =
userInfo.preferred_username ??
userInfo.login ??
Expand All @@ -207,7 +281,6 @@ export async function loginWithOpenIdFinalize(body) {
);
if (countUsersWithUserName === 0) {
userId = uuid.v4();
// Check if user was created by another transaction
const existingUser = accountDb.first(
'SELECT id FROM users WHERE user_name = ?',
[identity],
Expand Down Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
throw error;
} else {
console.error('An unexpected error occurred:', error);
return { error: 'unknown-error' };
}

}
}

const token = uuid.v4();

let expiration;
if (finalConfig.token_expiration === 'openid-provider') {
expiration = tokenSet.expires_at ?? TOKEN_EXPIRATION_NEVER;
Expand Down Expand Up @@ -314,7 +386,6 @@ export function isValidRedirectUrl(url) {
try {
const redirectUrl = new URL(url);
const serverUrl = new URL(serverHostname);

if (
redirectUrl.hostname === serverUrl.hostname ||
redirectUrl.hostname === 'localhost'
Expand Down
6 changes: 6 additions & 0 deletions upcoming-release-notes/544.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
category: Maintenance
authors: [lelemm]
---

Bring openid-client lib to latest version
54 changes: 18 additions & 36 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1561,7 +1561,7 @@ __metadata:
jws: "npm:^4.0.0"
migrate: "npm:^2.0.1"
nordigen-node: "npm:^1.4.0"
openid-client: "npm:^5.4.2"
openid-client: "npm:^6.1.7"
prettier: "npm:^2.8.3"
supertest: "npm:^6.3.1"
typescript: "npm:^4.9.5"
Expand Down Expand Up @@ -4255,10 +4255,10 @@ __metadata:
languageName: node
linkType: hard

"jose@npm:^4.15.5":
version: 4.15.9
resolution: "jose@npm:4.15.9"
checksum: 10c0/4ed4ddf4a029db04bd167f2215f65d7245e4dc5f36d7ac3c0126aab38d66309a9e692f52df88975d99429e357e5fd8bab340ff20baab544d17684dd1d940a0f4
"jose@npm:^5.9.6":
version: 5.9.6
resolution: "jose@npm:5.9.6"
checksum: 10c0/d6bcd8c7d655b5cda8e182952a76f0c093347f5476d74795405bb91563f7ab676f61540310dd4b1531c60d685335ceb600571a409551d2cbd2ab3e9f9fbf1e4d
languageName: node
linkType: hard

Expand Down Expand Up @@ -4475,15 +4475,6 @@ __metadata:
languageName: node
linkType: hard

"lru-cache@npm:^6.0.0":
version: 6.0.0
resolution: "lru-cache@npm:6.0.0"
dependencies:
yallist: "npm:^4.0.0"
checksum: 10c0/cb53e582785c48187d7a188d3379c181b5ca2a9c78d2bce3e7dee36f32761d1c42983da3fe12b55cb74e1779fa94cdc2e5367c028a9b35317184ede0c07a30a9
languageName: node
linkType: hard

"make-dir@npm:^3.1.0":
version: 3.1.0
resolution: "make-dir@npm:3.1.0"
Expand Down Expand Up @@ -4963,34 +4954,27 @@ __metadata:
languageName: node
linkType: hard

"oauth4webapi@npm:^3.1.4":
version: 3.1.4
resolution: "oauth4webapi@npm:3.1.4"
checksum: 10c0/81e471750f4903121efcef4edb1b73d725ae6d3b9646a0febd45e29ed05b62faba14a69e433181eae441913684a02d681c4e561dcac578de9cb45dd719f53464
languageName: node
linkType: hard

"object-assign@npm:^4, object-assign@npm:^4.1.1":
version: 4.1.1
resolution: "object-assign@npm:4.1.1"
checksum: 10c0/1f4df9945120325d041ccf7b86f31e8bcc14e73d29171e37a7903050e96b81323784ec59f93f102ec635bcf6fa8034ba3ea0a8c7e69fa202b87ae3b6cec5a414
languageName: node
linkType: hard

"object-hash@npm:^2.2.0":
version: 2.2.0
resolution: "object-hash@npm:2.2.0"
checksum: 10c0/1527de843926c5442ed61f8bdddfc7dc181b6497f725b0e89fcf50a55d9c803088763ed447cac85a5aa65345f1e99c2469ba679a54349ef3c4c0aeaa396a3eb9
languageName: node
linkType: hard

"object-inspect@npm:^1.13.1":
version: 1.13.1
resolution: "object-inspect@npm:1.13.1"
checksum: 10c0/fad603f408e345c82e946abdf4bfd774260a5ed3e5997a0b057c44153ac32c7271ff19e3a5ae39c858da683ba045ccac2f65245c12763ce4e8594f818f4a648d
languageName: node
linkType: hard

"oidc-token-hash@npm:^5.0.3":
version: 5.0.3
resolution: "oidc-token-hash@npm:5.0.3"
checksum: 10c0/d0dc0551406f09577874155cc83cf69c39e4b826293d50bb6c37936698aeca17d4bcee356ab910c859e53e83f2728a2acbd041020165191353b29de51fbca615
languageName: node
linkType: hard

"on-finished@npm:2.4.1":
version: 2.4.1
resolution: "on-finished@npm:2.4.1"
Expand Down Expand Up @@ -5034,15 +5018,13 @@ __metadata:
languageName: node
linkType: hard

"openid-client@npm:^5.4.2":
version: 5.6.5
resolution: "openid-client@npm:5.6.5"
"openid-client@npm:^6.1.7":
version: 6.1.7
resolution: "openid-client@npm:6.1.7"
dependencies:
jose: "npm:^4.15.5"
lru-cache: "npm:^6.0.0"
object-hash: "npm:^2.2.0"
oidc-token-hash: "npm:^5.0.3"
checksum: 10c0/4308dcd37a9ffb1efc2ede0bc556ae42ccc2569e71baa52a03ddfa44407bf403d4534286f6f571381c5eaa1845c609ed699a5eb0d350acfb8c3bacb72c2a6890
jose: "npm:^5.9.6"
oauth4webapi: "npm:^3.1.4"
checksum: 10c0/195d26897bf6eb95cfd5b9e91e8bcac46c56b0570713cef6f74b78f9d3a23df0869b2f26e7d78ae6d511e8f3b50f33298aa1bdc7bc32a46d41ca7d4ea10df460
languageName: node
linkType: hard

Expand Down
Loading