🚀 feat: Refactor customOpenIdData handling to use an object instead of Map and update return types

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<string>} customQuery - Either a full query string (if it contains operators)
    *   or an array of fields to select.
-   * @returns {Promise<Map<string, any>>} A promise that resolves to a map of custom fields.
+   * @returns {Promise<Record<string, unknown>>} 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<string>} customQuery - Fields to select from the Microsoft Graph API.
-   * @returns {Promise<Map<string, any>>} A promise that resolves to a map of custom fields.
+   * @returns {Promise<Record<string, unknown>>} 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
+      const result = await this.client.api('/me').select(fields).get();
-        .api('/me')
-        .select(fields)
-        .get();
 
-      const cleanedData = this.cleanData(result);
+      // Clean and return the data as a plain object
-      return new Map(Object.entries(cleanedData));
+      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 });
+      logger.error(`[MicrosoftDataMapper] Error fetching user data: ${error.message}`, {
-      return new Map();
+        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;
+module.exports = OpenIdDataMapper;

api/strategies/openidStrategy.js

@@ -25,7 +25,9 @@ try {
  * @returns {Promise<Buffer|string>}
  */
 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 (typeof input === 'string') {
-  if (Array.isArray(input)) {return input.join('_');}
+    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 (!decodedToken) {
-  if (!parameterPath) {return [];}
+    return [];
-  const roles = parameterPath.split('.').reduce((obj, key) => (obj?.[key] ?? null), decodedToken);
+  }
+  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<Object>} 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'))) {
-  if (!pictureUrl || (user.avatar && user.avatar.includes('manual=true'))) {return user;}
+    return user;
+  }
 
-  const fileName = crypto
+  const fileName = crypto ? (await hashToken(user.openidId)) + '.png' : `${user.openidId}.png`;
-    ? (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
+    const adminRoles = adminRolesEnv ? adminRolesEnv.split(',').map((role) => role.trim()) : [];
-      ? 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;
+module.exports = setupOpenId;

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;
-
+    const { user } = await validate(tokenset, userinfo);
-    // Act
-    await validate(tokenset, userinfo);
-
-    // Assert – fetch should not be called and avatar should remain undefined or empty
     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'] });
   });
 });