From a1647d76e0af9480aec43c4cb645300c23f2afc4 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sun, 27 Oct 2024 11:41:48 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=90=20feat:=20Enhance=20OpenID=20User?= =?UTF-8?q?=20Info=20Handling=20(#4561)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 --- .env.example | 4 + api/strategies/openidStrategy.js | 53 ++++++-- api/strategies/openidStrategy.spec.js | 175 ++++++++++++++++++++++++++ 3 files changed, 219 insertions(+), 13 deletions(-) create mode 100644 api/strategies/openidStrategy.spec.js diff --git a/.env.example b/.env.example index dd99dce756..5fd1711a01 100644 --- a/.env.example +++ b/.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= diff --git a/api/strategies/openidStrategy.js b/api/strategies/openidStrategy.js index d818725e00..c12b088bce 100644 --- a/api/strategies/openidStrategy.js +++ b/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( - userinfo.username || userinfo.given_name || userinfo.email, - ); + 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 = { diff --git a/api/strategies/openidStrategy.spec.js b/api/strategies/openidStrategy.spec.js new file mode 100644 index 0000000000..e2e7af87a6 --- /dev/null +++ b/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.', + ); + }); + }); + }); +});