Allow the default space to be accessed via `/s/default` by legrego · Pull Request #77109 · elastic/kibana

legrego · 2020-09-09T20:06:30Z

Summary

Allows the default space to be accessed via /s/default as well as /.

Resolves #62053

legrego · 2020-09-24T11:46:16Z

x-pack/plugins/spaces/common/lib/spaces_url_parser.ts

+    return {
+      spaceId: DEFAULT_SPACE_ID,
+      pathHasExplicitSpaceIdentifier: false,
+    };


I don't love that this function now has two responsibilities, but this function is called in some hot paths, and I didn't want to have to do regex matching twice when I'm already doing the work here to extract the space identifier.

legrego · 2020-09-24T11:46:32Z

x-pack/plugins/spaces/common/lib/spaces_url_parser.ts

  return `${basePath}${requestedPath}`;
 }
+
+function stripServerBasePath(requestBasePath: string, serverBasePath: string) {


No functional changes: extracted for readability

legrego · 2020-09-24T11:47:09Z

x-pack/plugins/spaces/server/lib/request_interceptors/on_request_interceptor.ts

+    const { spaceId, pathHasExplicitSpaceIdentifier } = getSpaceIdFromPath(path, serverBasePath);

-    if (spaceId !== DEFAULT_SPACE_ID) {
+    if (pathHasExplicitSpaceIdentifier) {


This is the actual change which enables the default space to be accessed via /s/default

elasticmachine · 2020-10-14T15:46:27Z

Pinging @elastic/kibana-security (Team:Security)

jportner

LGTM!

It might be worthwhile to simplify this reporting code as well (CC @tsullivan):

kibana/x-pack/plugins/reporting/server/core.ts

Lines 223 to 226 in d10d70b

    
           if (spaceId && spaceId !== DEFAULT_SPACE_ID) { 
        
             logger.info(`Generating request for space: ${spaceId}`); 
        
             this.getPluginSetupDeps().basePath.set(fakeRequest, `/s/${spaceId}`); 
        
           }

jportner · 2020-10-15T17:07:24Z

x-pack/plugins/spaces/common/lib/spaces_url_parser.test.ts

@@ -10,41 +10,60 @@ describe('getSpaceIdFromPath', () => {
  describe('without a serverBasePath defined', () => {


Just from naively reading the tests, it's not clear that the default space can be used as an explicit space identifier.

Optional nit: what do you think about adding some additional test cases to make that more clear?

diff --git a/x-pack/plugins/spaces/common/lib/spaces_url_parser.test.ts b/x-pack/plugins/spaces/common/lib/spaces_url_parser.test.ts index d563a0c86cd..c4379574f85 100644 --- a/x-pack/plugins/spaces/common/lib/spaces_url_parser.test.ts +++ b/x-pack/plugins/spaces/common/lib/spaces_url_parser.test.ts @@ -16,6 +16,14 @@ describe('getSpaceIdFromPath', () => { }); }); + test('it identifies the space url context with the default space', () => { + const basePath = `/s/${DEFAULT_SPACE_ID}`; + expect(getSpaceIdFromPath(basePath)).toEqual({ + spaceId: DEFAULT_SPACE_ID, + pathHasExplicitSpaceIdentifier: true, + }); + }); + test('ignores space identifiers in the middle of the path', () => { const basePath = `/this/is/a/crazy/path/s/my-awesome-space-lives-here`; expect(getSpaceIdFromPath(basePath)).toEqual({ @@ -42,6 +50,14 @@ describe('getSpaceIdFromPath', () => { }); }); + test('it identifies the space url context with the default space', () => { + const basePath = `/s/${DEFAULT_SPACE_ID}`; + expect(getSpaceIdFromPath(basePath)).toEqual({ + spaceId: DEFAULT_SPACE_ID, + pathHasExplicitSpaceIdentifier: true, + }); + }); + test('it identifies the space url context following the server base path', () => { const basePath = `/server-base-path-here/s/my-awesome-space-lives-here`; expect(getSpaceIdFromPath(basePath, '/server-base-path-here')).toEqual({ @@ -50,6 +66,14 @@ describe('getSpaceIdFromPath', () => { }); }); + test('it identifies the space url context with the default space following the server base path', () => { + const basePath = `/server-base-path-here/s/${DEFAULT_SPACE_ID}`; + expect(getSpaceIdFromPath(basePath, '/server-base-path-here')).toEqual({ + spaceId: DEFAULT_SPACE_ID, + pathHasExplicitSpaceIdentifier: true, + }); + }); + test('ignores space identifiers in the middle of the path', () => { const basePath = `/this/is/a/crazy/path/s/my-awesome-space-lives-here`; expect(getSpaceIdFromPath(basePath, '/this/is/a')).toEqual({

Thanks for the suggestion! Done in 2084f12 (#77109)

…it-default-space-path

kibanamachine · 2020-10-16T02:37:18Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 5ff8450

Metrics [docs]

page load bundle size

id	before	after	diff
`spaces`	364.2KB	364.4KB	+246.0B

History

💚 Build #76246 succeeded 64786c2
💔 Build #76188 failed d4a89e58bb96aacaae356db8d22722cdef4c7039
💔 Build #74897 failed cd783c1330216304df6dda4d9e0c4ff54f939003
💚 Build #73523 succeeded c020f07e1e8797c3f12e8cb2458b7e31520bc742

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

#80815) * Allow the default space to be accessed via /s/default * apply suggestions from code review

* master: (51 commits) [Discover] Unskip flaky test (elastic#80670) Fix security solution template label (elastic#80754) [Ingest]: ignore 404, check if there are transforms in results. (elastic#80721) Moving loader to logo in header, add a slight 250ms pause (elastic#78879) [Security Solution][Cases] Fix bug with case connectors (elastic#80642) Update known-plugins.asciidoc (elastic#75388) [Lens] Add median operation (elastic#79453) Fix navigateToApp logic when navigating to the current app. (elastic#80809) [Visualizations] Fix bad color mapping with multiple split series (elastic#80801) [ILM] Add esErrorHandler for the new es js client (elastic#80302) Fix codeowners (elastic#80826) skip flaky suite (elastic#79463) [Timelion] Remove kui usage (elastic#80287) [Ingest Manager] add skipIfNoDockerRegistry to package_install_complete test (elastic#80779) [Alerting UI] Disable "Save" button for Alerts with broken Connectors (elastic#80579) Allow the default space to be accessed via `/s/default` (elastic#77109) Add script to identify plugin dependencies for TS project references migration (elastic#80463) [Search] Client side session service (elastic#76889) feat: 🎸 add separator for different context menu groups (elastic#80498) Lazy load reporting (elastic#80492) ...

legrego force-pushed the spaces/explicit-default-space-path branch 2 times, most recently from cd783c1 to d4a89e5 Compare September 22, 2020 14:09

Allow the default space to be accessed via /s/default

64786c2

legrego force-pushed the spaces/explicit-default-space-path branch from d4a89e5 to 64786c2 Compare September 22, 2020 16:47

legrego added Feature:Security/Spaces Platform Security - Spaces feature Team:Security Platform Security: Auth, Users, Roles, Spaces, Audit Logging, etc t// v7.11.0 v8.0.0 release_note:enhancement labels Oct 14, 2020

legrego commented Oct 14, 2020

View reviewed changes

legrego marked this pull request as ready for review October 14, 2020 15:46

legrego requested a review from a team as a code owner October 14, 2020 15:46

jportner self-requested a review October 15, 2020 16:42

jportner approved these changes Oct 15, 2020

View reviewed changes

legrego added 2 commits October 15, 2020 20:57

apply suggestions from code review

2084f12

Merge branch 'master' of github.com:elastic/kibana into spaces/explic…

5ff8450

…it-default-space-path

legrego merged commit 51ac14b into elastic:master Oct 16, 2020

legrego deleted the spaces/explicit-default-space-path branch October 16, 2020 11:22

legrego mentioned this pull request Oct 16, 2020

[7.x] Allow the default space to be accessed via /s/default (#77109) #80815

Merged

legrego added a commit that referenced this pull request Oct 16, 2020

[7.x] Allow the default space to be accessed via /s/default (#77109) (

c229cc9

#80815) * Allow the default space to be accessed via /s/default * apply suggestions from code review

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow the default space to be accessed via `/s/default`#77109

Allow the default space to be accessed via `/s/default`#77109
legrego merged 3 commits intoelastic:masterfrom
legrego:spaces/explicit-default-space-path

legrego commented Sep 9, 2020

Uh oh!

legrego Sep 24, 2020

Uh oh!

legrego Sep 24, 2020

Uh oh!

legrego Sep 24, 2020

Uh oh!

elasticmachine commented Oct 14, 2020

Uh oh!

jportner left a comment

Uh oh!

jportner Oct 15, 2020

Uh oh!

legrego Oct 16, 2020

Uh oh!

kibanamachine commented Oct 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	if (spaceId && spaceId !== DEFAULT_SPACE_ID) {
	logger.info(`Generating request for space: ${spaceId}`);
	this.getPluginSetupDeps().basePath.set(fakeRequest, `/s/${spaceId}`);
	}

		@@ -10,41 +10,60 @@ describe('getSpaceIdFromPath', () => {
		describe('without a serverBasePath defined', () => {

Conversation

legrego commented Sep 9, 2020

Summary

Uh oh!

legrego Sep 24, 2020

Choose a reason for hiding this comment

Uh oh!

legrego Sep 24, 2020

Choose a reason for hiding this comment

Uh oh!

legrego Sep 24, 2020

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Oct 14, 2020

Uh oh!

jportner left a comment

Choose a reason for hiding this comment

Uh oh!

jportner Oct 15, 2020

Choose a reason for hiding this comment

Uh oh!

legrego Oct 16, 2020

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Oct 16, 2020

💚 Build Succeeded

Metrics [docs]

page load bundle size

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants