fix(core): Refactor how base urls are defined to make urls consistent across the backend #6713

netroy · 2023-07-20T20:11:14Z

replaces #6245

github-actions · 2023-07-20T20:11:45Z

Great PR! Please pay attention to the following items before merging:

Files matching packages/**:

If fixing bug, added test to cover scenario.
If addressing forum or Github issue, added link to description.

Files matching packages/**/*.ts:

Added unit tests to cover new or updated functionality.

Files matching packages/nodes-base/nodes/**:

Added workflow tests for nodes if possible.

Make sure to check off this list before asking for review.

codecov · 2023-07-20T20:26:25Z

Codecov Report

Attention: 61 lines in your changes are missing coverage. Please review.

Comparison is base (05586a9) 33.59% compared to head (c4b38e9) 33.60%.
Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #6713   +/-   ##
=======================================
  Coverage   33.59%   33.60%           
=======================================
  Files        3401     3402    +1     
  Lines      207699   207722   +23     
  Branches    22467    22469    +2     
=======================================
+ Hits        69781    69802   +21     
- Misses     136771   136772    +1     
- Partials     1147     1148    +1

Files	Coverage Δ
packages/cli/src/GenericHelpers.ts	`79.48% <ø> (+0.32%)`	⬆️
packages/cli/src/PublicApi/index.ts	`86.20% <100.00%> (+0.24%)`	⬆️
...ges/cli/src/UserManagement/UserManagementHelper.ts	`83.63% <ø> (-0.24%)`	⬇️
...i/src/UserManagement/email/UserManagementMailer.ts	`48.57% <100.00%> (+1.51%)`	⬆️
packages/cli/src/WebhookHelpers.ts	`32.03% <ø> (-0.42%)`	⬇️
packages/cli/src/config/schema.ts	`83.33% <ø> (ø)`
...es/cli/src/controllers/passwordReset.controller.ts	`73.68% <100.00%> (-0.88%)`	⬇️
packages/cli/src/services/user.service.ts	`88.09% <100.00%> (+12.65%)`	⬆️
packages/cli/src/sso/saml/constants.ts	`100.00% <100.00%> (ø)`
packages/cli/src/sso/saml/serviceProvider.ee.ts	`100.00% <100.00%> (ø)`
... and 14 more

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

netroy · 2023-07-20T20:16:58Z

packages/cli/src/WorkflowExecuteAdditionalData.ts

-			workflowData.id
-		}/executions/${executionId}`;
+		const { editorBaseUrl } = Container.get(URLService);
+		pastExecutionUrl = `${editorBaseUrl}workflow/${workflowData.id}/executions/${executionId}`;


changed this to use editor url instead of webhook url, as i believe that is the correct url.

packages/cli/src/commands/start.ts

netroy · 2023-07-20T20:19:25Z

packages/cli/src/config/schema.ts

+	baseUrl: {
+		format: String,
+		default: '',
+		env: 'N8N_BASE_URL',


added a new env variable, as WEBHOOK_URL was never meant to be used for non webhook urls (even though in some please it incorrectly is used that way).

The problem with this new variable is that users were supposed to set up n8n using N8N_HOST, N8N_PATH and N8N_PORT

netroy · 2023-07-20T20:21:42Z

packages/cli/src/sso/saml/constants.ts

@@ -1,15 +1,15 @@
-export class SamlUrls {
-	static readonly samlRESTRoot = '/rest/sso/saml';


a hardcoded /rest somehow managed to sneak past reviews.

netroy · 2023-07-20T20:23:26Z

packages/cli/src/sso/saml/routes/saml.controller.ee.ts

@@ -138,10 +138,10 @@ export class SamlController {
 				if (isSamlLicensedAndEnabled()) {
 					await issueCookie(res, loginResult.authenticatedUser);
 					if (loginResult.onboardingRequired) {
-						return res.redirect(getInstanceBaseUrl() + SamlUrls.samlOnboarding);
+						return res.redirect(this.urlService.editorBaseUrl + SamlUrls.samlOnboarding);


this should be using editorBaseUrl so that these urls work with --tunnel, and also when someone is overriding the base url.

netroy · 2023-07-20T20:25:45Z

packages/cli/src/sso/saml/saml.service.ee.ts

@@ -138,14 +141,14 @@ export class SamlService {

 	private getRedirectLoginRequestUrl(relayState?: string): BindingContext {
 		const sp = this.getServiceProviderInstance();
-		sp.entitySetting.relayState = relayState ?? getInstanceBaseUrl();
+		sp.entitySetting.relayState = relayState ?? this.urlService.editorBaseUrl;


same here. I'm wondering if we should add createBackendUrl and createFrontendUrl methods in URLService, so that people are forced to declare if the generated url is meant to be handled by the frontend, or the backend.

That's a fair point, maybe in the future we have these services running in separate places (rest API and FE). I'm afraid this would make the PR larger, useless at this time and harder to test. For now I think it's ok to assume that editor and rest api are always on the same host (as they actually are)

packages/cli/src/user/user.service.ts

packages/cli/src/services/url.service.ts

ivov

Thanks for taking this on!

From a quick test, running packages/cli/bin/n8n start --tunnel fails to open in browser, after a while ends up with 504 Gateway Time-out

packages/cli/src/services/url.service.ts

ivov · 2023-07-21T08:10:37Z

packages/cli/src/config/schema.ts

+		format: String,
+		default: '',
+		env: 'WEBHOOK_URL',
+		doc: 'Used to manually provide the Webhook URL when running n8n behind a reverse proxy.',


Let's specify in the description what these default to?

docker/compose/subfolderWithSSL/docker-compose.yml

ivov · 2023-07-21T08:14:08Z

packages/cli/src/services/url.service.ts

+		webhook: string;
+		oauth1Callback: string;
+		oauth2Callback: string;
+	};


Let's add N8N_VERSION_NOTIFICATIONS_ENDPOINT?

Let's add N8N_LICENSE_SERVER_URL?

Let's add EXTERNAL_FRONTEND_HOOKS_URLS?

Let's add N8N_VERSION_NOTIFICATIONS_INFO_URL?

Let's add N8N_PUBLIC_API_ENDPOINT?

packages/cli/src/services/url.service.ts

ivov · 2023-07-21T08:26:12Z

packages/cli/src/services/url.service.ts

+		return this.urls.editor;
+	}
+
+	/** This is the base url for webhooks */


Not sure if the comment is adding much.

netroy · 2023-07-21T09:33:56Z

Thanks for taking this on!

From a quick test, running packages/cli/bin/n8n start --tunnel fails to open in browser, after a while ends up with 504 Gateway Time-out

That's because the hooks server is acting up again.

krynble · 2023-07-21T09:42:46Z

packages/cli/src/WorkflowExecuteAdditionalData.ts

+		webhookBaseUrl: webhookBaseUrl + config.getEnv('endpoints.webhook'),
+		webhookWaitingBaseUrl: webhookBaseUrl + config.getEnv('endpoints.webhookWaiting'),
+		webhookTestBaseUrl: webhookBaseUrl + config.getEnv('endpoints.webhookTest'),


Maybe expose those 3 URLs as part of the urlService so it's always centralized. Right now multiple places of the code can generate this URL, possibly leading to inconsistencies.

krynble · 2023-07-21T09:52:31Z

packages/cli/src/services/url.service.ts

+		// TODO: should this use `webhookBaseUrl` ?
+		const restBaseUrl = this.stripSlash(`${baseUrl}${config.getEnv('endpoints.rest')}`);


This is correct, the backend (rest API) always resides with the editor.
The webhooks URL can (if set up like this) point to another pool of servers.

let's say you deploy n8n using https://n8n.omar.cloud pointing to editor + rest APIs and then another subdomain such as https://webhook.omar.cloud that points to another pool of servers only running webhook-type servers for queue mode.

krynble · 2023-07-21T09:54:05Z

packages/cli/src/sso/saml/constants.ts

@@ -1,15 +1,15 @@
-export class SamlUrls {
-	static readonly samlRESTRoot = '/rest/sso/saml';


krynble · 2023-07-21T09:56:33Z

packages/cli/src/config/schema.ts

+	webhookBaseUrl: {
+		format: String,
+		default: '',
+		env: 'WEBHOOK_URL',


Renaming environment variables breaks so many things, I advise against making this change now

krynble · 2023-07-21T09:58:04Z

packages/cli/src/config/schema.ts

+	baseUrl: {
+		format: String,
+		default: '',
+		env: 'N8N_BASE_URL',


The problem with this new variable is that users were supposed to set up n8n using N8N_HOST, N8N_PATH and N8N_PORT

krynble · 2023-07-21T09:59:47Z

packages/cli/src/sso/saml/saml.service.ee.ts

@@ -138,14 +141,14 @@ export class SamlService {

 	private getRedirectLoginRequestUrl(relayState?: string): BindingContext {
 		const sp = this.getServiceProviderInstance();
-		sp.entitySetting.relayState = relayState ?? getInstanceBaseUrl();
+		sp.entitySetting.relayState = relayState ?? this.urlService.editorBaseUrl;


That's a fair point, maybe in the future we have these services running in separate places (rest API and FE). I'm afraid this would make the PR larger, useless at this time and harder to test. For now I think it's ok to assume that editor and rest api are always on the same host (as they actually are)

ivov · 2023-09-15T14:55:15Z

I'd love to eventually merge this in - if any of my comments take long, please feel free to disregard!

netroy · 2023-10-25T20:26:52Z

rebased. but going to break this out into 2 PRs, one introducing the URLService and it's tests, and other with any potential breaking changes.

This change kept coming up in #6713, #7773, and #8135. So this PR moves the existing code without actually changing anything, to help get rid of some of the circular dependencies. ## Review / Merge checklist - [x] PR title and summary are descriptive.

netroy · 2024-06-20T15:41:52Z

closing this PR since quite a bit of this was already merged, and the remaining bits will need be done from scratch again.

netroy commented Jul 20, 2023

View reviewed changes

n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team node/improvement New feature or request labels Jul 20, 2023

netroy mentioned this pull request Jul 21, 2023

fix(core): Load WEBHOOK_URL via config to support setting the value via a file #6245

Closed

ivov requested changes Jul 21, 2023

View reviewed changes

krynble suggested changes Jul 21, 2023

View reviewed changes

netroy force-pushed the refactor-urls branch from 85abefd to 42260b4 Compare July 24, 2023 21:47

refactor(core): Move all base url logic into URLService (no-changelog)

c4b38e9

netroy force-pushed the refactor-urls branch from f205c0f to c4b38e9 Compare October 25, 2023 20:25

netroy marked this pull request as draft October 25, 2023 20:26

netroy mentioned this pull request Dec 22, 2023

refactor(core): Move all base URLs to UrlService (no-changelog) #8141

Merged

1 task

netroy closed this Jun 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core): Refactor how base urls are defined to make urls consistent across the backend #6713

fix(core): Refactor how base urls are defined to make urls consistent across the backend #6713

netroy commented Jul 20, 2023

github-actions bot commented Jul 20, 2023

codecov bot commented Jul 20, 2023 •

edited

Loading

netroy Jul 20, 2023

netroy Jul 20, 2023

krynble Jul 21, 2023

netroy Jul 20, 2023

krynble Jul 21, 2023

netroy Jul 20, 2023 •

edited

Loading

netroy Jul 20, 2023

krynble Jul 21, 2023

ivov left a comment

ivov Jul 21, 2023

ivov Jul 21, 2023

ivov Jul 21, 2023

ivov Jul 21, 2023

ivov Jul 21, 2023

ivov Jul 21, 2023

ivov Jul 21, 2023

netroy commented Jul 21, 2023

krynble Jul 21, 2023

krynble Jul 21, 2023

krynble Jul 21, 2023

krynble Jul 21, 2023

krynble Jul 21, 2023

krynble Jul 21, 2023

ivov commented Sep 15, 2023

netroy commented Oct 25, 2023

netroy commented Jun 20, 2024

		@@ -1,15 +1,15 @@
		export class SamlUrls {
		static readonly samlRESTRoot = '/rest/sso/saml';

		// TODO: should this use `webhookBaseUrl` ?
		const restBaseUrl = this.stripSlash(`${baseUrl}${config.getEnv('endpoints.rest')}`);

fix(core): Refactor how base urls are defined to make urls consistent across the backend #6713

fix(core): Refactor how base urls are defined to make urls consistent across the backend #6713

Conversation

netroy commented Jul 20, 2023

github-actions bot commented Jul 20, 2023

codecov bot commented Jul 20, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netroy Jul 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netroy commented Jul 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivov commented Sep 15, 2023

netroy commented Oct 25, 2023

netroy commented Jun 20, 2024

codecov bot commented Jul 20, 2023 •

edited

Loading

netroy Jul 20, 2023 •

edited

Loading