🔐 feat: Enhance OpenID User Info Handling (#4561)

* oidc-changes Initial attempt at testing openidStrategy and adding OPENID_USERNAME_CLAIM setting

* oidc-changes Add OPENID_NAME_CLAIM

* oidc-changes cleanup oidc test code

* oidc-changes using mongo memory server for test

* oidc-changes Change tests to expect username all lowercase

* oidc-changes Add more tests

* chore: linting

* refactor: Simplify OpenID full name retrieval logic

* refactor: Simplify OpenID user info retrieval logic

* refactor: move helper to openidStrategy.js

---------

Co-authored-by: alihacks <alihacks@pm.me>

.env.example

@@ -399,6 +399,10 @@ OPENID_CALLBACK_URL=/oauth/openid/callback
 OPENID_REQUIRED_ROLE=
 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
+OPENID_USERNAME_CLAIM=
+# Set to determine which user info property returned from OpenID Provider to store as the User's name
+OPENID_NAME_CLAIM=
 
 OPENID_BUTTON_LABEL=
 OPENID_IMAGE_URL=

api/strategies/openidStrategy.js

@@ -14,6 +14,7 @@ try {
 } catch (err) {
   logger.error('[openidStrategy] crypto support is disabled!', err);
 }
+
 /**
  * Downloads an image from a URL using an access token.
  * @param {string} url
@@ -53,6 +54,36 @@ const downloadImage = async (url, accessToken) => {
   }
 };
 
+/**
+ * Determines the full name of a user based on OpenID userinfo and environment configuration.
+ *
+ * @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) {
+  if (process.env.OPENID_NAME_CLAIM) {
+    return userinfo[process.env.OPENID_NAME_CLAIM];
+  }
+
+  if (userinfo.given_name && userinfo.family_name) {
+    return `${userinfo.given_name} ${userinfo.family_name}`;
+  }
+
+  if (userinfo.given_name) {
+    return userinfo.given_name;
+  }
+
+  if (userinfo.family_name) {
+    return userinfo.family_name;
+  }
+
+  return userinfo.username || userinfo.email;
+}
+
 /**
  * Converts an input into a string suitable for a username.
  * If the input is a string, it will be returned as is.
@@ -117,16 +148,7 @@ async function setupOpenId() {
             );
           }
 
-          let fullName = '';
-          if (userinfo.given_name && userinfo.family_name) {
-            fullName = userinfo.given_name + ' ' + userinfo.family_name;
-          } else if (userinfo.given_name) {
-            fullName = userinfo.given_name;
-          } else if (userinfo.family_name) {
-            fullName = userinfo.family_name;
-          } else {
-            fullName = userinfo.username || userinfo.email;
-          }
+          const fullName = getFullName(userinfo);
 
           if (requiredRole) {
             let decodedToken = '';
@@ -158,9 +180,14 @@ async function setupOpenId() {
             }
           }
 
-          const username = convertToUsername(
+          let username = '';
+          if (process.env.OPENID_USERNAME_CLAIM) {
+            username = userinfo[process.env.OPENID_USERNAME_CLAIM];
+          } else {
+            username = convertToUsername(
               userinfo.username || userinfo.given_name || userinfo.email,
             );
+          }
 
           if (!user) {
             user = {

api/strategies/openidStrategy.spec.js

@@ -0,0 +1,175 @@
+const jwtDecode = require('jsonwebtoken/decode');
+const { Issuer, Strategy: OpenIDStrategy } = require('openid-client');
+const mongoose = require('mongoose');
+const { MongoMemoryServer } = require('mongodb-memory-server');
+const User = require('~/models/User');
+const setupOpenId = require('./openidStrategy');
+
+jest.mock('jsonwebtoken/decode');
+jest.mock('openid-client');
+
+jest.mock('~/server/services/Files/strategies', () => ({
+  getStrategyFunctions: jest.fn(() => ({
+    saveBuffer: jest.fn(),
+  })),
+}));
+
+Issuer.discover = jest.fn().mockResolvedValue({
+  Client: jest.fn(),
+});
+
+describe('setupOpenId', () => {
+  const OLD_ENV = process.env;
+  describe('OpenIDStrategy', () => {
+    let validateFn, mongoServer;
+
+    beforeAll(async () => {
+      mongoServer = await MongoMemoryServer.create();
+      const mongoUri = mongoServer.getUri();
+      await mongoose.connect(mongoUri);
+    });
+
+    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',
+    };
+
+    const userinfo = {
+      sub: '1234',
+      email: 'test@example.com',
+      email_verified: true,
+      given_name: 'First',
+      family_name: 'Last',
+      name: 'My Full',
+      username: 'flast',
+    };
+
+    const userModel = {
+      openidId: userinfo.sub,
+      email: userinfo.email,
+    };
+
+    it('should set username correctly for a new user when username claim exists', async () => {
+      const expectUsername = userinfo.username.toLowerCase();
+      await validateFn(tokenset, userinfo, (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 given_name claim exists, but username does not', async () => {
+      let userinfo_modified = { ...userinfo };
+      delete userinfo_modified.username;
+      const expectUsername = userinfo.given_name.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 email claim exists, but username and given_name do not', async () => {
+      let userinfo_modified = { ...userinfo };
+      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();
+
+      await validateFn(tokenset, userinfo, (err, user) => {
+        expect(err).toBe(null);
+        expect(user.username).toBe(expectUsername);
+      });
+      await expect(User.exists({ username: expectUsername })).resolves.not.toBeNull();
+    });
+
+    it('should set name correctly for a new user with first and last names', async () => {
+      const expectName = userinfo.given_name + ' ' + userinfo.family_name;
+      await validateFn(tokenset, userinfo, (err, user) => {
+        expect(err).toBe(null);
+        expect(user.name).toBe(expectName);
+      });
+      await expect(User.exists({ name: expectName })).resolves.not.toBeNull();
+    });
+
+    it('should set name correctly for a new user using OPENID_NAME_CLAIM', async () => {
+      const expectName = 'Custom Name';
+      process.env.OPENID_NAME_CLAIM = 'name';
+      let userinfo_modified = { ...userinfo, name: expectName };
+
+      await validateFn(tokenset, userinfo_modified, (err, user) => {
+        expect(err).toBe(null);
+        expect(user.name).toBe(expectName);
+      });
+      await expect(User.exists({ name: expectName })).resolves.not.toBeNull();
+    });
+
+    it('should should update existing user after login', async () => {
+      const expectUsername = userinfo.username.toLowerCase();
+      await User.create(userModel);
+
+      await validateFn(tokenset, userinfo, (err) => {
+        expect(err).toBe(null);
+      });
+      const newUser = await User.findOne({ openidId: userModel.openidId });
+      await expect(newUser.provider).toBe('openid');
+      await expect(newUser.username).toBe(expectUsername);
+      await expect(newUser.name).toBe(userinfo.given_name + ' ' + userinfo.family_name);
+    });
+
+    it('should should enforce required role', async () => {
+      jwtDecode.mockReturnValue({
+        roles: ['SomeOtherRole'],
+      });
+      await validateFn(tokenset, userinfo, (err, user, details) => {
+        expect(err).toBe(null);
+        expect(user).toBe(false);
+        expect(details.message).toBe(
+          'You must have the "' + process.env.OPENID_REQUIRED_ROLE + '" role to log in.',
+        );
+      });
+    });
+  });
+});