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

#80815) * Allow the default space to be accessed via /s/default * apply suggestions from code review
elastic · Oct 16, 2020 · c229cc9 · c229cc9
1 parent 59e6011
commit c229cc9
Show file tree

Hide file tree

Showing 11 changed files with 208 additions and 119 deletions.
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
@@ -10,41 +10,76 @@ describe('getSpaceIdFromPath', () => {
   describe('without a serverBasePath defined', () => {
     test('it identifies the space url context', () => {
       const basePath = `/s/my-awesome-space-lives-here`;
-      expect(getSpaceIdFromPath(basePath)).toEqual('my-awesome-space-lives-here');
+      expect(getSpaceIdFromPath(basePath)).toEqual({
+        spaceId: 'my-awesome-space-lives-here',
+        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(DEFAULT_SPACE_ID);
+      expect(getSpaceIdFromPath(basePath)).toEqual({
+        spaceId: DEFAULT_SPACE_ID,
+        pathHasExplicitSpaceIdentifier: false,
+      });
     });
 
     test('it handles base url without a space url context', () => {
       const basePath = `/this/is/a/crazy/path/s`;
-      expect(getSpaceIdFromPath(basePath)).toEqual(DEFAULT_SPACE_ID);
+      expect(getSpaceIdFromPath(basePath)).toEqual({
+        spaceId: DEFAULT_SPACE_ID,
+        pathHasExplicitSpaceIdentifier: false,
+      });
+    });
+
+    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,
+      });
     });
   });
 
   describe('with a serverBasePath defined', () => {
     test('it identifies the space url context', () => {
       const basePath = `/s/my-awesome-space-lives-here`;
-      expect(getSpaceIdFromPath(basePath, '/')).toEqual('my-awesome-space-lives-here');
+      expect(getSpaceIdFromPath(basePath, '/')).toEqual({
+        spaceId: 'my-awesome-space-lives-here',
+        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(
-        'my-awesome-space-lives-here'
-      );
+      expect(getSpaceIdFromPath(basePath, '/server-base-path-here')).toEqual({
+        spaceId: 'my-awesome-space-lives-here',
+        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(DEFAULT_SPACE_ID);
+      expect(getSpaceIdFromPath(basePath, '/this/is/a')).toEqual({
+        spaceId: DEFAULT_SPACE_ID,
+        pathHasExplicitSpaceIdentifier: false,
+      });
+    });
+
+    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('it handles base url without a space url context', () => {
       const basePath = `/this/is/a/crazy/path/s`;
-      expect(getSpaceIdFromPath(basePath, basePath)).toEqual(DEFAULT_SPACE_ID);
+      expect(getSpaceIdFromPath(basePath, basePath)).toEqual({
+        spaceId: DEFAULT_SPACE_ID,
+        pathHasExplicitSpaceIdentifier: false,
+      });
     });
   });
 });

diff --git a/x-pack/plugins/spaces/common/lib/spaces_url_parser.ts b/x-pack/plugins/spaces/common/lib/spaces_url_parser.ts
@@ -5,20 +5,22 @@
  */
 import { DEFAULT_SPACE_ID } from '../constants';
 
+const spaceContextRegex = /^\/s\/([a-z0-9_\-]+)/;
+
 export function getSpaceIdFromPath(
   requestBasePath: string = '/',
   serverBasePath: string = '/'
-): string {
-  let pathToCheck: string = requestBasePath;
+): { spaceId: string; pathHasExplicitSpaceIdentifier: boolean } {
+  const pathToCheck: string = stripServerBasePath(requestBasePath, serverBasePath);
 
-  if (serverBasePath && serverBasePath !== '/' && requestBasePath.startsWith(serverBasePath)) {
-    pathToCheck = requestBasePath.substr(serverBasePath.length);
-  }
   // Look for `/s/space-url-context` in the base path
-  const matchResult = pathToCheck.match(/^\/s\/([a-z0-9_\-]+)/);
+  const matchResult = pathToCheck.match(spaceContextRegex);
 
   if (!matchResult || matchResult.length === 0) {
-    return DEFAULT_SPACE_ID;
+    return {
+      spaceId: DEFAULT_SPACE_ID,
+      pathHasExplicitSpaceIdentifier: false,
+    };
   }
 
   // Ignoring first result, we only want the capture group result at index 1
@@ -28,7 +30,10 @@ export function getSpaceIdFromPath(
     throw new Error(`Unable to determine Space ID from request path: ${requestBasePath}`);
   }
 
-  return spaceId;
+  return {
+    spaceId,
+    pathHasExplicitSpaceIdentifier: true,
+  };
 }
 
 export function addSpaceIdToPath(
@@ -45,3 +50,10 @@ export function addSpaceIdToPath(
   }
   return `${basePath}${requestedPath}`;
 }
+
+function stripServerBasePath(requestBasePath: string, serverBasePath: string) {
+  if (serverBasePath && serverBasePath !== '/' && requestBasePath.startsWith(serverBasePath)) {
+    return requestBasePath.substr(serverBasePath.length);
+  }
+  return requestBasePath;
+}
diff --git a/x-pack/plugins/spaces/server/lib/request_interceptors/on_request_interceptor.ts b/x-pack/plugins/spaces/server/lib/request_interceptors/on_request_interceptor.ts
@@ -10,7 +10,6 @@ import {
   CoreSetup,
 } from 'src/core/server';
 import { format } from 'url';
-import { DEFAULT_SPACE_ID } from '../../../common/constants';
 import { modifyUrl } from '../utils/url';
 import { getSpaceIdFromPath } from '../../../common';
 
@@ -28,9 +27,9 @@ export function initSpacesOnRequestInterceptor({ http }: OnRequestInterceptorDep
 
     // If navigating within the context of a space, then we store the Space's URL Context on the request,
     // and rewrite the request to not include the space identifier in the URL.
-    const spaceId = getSpaceIdFromPath(path, serverBasePath);
+    const { spaceId, pathHasExplicitSpaceIdentifier } = getSpaceIdFromPath(path, serverBasePath);
 
-    if (spaceId !== DEFAULT_SPACE_ID) {
+    if (pathHasExplicitSpaceIdentifier) {
       const reqBasePath = `/s/${spaceId}`;
 
       http.basePath.set(request, reqBasePath);

diff --git a/x-pack/plugins/spaces/server/spaces_service/spaces_service.test.ts b/x-pack/plugins/spaces/server/spaces_service/spaces_service.test.ts
@@ -58,7 +58,7 @@ const createService = async (serverBasePath: string = '') => {
     serverBasePath,
   } as HttpServiceSetup['basePath'];
   httpSetup.basePath.get = jest.fn().mockImplementation((request: KibanaRequest) => {
-    const spaceId = getSpaceIdFromPath(request.url.path);
+    const { spaceId } = getSpaceIdFromPath(request.url.path);
 
     if (spaceId !== DEFAULT_SPACE_ID) {
       return `/s/${spaceId}`;

diff --git a/x-pack/plugins/spaces/server/spaces_service/spaces_service.ts b/x-pack/plugins/spaces/server/spaces_service/spaces_service.ts
@@ -63,7 +63,7 @@ export class SpacesService {
         ? (request as Record<string, any>).getBasePath()
         : http.basePath.get(request);
 
-      const spaceId = getSpaceIdFromPath(basePath, http.basePath.serverBasePath);
+      const { spaceId } = getSpaceIdFromPath(basePath, http.basePath.serverBasePath);
 
       return spaceId;
     };

diff --git a/x-pack/test/api_integration/apis/spaces/get_active_space.ts b/x-pack/test/api_integration/apis/spaces/get_active_space.ts
@@ -35,6 +35,20 @@ export default function ({ getService }: FtrProviderContext) {
       });
     });
 
+    it('returns the default space when explicitly referenced', async () => {
+      await supertest
+        .get('/s/default/internal/spaces/_active_space')
+        .set('kbn-xsrf', 'xxx')
+        .expect(200, {
+          id: 'default',
+          name: 'Default',
+          description: 'This is your default space!',
+          color: '#00bfb3',
+          disabledFeatures: [],
+          _reserved: true,
+        });
+    });
+
     it('returns the foo space', async () => {
       await supertest
         .get('/s/foo-space/internal/spaces/_active_space')

diff --git a/x-pack/test/spaces_api_integration/common/lib/space_test_utils.ts b/x-pack/test/spaces_api_integration/common/lib/space_test_utils.ts
@@ -13,3 +13,24 @@ export function getUrlPrefix(spaceId?: string) {
 export function getIdPrefix(spaceId?: string) {
   return spaceId === DEFAULT_SPACE_ID ? '' : `${spaceId}-`;
 }
+
+export function getTestScenariosForSpace(spaceId: string) {
+  const explicitScenario = {
+    spaceId,
+    urlPrefix: `/s/${spaceId}`,
+    scenario: `when referencing the ${spaceId} space explicitly in the URL`,
+  };
+
+  if (spaceId === DEFAULT_SPACE_ID) {
+    return [
+      {
+        spaceId,
+        urlPrefix: ``,
+        scenario: 'when referencing the default space implicitly',
+      },
+      explicitScenario,
+    ];
+  }
+
+  return [explicitScenario];
+}
diff --git a/x-pack/test/spaces_api_integration/common/suites/create.ts b/x-pack/test/spaces_api_integration/common/suites/create.ts
@@ -6,7 +6,7 @@
 
 import expect from '@kbn/expect';
 import { SuperTest } from 'supertest';
-import { getUrlPrefix } from '../lib/space_test_utils';
+import { getTestScenariosForSpace } from '../lib/space_test_utils';
 import { DescribeFn, TestDefinitionAuthentication } from '../lib/types';
 
 interface CreateTest {
@@ -67,56 +67,58 @@ export function createTestSuiteFactory(esArchiver: any, supertest: SuperTest<any
     { user = {}, spaceId, tests }: CreateTestDefinition
   ) => {
     describeFn(description, () => {
-      before(() => esArchiver.load('saved_objects/spaces'));
-      after(() => esArchiver.unload('saved_objects/spaces'));
+      beforeEach(() => esArchiver.load('saved_objects/spaces'));
+      afterEach(() => esArchiver.unload('saved_objects/spaces'));
 
-      it(`should return ${tests.newSpace.statusCode}`, async () => {
-        return supertest
-          .post(`${getUrlPrefix(spaceId)}/api/spaces/space`)
-          .auth(user.username, user.password)
-          .send({
-            name: 'marketing',
-            id: 'marketing',
-            description: 'a description',
-            color: '#5c5959',
-            disabledFeatures: [],
-          })
-          .expect(tests.newSpace.statusCode)
-          .then(tests.newSpace.response);
-      });
-
-      describe('when it already exists', () => {
-        it(`should return ${tests.alreadyExists.statusCode}`, async () => {
+      getTestScenariosForSpace(spaceId).forEach(({ urlPrefix, scenario }) => {
+        it(`should return ${tests.newSpace.statusCode} ${scenario}`, async () => {
           return supertest
-            .post(`${getUrlPrefix(spaceId)}/api/spaces/space`)
+            .post(`${urlPrefix}/api/spaces/space`)
             .auth(user.username, user.password)
             .send({
-              name: 'space_1',
-              id: 'space_1',
-              color: '#ffffff',
+              name: 'marketing',
+              id: 'marketing',
               description: 'a description',
+              color: '#5c5959',
               disabledFeatures: [],
             })
-            .expect(tests.alreadyExists.statusCode)
-            .then(tests.alreadyExists.response);
+            .expect(tests.newSpace.statusCode)
+            .then(tests.newSpace.response);
         });
-      });
 
-      describe('when _reserved is specified', () => {
-        it(`should return ${tests.reservedSpecified.statusCode} and ignore _reserved`, async () => {
-          return supertest
-            .post(`${getUrlPrefix(spaceId)}/api/spaces/space`)
-            .auth(user.username, user.password)
-            .send({
-              name: 'reserved space',
-              id: 'reserved',
-              description: 'a description',
-              color: '#5c5959',
-              _reserved: true,
-              disabledFeatures: [],
-            })
-            .expect(tests.reservedSpecified.statusCode)
-            .then(tests.reservedSpecified.response);
+        describe('when it already exists', () => {
+          it(`should return ${tests.alreadyExists.statusCode} ${scenario}`, async () => {
+            return supertest
+              .post(`${urlPrefix}/api/spaces/space`)
+              .auth(user.username, user.password)
+              .send({
+                name: 'space_1',
+                id: 'space_1',
+                color: '#ffffff',
+                description: 'a description',
+                disabledFeatures: [],
+              })
+              .expect(tests.alreadyExists.statusCode)
+              .then(tests.alreadyExists.response);
+          });
+        });
+
+        describe('when _reserved is specified', () => {
+          it(`should return ${tests.reservedSpecified.statusCode} and ignore _reserved ${scenario}`, async () => {
+            return supertest
+              .post(`${urlPrefix}/api/spaces/space`)
+              .auth(user.username, user.password)
+              .send({
+                name: 'reserved space',
+                id: 'reserved',
+                description: 'a description',
+                color: '#5c5959',
+                _reserved: true,
+                disabledFeatures: [],
+              })
+              .expect(tests.reservedSpecified.statusCode)
+              .then(tests.reservedSpecified.response);
+          });
         });
       });
     });