From 1400d9b76d44b214e4d4d44ecf0dd9b5e95829ba Mon Sep 17 00:00:00 2001 From: kriswest Date: Thu, 10 Apr 2025 15:58:20 +0100 Subject: [PATCH 1/2] feat: support direct querying of AD group membership via LDAP --- .husky/commit-msg | 3 - config.schema.json | 60 ++- proxy.config.json | 4 +- src/config/index.ts | 6 +- .../push-action/checkCommitMessages.ts | 2 +- src/service/passport/activeDirectory.js | 26 +- src/service/passport/index.js | 2 +- src/service/passport/ldaphelper.js | 46 ++- test/addRepoTest.test.js | 5 +- test/testUserCreation.test.js | 3 +- website/docs/configuration/reference.mdx | 350 ++++++++++++++---- 11 files changed, 400 insertions(+), 107 deletions(-) diff --git a/.husky/commit-msg b/.husky/commit-msg index 53b8922aa..ec20f1ea9 100755 --- a/.husky/commit-msg +++ b/.husky/commit-msg @@ -1,4 +1 @@ -#!/usr/bin/env sh -. "$(dirname -- "$0")/_/husky.sh" - npx --no -- commitlint --edit ${1} && npm run lint diff --git a/config.schema.json b/config.schema.json index 771e83d0c..faf83c4e2 100644 --- a/config.schema.json +++ b/config.schema.json @@ -10,7 +10,32 @@ "sessionMaxAgeHours": { "type": "number" }, "api": { "description": "Third party APIs", - "type": "object" + "type": "object", + "properties": { + "ls": { + "type": "object", + "description": "Configuration used in conjunction with ActiveDirectory auth, which relates to a REST API used to check user group membership, as opposed to direct querying via LDAP.
If this configuration is set direct querying of group membership via LDAP will be disabled.", + "properties": { + "userInADGroup": { + "type": "string", + "description": "URL template for a GET request that confirms a user's membership of a specific group. Should respond with a non-empty 200 status if the user is a member of the group, an empty response or non-200 status indicates that the user is not a group member. If set, this URL will be queried and direct queries via LDAP will be disabled. The template should contain the following string placeholders, which will be replaced to produce the final URL:", + "examples": [ + "https://somedomain.com/some/path/checkUserGroups?domain=&name=&id=" + ] + } + } + }, + "github": { + "type": "object", + "properties": { + "baseUrl": { + "type": "string", + "format": "uri", + "examples": ["https://api.github.com"] + } + } + } + } }, "commitConfig": { "description": "Enforce rules and patterns on commits including e-mail and message", @@ -103,10 +128,39 @@ }, "authentication": { "type": "object", + "description": "Configuration for an authentication source", "properties": { - "type": { "type": "string" }, + "type": { "type": "string", "enum": ["local", "ActiveDirectory", "OpenIdConnect"] }, "enabled": { "type": "boolean" }, - "options": { "type": "object" } + "adminGroup": { + "type": "string", + "description": "Group that indicates that a user is an admin" + }, + "userGroup": { + "type": "string", + "description": "Group that indicates that a user should be able to login to the Git Proxy UI and can work as a reviewer" + }, + "domain": { "type": "string", "description": "Active Directory domain" }, + "adConfig": { + "type": "object", + "description": "Additional Active Directory configuration supporting LDAP connection which can be used to confirm group membership. For the full set of available options see the activedirectory 2 NPM module docs at https://www.npmjs.com/package/activedirectory2#activedirectoryoptions

Please note that if the Third Party APIs config `api.ls.userInADGroup` is set then the REST API it represents is used in preference to direct querying of group memebership via LDAP.", + "properties": { + "url": { + "type": "string", + "description": "Active Directory server to connect to, e.g. `ldap://ad.example.com`." + }, + "baseDN": { + "type": "string", + "description": "The root DN from which all searches will be performed, e.g. `dc=example,dc=com`." + }, + "username": { + "type": "string", + "description": "An account name capable of performing the operations desired." + }, + "password": { "type": "string", "description": "Password for the given `username`." } + }, + "required": ["url", "baseDN", "username", "password"] + } }, "required": ["type", "enabled"] } diff --git a/proxy.config.json b/proxy.config.json index 14d016e4d..0e546c8f2 100644 --- a/proxy.config.json +++ b/proxy.config.json @@ -47,7 +47,9 @@ "adConfig": { "url": "", "baseDN": "", - "searchBase": "" + "searchBase": "", + "username": "", + "password": "" } } ], diff --git a/src/config/index.ts b/src/config/index.ts index a1779ed9d..2a6fb1f90 100644 --- a/src/config/index.ts +++ b/src/config/index.ts @@ -69,7 +69,7 @@ export const getDatabase = () => { } } - throw Error('No database cofigured!'); + throw Error('No database configured!'); }; // Gets the configured authentication method, defaults to local @@ -85,7 +85,7 @@ export const getAuthentication = () => { } } - throw Error('No authentication cofigured!'); + throw Error('No authentication configured!'); }; // Log configuration to console @@ -170,7 +170,7 @@ export const getPlugins = () => { _plugins = _userSettings.plugins; } return _plugins; -} +}; export const getSSLKeyPath = () => { if (_userSettings && _userSettings.sslKeyPemPath) { diff --git a/src/proxy/processors/push-action/checkCommitMessages.ts b/src/proxy/processors/push-action/checkCommitMessages.ts index 577a572af..506f872cf 100644 --- a/src/proxy/processors/push-action/checkCommitMessages.ts +++ b/src/proxy/processors/push-action/checkCommitMessages.ts @@ -70,7 +70,7 @@ const exec = async (req: any, action: Action): Promise => { step.error = true; step.log(`The following commit messages are illegal: ${illegalMessages}`); step.setError( - '\n\n\n\nYour push has been blocked.\nPlease ensure your commit message(s) does not contain sensitive information or URLs.\n\n\n', + `\n\n\nYour push has been blocked.\nPlease ensure your commit message(s) does not contain sensitive information or URLs.\n\nThe following commit messages are illegal: ${illegalMessages}\n\n`, ); action.addStep(step); diff --git a/src/service/passport/activeDirectory.js b/src/service/passport/activeDirectory.js index 466f57b16..596f4718e 100644 --- a/src/service/passport/activeDirectory.js +++ b/src/service/passport/activeDirectory.js @@ -29,16 +29,27 @@ const configure = () => { profile._json.userPrincipalName }, profile=${JSON.stringify(profile)}`, ); - // First check to see if the user is in the usergroups - const isUser = await ldaphelper.isUserInAdGroup(profile.username, domain, userGroup); - - if (!isUser) { - const message = `User it not a member of ${userGroup}`; + // First check to see if the user is in the AD user group + try { + const isUser = await ldaphelper.isUserInAdGroup(req, profile, ad, domain, userGroup); + if (!isUser) { + const message = `User it not a member of ${userGroup}`; + return done(message, null); + } + } catch (e) { + const message = `An error occurred while checking if the user is a member of the user group: ${JSON.stringify(e)}`; return done(message, null); } - + // Now check if the user is an admin - const isAdmin = await ldaphelper.isUserInAdGroup(profile.username, domain, adminGroup); + let isAdmin = false; + try { + isAdmin = await ldaphelper.isUserInAdGroup(req, profile, ad, domain, adminGroup); + + } catch (e) { + const message = `An error occurred while checking if the user is a member of the admin group: ${JSON.stringify(e)}`; + console.error(message, e); + } profile.admin = isAdmin; console.log(`passport.activeDirectory: ${profile.username} admin=${isAdmin}`); @@ -65,6 +76,7 @@ const configure = () => { passport.deserializeUser(function (user, done) { done(null, user); }); + passport.type = "ActiveDirectory"; return passport; }; diff --git a/src/service/passport/index.js b/src/service/passport/index.js index a2d7931ef..478800672 100644 --- a/src/service/passport/index.js +++ b/src/service/passport/index.js @@ -19,7 +19,7 @@ const configure = async () => { _passport = await oidc.configure(); break; default: - throw Error(`uknown authentication type ${type}`); + throw Error(`unknown authentication type ${type}`); } if (!_passport.type) { _passport.type = type; diff --git a/src/service/passport/ldaphelper.js b/src/service/passport/ldaphelper.js index 886b2c4a4..aa32f04f8 100644 --- a/src/service/passport/ldaphelper.js +++ b/src/service/passport/ldaphelper.js @@ -1,18 +1,48 @@ -const axios = require('axios'); +const config = require('../../config').getAuthentication(); const thirdpartyApiConfig = require('../../config').getAPIs(); -const client = axios.create({ - responseType: 'json', - headers: { - 'content-type': 'application/json', - }, -}); +const axios = require('axios'); + +const isUserInAdGroup = (req, profile, ad, domain, name) => { + // determine, via config, if we're using HTTP or AD directly + if (thirdpartyApiConfig?.ls?.userInADGroup) { + return isUserInAdGroupViaHttp(profile.username, domain, name); + } else if (config.adConfig) { + return isUserInAdGroupViaAD(req, profile, ad, domain, name); + } else { + console.error('Unable to check user groups as config is incomplete or unreadable'); + } +}; + +const isUserInAdGroupViaAD = (req, profile, ad, domain, name) => { + // TODO: Added for debugging , needs to be removed + console.log(`checking if id: ${profile.username}, on domain: ${domain}, is a member of group name: ${name}`); -const isUserInAdGroup = (id, domain, name) => { + return new Promise((resolve, reject) => { + ad.isUserMemberOf(profile.username, name, function (err, isMember) { + if (err) { + const msg = 'ERROR isUserMemberOf: ' + JSON.stringify(err); + reject(msg); + } else { + console.log(profile.username + ' isMemberOf ' + name + ': ' + isMember); + resolve(isMember); + } + }); + }); +}; + +const isUserInAdGroupViaHttp = (id, domain, name) => { const url = String(thirdpartyApiConfig.ls.userInADGroup) .replace('', domain) .replace('', name) .replace('', id); + const client = axios.create({ + responseType: 'json', + headers: { + 'content-type': 'application/json', + }, + }); + console.log(`checking if user is in group ${url}`); return client .get(url) diff --git a/test/addRepoTest.test.js b/test/addRepoTest.test.js index 04983f63c..b86fa222c 100644 --- a/test/addRepoTest.test.js +++ b/test/addRepoTest.test.js @@ -28,7 +28,7 @@ describe('add new repo', async () => { await db.deleteUser('u1'); await db.deleteUser('u2'); await db.createUser('u1', 'abc', 'test@test.com', 'test', true); - await db.createUser('u2', 'abc', 'test@test.com', 'test', true); + await db.createUser('u2', 'abc', 'test2@test.com', 'test', true); }); it('login', async function () { @@ -187,5 +187,8 @@ describe('add new repo', async () => { after(async function () { await service.httpServer.close(); + await db.deleteRepo('test-repo'); + await db.deleteUser('u1'); + await db.deleteUser('u2'); }); }); diff --git a/test/testUserCreation.test.js b/test/testUserCreation.test.js index c07dd0e7b..631be0069 100644 --- a/test/testUserCreation.test.js +++ b/test/testUserCreation.test.js @@ -55,8 +55,7 @@ describe('user creation', async () => { }); it('login as new user', async function () { - // we don't know the users tempoary password - so force update a - // pasword + // we don't know the users temporary password - so force update a password const user = await db.findUser('login-test-user'); await bcrypt.hash('test1234', 10, async function (err, hash) { diff --git a/website/docs/configuration/reference.mdx b/website/docs/configuration/reference.mdx index 6a7eceedf..e1da31a40 100644 --- a/website/docs/configuration/reference.mdx +++ b/website/docs/configuration/reference.mdx @@ -7,11 +7,11 @@ description: JSON schema reference documentation for GitProxy **Title:** GitProxy configuration file -| | | -| ------------------------- | ------------------------------------------------------- | -| **Type** | `object` | -| **Required** | No | -| **Additional properties** | [[Not allowed]](# "Additional Properties not allowed.") | +| | | +| ------------------------- | ----------- | +| **Type** | `object` | +| **Required** | No | +| **Additional properties** | Not allowed | **Description:** Configuration for customizing git-proxy @@ -63,14 +63,89 @@ description: JSON schema reference documentation for GitProxy
-| | | -| ------------------------- | ------------------------------------------------------------------------- | -| **Type** | `object` | -| **Required** | No | -| **Additional properties** | [[Any type: allowed]](# "Additional Properties of any type are allowed.") | +| | | +| ------------------------- | ---------------- | +| **Type** | `object` | +| **Required** | No | +| **Additional properties** | Any type allowed | **Description:** Third party APIs +
+ + 4.1. [Optional] Property GitProxy configuration file > api > ls + +
+ +| | | +| ------------------------- | ---------------- | +| **Type** | `object` | +| **Required** | No | +| **Additional properties** | Any type allowed | + +**Description:** Configuration used in conjunction with ActiveDirectory auth, which relates to a REST API used to check user group membership, as opposed to direct querying via LDAP.
If this configuration is set direct querying of group membership via LDAP will be disabled. + +
+ + 4.1.1. [Optional] Property GitProxy configuration file > api > ls > userInADGroup + +
+ +| | | +| ------------ | -------- | +| **Type** | `string` | +| **Required** | No | + +**Description:** URL template for a GET request that confirms a user's membership of a specific group. Should respond with a non-empty 200 status if the user is a member of the group, an empty response or non-200 status indicates that the user is not a group member. If set, this URL will be queried and direct queries via LDAP will be disabled. The template should contain the following string placeholders, which will be replaced to produce the final URL:
  • "<domain>": AD domain,
  • "<name>": The group name to check membership of.
  • "<id>": The username to check group membership for.
+ +**Example:** + +```json +"https://somedomain.com/some/path/checkUserGroups?domain=&name=&id=" +``` + +
+
+ +
+
+ +
+ + 4.2. [Optional] Property GitProxy configuration file > api > github + +
+ +| | | +| ------------------------- | ---------------- | +| **Type** | `object` | +| **Required** | No | +| **Additional properties** | Any type allowed | + +
+ + 4.2.1. [Optional] Property GitProxy configuration file > api > github > baseUrl + +
+ +| | | +| ------------ | -------- | +| **Type** | `string` | +| **Required** | No | +| **Format** | `uri` | + +**Example:** + +```json +"https://api.github.com" +``` + +
+
+ +
+
+
@@ -80,11 +155,11 @@ description: JSON schema reference documentation for GitProxy
-| | | -| ------------------------- | ------------------------------------------------------------------------- | -| **Type** | `object` | -| **Required** | No | -| **Additional properties** | [[Any type: allowed]](# "Additional Properties of any type are allowed.") | +| | | +| ------------------------- | ---------------- | +| **Type** | `object` | +| **Required** | No | +| **Additional properties** | Any type allowed | **Description:** Enforce rules and patterns on commits including e-mail and message @@ -97,11 +172,11 @@ description: JSON schema reference documentation for GitProxy
-| | | -| ------------------------- | ------------------------------------------------------------------------- | -| **Type** | `object` | -| **Required** | No | -| **Additional properties** | [[Any type: allowed]](# "Additional Properties of any type are allowed.") | +| | | +| ------------------------- | ---------------- | +| **Type** | `object` | +| **Required** | No | +| **Additional properties** | Any type allowed | **Description:** Customisable questions to add to attestation form @@ -114,11 +189,11 @@ description: JSON schema reference documentation for GitProxy
-| | | -| ------------------------- | ------------------------------------------------------------------------- | -| **Type** | `object` | -| **Required** | No | -| **Additional properties** | [[Any type: allowed]](# "Additional Properties of any type are allowed.") | +| | | +| ------------------------- | ---------------- | +| **Type** | `object` | +| **Required** | No | +| **Additional properties** | Any type allowed | **Description:** Provide domains to use alternative to the defaults @@ -235,12 +310,12 @@ description: JSON schema reference documentation for GitProxy ### 13.1. GitProxy configuration file > authorisedList > authorisedRepo -| | | -| ------------------------- | ------------------------------------------------------------------------- | -| **Type** | `object` | -| **Required** | No | -| **Additional properties** | [[Any type: allowed]](# "Additional Properties of any type are allowed.") | -| **Defined in** | #/definitions/authorisedRepo | +| | | +| ------------------------- | ---------------------------- | +| **Type** | `object` | +| **Required** | No | +| **Additional properties** | Any type allowed | +| **Defined in** | #/definitions/authorisedRepo |
@@ -306,12 +381,12 @@ description: JSON schema reference documentation for GitProxy ### 14.1. GitProxy configuration file > sink > database -| | | -| ------------------------- | ------------------------------------------------------------------------- | -| **Type** | `object` | -| **Required** | No | -| **Additional properties** | [[Any type: allowed]](# "Additional Properties of any type are allowed.") | -| **Defined in** | #/definitions/database | +| | | +| ------------------------- | ---------------------- | +| **Type** | `object` | +| **Required** | No | +| **Additional properties** | Any type allowed | +| **Defined in** | #/definitions/database |
@@ -361,11 +436,11 @@ description: JSON schema reference documentation for GitProxy
-| | | -| ------------------------- | ------------------------------------------------------------------------- | -| **Type** | `object` | -| **Required** | No | -| **Additional properties** | [[Any type: allowed]](# "Additional Properties of any type are allowed.") | +| | | +| ------------------------- | ---------------- | +| **Type** | `object` | +| **Required** | No | +| **Additional properties** | Any type allowed |
@@ -376,11 +451,11 @@ description: JSON schema reference documentation for GitProxy
-| | | -| ------------------------- | ------------------------------------------------------------------------- | -| **Type** | `object` | -| **Required** | No | -| **Additional properties** | [[Any type: allowed]](# "Additional Properties of any type are allowed.") | +| | | +| ------------------------- | ---------------- | +| **Type** | `object` | +| **Required** | No | +| **Additional properties** | Any type allowed |
@@ -401,18 +476,20 @@ description: JSON schema reference documentation for GitProxy **Description:** List of authentication sources. The first source in the configuration with enabled=true will be used. -| Each item of this array must be | Description | -| --------------------------------------- | ----------- | -| [authentication](#authentication_items) | - | +| Each item of this array must be | Description | +| --------------------------------------- | ------------------------------------------ | +| [authentication](#authentication_items) | Configuration for an authentication source | ### 15.1. GitProxy configuration file > authentication > authentication -| | | -| ------------------------- | ------------------------------------------------------------------------- | -| **Type** | `object` | -| **Required** | No | -| **Additional properties** | [[Any type: allowed]](# "Additional Properties of any type are allowed.") | -| **Defined in** | #/definitions/authentication | +| | | +| ------------------------- | ---------------------------- | +| **Type** | `object` | +| **Required** | No | +| **Additional properties** | Any type allowed | +| **Defined in** | #/definitions/authentication | + +**Description:** Configuration for an authentication source
@@ -420,10 +497,15 @@ description: JSON schema reference documentation for GitProxy
-| | | -| ------------ | -------- | -| **Type** | `string` | -| **Required** | Yes | +| | | +| ------------ | ------------------ | +| **Type** | `enum (of string)` | +| **Required** | Yes | + +Must be one of: +* "local" +* "ActiveDirectory" +* "OpenIdConnect"
@@ -444,15 +526,129 @@ description: JSON schema reference documentation for GitProxy
- 15.1.3. [Optional] Property GitProxy configuration file > authentication > authentication items > options + 15.1.3. [Optional] Property GitProxy configuration file > authentication > authentication items > adminGroup
-| | | -| ------------------------- | ------------------------------------------------------------------------- | -| **Type** | `object` | -| **Required** | No | -| **Additional properties** | [[Any type: allowed]](# "Additional Properties of any type are allowed.") | +| | | +| ------------ | -------- | +| **Type** | `string` | +| **Required** | No | + +**Description:** Group that indicates that a user is an admin + +
+
+ +
+ + 15.1.4. [Optional] Property GitProxy configuration file > authentication > authentication items > userGroup + +
+ +| | | +| ------------ | -------- | +| **Type** | `string` | +| **Required** | No | + +**Description:** Group that indicates that a user should be able to login to the Git Proxy UI and can work as a reviewer + +
+
+ +
+ + 15.1.5. [Optional] Property GitProxy configuration file > authentication > authentication items > domain + +
+ +| | | +| ------------ | -------- | +| **Type** | `string` | +| **Required** | No | + +**Description:** Active Directory domain + +
+
+ +
+ + 15.1.6. [Optional] Property GitProxy configuration file > authentication > authentication items > adConfig + +
+ +| | | +| ------------------------- | ---------------- | +| **Type** | `object` | +| **Required** | No | +| **Additional properties** | Any type allowed | + +**Description:** Additional Active Directory configuration supporting LDAP connection which can be used to confirm group membership. For the full set of available options see the activedirectory 2 NPM module docs at https://www.npmjs.com/package/activedirectory2#activedirectoryoptions

Please note that if the Third Party APIs config `api.ls.userInADGroup` is set then the REST API it represents is used in preference to direct querying of group memebership via LDAP. + +
+ + 15.1.6.1. [Required] Property GitProxy configuration file > authentication > authentication items > adConfig > url + +
+ +| | | +| ------------ | -------- | +| **Type** | `string` | +| **Required** | Yes | + +**Description:** Active Directory server to connect to, e.g. `ldap://ad.example.com`. + +
+
+ +
+ + 15.1.6.2. [Required] Property GitProxy configuration file > authentication > authentication items > adConfig > baseDN + +
+ +| | | +| ------------ | -------- | +| **Type** | `string` | +| **Required** | Yes | + +**Description:** The root DN from which all searches will be performed, e.g. `dc=example,dc=com`. + +
+
+ +
+ + 15.1.6.3. [Required] Property GitProxy configuration file > authentication > authentication items > adConfig > username + +
+ +| | | +| ------------ | -------- | +| **Type** | `string` | +| **Required** | Yes | + +**Description:** An account name capable of performing the operations desired. + +
+
+ +
+ + 15.1.6.4. [Required] Property GitProxy configuration file > authentication > authentication items > adConfig > password + +
+ +| | | +| ------------ | -------- | +| **Type** | `string` | +| **Required** | Yes | + +**Description:** Password for the given `username`. + +
+
@@ -466,11 +662,11 @@ description: JSON schema reference documentation for GitProxy
-| | | -| ------------------------- | ------------------------------------------------------------------------- | -| **Type** | `object` | -| **Required** | No | -| **Additional properties** | [[Any type: allowed]](# "Additional Properties of any type are allowed.") | +| | | +| ------------------------- | ---------------- | +| **Type** | `object` | +| **Required** | No | +| **Additional properties** | Any type allowed | **Description:** Toggle the generation of temporary password for git-proxy admin user @@ -494,11 +690,11 @@ description: JSON schema reference documentation for GitProxy
-| | | -| ------------------------- | ------------------------------------------------------------------------- | -| **Type** | `object` | -| **Required** | No | -| **Additional properties** | [[Any type: allowed]](# "Additional Properties of any type are allowed.") | +| | | +| ------------------------- | ---------------- | +| **Type** | `object` | +| **Required** | No | +| **Additional properties** | Any type allowed | **Description:** Generic object to configure nodemailer. For full type information, please see https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/nodemailer @@ -509,4 +705,4 @@ description: JSON schema reference documentation for GitProxy ---------------------------------------------------------------------------------------------------------------------------- -Generated using [json-schema-for-humans](https://github.com/coveooss/json-schema-for-humans) on 2024-10-22 at 16:45:32 +0100 +Generated using [json-schema-for-humans](https://github.com/coveooss/json-schema-for-humans) on 2025-04-02 at 15:34:59 +0100 From f5526245e4def8e8bee1227361c46ad344494d31 Mon Sep 17 00:00:00 2001 From: Kris West Date: Fri, 11 Apr 2025 16:00:07 +0100 Subject: [PATCH 2/2] test: don't clean-up test-repo as cypress tests rely on it --- test/addRepoTest.test.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/test/addRepoTest.test.js b/test/addRepoTest.test.js index b86fa222c..6df809a66 100644 --- a/test/addRepoTest.test.js +++ b/test/addRepoTest.test.js @@ -187,8 +187,10 @@ describe('add new repo', async () => { after(async function () { await service.httpServer.close(); - await db.deleteRepo('test-repo'); - await db.deleteUser('u1'); - await db.deleteUser('u2'); + + // don't clean up data as cypress tests rely on it being present + // await db.deleteRepo('test-repo'); + // await db.deleteUser('u1'); + // await db.deleteUser('u2'); }); });