🛜 ci: OpenID Strategy Test Async Handling (#5613)

api/strategies/openidStrategy.spec.js

@@ -1,70 +1,74 @@
+const fetch = require('node-fetch');
 const jwtDecode = require('jsonwebtoken/decode');
 const { Issuer, Strategy: OpenIDStrategy } = require('openid-client');
-const mongoose = require('mongoose');
+const { findUser, createUser, updateUser } = require('~/models/userMethods');
-const { MongoMemoryServer } = require('mongodb-memory-server');
-const User = require('~/models/User');
 const setupOpenId = require('./openidStrategy');
 
-jest.mock('jsonwebtoken/decode');
+// --- Mocks ---
+jest.mock('node-fetch');
 jest.mock('openid-client');
-
+jest.mock('jsonwebtoken/decode');
 jest.mock('~/server/services/Files/strategies', () => ({
   getStrategyFunctions: jest.fn(() => ({
-    saveBuffer: jest.fn(),
+    // You can modify this mock as needed (here returning a dummy function)
+    saveBuffer: jest.fn().mockResolvedValue('/fake/path/to/avatar.png'),
   })),
 }));
+jest.mock('~/models/userMethods', () => ({
+  findUser: jest.fn(),
+  createUser: jest.fn(),
+  updateUser: jest.fn(),
+}));
+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
+}));
+jest.mock('~/config', () => ({
+  logger: {
+    info: jest.fn(),
+    debug: jest.fn(),
+    error: jest.fn(),
+  },
+}));
 
+// Mock Issuer.discover so that setupOpenId gets a fake issuer and client
 Issuer.discover = jest.fn().mockResolvedValue({
-  Client: jest.fn(),
+  id_token_signing_alg_values_supported: ['RS256'],
+  Client: jest.fn().mockImplementation((clientMetadata) => {
+    return {
+      metadata: clientMetadata,
+    };
+  }),
+});
+
+// To capture the verify callback from the strategy, we grab it from the mock constructor
+let verifyCallback;
+OpenIDStrategy.mockImplementation((options, verify) => {
+  verifyCallback = verify;
+  return { name: 'openid', options, verify };
 });
 
 describe('setupOpenId', () => {
-  const OLD_ENV = process.env;
+  // Helper to wrap the verify callback in a promise
-  describe('OpenIDStrategy', () => {
+  const validate = (tokenset, userinfo) =>
-    let validateFn, mongoServer;
+    new Promise((resolve, reject) => {
-
+      verifyCallback(tokenset, userinfo, (err, user, details) => {
-    beforeAll(async () => {
+        if (err) {
-      mongoServer = await MongoMemoryServer.create();
+          reject(err);
-      const mongoUri = mongoServer.getUri();
+        } else {
-      await mongoose.connect(mongoUri);
+          resolve({ user, details });
+        }
       });
-
-    afterAll(async () => {
-      process.env = OLD_ENV;
-      await mongoose.disconnect();
-      await mongoServer.stop();
-    });
-
-    beforeEach(async () => {
-      jest.clearAllMocks();
-      await User.deleteMany({});
-      process.env = {
-        ...OLD_ENV,
-        OPENID_ISSUER: 'https://fake-issuer.com',
-        OPENID_CLIENT_ID: 'fake_client_id',
-        OPENID_CLIENT_SECRET: 'fake_client_secret',
-        DOMAIN_SERVER: 'https://example.com',
-        OPENID_CALLBACK_URL: '/callback',
-        OPENID_SCOPE: 'openid profile email',
-        OPENID_REQUIRED_ROLE: 'requiredRole',
-        OPENID_REQUIRED_ROLE_PARAMETER_PATH: 'roles',
-        OPENID_REQUIRED_ROLE_TOKEN_KIND: 'id',
-      };
-
-      jwtDecode.mockReturnValue({
-        roles: ['requiredRole'],
-      });
-
-      //call setup so we can grab a reference to the validate function
-      await setupOpenId();
-      validateFn = OpenIDStrategy.mock.calls[0][1];
     });
 
   const tokenset = {
     id_token: 'fake_id_token',
+    access_token: 'fake_access_token',
   };
 
-    const userinfo = {
+  const baseUserinfo = {
     sub: '1234',
     email: 'test@example.com',
     email_verified: true,
@@ -72,104 +76,226 @@ describe('setupOpenId', () => {
     family_name: 'Last',
     name: 'My Full',
     username: 'flast',
+    picture: 'https://example.com/avatar.png',
   };
 
-    const userModel = {
+  beforeEach(async () => {
+    // Clear previous mock calls and reset implementations
+    jest.clearAllMocks();
+
+    // Reset environment variables needed by the strategy
+    process.env.OPENID_ISSUER = 'https://fake-issuer.com';
+    process.env.OPENID_CLIENT_ID = 'fake_client_id';
+    process.env.OPENID_CLIENT_SECRET = 'fake_client_secret';
+    process.env.DOMAIN_SERVER = 'https://example.com';
+    process.env.OPENID_CALLBACK_URL = '/callback';
+    process.env.OPENID_SCOPE = 'openid profile email';
+    process.env.OPENID_REQUIRED_ROLE = 'requiredRole';
+    process.env.OPENID_REQUIRED_ROLE_PARAMETER_PATH = 'roles';
+    process.env.OPENID_REQUIRED_ROLE_TOKEN_KIND = 'id';
+    delete process.env.OPENID_USERNAME_CLAIM;
+    delete process.env.OPENID_NAME_CLAIM;
+    delete process.env.PROXY;
+
+    // Default jwtDecode mock returns a token that includes the required role.
+    jwtDecode.mockReturnValue({
+      roles: ['requiredRole'],
+    });
+
+    // By default, assume that no user is found, so createUser will be called
+    findUser.mockResolvedValue(null);
+    createUser.mockImplementation(async (userData) => {
+      // simulate created user with an _id property
+      return { _id: 'newUserId', ...userData };
+    });
+    updateUser.mockImplementation(async (id, userData) => {
+      return { _id: id, ...userData };
+    });
+
+    // For image download, simulate a successful response
+    const fakeBuffer = Buffer.from('fake image');
+    const fakeResponse = {
+      ok: true,
+      buffer: jest.fn().mockResolvedValue(fakeBuffer),
+    };
+    fetch.mockResolvedValue(fakeResponse);
+
+    // Finally, call the setup function so that passport.use gets called
+    await 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({
+        provider: 'openid',
         openidId: userinfo.sub,
+        username: userinfo.username,
         email: userinfo.email,
-    };
+        name: `${userinfo.given_name} ${userinfo.family_name}`,
+      }),
+      true,
+      true,
+    );
+  });
 
-    it('should set username correctly for a new user when username claim exists', async () => {
+  it('should use given_name as username when username claim is missing', async () => {
-      const expectUsername = userinfo.username.toLowerCase();
+    // Arrange – remove username from userinfo
-      await validateFn(tokenset, userinfo, (err, user) => {
+    const userinfo = { ...baseUserinfo };
-        expect(err).toBe(null);
+    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 }),
+      true,
+      true,
+    );
   });
 
-      await expect(User.exists({ username: expectUsername })).resolves.not.toBeNull();
+  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;
 
-    it('should set username correctly for a new user when given_name claim exists, but username does not', async () => {
+    // Act
-      let userinfo_modified = { ...userinfo };
+    const { user } = await validate(tokenset, userinfo);
-      delete userinfo_modified.username;
-      const expectUsername = userinfo.given_name.toLowerCase();
 
-      await validateFn(tokenset, userinfo_modified, (err, user) => {
+    // Assert
-        expect(err).toBe(null);
     expect(user.username).toBe(expectUsername);
-      });
+    expect(createUser).toHaveBeenCalledWith(
-      await expect(User.exists({ username: expectUsername })).resolves.not.toBeNull();
+      expect.objectContaining({ username: expectUsername }),
+      true,
+      true,
+    );
   });
 
-    it('should set username correctly for a new user when email claim exists, but username and given_name do not', async () => {
+  it('should override username with OPENID_USERNAME_CLAIM when set', async () => {
-      let userinfo_modified = { ...userinfo };
+    // Arrange – set OPENID_USERNAME_CLAIM so that the sub claim is used
-      delete userinfo_modified.username;
-      delete userinfo_modified.given_name;
-      const expectUsername = userinfo.email.toLowerCase();
-
-      await validateFn(tokenset, userinfo_modified, (err, user) => {
-        expect(err).toBe(null);
-        expect(user.username).toBe(expectUsername);
-      });
-      await expect(User.exists({ username: expectUsername })).resolves.not.toBeNull();
-    });
-
-    it('should set username correctly for a new user when using OPENID_USERNAME_CLAIM', async () => {
     process.env.OPENID_USERNAME_CLAIM = 'sub';
-      const expectUsername = userinfo.sub.toLowerCase();
+    const userinfo = { ...baseUserinfo };
 
-      await validateFn(tokenset, userinfo, (err, user) => {
+    // Act
-        expect(err).toBe(null);
+    const { user } = await validate(tokenset, userinfo);
-        expect(user.username).toBe(expectUsername);
+
-      });
+    // Assert – username should equal the sub (converted as-is)
-      await expect(User.exists({ username: expectUsername })).resolves.not.toBeNull();
+    expect(user.username).toBe(userinfo.sub);
+    expect(createUser).toHaveBeenCalledWith(
+      expect.objectContaining({ username: userinfo.sub }),
+      true,
+      true,
+    );
   });
 
-    it('should set name correctly for a new user with first and last names', async () => {
+  it('should set the full name correctly when given_name and family_name exist', async () => {
-      const expectName = userinfo.given_name + ' ' + userinfo.family_name;
+    // Arrange
-      await validateFn(tokenset, userinfo, (err, user) => {
+    const userinfo = { ...baseUserinfo };
-        expect(err).toBe(null);
+    const expectedFullName = `${userinfo.given_name} ${userinfo.family_name}`;
-        expect(user.name).toBe(expectName);
+
-      });
+    // Act
-      await expect(User.exists({ name: expectName })).resolves.not.toBeNull();
+    const { user } = await validate(tokenset, userinfo);
+
+    // Assert
+    expect(user.name).toBe(expectedFullName);
   });
 
-    it('should set name correctly for a new user using OPENID_NAME_CLAIM', async () => {
+  it('should override full name with OPENID_NAME_CLAIM when set', async () => {
-      const expectName = 'Custom Name';
+    // Arrange – use the name claim as the full name
     process.env.OPENID_NAME_CLAIM = 'name';
-      let userinfo_modified = { ...userinfo, name: expectName };
+    const userinfo = { ...baseUserinfo, name: 'Custom Name' };
 
-      await validateFn(tokenset, userinfo_modified, (err, user) => {
+    // Act
-        expect(err).toBe(null);
+    const { user } = await validate(tokenset, userinfo);
-        expect(user.name).toBe(expectName);
+
-      });
+    // Assert
-      await expect(User.exists({ name: expectName })).resolves.not.toBeNull();
+    expect(user.name).toBe('Custom Name');
   });
 
-    it('should should update existing user after login', async () => {
+  it('should update an existing user on login', async () => {
-      const expectUsername = userinfo.username.toLowerCase();
+    // Arrange – simulate that a user already exists
-      await User.create(userModel);
+    const existingUser = {
-
+      _id: 'existingUserId',
-      await validateFn(tokenset, userinfo, (err) => {
+      provider: 'local',
-        expect(err).toBe(null);
+      email: baseUserinfo.email,
-      });
+      openidId: '',
-      const newUser = await User.findOne({ openidId: userModel.openidId });
+      username: '',
-      await expect(newUser.provider).toBe('openid');
+      name: '',
-      await expect(newUser.username).toBe(expectUsername);
+    };
-      await expect(newUser.name).toBe(userinfo.given_name + ' ' + userinfo.family_name);
+    findUser.mockImplementation(async (query) => {
+      if (query.openidId === baseUserinfo.sub || query.email === baseUserinfo.email) {
+        return existingUser;
+      }
+      return null;
     });
 
-    it('should should enforce required role', async () => {
+    const userinfo = { ...baseUserinfo };
+
+    // Act
+    const { user } = await validate(tokenset, userinfo);
+
+    // Assert – updateUser should be called and the user object updated
+    expect(updateUser).toHaveBeenCalledWith(existingUser._id, expect.objectContaining({
+      provider: 'openid',
+      openidId: baseUserinfo.sub,
+      username: baseUserinfo.username,
+      name: `${baseUserinfo.given_name} ${baseUserinfo.family_name}`,
+    }));
+  });
+
+  it('should enforce the required role and reject login if missing', async () => {
+    // Arrange – simulate a token without the required role.
     jwtDecode.mockReturnValue({
       roles: ['SomeOtherRole'],
     });
-      await validateFn(tokenset, userinfo, (err, user, details) => {
+    const userinfo = { ...baseUserinfo };
-        expect(err).toBe(null);
+
+    // 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 "' + process.env.OPENID_REQUIRED_ROLE + '" role to log in.',
+      '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
+    const { user } = 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.
   });
 });