diff --git a/api/strategies/OpenId/openidDataMapper.js b/api/strategies/OpenId/openidDataMapper.js index f5200db90d..1598795f0b 100644 --- a/api/strategies/OpenId/openidDataMapper.js +++ b/api/strategies/OpenId/openidDataMapper.js @@ -1,7 +1,6 @@ const fetch = require('node-fetch'); const { HttpsProxyAgent } = require('https-proxy-agent'); const { logger } = require('~/config'); -const { URL } = require('url'); // Microsoft SDK const { Client: MicrosoftGraphClient } = require('@microsoft/microsoft-graph-client'); @@ -15,7 +14,7 @@ class BaseDataMapper { * @param {string} accessToken - The access token to authenticate the request. * @param {string|Array} customQuery - Either a full query string (if it contains operators) * or an array of fields to select. - * @returns {Promise>} A promise that resolves to a map of custom fields. + * @returns {Promise>} A promise that resolves to an object of custom fields. * @throws {Error} Throws an error if not implemented in the subclass. */ async mapCustomData(accessToken, customQuery) { @@ -83,7 +82,7 @@ class MicrosoftDataMapper extends BaseDataMapper { * * @param {string} accessToken - The access token to authenticate the request. * @param {string|Array} customQuery - Fields to select from the Microsoft Graph API. - * @returns {Promise>} A promise that resolves to a map of custom fields. + * @returns {Promise>} A promise that resolves to an object of custom fields. */ async mapCustomData(accessToken, customQuery) { try { @@ -91,7 +90,7 @@ class MicrosoftDataMapper extends BaseDataMapper { if (!customQuery) { logger.warn('[MicrosoftDataMapper] No customQuery provided.'); - return new Map(); + return {}; } // Convert customQuery to a comma-separated string if it's an array @@ -99,25 +98,24 @@ class MicrosoftDataMapper extends BaseDataMapper { if (!fields) { logger.warn('[MicrosoftDataMapper] No fields specified in customQuery.'); - return new Map(); + return {}; } - const result = await this.client - .api('/me') - .select(fields) - .get(); + const result = await this.client.api('/me').select(fields).get(); - const cleanedData = this.cleanData(result); - return new Map(Object.entries(cleanedData)); + // Clean and return the data as a plain object + return this.cleanData(result); } catch (error) { // Handle specific Microsoft Graph errors if needed - logger.error(`[MicrosoftDataMapper] Error fetching user data: ${error.message}`, { stack: error.stack }); - return new Map(); + logger.error(`[MicrosoftDataMapper] Error fetching user data: ${error.message}`, { + stack: error.stack, + }); + return {}; } } /** - * Recursively remove all keys starting with @odata. from an object and convert Maps. + * Recursively remove all keys starting with @odata. from an object or array. * * @param {object|Array} obj - The object or array to clean. * @returns {object|Array} - The cleaned object or array. @@ -164,4 +162,4 @@ class OpenIdDataMapper { } } -module.exports = OpenIdDataMapper; \ No newline at end of file +module.exports = OpenIdDataMapper; diff --git a/api/strategies/openidStrategy.js b/api/strategies/openidStrategy.js index 51505f7489..1f941e194a 100644 --- a/api/strategies/openidStrategy.js +++ b/api/strategies/openidStrategy.js @@ -25,7 +25,9 @@ try { * @returns {Promise} */ const downloadImage = async (url, accessToken) => { - if (!url) {return '';} + if (!url) { + return ''; + } const options = { method: 'GET', @@ -82,8 +84,12 @@ const getFullName = (userinfo) => { * @returns {string} The processed input as a string suitable for a username. */ const convertToUsername = (input, defaultValue = '') => { - if (typeof input === 'string') {return input;} - if (Array.isArray(input)) {return input.join('_');} + if (typeof input === 'string') { + return input; + } + if (Array.isArray(input)) { + return input.join('_'); + } return defaultValue; }; @@ -95,7 +101,9 @@ const convertToUsername = (input, defaultValue = '') => { const safeDecode = (token) => { try { const decoded = jwtDecode(token); - if (decoded && typeof decoded === 'object') {return decoded;} + if (decoded && typeof decoded === 'object') { + return decoded; + } logger.error('[openidStrategy] Decoded token is not an object.'); } catch (error) { logger.error('[openidStrategy] Error decoding token:', error); @@ -110,9 +118,13 @@ const safeDecode = (token) => { * @returns {string[]} */ const extractRolesFromToken = (decodedToken, parameterPath) => { - if (!decodedToken) {return [];} - if (!parameterPath) {return [];} - const roles = parameterPath.split('.').reduce((obj, key) => (obj?.[key] ?? null), decodedToken); + if (!decodedToken) { + return []; + } + if (!parameterPath) { + return []; + } + const roles = parameterPath.split('.').reduce((obj, key) => obj?.[key] ?? null, decodedToken); if (!Array.isArray(roles)) { logger.error('[openidStrategy] Roles extracted from token are not in array format.'); return []; @@ -128,12 +140,11 @@ const extractRolesFromToken = (decodedToken, parameterPath) => { * @returns {Promise} The updated user object. */ const updateUserAvatar = async (user, pictureUrl, accessToken) => { - // The type annotation /** @type {string | undefined} */ is maintained via the JSDoc above. - if (!pictureUrl || (user.avatar && user.avatar.includes('manual=true'))) {return user;} + if (!pictureUrl || (user.avatar && user.avatar.includes('manual=true'))) { + return user; + } - const fileName = crypto - ? (await hashToken(user.openidId)) + '.png' - : `${user.openidId}.png`; + const fileName = crypto ? (await hashToken(user.openidId)) + '.png' : `${user.openidId}.png`; const imageBuffer = await downloadImage(pictureUrl, accessToken); if (imageBuffer) { @@ -184,9 +195,7 @@ async function setupOpenId() { const requiredRoleParameterPath = process.env.OPENID_REQUIRED_ROLE_PARAMETER_PATH; const requiredRoleTokenKind = process.env.OPENID_REQUIRED_ROLE_TOKEN_KIND; const adminRolesEnv = process.env.OPENID_ADMIN_ROLE; - const adminRoles = adminRolesEnv - ? adminRolesEnv.split(',').map((role) => role.trim()) - : []; + const adminRoles = adminRolesEnv ? adminRolesEnv.split(',').map((role) => role.trim()) : []; const openidLogin = new OpenIDStrategy( { @@ -209,7 +218,8 @@ async function setupOpenId() { : convertToUsername(userinfo.username || userinfo.given_name || userinfo.email); // Use the token specified by configuration to extract roles. - const token = requiredRoleTokenKind === 'access' ? tokenset.access_token : tokenset.id_token; + const token = + requiredRoleTokenKind === 'access' ? tokenset.access_token : tokenset.id_token; const decodedToken = safeDecode(token); const tokenBasedRoles = extractRolesFromToken(decodedToken, requiredRoleParameterPath); @@ -223,10 +233,12 @@ async function setupOpenId() { // Determine system role. const isAdmin = tokenBasedRoles.some((role) => adminRoles.includes(role)); const assignedRole = isAdmin ? SystemRoles.ADMIN : SystemRoles.USER; - logger.debug(`[openidStrategy] Assigned system role: ${assignedRole} (isAdmin: ${isAdmin})`); + logger.debug( + `[openidStrategy] Assigned system role: ${assignedRole} (isAdmin: ${isAdmin})`, + ); // Map custom OpenID data if configured. - let customOpenIdData = new Map(); + let customOpenIdData = {}; if (process.env.OPENID_CUSTOM_DATA) { const dataMapper = OpenIdDataMapper.getMapper( process.env.OPENID_PROVIDER.toLowerCase(), @@ -236,7 +248,7 @@ async function setupOpenId() { process.env.OPENID_CUSTOM_DATA, ); if (tokenBasedRoles.length) { - customOpenIdData.set('roles', tokenBasedRoles); + customOpenIdData.roles = tokenBasedRoles; } else { logger.warn('[openidStrategy] tokenBasedRoles is missing or invalid.'); } @@ -301,4 +313,4 @@ async function setupOpenId() { } } -module.exports = setupOpenId; \ No newline at end of file +module.exports = setupOpenId; diff --git a/api/strategies/openidStrategy.spec.js b/api/strategies/openidStrategy.spec.js index cea7c5e4a6..3690a458a6 100644 --- a/api/strategies/openidStrategy.spec.js +++ b/api/strategies/openidStrategy.spec.js @@ -3,6 +3,7 @@ const jwtDecode = require('jsonwebtoken/decode'); const { Issuer, Strategy: OpenIDStrategy } = require('openid-client'); const { findUser, createUser, updateUser } = require('~/models/userMethods'); const setupOpenId = require('./openidStrategy'); +const OpenIdDataMapper = require('./OpenId/openidDataMapper'); // --- Mocks --- jest.mock('node-fetch'); @@ -10,7 +11,6 @@ jest.mock('openid-client'); jest.mock('jsonwebtoken/decode'); jest.mock('~/server/services/Files/strategies', () => ({ getStrategyFunctions: jest.fn(() => ({ - // You can modify this mock as needed (here returning a dummy function) saveBuffer: jest.fn().mockResolvedValue('/fake/path/to/avatar.png'), })), })); @@ -23,7 +23,7 @@ jest.mock('~/server/utils/crypto', () => ({ hashToken: jest.fn().mockResolvedValue('hashed-token'), })); jest.mock('~/server/utils', () => ({ - isEnabled: jest.fn(() => false), // default to false, override per test if needed + isEnabled: jest.fn(() => false), })); jest.mock('~/config', () => ({ logger: { @@ -43,7 +43,7 @@ Issuer.discover = jest.fn().mockResolvedValue({ }), }); -// To capture the verify callback from the strategy, we grab it from the mock constructor +// Capture the verify callback from the strategy via the mock constructor let verifyCallback; OpenIDStrategy.mockImplementation((options, verify) => { verifyCallback = verify; @@ -80,7 +80,6 @@ describe('setupOpenId', () => { }; beforeEach(async () => { - // Clear previous mock calls and reset implementations jest.clearAllMocks(); // Reset environment variables needed by the strategy @@ -96,6 +95,8 @@ describe('setupOpenId', () => { delete process.env.OPENID_USERNAME_CLAIM; delete process.env.OPENID_NAME_CLAIM; delete process.env.PROXY; + delete process.env.OPENID_CUSTOM_DATA; + delete process.env.OPENID_PROVIDER; // Default jwtDecode mock returns a token that includes the required role. jwtDecode.mockReturnValue({ @@ -125,13 +126,8 @@ describe('setupOpenId', () => { }); it('should create a new user with correct username when username claim exists', async () => { - // Arrange – our userinfo already has username 'flast' const userinfo = { ...baseUserinfo }; - - // Act const { user } = await validate(tokenset, userinfo); - - // Assert expect(user.username).toBe(userinfo.username); expect(createUser).toHaveBeenCalledWith( expect.objectContaining({ @@ -147,16 +143,10 @@ describe('setupOpenId', () => { }); it('should use given_name as username when username claim is missing', async () => { - // Arrange – remove username from userinfo const userinfo = { ...baseUserinfo }; delete userinfo.username; - // Expect the username to be the given name (unchanged case) const expectUsername = userinfo.given_name; - - // Act const { user } = await validate(tokenset, userinfo); - - // Assert expect(user.username).toBe(expectUsername); expect(createUser).toHaveBeenCalledWith( expect.objectContaining({ username: expectUsername }), @@ -166,16 +156,11 @@ describe('setupOpenId', () => { }); it('should use email as username when username and given_name are missing', async () => { - // Arrange – remove username and given_name const userinfo = { ...baseUserinfo }; delete userinfo.username; delete userinfo.given_name; const expectUsername = userinfo.email; - - // Act const { user } = await validate(tokenset, userinfo); - - // Assert expect(user.username).toBe(expectUsername); expect(createUser).toHaveBeenCalledWith( expect.objectContaining({ username: expectUsername }), @@ -185,14 +170,9 @@ describe('setupOpenId', () => { }); it('should override username with OPENID_USERNAME_CLAIM when set', async () => { - // Arrange – set OPENID_USERNAME_CLAIM so that the sub claim is used process.env.OPENID_USERNAME_CLAIM = 'sub'; const userinfo = { ...baseUserinfo }; - - // Act const { user } = await validate(tokenset, userinfo); - - // Assert – username should equal the sub (converted as-is) expect(user.username).toBe(userinfo.sub); expect(createUser).toHaveBeenCalledWith( expect.objectContaining({ username: userinfo.sub }), @@ -202,31 +182,20 @@ describe('setupOpenId', () => { }); it('should set the full name correctly when given_name and family_name exist', async () => { - // Arrange const userinfo = { ...baseUserinfo }; const expectedFullName = `${userinfo.given_name} ${userinfo.family_name}`; - - // Act const { user } = await validate(tokenset, userinfo); - - // Assert expect(user.name).toBe(expectedFullName); }); it('should override full name with OPENID_NAME_CLAIM when set', async () => { - // Arrange – use the name claim as the full name process.env.OPENID_NAME_CLAIM = 'name'; const userinfo = { ...baseUserinfo, name: 'Custom Name' }; - - // Act const { user } = await validate(tokenset, userinfo); - - // Assert expect(user.name).toBe('Custom Name'); }); it('should update an existing user on login', async () => { - // Arrange – simulate that a user already exists const existingUser = { _id: 'existingUserId', provider: 'local', @@ -241,13 +210,8 @@ describe('setupOpenId', () => { } return null; }); - const userinfo = { ...baseUserinfo }; - - // Act await validate(tokenset, userinfo); - - // Assert – updateUser should be called and the user object updated expect(updateUser).toHaveBeenCalledWith( existingUser._id, expect.objectContaining({ @@ -260,43 +224,41 @@ describe('setupOpenId', () => { }); it('should enforce the required role and reject login if missing', async () => { - // Arrange – simulate a token without the required role. jwtDecode.mockReturnValue({ roles: ['SomeOtherRole'], }); const userinfo = { ...baseUserinfo }; - - // Act const { user, details } = await validate(tokenset, userinfo); - - // Assert – verify that the strategy rejects login expect(user).toBe(false); expect(details.message).toBe('You must have the "requiredRole" role to log in.'); }); it('should attempt to download and save the avatar if picture is provided', async () => { - // Arrange – ensure userinfo contains a picture URL const userinfo = { ...baseUserinfo }; - - // Act const { user } = await validate(tokenset, userinfo); - - // Assert – verify that download was attempted and the avatar field was set via updateUser expect(fetch).toHaveBeenCalled(); - // Our mock getStrategyFunctions.saveBuffer returns '/fake/path/to/avatar.png' expect(user.avatar).toBe('/fake/path/to/avatar.png'); }); it('should not attempt to download avatar if picture is not provided', async () => { - // Arrange – remove picture const userinfo = { ...baseUserinfo }; delete userinfo.picture; - - // Act - await validate(tokenset, userinfo); - - // Assert – fetch should not be called and avatar should remain undefined or empty + const { user } = await validate(tokenset, userinfo); expect(fetch).not.toHaveBeenCalled(); - // Depending on your implementation, user.avatar may be undefined or an empty string. + expect(user.avatar).toBeFalsy(); + }); + + it('should map customOpenIdData as an object when OPENID_CUSTOM_DATA is set', async () => { + process.env.OPENID_CUSTOM_DATA = 'some,fields'; + process.env.OPENID_PROVIDER = 'microsoft'; + const fakeCustomData = { foo: 'bar' }; + const fakeDataMapper = { mapCustomData: jest.fn().mockResolvedValue(fakeCustomData) }; + OpenIdDataMapper.getMapper = jest.fn(() => fakeDataMapper); + + const userinfo = { ...baseUserinfo }; + const { user } = await validate(tokenset, userinfo); + expect(OpenIdDataMapper.getMapper).toHaveBeenCalledWith('microsoft'); + expect(fakeDataMapper.mapCustomData).toHaveBeenCalledWith(tokenset.access_token, 'some,fields'); + expect(user.customOpenIdData).toEqual({ ...fakeCustomData, roles: ['requiredRole'] }); }); });