diff --git a/.env.example b/.env.example index 2b6cd104ea..3ba5d211e4 100644 --- a/.env.example +++ b/.env.example @@ -420,11 +420,12 @@ OPENID_CLIENT_ID= OPENID_CLIENT_SECRET= OPENID_ISSUER= OPENID_SESSION_SECRET= +# OPENID_USE_PKCE= OPENID_SCOPE="openid profile email" OPENID_CALLBACK_URL=/oauth/openid/callback OPENID_REQUIRED_ROLE= # Set to 'userinfo' or 'token' to determine witch role source to use, Default is 'token' -# OPENID_REQUIRED_ROLE_SOURCE= +OPENID_REQUIRED_ROLE_SOURCE= OPENID_REQUIRED_ROLE_TOKEN_KIND= OPENID_REQUIRED_ROLE_PARAMETER_PATH= # Set to determine which user info property returned from OpenID Provider to store as the User's username diff --git a/api/strategies/openidStrategy.js b/api/strategies/openidStrategy.js index 7be3775a0e..27723cbad4 100644 --- a/api/strategies/openidStrategy.js +++ b/api/strategies/openidStrategy.js @@ -2,7 +2,12 @@ const fetch = require('node-fetch'); const passport = require('passport'); const jwtDecode = require('jsonwebtoken/decode'); const { HttpsProxyAgent } = require('https-proxy-agent'); -const { Issuer, Strategy: OpenIDStrategy, custom } = require('openid-client'); +const { + Issuer, + Strategy: OpenIDStrategy, + custom, + AuthorizationParameters, +} = require('openid-client'); const { getStrategyFunctions } = require('~/server/services/Files/strategies'); const { findUser, createUser, updateUser } = require('~/models/userMethods'); const { hashToken } = require('~/server/utils/crypto'); @@ -61,10 +66,6 @@ async function downloadImage(url, accessToken) { * * @function getFullName * @param {Object} userinfo - The user information object from OpenID Connect - * @param {string} [userinfo.given_name] - The user's first name - * @param {string} [userinfo.family_name] - The user's last name - * @param {string} [userinfo.username] - The user's username - * @param {string} [userinfo.email] - The user's email address * @returns {string} The determined full name of the user */ function getFullName(userinfo) { @@ -180,7 +181,7 @@ function getUserRoles(tokenSet, userinfo, rolePath, tokenKind, roleSource) { } /** - * Registers and configures the OpenID Connect strategy with Passport, enabling PKCE. + * Registers and configures the OpenID Connect strategy with Passport, enabling PKCE when toggled. * * @async * @function setupOpenId @@ -198,13 +199,7 @@ async function setupOpenId() { // Discover issuer configuration const issuer = await Issuer.discover(process.env.OPENID_ISSUER); logger.info(`[openidStrategy] Discovered issuer: ${issuer.issuer}`); - /* Supported Algorithms, openid-client v5 doesn't set it automatically as discovered from server. - - id_token_signed_response_alg // defaults to 'RS256' - - request_object_signing_alg // defaults to 'RS256' - - userinfo_signed_response_alg // not in v5 - - introspection_signed_response_alg // not in v5 - - authorization_signed_response_alg // not in v5 - */ + /** @type {import('openid-client').ClientMetadata} */ const clientMetadata = { client_id: process.env.OPENID_CLIENT_ID, @@ -220,13 +215,19 @@ async function setupOpenId() { const client = new issuer.Client(clientMetadata); - // If you want a refresh token, add offline_access to scope, e.g. 'openid profile email offline_access' + // Determine whether to enable PKCE + const usePKCE = process.env.OPENID_USE_PKCE === 'true'; + + // Set up authorization parameters. Include code_challenge_method if PKCE is enabled. const openidScope = process.env.OPENID_SCOPE || 'openid profile email'; + /** @type {import('openid-client').AuthorizationParameters} */ const params = { scope: openidScope, - code_challenge_method: 'S256', // PKCE response_type: 'code', }; + if (usePKCE) { + params.code_challenge_method = 'S256'; // Enable PKCE by specifying the code challenge method + } // Role-based config const requiredRole = process.env.OPENID_REQUIRED_ROLE; @@ -234,9 +235,13 @@ async function setupOpenId() { const tokenKind = process.env.OPENID_REQUIRED_ROLE_TOKEN_KIND || 'id'; // 'id'|'access' const roleSource = process.env.OPENID_REQUIRED_ROLE_SOURCE || 'token'; // 'token'|'userinfo' - // Create the Passport strategy + // Create the Passport strategy using the new type-correct instantiation and toggle for PKCE const openidStrategy = new OpenIDStrategy( - { client, params }, + { + client, + params, + usePKCE, + }, async (tokenSet, userinfo, done) => { try { logger.info(`[openidStrategy] Verifying login for sub=${userinfo.sub}`); diff --git a/api/strategies/openidStrategy.spec.js b/api/strategies/openidStrategy.spec.js index c10fee4c48..a2eeefd5a1 100644 --- a/api/strategies/openidStrategy.spec.js +++ b/api/strategies/openidStrategy.spec.js @@ -98,6 +98,7 @@ describe('setupOpenId', () => { delete process.env.OPENID_USERNAME_CLAIM; delete process.env.OPENID_NAME_CLAIM; delete process.env.PROXY; + delete process.env.OPENID_USE_PKCE; // By default, jwtDecode returns a token that includes the required role. jwtDecode.mockReturnValue({ @@ -393,4 +394,29 @@ describe('setupOpenId', () => { ); expect(user.avatar).toBe(existingUser.avatar); }); + + it('should pass usePKCE true and set code_challenge_method in params when OPENID_USE_PKCE is "true"', async () => { + process.env.OPENID_USE_PKCE = 'true'; + await setupOpenId(); + // Get the options from the last call of OpenIDStrategy + const callOptions = OpenIDStrategy.mock.calls[OpenIDStrategy.mock.calls.length - 1][0]; + expect(callOptions.usePKCE).toBe(true); + expect(callOptions.params.code_challenge_method).toBe('S256'); + }); + + it('should pass usePKCE false and not set code_challenge_method in params when OPENID_USE_PKCE is "false"', async () => { + process.env.OPENID_USE_PKCE = 'false'; + await setupOpenId(); + const callOptions = OpenIDStrategy.mock.calls[OpenIDStrategy.mock.calls.length - 1][0]; + expect(callOptions.usePKCE).toBe(false); + expect(callOptions.params.code_challenge_method).toBeUndefined(); + }); + + it('should default to usePKCE false when OPENID_USE_PKCE is not defined', async () => { + delete process.env.OPENID_USE_PKCE; + await setupOpenId(); + const callOptions = OpenIDStrategy.mock.calls[OpenIDStrategy.mock.calls.length - 1][0]; + expect(callOptions.usePKCE).toBe(false); + expect(callOptions.params.code_challenge_method).toBeUndefined(); + }); });