From dc208cdb59acededa0356912b69096e832fc07c6 Mon Sep 17 00:00:00 2001 From: Jack Kelly Date: Wed, 25 Jun 2025 16:12:08 +0100 Subject: [PATCH 1/3] fix: allow for auth with activedirectory again Current /login is forced to use local auth. This change resolves this regression in a backwards compatible manner while still supporting alternative auth like OIDC. Auth methods compatible with /login are filtered out from the git-proxy config, if there isn't one /login is essentially disabled to avoid throwing. If there is multiple methods it'll prioritize the first. In the future we can support multiple user & password login methods simultaneously with relevant TODOs added. --- src/service/routes/auth.js | 73 ++++++++++++++++++++++++++++---------- 1 file changed, 55 insertions(+), 18 deletions(-) diff --git a/src/service/routes/auth.js b/src/service/routes/auth.js index 9544931d9..600ae5765 100644 --- a/src/service/routes/auth.js +++ b/src/service/routes/auth.js @@ -1,6 +1,9 @@ const express = require('express'); const router = new express.Router(); const passport = require('../passport').getPassport(); +const { getAuthMethods } = require('../../config'); +const passportLocal = require('../passport/local'); +const passportAD = require('../passport/activeDirectory'); const authStrategies = require('../passport').authStrategies; const db = require('../../db'); const { GIT_PROXY_UI_HOST: uiHost = 'http://localhost', GIT_PROXY_UI_PORT: uiPort = 3000 } = @@ -23,25 +26,59 @@ router.get('/', (req, res) => { }); }); -router.post('/login', passport.authenticate(authStrategies['local'].type), async (req, res) => { - try { - const currentUser = { ...req.user }; - delete currentUser.password; - console.log( - `serivce.routes.auth.login: user logged in, username=${ - currentUser.username - } profile=${JSON.stringify(currentUser)}`, - ); - res.send({ - message: 'success', - user: currentUser, - }); - } catch (e) { - console.log(`service.routes.auth.login: Error logging user in ${JSON.stringify(e)}`); - res.status(500).send('Failed to login').end(); - return; +// login strategies that will work with /login e.g. take username and password +const appropriateLoginStrategies = [passportLocal.type, passportAD.type]; +// getLoginStrategy fetches the enabled auth methods and identifies if there's an appropriate +// auth method for username and password login. If there isn't it returns null, if there is it +// returns the first. +const getLoginStrategy = () => { + // returns only enabled auth methods + // returns at least one enabled auth method + const enabledAppropriateLoginStrategies = getAuthMethods().filter((am) => + appropriateLoginStrategies.includes(am.type.toLowerCase()), + ); + // for where no login strategies which work for /login are enabled + // just return null + if (enabledAppropriateLoginStrategies.length === 0) { + return null; } -}); + // return the first enabled auth method + return enabledAppropriateLoginStrategies[0].type.toLowerCase(); +}; + +// TODO: provide separate auth endpoints for each auth strategy or chain compatibile auth strategies +// TODO: if providing separate auth methods, inform the frontend so it has relevant UI elements and appropriate client-side behavior +router.post( + '/login', + (req, res, next) => { + const authType = getLoginStrategy(); + if (authType === null) { + res.status(403).send('Username and Password based Login is not enabled at this time').end(); + return; + } + console.log('going to auth with', authType); + return passport.authenticate(authType)(req, res, next); + }, + async (req, res) => { + try { + const currentUser = { ...req.user }; + delete currentUser.password; + console.log( + `serivce.routes.auth.login: user logged in, username=${ + currentUser.username + } profile=${JSON.stringify(currentUser)}`, + ); + res.send({ + message: 'success', + user: currentUser, + }); + } catch (e) { + console.log(`service.routes.auth.login: Error logging user in ${JSON.stringify(e)}`); + res.status(500).send('Failed to login').end(); + return; + } + }, +); router.get('/oidc', passport.authenticate(authStrategies['openidconnect'].type)); From 6793faebc912ff506d2153e59de61ae9c480832c Mon Sep 17 00:00:00 2001 From: Jack Kelly Date: Wed, 25 Jun 2025 16:16:56 +0100 Subject: [PATCH 2/3] fix: make loadFromGit non-interactive loadFromGit should be sufficiently authenticated and if it isn't it's not expected to get interactive input. Inform git not to wait for interactive action. This also resolves an issue in the tests where "should throw error if repository is valid URL but not a git repository" test times out waiting for a username. --- src/config/ConfigLoader.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/config/ConfigLoader.ts b/src/config/ConfigLoader.ts index a32dd3f33..8c3a0c14f 100644 --- a/src/config/ConfigLoader.ts +++ b/src/config/ConfigLoader.ts @@ -316,6 +316,8 @@ export class ConfigLoader extends EventEmitter { const execOptions = { cwd: process.cwd(), env: { + // dont wait for credentials; the command should be sufficiently authed + GIT_TERMINAL_PROMPT: 0, ...process.env, ...(source.auth?.type === 'ssh' ? { From accf2231796e65b9ad8f563a248ffe0e58138e4b Mon Sep 17 00:00:00 2001 From: Jack Kelly Date: Wed, 25 Jun 2025 16:20:06 +0100 Subject: [PATCH 3/3] test: move valid url missing git repo test under our control If we want to ensure this git repository isn't created it should be pointed at the FINOS org, the parent of this repo currently. In theory test-org (an org that currently exists) could add a repo called test-project. --- test/ConfigLoader.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/ConfigLoader.test.js b/test/ConfigLoader.test.js index df91c49d5..ac408a2ed 100644 --- a/test/ConfigLoader.test.js +++ b/test/ConfigLoader.test.js @@ -437,7 +437,7 @@ describe('ConfigLoader', () => { it('should throw error if repository is a valid URL but not a git repository', async function () { const source = { type: 'git', - repository: 'https://github.com/test-org/test-repo.git', + repository: 'https://github.com/finos/made-up-test-repo.git', path: 'proxy.config.json', branch: 'main', enabled: true,