From 2d2fc9d20ebb07cd53e47e36bbe2b428209b7b99 Mon Sep 17 00:00:00 2001 From: Gustaf Lundh Date: Mon, 21 Aug 2023 18:26:58 +0200 Subject: [PATCH 1/3] Gerrit: Gitiles auth-links could still be broken. This change includes a much more robust way to insert the /a/- prefix in the correct place. We now check that gitiles-files are actually served from the Gerrit-server and if so, the authentication prefix is inserted after Gerrit's baseUrl. Signed-off-by: Gustaf Lundh --- .changeset/grumpy-papayas-tie.md | 5 +++++ packages/integration/src/gerrit/core.test.ts | 14 +++++++------ packages/integration/src/gerrit/core.ts | 21 ++++++++++++-------- 3 files changed, 26 insertions(+), 14 deletions(-) create mode 100644 .changeset/grumpy-papayas-tie.md diff --git a/.changeset/grumpy-papayas-tie.md b/.changeset/grumpy-papayas-tie.md new file mode 100644 index 0000000000..a5635afc67 --- /dev/null +++ b/.changeset/grumpy-papayas-tie.md @@ -0,0 +1,5 @@ +--- +'@backstage/integration': patch +--- + +Gerrit: Gitiles auth-links could still be broken diff --git a/packages/integration/src/gerrit/core.test.ts b/packages/integration/src/gerrit/core.test.ts index 6f6a8ed58f..3226c7bf3e 100644 --- a/packages/integration/src/gerrit/core.test.ts +++ b/packages/integration/src/gerrit/core.test.ts @@ -37,15 +37,17 @@ describe('gerrit core', () => { describe('buildGerritGitilesArchiveUrl', () => { const config: GerritIntegrationConfig = { host: 'gerrit.com', + baseUrl: 'https://gerrit.com', gitilesBaseUrl: 'https://gerrit.com/gitiles', }; const configWithPath: GerritIntegrationConfig = { host: 'gerrit.com', - gitilesBaseUrl: 'https://gerrit.com/path/gitiles', + baseUrl: 'https://gerrit.com/gerrit', + gitilesBaseUrl: 'https://gerrit.com/gerrit/plugins/gitiles', }; - const configWithPathSlash: GerritIntegrationConfig = { + const configWithDedicatedGitiles: GerritIntegrationConfig = { host: 'gerrit.com', - gitilesBaseUrl: 'https://gerrit.com/path1/path2/gitiles/', + gitilesBaseUrl: 'https://dedicated-gitiles-server.com/gerrit/gitiles', }; it('can create an archive url for a branch', () => { expect(buildGerritGitilesArchiveUrl(config, 'repo', 'dev', '')).toEqual( @@ -84,19 +86,19 @@ describe('gerrit core', () => { expect( buildGerritGitilesArchiveUrl(authConfig, 'repo', 'dev', 'docs'), ).toEqual( - 'https://gerrit.com/path/a/gitiles/repo/+archive/refs/heads/dev/docs.tar.gz', + 'https://gerrit.com/gerrit/a/plugins/gitiles/repo/+archive/refs/heads/dev/docs.tar.gz', ); }); it('can create an authenticated url when auth is enabled and an url-path + slash is used', () => { const authConfig = { - ...configWithPathSlash, + ...configWithDedicatedGitiles, username: 'username', password: 'password', }; expect( buildGerritGitilesArchiveUrl(authConfig, 'repo', 'dev', 'docs'), ).toEqual( - 'https://gerrit.com/path1/path2/a/gitiles/repo/+archive/refs/heads/dev/docs.tar.gz', + 'https://dedicated-gitiles-server.com/gerrit/gitiles/repo/+archive/refs/heads/dev/docs.tar.gz', ); }); }); diff --git a/packages/integration/src/gerrit/core.ts b/packages/integration/src/gerrit/core.ts index e104ffc7fb..b69aca429d 100644 --- a/packages/integration/src/gerrit/core.ts +++ b/packages/integration/src/gerrit/core.ts @@ -14,6 +14,7 @@ * limitations under the License. */ +import { logger } from '@azure/identity'; import { trimStart } from 'lodash'; import { GerritIntegrationConfig } from './config'; @@ -156,14 +157,18 @@ export function getAuthenticationPrefix( export function getGitilesAuthenticationUrl( config: GerritIntegrationConfig, ): string { - const parsedUrl = new URL(config.gitilesBaseUrl!); - parsedUrl.pathname = parsedUrl.pathname.replace(/\/?$/, ''); - parsedUrl.pathname = parsedUrl.pathname.replace( - /\/([^\/]*)$/, - `${getAuthenticationPrefix(config)}$1`, - ); - - return parsedUrl.toString(); + if (config.gitilesBaseUrl!.startsWith(config.baseUrl!)) { + return config.gitilesBaseUrl!.replace( + config.baseUrl!.concat('/'), + config.baseUrl!.concat(getAuthenticationPrefix(config)), + ); + } + if (config.password) { + logger.warning( + 'Since the baseUrl (Gerrit) is not part of the gitilesBaseUrl, an authentication URL could not be constructed.', + ); + } + return config.gitilesBaseUrl!; } /** From b26c173fabf0f40697c9f7e2a8fffd878e300c33 Mon Sep 17 00:00:00 2001 From: Gustaf Lundh Date: Tue, 22 Aug 2023 10:19:33 +0200 Subject: [PATCH 2/3] Check for unset config params. Signed-off-by: Gustaf Lundh --- packages/integration/src/gerrit/core.test.ts | 1 + packages/integration/src/gerrit/core.ts | 13 +++++++++---- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/packages/integration/src/gerrit/core.test.ts b/packages/integration/src/gerrit/core.test.ts index 3226c7bf3e..19bddcf518 100644 --- a/packages/integration/src/gerrit/core.test.ts +++ b/packages/integration/src/gerrit/core.test.ts @@ -47,6 +47,7 @@ describe('gerrit core', () => { }; const configWithDedicatedGitiles: GerritIntegrationConfig = { host: 'gerrit.com', + baseUrl: 'https://gerrit.com/gerrit', gitilesBaseUrl: 'https://dedicated-gitiles-server.com/gerrit/gitiles', }; it('can create an archive url for a branch', () => { diff --git a/packages/integration/src/gerrit/core.ts b/packages/integration/src/gerrit/core.ts index b69aca429d..4b7242730e 100644 --- a/packages/integration/src/gerrit/core.ts +++ b/packages/integration/src/gerrit/core.ts @@ -157,10 +157,15 @@ export function getAuthenticationPrefix( export function getGitilesAuthenticationUrl( config: GerritIntegrationConfig, ): string { - if (config.gitilesBaseUrl!.startsWith(config.baseUrl!)) { - return config.gitilesBaseUrl!.replace( - config.baseUrl!.concat('/'), - config.baseUrl!.concat(getAuthenticationPrefix(config)), + if (!config.baseUrl || !config.gitilesBaseUrl) { + throw new Error( + 'Unexpected Gerrit config values. baseUrl or gitilesBaseUrl not set.', + ); + } + if (config.gitilesBaseUrl.startsWith(config.baseUrl)) { + return config.gitilesBaseUrl.replace( + config.baseUrl.concat('/'), + config.baseUrl.concat(getAuthenticationPrefix(config)), ); } if (config.password) { From 9726f3f0b82a43b505869c3fa66b797335897eb3 Mon Sep 17 00:00:00 2001 From: Gustaf Lundh Date: Tue, 22 Aug 2023 12:57:37 +0200 Subject: [PATCH 3/3] Log-entry should be a thrown error Signed-off-by: Gustaf Lundh --- .changeset/grumpy-papayas-tie.md | 2 +- packages/integration/src/gerrit/core.test.ts | 12 +++++++++++- packages/integration/src/gerrit/core.ts | 4 +--- 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/.changeset/grumpy-papayas-tie.md b/.changeset/grumpy-papayas-tie.md index a5635afc67..48580673d8 100644 --- a/.changeset/grumpy-papayas-tie.md +++ b/.changeset/grumpy-papayas-tie.md @@ -2,4 +2,4 @@ '@backstage/integration': patch --- -Gerrit: Gitiles auth-links could still be broken +Additional fix for Gitiles auth links diff --git a/packages/integration/src/gerrit/core.test.ts b/packages/integration/src/gerrit/core.test.ts index 19bddcf518..5fbb41763a 100644 --- a/packages/integration/src/gerrit/core.test.ts +++ b/packages/integration/src/gerrit/core.test.ts @@ -90,12 +90,22 @@ describe('gerrit core', () => { 'https://gerrit.com/gerrit/a/plugins/gitiles/repo/+archive/refs/heads/dev/docs.tar.gz', ); }); - it('can create an authenticated url when auth is enabled and an url-path + slash is used', () => { + it('Cannot build an authenticated url when a dedicated Gitiles server is used', () => { const authConfig = { ...configWithDedicatedGitiles, username: 'username', password: 'password', }; + expect(() => + buildGerritGitilesArchiveUrl(authConfig, 'repo', 'dev', 'docs'), + ).toThrow( + 'Since the baseUrl (Gerrit) is not part of the gitilesBaseUrl, an authentication URL could not be constructed.', + ); + }); + it('Build a non-authenticated url when a dedicated Gitiles server is used', () => { + const authConfig = { + ...configWithDedicatedGitiles, + }; expect( buildGerritGitilesArchiveUrl(authConfig, 'repo', 'dev', 'docs'), ).toEqual( diff --git a/packages/integration/src/gerrit/core.ts b/packages/integration/src/gerrit/core.ts index 4b7242730e..b3bdbc30e4 100644 --- a/packages/integration/src/gerrit/core.ts +++ b/packages/integration/src/gerrit/core.ts @@ -13,8 +13,6 @@ * See the License for the specific language governing permissions and * limitations under the License. */ - -import { logger } from '@azure/identity'; import { trimStart } from 'lodash'; import { GerritIntegrationConfig } from './config'; @@ -169,7 +167,7 @@ export function getGitilesAuthenticationUrl( ); } if (config.password) { - logger.warning( + throw new Error( 'Since the baseUrl (Gerrit) is not part of the gitilesBaseUrl, an authentication URL could not be constructed.', ); }