Changes for additional PR review feedback

x-pack/plugins/security/common/constants.ts

@@ -4,6 +4,16 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
+/**
+ * The identifier in a saved object's `namespaces` array when it is shared globally to all spaces.
+ */
+export const ALL_SPACES_ID = '*';
+
+/**
+ * The identifier in a saved object's `namespaces` array when it is shared to an unknown space (e.g., one that the end user is not authorized to see).
+ */
+export const UNKNOWN_SPACE = '?';
+
 export const GLOBAL_RESOURCE = '*';
 export const APPLICATION_PREFIX = 'kibana-';
 export const RESERVED_PRIVILEGES_APPLICATION_WILDCARD = 'kibana-*';

x-pack/plugins/security/server/authorization/check_saved_objects_privileges.test.ts

@@ -95,6 +95,38 @@ describe('#checkSavedObjectsPrivileges', () => {
       ];
       expect(mockCheckPrivileges.atSpaces).toHaveBeenCalledWith(spaceIds, { kibana: actions });
     });
+
+    test(`uses checkPrivileges.globally when checking for "all spaces" (*)`, async () => {
+      const expectedResult = Symbol();
+      mockCheckPrivileges.globally.mockReturnValue(expectedResult as any);
+      mockSpacesService = undefined;
+      const checkSavedObjectsPrivileges = createFactory();
+
+      const namespaces = [undefined, 'default', namespace1, namespace1, '*'];
+      const result = await checkSavedObjectsPrivileges(actions, namespaces);
+
+      expect(result).toBe(expectedResult);
+      expect(mockCheckPrivilegesWithRequest).toHaveBeenCalledTimes(1);
+      expect(mockCheckPrivilegesWithRequest).toHaveBeenCalledWith(request);
+      expect(mockCheckPrivileges.globally).toHaveBeenCalledTimes(1);
+      expect(mockCheckPrivileges.globally).toHaveBeenCalledWith({ kibana: actions });
+    });
+
+    test(`uses checkPrivileges.globally when Spaces is disabled`, async () => {
+      const expectedResult = Symbol();
+      mockCheckPrivileges.globally.mockReturnValue(expectedResult as any);
+      mockSpacesService = undefined;
+      const checkSavedObjectsPrivileges = createFactory();
+
+      const namespaces = [undefined, 'default', namespace1, namespace1, '*'];
+      const result = await checkSavedObjectsPrivileges(actions, namespaces);
+
+      expect(result).toBe(expectedResult);
+      expect(mockCheckPrivilegesWithRequest).toHaveBeenCalledTimes(1);
+      expect(mockCheckPrivilegesWithRequest).toHaveBeenCalledWith(request);
+      expect(mockCheckPrivileges.globally).toHaveBeenCalledTimes(1);
+      expect(mockCheckPrivileges.globally).toHaveBeenCalledWith({ kibana: actions });
+    });
   });
 
   describe('when checking a single namespace', () => {
@@ -115,6 +147,21 @@ describe('#checkSavedObjectsPrivileges', () => {
       expect(mockCheckPrivileges.atSpace).toHaveBeenCalledWith(spaceId, { kibana: actions });
     });
 
+    test(`uses checkPrivileges.globally when checking for "all spaces" (*)`, async () => {
+      const expectedResult = Symbol();
+      mockCheckPrivileges.globally.mockReturnValue(expectedResult as any);
+      mockSpacesService = undefined;
+      const checkSavedObjectsPrivileges = createFactory();
+
+      const result = await checkSavedObjectsPrivileges(actions, '*');
+
+      expect(result).toBe(expectedResult);
+      expect(mockCheckPrivilegesWithRequest).toHaveBeenCalledTimes(1);
+      expect(mockCheckPrivilegesWithRequest).toHaveBeenCalledWith(request);
+      expect(mockCheckPrivileges.globally).toHaveBeenCalledTimes(1);
+      expect(mockCheckPrivileges.globally).toHaveBeenCalledWith({ kibana: actions });
+    });
+
     test(`uses checkPrivileges.globally when Spaces is disabled`, async () => {
       const expectedResult = Symbol();
       mockCheckPrivileges.globally.mockReturnValue(expectedResult as any);

x-pack/plugins/security/server/authorization/check_saved_objects_privileges.ts

@@ -5,6 +5,7 @@
  */
 
 import { KibanaRequest } from '../../../../../src/core/server';
+import { ALL_SPACES_ID } from '../../common/constants';
 import { SpacesService } from '../plugin';
 import { CheckPrivilegesWithRequest, CheckPrivilegesResponse } from './types';
 
@@ -33,24 +34,32 @@ export const checkSavedObjectsPrivilegesWithRequestFactory = (
       namespaceOrNamespaces?: string | Array<undefined | string>
     ) {
       const spacesService = getSpacesService();
-      if (!spacesService) {
-        // Spaces disabled, authorizing globally
-        return await checkPrivilegesWithRequest(request).globally({ kibana: actions });
-      } else if (Array.isArray(namespaceOrNamespaces)) {
-        // Spaces enabled, authorizing against multiple spaces
-        if (!namespaceOrNamespaces.length) {
-          throw new Error(`Can't check saved object privileges for 0 namespaces`);
+      const privileges = { kibana: actions };
+
+      if (spacesService) {
+        if (Array.isArray(namespaceOrNamespaces)) {
+          // Spaces enabled, authorizing against multiple spaces
+          if (!namespaceOrNamespaces.length) {
+            throw new Error(`Can't check saved object privileges for 0 namespaces`);
+          }
+          const spaceIds = uniq(
+            namespaceOrNamespaces.map((x) => spacesService.namespaceToSpaceId(x))
+          );
+
+          if (!spaceIds.includes(ALL_SPACES_ID)) {
+            return await checkPrivilegesWithRequest(request).atSpaces(spaceIds, privileges);
+          }
+        } else {
+          // Spaces enabled, authorizing against a single space
+          const spaceId = spacesService.namespaceToSpaceId(namespaceOrNamespaces);
+          if (spaceId !== ALL_SPACES_ID) {
+            return await checkPrivilegesWithRequest(request).atSpace(spaceId, privileges);
+          }
         }
-        const spaceIds = uniq(
-          namespaceOrNamespaces.map((x) => spacesService.namespaceToSpaceId(x))
-        );
-
-        return await checkPrivilegesWithRequest(request).atSpaces(spaceIds, { kibana: actions });
-      } else {
-        // Spaces enabled, authorizing against a single space
-        const spaceId = spacesService.namespaceToSpaceId(namespaceOrNamespaces);
-        return await checkPrivilegesWithRequest(request).atSpace(spaceId, { kibana: actions });
       }
+
+      // Spaces plugin is disabled OR we are checking privileges for "all spaces", authorizing globally
+      return await checkPrivilegesWithRequest(request).globally(privileges);
     };
   };
 };

x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts

@@ -172,7 +172,7 @@ const expectObjectNamespaceFiltering = async (
   );
   expect(clientOpts.checkSavedObjectsPrivilegesAsCurrentUser).toHaveBeenLastCalledWith(
     'login:',
-    namespaces
+    namespaces.filter((x) => x !== '*') // when we check what namespaces to redact, we don't check privileges for '*', only actual space IDs
   );
 };
 

x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts

@@ -18,6 +18,7 @@ import {
   SavedObjectsDeleteFromNamespacesOptions,
   SavedObjectsUtils,
 } from '../../../../../src/core/server';
+import { ALL_SPACES_ID, UNKNOWN_SPACE } from '../../common/constants';
 import { SecurityAuditLogger } from '../audit';
 import { Actions, CheckSavedObjectsPrivileges } from '../authorization';
 import { CheckPrivilegesResponse } from '../authorization/types';
@@ -55,8 +56,6 @@ interface EnsureAuthorizedTypeResult {
   isGloballyAuthorized?: boolean;
 }
 
-const ALL_SPACES_ID = '*';
-
 export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContract {
   private readonly actions: Actions;
   private readonly auditLogger: PublicMethodsOf<SecurityAuditLogger>;
@@ -391,7 +390,7 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
 
   private redactAndSortNamespaces(spaceIds: string[], privilegeMap: Record<string, boolean>) {
     return spaceIds
-      .map((x) => (x === ALL_SPACES_ID || privilegeMap[x] ? x : '?'))
+      .map((x) => (x === ALL_SPACES_ID || privilegeMap[x] ? x : UNKNOWN_SPACE))
       .sort(namespaceComparator);
   }
 
@@ -406,7 +405,12 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
       return savedObject;
     }
 
-    const privilegeMap = await this.getNamespacesPrivilegeMap(savedObject.namespaces);
+    const namespaces = savedObject.namespaces.filter((x) => x !== ALL_SPACES_ID); // all users can see the "all spaces" ID
+    if (namespaces.length === 0) {
+      return savedObject;
+    }
+
+    const privilegeMap = await this.getNamespacesPrivilegeMap(namespaces);
 
     return {
       ...savedObject,
@@ -421,7 +425,9 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
       return response;
     }
     const { saved_objects: savedObjects } = response;
-    const namespaces = uniq(savedObjects.flatMap((savedObject) => savedObject.namespaces || []));
+    const namespaces = uniq(
+      savedObjects.flatMap((savedObject) => savedObject.namespaces || [])
+    ).filter((x) => x !== ALL_SPACES_ID); // all users can see the "all spaces" ID
     if (namespaces.length === 0) {
       return response;
     }
@@ -454,9 +460,9 @@ function uniq<T>(arr: T[]): T[] {
 function namespaceComparator(a: string, b: string) {
   const A = a.toUpperCase();
   const B = b.toUpperCase();
-  if (A === '?' && B !== '?') {
+  if (A === UNKNOWN_SPACE && B !== UNKNOWN_SPACE) {
     return 1;
-  } else if (A !== '?' && B === '?') {
+  } else if (A !== UNKNOWN_SPACE && B === UNKNOWN_SPACE) {
     return -1;
   }
   return A > B ? 1 : A < B ? -1 : 0;

x-pack/plugins/spaces/common/constants.ts

@@ -6,6 +6,16 @@
 
 export const DEFAULT_SPACE_ID = `default`;
 
+/**
+ * The identifier in a saved object's `namespaces` array when it is shared globally to all spaces.
+ */
+export const ALL_SPACES_ID = '*';
+
+/**
+ * The identifier in a saved object's `namespaces` array when it is shared to an unknown space (e.g., one that the end user is not authorized to see).
+ */
+export const UNKNOWN_SPACE = '?';
+
 /**
  * The minimum number of spaces required to show a search control.
  */

x-pack/plugins/spaces/public/lib/documentation_links.test.ts

@@ -0,0 +1,25 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License;
+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+import { docLinksServiceMock } from '../../../../../src/core/public/mocks';
+import { DocumentationLinksService } from './documentation_links';
+
+describe('DocumentationLinksService', () => {
+  const setup = () => {
+    const docLinks = docLinksServiceMock.createStartContract();
+    const service = new DocumentationLinksService(docLinks);
+    return { docLinks, service };
+  };
+
+  describe('#getKibanaPrivilegesDocUrl', () => {
+    it('returns expected value', () => {
+      const { service } = setup();
+      expect(service.getKibanaPrivilegesDocUrl()).toMatchInlineSnapshot(
+        `"https://www.elastic.co/guide/en/kibana/mocked-test-branch/kibana-privileges.html"`
+      );
+    });
+  });
+});

x-pack/plugins/spaces/public/lib/documentation_links.ts

@@ -0,0 +1,19 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License;
+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+import { DocLinksStart } from 'src/core/public';
+
+export class DocumentationLinksService {
+  private readonly kbn: string;
+
+  constructor(docLinks: DocLinksStart) {
+    this.kbn = `${docLinks.ELASTIC_WEBSITE_URL}guide/en/kibana/${docLinks.DOC_LINK_VERSION}/`;
+  }
+
+  public getKibanaPrivilegesDocUrl() {
+    return `${this.kbn}kibana-privileges.html`;
+  }
+}

x-pack/plugins/spaces/public/lib/index.ts

@@ -0,0 +1,9 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License;
+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+import { DocumentationLinksService } from './documentation_links';
+
+export { DocumentationLinksService };

x-pack/plugins/spaces/public/share_saved_objects_to_space/components/no_spaces_available.tsx

@@ -15,7 +15,7 @@ interface Props {
 
 export const NoSpacesAvailable = (props: Props) => {
   const { capabilities, getUrlForApp } = props.application;
-  const canCreateNewSpaces = capabilities.spaces?.manage;
+  const canCreateNewSpaces = capabilities.spaces.manage;
   if (!canCreateNewSpaces) {
     return null;
   }

x-pack/plugins/spaces/public/share_saved_objects_to_space/components/selectable_spaces_control.tsx

@@ -21,6 +21,8 @@ import { i18n } from '@kbn/i18n';
 import { FormattedMessage } from '@kbn/i18n/react';
 import { CoreStart } from 'src/core/public';
 import { NoSpacesAvailable } from './no_spaces_available';
+import { ALL_SPACES_ID, UNKNOWN_SPACE } from '../../../common/constants';
+import { DocumentationLinksService } from '../../lib';
 import { SpaceAvatar } from '../../space_avatar';
 import { SpaceTarget } from '../types';
 
@@ -33,8 +35,6 @@ interface Props {
 
 type SpaceOption = EuiSelectableOption & { ['data-space-id']: string };
 
-const ALL_SPACES_ID = '*';
-const UNKNOWN_SPACE = '?';
 const ROW_HEIGHT = 40;
 const activeSpaceProps = {
   append: <EuiBadge color="hollow">Current</EuiBadge>,
@@ -77,7 +77,9 @@ export const SelectableSpacesControl = (props: Props) => {
       return null;
     }
 
-    const kibanaPrivilegesUrl = `${docLinks.ELASTIC_WEBSITE_URL}guide/en/kibana/${docLinks.DOC_LINK_VERSION}/kibana-privileges.html`;
+    const kibanaPrivilegesUrl = new DocumentationLinksService(
+      docLinks!
+    ).getKibanaPrivilegesDocUrl();
     return (
       <>
         <EuiSpacer size="xs" />
@@ -102,7 +104,7 @@ export const SelectableSpacesControl = (props: Props) => {
   };
   const getNoSpacesAvailable = () => {
     if (spaces.length < 2) {
-      return <NoSpacesAvailable application={application} />;
+      return <NoSpacesAvailable application={application!} />;
     }
     return null;
   };

x-pack/plugins/spaces/public/share_saved_objects_to_space/components/share_mode_control.tsx

@@ -20,6 +20,7 @@ import {
 import { i18n } from '@kbn/i18n';
 import { CoreStart } from 'src/core/public';
 import { SelectableSpacesControl } from './selectable_spaces_control';
+import { ALL_SPACES_ID } from '../../../common/constants';
 import { SpaceTarget } from '../types';
 
 interface Props {
@@ -31,8 +32,6 @@ interface Props {
   disabled?: boolean;
 }
 
-const ALL_SPACES_ID = '*';
-
 function createLabel({
   title,
   text,

x-pack/plugins/spaces/public/share_saved_objects_to_space/components/share_to_space_flyout.test.tsx

@@ -86,6 +86,12 @@ const setup = async (opts: SetupOpts = {}) => {
   } as SavedObjectsManagementRecord;
 
   const { getStartServices } = coreMock.createSetup();
+  const startServices = coreMock.createStart();
+  startServices.application.capabilities = {
+    ...startServices.application.capabilities,
+    spaces: { manage: true },
+  };
+  getStartServices.mockResolvedValue([startServices, , ,]);
 
   const wrapper = mountWithIntl(
     <ShareSavedObjectsToSpaceFlyout

x-pack/plugins/spaces/public/share_saved_objects_to_space/components/share_to_space_flyout.tsx

@@ -23,6 +23,7 @@ import { i18n } from '@kbn/i18n';
 import { FormattedMessage } from '@kbn/i18n/react';
 import { ToastsStart, StartServicesAccessor, CoreStart } from 'src/core/public';
 import { SavedObjectsManagementRecord } from '../../../../../../src/plugins/saved_objects_management/public';
+import { ALL_SPACES_ID, UNKNOWN_SPACE } from '../../../common/constants';
 import { Space } from '../../../common/model/space';
 import { SpacesManager } from '../../spaces_manager';
 import { ShareToSpaceForm } from './share_to_space_form';
@@ -39,7 +40,6 @@ interface Props {
   getStartServices: StartServicesAccessor<PluginsStart>;
 }
 
-const ALL_SPACES_ID = '*';
 const arraysAreEqual = (a: unknown[], b: unknown[]) =>
   a.every((x) => b.includes(x)) && b.every((x) => a.includes(x));
 
@@ -98,10 +98,10 @@ export const ShareSavedObjectsToSpaceFlyout = (props: Props) => {
       return { isSelectionChanged: false, spacesToAdd: [], spacesToRemove: [] };
     }
     const initialSelection = currentNamespaces.filter(
-      (spaceId) => spaceId !== activeSpace.id && spaceId !== '?'
+      (spaceId) => spaceId !== activeSpace.id && spaceId !== UNKNOWN_SPACE
     );
     const { selectedSpaceIds } = shareOptions;
-    const filteredSelection = selectedSpaceIds.filter((x) => x !== '?');
+    const filteredSelection = selectedSpaceIds.filter((x) => x !== UNKNOWN_SPACE);
     const isSharedToAllSpaces =
       !initialSelection.includes(ALL_SPACES_ID) && filteredSelection.includes(ALL_SPACES_ID);
     const isUnsharedFromAllSpaces =

x-pack/plugins/spaces/public/share_saved_objects_to_space/share_saved_objects_to_space_column.tsx

@@ -13,11 +13,10 @@ import { FormattedMessage } from '@kbn/i18n/react';
 import { SavedObjectsManagementColumn } from '../../../../../src/plugins/saved_objects_management/public';
 import { SpaceTarget } from './types';
 import { SpacesManager } from '../spaces_manager';
+import { ALL_SPACES_ID, UNKNOWN_SPACE } from '../../common/constants';
 import { getSpaceColor } from '..';
 
 const SPACES_DISPLAY_COUNT = 5;
-const ALL_SPACES_ID = '*';
-const UNKNOWN_SPACE = '?';
 
 type SpaceMap = Map<string, SpaceTarget>;
 interface ColumnDataProps {