From 8d68b1cb22e991f005d11cb02b6bf9c59c4de44f Mon Sep 17 00:00:00 2001 From: Larry Gregory Date: Fri, 14 Feb 2020 08:42:11 -0500 Subject: [PATCH 01/11] remove kibanaIndex from LegacyAPI --- x-pack/legacy/plugins/spaces/index.ts | 5 --- .../server/lib/spaces_usage_collector.test.ts | 21 ++++++++++-- .../server/lib/spaces_usage_collector.ts | 18 ++++------- x-pack/plugins/spaces/server/plugin.ts | 32 +++++++------------ 4 files changed, 36 insertions(+), 40 deletions(-) diff --git a/x-pack/legacy/plugins/spaces/index.ts b/x-pack/legacy/plugins/spaces/index.ts index ab3388ae96475..9a11f92ffb402 100644 --- a/x-pack/legacy/plugins/spaces/index.ts +++ b/x-pack/legacy/plugins/spaces/index.ts @@ -110,14 +110,9 @@ export const spaces = (kibana: Record) => throw new Error('New Platform XPack Spaces plugin is not available.'); } - const config = server.config(); - const { registerLegacyAPI, createDefaultSpace } = spacesPlugin.__legacyCompat; registerLegacyAPI({ - legacyConfig: { - kibanaIndex: config.get('kibana.index'), - }, savedObjects: server.savedObjects, auditLogger: { create: (pluginId: string) => diff --git a/x-pack/plugins/spaces/server/lib/spaces_usage_collector.test.ts b/x-pack/plugins/spaces/server/lib/spaces_usage_collector.test.ts index b343bac9343a3..aa1b38d009825 100644 --- a/x-pack/plugins/spaces/server/lib/spaces_usage_collector.test.ts +++ b/x-pack/plugins/spaces/server/lib/spaces_usage_collector.test.ts @@ -9,6 +9,7 @@ import * as Rx from 'rxjs'; import { PluginsSetup } from '../plugin'; import { Feature } from '../../../features/server'; import { ILicense, LicensingPluginSetup } from '../../../licensing/server'; +import { pluginInitializerContextConfigMock } from 'src/core/server/mocks'; interface SetupOpts { license?: Partial; @@ -73,11 +74,25 @@ describe('with a basic license', () => { license: { isAvailable: true, type: 'basic' }, }); const { fetch: getSpacesUsage } = getSpacesUsageCollector(usageCollecion as any, { - kibanaIndex: '.kibana', + kibanaIndexConfig: pluginInitializerContextConfigMock({}).legacy.globalConfig$, features, licensing, }); usageStats = await getSpacesUsage(defaultCallClusterMock); + + expect(defaultCallClusterMock).toHaveBeenCalledWith('search', { + body: { + aggs: { + disabledFeatures: { + terms: { field: 'space.disabledFeatures', include: ['feature1', 'feature2'], size: 2 }, + }, + }, + query: { term: { type: { value: 'space' } } }, + size: 0, + track_total_hits: true, + }, + index: '.kibana-tests', + }); }); test('sets enabled to true', () => { @@ -107,7 +122,7 @@ describe('with no license', () => { beforeAll(async () => { const { features, licensing, usageCollecion } = setup({ license: { isAvailable: false } }); const { fetch: getSpacesUsage } = getSpacesUsageCollector(usageCollecion as any, { - kibanaIndex: '.kibana', + kibanaIndexConfig: pluginInitializerContextConfigMock({}).legacy.globalConfig$, features, licensing, }); @@ -138,7 +153,7 @@ describe('with platinum license', () => { license: { isAvailable: true, type: 'platinum' }, }); const { fetch: getSpacesUsage } = getSpacesUsageCollector(usageCollecion as any, { - kibanaIndex: '.kibana', + kibanaIndexConfig: pluginInitializerContextConfigMock({}).legacy.globalConfig$, features, licensing, }); diff --git a/x-pack/plugins/spaces/server/lib/spaces_usage_collector.ts b/x-pack/plugins/spaces/server/lib/spaces_usage_collector.ts index eb6843cfe4538..c169b36a42d59 100644 --- a/x-pack/plugins/spaces/server/lib/spaces_usage_collector.ts +++ b/x-pack/plugins/spaces/server/lib/spaces_usage_collector.ts @@ -4,11 +4,10 @@ * you may not use this file except in compliance with the Elastic License. */ -import { get } from 'lodash'; import { CallAPIOptions } from 'src/core/server'; import { take } from 'rxjs/operators'; import { UsageCollectionSetup } from 'src/plugins/usage_collection/server'; -// @ts-ignore +import { Observable } from 'rxjs'; import { KIBANA_STATS_TYPE_MONITORING } from '../../../../legacy/plugins/monitoring/common/constants'; import { KIBANA_SPACES_STATS_TYPE } from '../../common/constants'; import { PluginsSetup } from '../plugin'; @@ -76,8 +75,8 @@ async function getSpacesUsage( const { hits, aggregations } = resp; - const count = get(hits, 'total.value', 0); - const disabledFeatureBuckets = get(aggregations, 'disabledFeatures.buckets', []); + const count = hits?.total?.value ?? 0; + const disabledFeatureBuckets = aggregations?.disabledFeatures?.buckets ?? []; const initialCounts = knownFeatureIds.reduce( (acc, featureId) => ({ ...acc, [featureId]: 0 }), @@ -116,7 +115,7 @@ export interface UsageStats { } interface CollectorDeps { - kibanaIndex: string; + kibanaIndexConfig: Observable<{ kibana: { index: string } }>; features: PluginsSetup['features']; licensing: PluginsSetup['licensing']; } @@ -136,12 +135,9 @@ export function getSpacesUsageCollector( const license = await deps.licensing.license$.pipe(take(1)).toPromise(); const available = license.isAvailable; // some form of spaces is available for all valid licenses - const usageStats = await getSpacesUsage( - callCluster, - deps.kibanaIndex, - deps.features, - available - ); + const kibanaIndex = (await deps.kibanaIndexConfig.toPromise()).kibana.index; + + const usageStats = await getSpacesUsage(callCluster, kibanaIndex, deps.features, available); return { available, diff --git a/x-pack/plugins/spaces/server/plugin.ts b/x-pack/plugins/spaces/server/plugin.ts index 90c2da6e69df8..e4800213a2ad9 100644 --- a/x-pack/plugins/spaces/server/plugin.ts +++ b/x-pack/plugins/spaces/server/plugin.ts @@ -41,9 +41,6 @@ export interface LegacyAPI { auditLogger: { create: (pluginId: string) => AuditLogger; }; - legacyConfig: { - kibanaIndex: string; - }; } export interface PluginsSetup { @@ -70,6 +67,8 @@ export class Plugin { private readonly config$: Observable; + private readonly kibanaIndexConfig: Observable<{ kibana: { index: string } }>; + private readonly log: Logger; private legacyAPI?: LegacyAPI; @@ -92,6 +91,7 @@ export class Plugin { constructor(initializerContext: PluginInitializerContext) { this.config$ = initializerContext.config.create(); + this.kibanaIndexConfig = initializerContext.config.legacy.globalConfig$; this.log = initializerContext.logger.get(); } @@ -146,6 +146,12 @@ export class Plugin { } }); + registerSpacesUsageCollector(plugins.usageCollection, { + kibanaIndexConfig: this.kibanaIndexConfig, + features: plugins.features, + licensing: plugins.licensing, + }); + if (plugins.security) { plugins.security.registerSpacesService(spacesService); } @@ -161,12 +167,7 @@ export class Plugin { __legacyCompat: { registerLegacyAPI: (legacyAPI: LegacyAPI) => { this.legacyAPI = legacyAPI; - this.setupLegacyComponents( - spacesService, - plugins.features, - plugins.licensing, - plugins.usageCollection - ); + this.setupLegacyComponents(spacesService); }, createDefaultSpace: async () => { return await createDefaultSpace({ @@ -180,12 +181,7 @@ export class Plugin { public stop() {} - private setupLegacyComponents( - spacesService: SpacesServiceSetup, - featuresSetup: FeaturesPluginSetup, - licensingSetup: LicensingPluginSetup, - usageCollectionSetup?: UsageCollectionSetup - ) { + private setupLegacyComponents(spacesService: SpacesServiceSetup) { const legacyAPI = this.getLegacyAPI(); const { addScopedSavedObjectsClientWrapperFactory, types } = legacyAPI.savedObjects; addScopedSavedObjectsClientWrapperFactory( @@ -193,11 +189,5 @@ export class Plugin { 'spaces', spacesSavedObjectsClientWrapperFactory(spacesService, types) ); - // Register a function with server to manage the collection of usage stats - registerSpacesUsageCollector(usageCollectionSetup, { - kibanaIndex: legacyAPI.legacyConfig.kibanaIndex, - features: featuresSetup, - licensing: licensingSetup, - }); } } From fe13a9a8e63c61ff973e79cb515f0e8b9582106a Mon Sep 17 00:00:00 2001 From: Larry Gregory Date: Fri, 14 Feb 2020 09:50:47 -0500 Subject: [PATCH 02/11] moving capabilities, adding tests --- src/plugins/usage_collection/server/mocks.ts | 41 +++++++++++ x-pack/plugins/features/server/mocks.ts | 27 +++++++ x-pack/plugins/features/server/plugin.ts | 23 ++++-- .../capabilities_provider.test.ts | 24 +++++++ .../capabilities/capabilities_provider.ts | 16 +++++ .../capabilities_switcher.test.ts} | 60 +++++++++++++--- .../capabilities_switcher.ts} | 30 ++++++-- .../spaces/server/capabilities/index.ts | 19 +++++ x-pack/plugins/spaces/server/plugin.test.ts | 72 +++++++++++++++++++ x-pack/plugins/spaces/server/plugin.ts | 17 ++--- 10 files changed, 293 insertions(+), 36 deletions(-) create mode 100644 src/plugins/usage_collection/server/mocks.ts create mode 100644 x-pack/plugins/features/server/mocks.ts create mode 100644 x-pack/plugins/spaces/server/capabilities/capabilities_provider.test.ts create mode 100644 x-pack/plugins/spaces/server/capabilities/capabilities_provider.ts rename x-pack/plugins/spaces/server/{lib/toggle_ui_capabilities.test.ts => capabilities/capabilities_switcher.test.ts} (67%) rename x-pack/plugins/spaces/server/{lib/toggle_ui_capabilities.ts => capabilities/capabilities_switcher.ts} (72%) create mode 100644 x-pack/plugins/spaces/server/capabilities/index.ts create mode 100644 x-pack/plugins/spaces/server/plugin.test.ts diff --git a/src/plugins/usage_collection/server/mocks.ts b/src/plugins/usage_collection/server/mocks.ts new file mode 100644 index 0000000000000..91f643738083a --- /dev/null +++ b/src/plugins/usage_collection/server/mocks.ts @@ -0,0 +1,41 @@ +/* + * Licensed to Elasticsearch B.V. under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch B.V. licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +// eslint-disable-next-line @kbn/eslint/no-restricted-paths +import { loggingServiceMock } from 'src/core/server/mocks'; +import { UsageCollectionSetup } from './plugin'; +import { CollectorSet } from './collector'; + +const createSetupContract = () => { + const setupContract = { + ...new CollectorSet({ + logger: loggingServiceMock.createLogger(), + maximumWaitTimeForAllCollectorsInS: 1, + }), + registerLegacySavedObjects: jest.fn() as jest.Mocked< + UsageCollectionSetup['registerLegacySavedObjects'] + >, + } as UsageCollectionSetup; + + return setupContract; +}; + +export const usageCollectionPluginMock = { + createSetupContract, +}; diff --git a/x-pack/plugins/features/server/mocks.ts b/x-pack/plugins/features/server/mocks.ts new file mode 100644 index 0000000000000..ebaa5f1a504ca --- /dev/null +++ b/x-pack/plugins/features/server/mocks.ts @@ -0,0 +1,27 @@ +/* + * 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 { PluginSetupContract, PluginStartContract } from './plugin'; + +const createSetup = (): jest.Mocked => { + return { + getFeatures: jest.fn(), + getFeaturesUICapabilities: jest.fn(), + registerFeature: jest.fn(), + registerLegacyAPI: jest.fn(), + }; +}; + +const createStart = (): jest.Mocked => { + return { + getFeatures: jest.fn(), + }; +}; + +export const featuresPluginMock = { + createSetup, + createStart, +}; diff --git a/x-pack/plugins/features/server/plugin.ts b/x-pack/plugins/features/server/plugin.ts index 96a8e68f8326d..755d8d9cfad06 100644 --- a/x-pack/plugins/features/server/plugin.ts +++ b/x-pack/plugins/features/server/plugin.ts @@ -30,6 +30,10 @@ export interface PluginSetupContract { registerLegacyAPI: (legacyAPI: LegacyAPI) => void; } +export interface PluginStartContract { + getFeatures(): Feature[]; +} + /** * Describes a set of APIs that are available in the legacy platform only and required by this plugin * to function properly. @@ -45,6 +49,8 @@ export interface LegacyAPI { export class Plugin { private readonly logger: Logger; + private featureRegistry!: FeatureRegistry; + private legacyAPI?: LegacyAPI; private readonly getLegacyAPI = () => { if (!this.legacyAPI) { @@ -61,18 +67,18 @@ export class Plugin { core: CoreSetup, { timelion }: { timelion?: TimelionSetupContract } ): Promise> { - const featureRegistry = new FeatureRegistry(); + this.featureRegistry = new FeatureRegistry(); defineRoutes({ router: core.http.createRouter(), - featureRegistry, + featureRegistry: this.featureRegistry, getLegacyAPI: this.getLegacyAPI, }); return deepFreeze({ - registerFeature: featureRegistry.register.bind(featureRegistry), - getFeatures: featureRegistry.getAll.bind(featureRegistry), - getFeaturesUICapabilities: () => uiCapabilitiesForFeatures(featureRegistry.getAll()), + registerFeature: this.featureRegistry.register.bind(this.featureRegistry), + getFeatures: this.featureRegistry.getAll.bind(this.featureRegistry), + getFeaturesUICapabilities: () => uiCapabilitiesForFeatures(this.featureRegistry.getAll()), registerLegacyAPI: (legacyAPI: LegacyAPI) => { this.legacyAPI = legacyAPI; @@ -82,14 +88,17 @@ export class Plugin { savedObjectTypes: this.legacyAPI.savedObjectTypes, includeTimelion: timelion !== undefined && timelion.uiEnabled, })) { - featureRegistry.register(feature); + this.featureRegistry.register(feature); } }, }); } - public start() { + public start(): RecursiveReadonly { this.logger.debug('Starting plugin'); + return { + getFeatures: this.featureRegistry.getAll.bind(this.featureRegistry), + }; } public stop() { diff --git a/x-pack/plugins/spaces/server/capabilities/capabilities_provider.test.ts b/x-pack/plugins/spaces/server/capabilities/capabilities_provider.test.ts new file mode 100644 index 0000000000000..8678bdceb70f9 --- /dev/null +++ b/x-pack/plugins/spaces/server/capabilities/capabilities_provider.test.ts @@ -0,0 +1,24 @@ +/* + * 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 { capabilitiesProvider } from './capabilities_provider'; + +describe('Capabilities provider', () => { + it('provides the expected capabilities', () => { + expect(capabilitiesProvider()).toMatchInlineSnapshot(` + Object { + "management": Object { + "kibana": Object { + "spaces": true, + }, + }, + "spaces": Object { + "manage": true, + }, + } + `); + }); +}); diff --git a/x-pack/plugins/spaces/server/capabilities/capabilities_provider.ts b/x-pack/plugins/spaces/server/capabilities/capabilities_provider.ts new file mode 100644 index 0000000000000..5976aabfa66e8 --- /dev/null +++ b/x-pack/plugins/spaces/server/capabilities/capabilities_provider.ts @@ -0,0 +1,16 @@ +/* + * 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. + */ + +export const capabilitiesProvider = () => ({ + spaces: { + manage: true, + }, + management: { + kibana: { + spaces: true, + }, + }, +}); diff --git a/x-pack/plugins/spaces/server/lib/toggle_ui_capabilities.test.ts b/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.test.ts similarity index 67% rename from x-pack/plugins/spaces/server/lib/toggle_ui_capabilities.test.ts rename to x-pack/plugins/spaces/server/capabilities/capabilities_switcher.test.ts index b92922def2eb8..5cd6e6c7f28a9 100644 --- a/x-pack/plugins/spaces/server/lib/toggle_ui_capabilities.test.ts +++ b/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.test.ts @@ -6,8 +6,12 @@ import { Feature } from '../../../../plugins/features/server'; import { Space } from '../../common/model/space'; -import { toggleUICapabilities } from './toggle_ui_capabilities'; -import { Capabilities } from 'src/core/public'; +import { setupCapabilitiesSwitcher } from './capabilities_switcher'; +import { Capabilities, CoreSetup } from 'src/core/server'; +import { coreMock, httpServerMock } from 'src/core/server/mocks'; +import { featuresPluginMock } from '../../../features/server/mocks'; +import { spacesServiceMock } from '../spaces_service/spaces_service.mock'; +import { PluginsSetup } from '../plugin'; const features: Feature[] = [ { @@ -91,8 +95,28 @@ const buildCapabilities = () => }, }) as Capabilities; -describe('toggleUiCapabilities', () => { - it('does not toggle capabilities when the space has no disabled features', () => { +const setup = (space: Space) => { + const coreSetup = coreMock.createSetup(); + + const featuresStart = featuresPluginMock.createStart(); + featuresStart.getFeatures.mockReturnValue(features); + + coreSetup.getStartServices.mockResolvedValue([ + coreMock.createStart(), + { features: featuresStart }, + ]); + + const spacesService = spacesServiceMock.createSetupContract(); + spacesService.getActiveSpace.mockResolvedValue(space); + + return setupCapabilitiesSwitcher( + (coreSetup as unknown) as CoreSetup, + spacesService + ); +}; + +describe('capabilitiesSwitcher', () => { + it('does not toggle capabilities when the space has no disabled features', async () => { const space: Space = { id: 'space', name: '', @@ -100,11 +124,15 @@ describe('toggleUiCapabilities', () => { }; const capabilities = buildCapabilities(); - const result = toggleUICapabilities(features, capabilities, space); + + const switcher = setup(space); + const request = httpServerMock.createKibanaRequest(); + const result = await switcher(request, capabilities); + expect(result).toEqual(buildCapabilities()); }); - it('ignores unknown disabledFeatures', () => { + it('ignores unknown disabledFeatures', async () => { const space: Space = { id: 'space', name: '', @@ -112,11 +140,15 @@ describe('toggleUiCapabilities', () => { }; const capabilities = buildCapabilities(); - const result = toggleUICapabilities(features, capabilities, space); + + const switcher = setup(space); + const request = httpServerMock.createKibanaRequest(); + const result = await switcher(request, capabilities); + expect(result).toEqual(buildCapabilities()); }); - it('disables the corresponding navLink, catalogue, management sections, and all capability flags for disabled features', () => { + it('disables the corresponding navLink, catalogue, management sections, and all capability flags for disabled features', async () => { const space: Space = { id: 'space', name: '', @@ -124,7 +156,10 @@ describe('toggleUiCapabilities', () => { }; const capabilities = buildCapabilities(); - const result = toggleUICapabilities(features, capabilities, space); + + const switcher = setup(space); + const request = httpServerMock.createKibanaRequest(); + const result = await switcher(request, capabilities); const expectedCapabilities = buildCapabilities(); @@ -137,7 +172,7 @@ describe('toggleUiCapabilities', () => { expect(result).toEqual(expectedCapabilities); }); - it('can disable everything', () => { + it('can disable everything', async () => { const space: Space = { id: 'space', name: '', @@ -145,7 +180,10 @@ describe('toggleUiCapabilities', () => { }; const capabilities = buildCapabilities(); - const result = toggleUICapabilities(features, capabilities, space); + + const switcher = setup(space); + const request = httpServerMock.createKibanaRequest(); + const result = await switcher(request, capabilities); const expectedCapabilities = buildCapabilities(); diff --git a/x-pack/plugins/spaces/server/lib/toggle_ui_capabilities.ts b/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.ts similarity index 72% rename from x-pack/plugins/spaces/server/lib/toggle_ui_capabilities.ts rename to x-pack/plugins/spaces/server/capabilities/capabilities_switcher.ts index 2de84ec05017b..0013313fc29da 100644 --- a/x-pack/plugins/spaces/server/lib/toggle_ui_capabilities.ts +++ b/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.ts @@ -4,15 +4,31 @@ * you may not use this file except in compliance with the Elastic License. */ import _ from 'lodash'; -import { UICapabilities } from 'ui/capabilities'; +import { Capabilities, CapabilitiesSwitcher, CoreSetup } from 'src/core/server'; import { Feature } from '../../../../plugins/features/server'; import { Space } from '../../common/model/space'; +import { SpacesServiceSetup } from '../spaces_service'; +import { PluginsSetup } from '../plugin'; -export function toggleUICapabilities( - features: Feature[], - capabilities: UICapabilities, - activeSpace: Space -) { +export function setupCapabilitiesSwitcher( + core: CoreSetup, + spacesService: SpacesServiceSetup +): CapabilitiesSwitcher { + return async (request, capabilities) => { + const [activeSpace, startServices] = await Promise.all([ + spacesService.getActiveSpace(request), + core.getStartServices(), + ]); + + const [, pluginsStart] = startServices; + + const features = pluginsStart.features.getFeatures(); + + return toggleCapabilities(features, capabilities, activeSpace); + }; +} + +function toggleCapabilities(features: Feature[], capabilities: Capabilities, activeSpace: Space) { const clonedCapabilities = _.cloneDeep(capabilities); toggleDisabledFeatures(features, clonedCapabilities, activeSpace); @@ -22,7 +38,7 @@ export function toggleUICapabilities( function toggleDisabledFeatures( features: Feature[], - capabilities: UICapabilities, + capabilities: Capabilities, activeSpace: Space ) { const disabledFeatureKeys = activeSpace.disabledFeatures; diff --git a/x-pack/plugins/spaces/server/capabilities/index.ts b/x-pack/plugins/spaces/server/capabilities/index.ts new file mode 100644 index 0000000000000..2a7117f3a58b3 --- /dev/null +++ b/x-pack/plugins/spaces/server/capabilities/index.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 { CoreSetup } from 'src/core/server'; +import { capabilitiesProvider } from './capabilities_provider'; +import { setupCapabilitiesSwitcher } from './capabilities_switcher'; +import { PluginsSetup } from '../plugin'; +import { SpacesServiceSetup } from '../spaces_service'; + +export const setupCapabilities = ( + core: CoreSetup, + spacesService: SpacesServiceSetup +) => { + core.capabilities.registerProvider(capabilitiesProvider); + core.capabilities.registerSwitcher(setupCapabilitiesSwitcher(core, spacesService)); +}; diff --git a/x-pack/plugins/spaces/server/plugin.test.ts b/x-pack/plugins/spaces/server/plugin.test.ts new file mode 100644 index 0000000000000..4e3f4f52cbeb4 --- /dev/null +++ b/x-pack/plugins/spaces/server/plugin.test.ts @@ -0,0 +1,72 @@ +/* + * 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 { CoreSetup } from 'src/core/server'; +import { coreMock } from 'src/core/server/mocks'; +import { featuresPluginMock } from '../../features/server/mocks'; +import { licensingMock } from '../../licensing/server/mocks'; +import { Plugin, PluginsSetup } from './plugin'; +import { usageCollectionPluginMock } from '../../../../src/plugins/usage_collection/server/mocks'; + +describe('Spaces Plugin', () => { + describe('#setup', () => { + it('can setup with all optional plugins disabled, exposing the expected contract', async () => { + const initializerContext = coreMock.createPluginInitializerContext({}); + const core = coreMock.createSetup() as CoreSetup; + const features = featuresPluginMock.createSetup(); + const licensing = licensingMock.createSetup(); + + const plugin = new Plugin(initializerContext); + const spacesSetup = await plugin.setup(core, { features, licensing }); + expect(spacesSetup).toMatchInlineSnapshot(` + Object { + "__legacyCompat": Object { + "createDefaultSpace": [Function], + "registerLegacyAPI": [Function], + }, + "spacesService": Object { + "getActiveSpace": [Function], + "getBasePath": [Function], + "getSpaceId": [Function], + "isInDefaultSpace": [Function], + "namespaceToSpaceId": [Function], + "scopedClient": [Function], + "spaceIdToNamespace": [Function], + }, + } + `); + }); + + it('registers the capabilities provider and switcher', async () => { + const initializerContext = coreMock.createPluginInitializerContext({}); + const core = coreMock.createSetup() as CoreSetup; + const features = featuresPluginMock.createSetup(); + const licensing = licensingMock.createSetup(); + + const plugin = new Plugin(initializerContext); + + await plugin.setup(core, { features, licensing }); + + expect(core.capabilities.registerProvider).toHaveBeenCalledTimes(1); + expect(core.capabilities.registerSwitcher).toHaveBeenCalledTimes(1); + }); + + it('registers the usage collector', async () => { + const initializerContext = coreMock.createPluginInitializerContext({}); + const core = coreMock.createSetup() as CoreSetup; + const features = featuresPluginMock.createSetup(); + const licensing = licensingMock.createSetup(); + + const usageCollection = usageCollectionPluginMock.createSetupContract(); + + const plugin = new Plugin(initializerContext); + + await plugin.setup(core, { features, licensing, usageCollection }); + + expect(usageCollection.getCollectorByType('spaces')).toBeDefined(); + }); + }); +}); diff --git a/x-pack/plugins/spaces/server/plugin.ts b/x-pack/plugins/spaces/server/plugin.ts index e4800213a2ad9..2e4e9aa599598 100644 --- a/x-pack/plugins/spaces/server/plugin.ts +++ b/x-pack/plugins/spaces/server/plugin.ts @@ -26,11 +26,11 @@ import { registerSpacesUsageCollector } from './lib/spaces_usage_collector'; import { SpacesService } from './spaces_service'; import { SpacesServiceSetup } from './spaces_service'; import { ConfigType } from './config'; -import { toggleUICapabilities } from './lib/toggle_ui_capabilities'; import { initSpacesRequestInterceptors } from './lib/request_interceptors'; import { initExternalSpacesApi } from './routes/api/external'; import { initInternalSpacesApi } from './routes/api/internal'; import { initSpacesViewsRoutes } from './routes/views'; +import { setupCapabilities } from './capabilities'; /** * Describes a set of APIs that is available in the legacy platform only and required by this plugin @@ -97,7 +97,10 @@ export class Plugin { public async start() {} - public async setup(core: CoreSetup, plugins: PluginsSetup): Promise { + public async setup( + core: CoreSetup, + plugins: PluginsSetup + ): Promise { const service = new SpacesService(this.log, this.getLegacyAPI); const spacesService = await service.setup({ @@ -136,15 +139,7 @@ export class Plugin { features: plugins.features, }); - core.capabilities.registerSwitcher(async (request, uiCapabilities) => { - try { - const activeSpace = await spacesService.getActiveSpace(request); - const features = plugins.features.getFeatures(); - return toggleUICapabilities(features, uiCapabilities, activeSpace); - } catch (e) { - return uiCapabilities; - } - }); + setupCapabilities(core, spacesService); registerSpacesUsageCollector(plugins.usageCollection, { kibanaIndexConfig: this.kibanaIndexConfig, From 33e48e7e0c8f3aef35d53402ceb77bea523da532 Mon Sep 17 00:00:00 2001 From: Larry Gregory Date: Fri, 14 Feb 2020 09:52:43 -0500 Subject: [PATCH 03/11] moving usage collection --- x-pack/plugins/spaces/server/plugin.ts | 2 +- x-pack/plugins/spaces/server/usage_collection/index.ts | 7 +++++++ .../spaces_usage_collector.test.ts | 0 .../{lib => usage_collection}/spaces_usage_collector.ts | 0 4 files changed, 8 insertions(+), 1 deletion(-) create mode 100644 x-pack/plugins/spaces/server/usage_collection/index.ts rename x-pack/plugins/spaces/server/{lib => usage_collection}/spaces_usage_collector.test.ts (100%) rename x-pack/plugins/spaces/server/{lib => usage_collection}/spaces_usage_collector.ts (100%) diff --git a/x-pack/plugins/spaces/server/plugin.ts b/x-pack/plugins/spaces/server/plugin.ts index 2e4e9aa599598..766c8d9f2815f 100644 --- a/x-pack/plugins/spaces/server/plugin.ts +++ b/x-pack/plugins/spaces/server/plugin.ts @@ -22,7 +22,7 @@ import { AuditLogger } from '../../../../server/lib/audit_logger'; import { spacesSavedObjectsClientWrapperFactory } from './lib/saved_objects_client/saved_objects_client_wrapper_factory'; import { SpacesAuditLogger } from './lib/audit_logger'; import { createSpacesTutorialContextFactory } from './lib/spaces_tutorial_context_factory'; -import { registerSpacesUsageCollector } from './lib/spaces_usage_collector'; +import { registerSpacesUsageCollector } from './usage_collection'; import { SpacesService } from './spaces_service'; import { SpacesServiceSetup } from './spaces_service'; import { ConfigType } from './config'; diff --git a/x-pack/plugins/spaces/server/usage_collection/index.ts b/x-pack/plugins/spaces/server/usage_collection/index.ts new file mode 100644 index 0000000000000..01df2b815f5ff --- /dev/null +++ b/x-pack/plugins/spaces/server/usage_collection/index.ts @@ -0,0 +1,7 @@ +/* + * 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. + */ + +export { registerSpacesUsageCollector } from './spaces_usage_collector'; diff --git a/x-pack/plugins/spaces/server/lib/spaces_usage_collector.test.ts b/x-pack/plugins/spaces/server/usage_collection/spaces_usage_collector.test.ts similarity index 100% rename from x-pack/plugins/spaces/server/lib/spaces_usage_collector.test.ts rename to x-pack/plugins/spaces/server/usage_collection/spaces_usage_collector.test.ts diff --git a/x-pack/plugins/spaces/server/lib/spaces_usage_collector.ts b/x-pack/plugins/spaces/server/usage_collection/spaces_usage_collector.ts similarity index 100% rename from x-pack/plugins/spaces/server/lib/spaces_usage_collector.ts rename to x-pack/plugins/spaces/server/usage_collection/spaces_usage_collector.ts From 0dc7685014e97bfa8ccb0c4f1b4430bcca21ecfc Mon Sep 17 00:00:00 2001 From: Larry Gregory Date: Fri, 14 Feb 2020 10:00:09 -0500 Subject: [PATCH 04/11] cleanup --- x-pack/legacy/plugins/spaces/index.ts | 13 ------------- .../on_post_auth_interceptor.test.ts | 1 - .../on_request_interceptor.test.ts | 5 ----- .../request_interceptors/on_request_interceptor.ts | 4 +--- .../lib/spaces_tutorial_context_factory.test.ts | 1 - .../routes/api/__fixtures__/create_legacy_api.ts | 3 --- .../server/spaces_service/spaces_service.test.ts | 1 - 7 files changed, 1 insertion(+), 27 deletions(-) diff --git a/x-pack/legacy/plugins/spaces/index.ts b/x-pack/legacy/plugins/spaces/index.ts index 9a11f92ffb402..757c1eb557c54 100644 --- a/x-pack/legacy/plugins/spaces/index.ts +++ b/x-pack/legacy/plugins/spaces/index.ts @@ -34,19 +34,6 @@ export const spaces = (kibana: Record) => publicDir: resolve(__dirname, 'public'), require: ['kibana', 'elasticsearch', 'xpack_main'], - uiCapabilities() { - return { - spaces: { - manage: true, - }, - management: { - kibana: { - spaces: true, - }, - }, - }; - }, - uiExports: { styleSheetPaths: resolve(__dirname, 'public/index.scss'), managementSections: [], diff --git a/x-pack/plugins/spaces/server/lib/request_interceptors/on_post_auth_interceptor.test.ts b/x-pack/plugins/spaces/server/lib/request_interceptors/on_post_auth_interceptor.test.ts index 92be88b91c652..a9fff5098c1d3 100644 --- a/x-pack/plugins/spaces/server/lib/request_interceptors/on_post_auth_interceptor.test.ts +++ b/x-pack/plugins/spaces/server/lib/request_interceptors/on_post_auth_interceptor.test.ts @@ -201,7 +201,6 @@ describe('onPostAuthInterceptor', () => { // interceptor to parse out the space id and rewrite the request's URL. Rather than duplicating that logic, // we are including the already tested interceptor here in the test chain. initSpacesOnRequestInterceptor({ - getLegacyAPI: () => legacyAPI, http: (http as unknown) as CoreSetup['http'], }); diff --git a/x-pack/plugins/spaces/server/lib/request_interceptors/on_request_interceptor.test.ts b/x-pack/plugins/spaces/server/lib/request_interceptors/on_request_interceptor.test.ts index 5e6cf67ee8c90..448bc39eb606e 100644 --- a/x-pack/plugins/spaces/server/lib/request_interceptors/on_request_interceptor.test.ts +++ b/x-pack/plugins/spaces/server/lib/request_interceptors/on_request_interceptor.test.ts @@ -16,7 +16,6 @@ import { } from '../../../../../../src/core/server'; import * as kbnTestServer from '../../../../../../src/test_utils/kbn_server'; -import { LegacyAPI } from '../../plugin'; import { elasticsearchServiceMock } from 'src/core/server/mocks'; describe('onRequestInterceptor', () => { @@ -110,10 +109,6 @@ describe('onRequestInterceptor', () => { elasticsearch.esNodesCompatibility$ = elasticsearchServiceMock.createInternalSetup().esNodesCompatibility$; initSpacesOnRequestInterceptor({ - getLegacyAPI: () => - ({ - legacyConfig: {}, - } as LegacyAPI), http: (http as unknown) as CoreSetup['http'], }); 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 index 22d704c1b7e13..c59851f8b8061 100644 --- 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 @@ -12,14 +12,12 @@ import { import { format } from 'url'; import { DEFAULT_SPACE_ID } from '../../../common/constants'; import { modifyUrl } from '../utils/url'; -import { LegacyAPI } from '../../plugin'; import { getSpaceIdFromPath } from '../../../common'; export interface OnRequestInterceptorDeps { - getLegacyAPI(): LegacyAPI; http: CoreSetup['http']; } -export function initSpacesOnRequestInterceptor({ getLegacyAPI, http }: OnRequestInterceptorDeps) { +export function initSpacesOnRequestInterceptor({ http }: OnRequestInterceptorDeps) { http.registerOnPreAuth(async function spacesOnPreAuthHandler( request: KibanaRequest, response: LifecycleResponseFactory, diff --git a/x-pack/plugins/spaces/server/lib/spaces_tutorial_context_factory.test.ts b/x-pack/plugins/spaces/server/lib/spaces_tutorial_context_factory.test.ts index a3396e98c3512..094ca8a11816e 100644 --- a/x-pack/plugins/spaces/server/lib/spaces_tutorial_context_factory.test.ts +++ b/x-pack/plugins/spaces/server/lib/spaces_tutorial_context_factory.test.ts @@ -23,7 +23,6 @@ import { securityMock } from '../../../security/server/mocks'; const log = loggingServiceMock.createLogger(); const legacyAPI: LegacyAPI = { - legacyConfig: {}, savedObjects: {} as SavedObjectsLegacyService, } as LegacyAPI; diff --git a/x-pack/plugins/spaces/server/routes/api/__fixtures__/create_legacy_api.ts b/x-pack/plugins/spaces/server/routes/api/__fixtures__/create_legacy_api.ts index 812b02e94f591..7765cc3c52e96 100644 --- a/x-pack/plugins/spaces/server/routes/api/__fixtures__/create_legacy_api.ts +++ b/x-pack/plugins/spaces/server/routes/api/__fixtures__/create_legacy_api.ts @@ -100,9 +100,6 @@ export const createLegacyAPI = ({ } as unknown) as jest.Mocked; const legacyAPI: jest.Mocked = { - legacyConfig: { - kibanaIndex: '', - }, auditLogger: {} as any, savedObjects: savedObjectsService, }; 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 index 68d096e046ed4..fc5ff39780524 100644 --- a/x-pack/plugins/spaces/server/spaces_service/spaces_service.test.ts +++ b/x-pack/plugins/spaces/server/spaces_service/spaces_service.test.ts @@ -28,7 +28,6 @@ const mockLogger = loggingServiceMock.createLogger(); const createService = async (serverBasePath: string = '') => { const legacyAPI = { - legacyConfig: {}, savedObjects: ({ getSavedObjectsRepository: jest.fn().mockReturnValue({ get: jest.fn().mockImplementation((type, id) => { From 90c73cbe1e6534a69ff8c721379167376577f926 Mon Sep 17 00:00:00 2001 From: Larry Gregory Date: Fri, 14 Feb 2020 13:01:39 -0500 Subject: [PATCH 05/11] don't toggle capabilities on unauthenticated routes --- src/core/server/http/http_server.mocks.ts | 6 +++++- .../capabilities/capabilities_switcher.test.ts | 17 +++++++++++++++++ .../capabilities/capabilities_switcher.ts | 6 ++++++ 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/src/core/server/http/http_server.mocks.ts b/src/core/server/http/http_server.mocks.ts index 81d756f47d760..7abcdf6c36b4e 100644 --- a/src/core/server/http/http_server.mocks.ts +++ b/src/core/server/http/http_server.mocks.ts @@ -42,6 +42,7 @@ interface RequestFixtureOptions { method?: RouteMethod; socket?: Socket; routeTags?: string[]; + routeAuthRequired?: false; } function createKibanaRequestMock({ @@ -53,6 +54,7 @@ function createKibanaRequestMock({ method = 'get', socket = new Socket(), routeTags, + routeAuthRequired, }: RequestFixtureOptions = {}) { const queryString = stringify(query, { sort: false }); @@ -70,7 +72,9 @@ function createKibanaRequestMock({ query: queryString, search: queryString ? `?${queryString}` : queryString, }, - route: { settings: { tags: routeTags } }, + route: { + settings: { tags: routeTags, auth: routeAuthRequired }, + }, raw: { req: { socket }, }, diff --git a/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.test.ts b/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.test.ts index 5cd6e6c7f28a9..cdee177bcb4f4 100644 --- a/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.test.ts +++ b/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.test.ts @@ -132,6 +132,23 @@ describe('capabilitiesSwitcher', () => { expect(result).toEqual(buildCapabilities()); }); + it('does not toggle capabilities when the request is not authenticated', async () => { + const space: Space = { + id: 'space', + name: '', + disabledFeatures: ['feature_1', 'feature_2', 'feature_3'], + }; + + const capabilities = buildCapabilities(); + + const switcher = setup(space); + const request = httpServerMock.createKibanaRequest({ routeAuthRequired: false }); + + const result = await switcher(request, capabilities); + + expect(result).toEqual(buildCapabilities()); + }); + it('ignores unknown disabledFeatures', async () => { const space: Space = { id: 'space', diff --git a/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.ts b/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.ts index 0013313fc29da..ae4a5bd73ddb8 100644 --- a/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.ts +++ b/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.ts @@ -15,6 +15,12 @@ export function setupCapabilitiesSwitcher( spacesService: SpacesServiceSetup ): CapabilitiesSwitcher { return async (request, capabilities) => { + const isAnonymousRequest = !request.route.options.authRequired; + + if (isAnonymousRequest) { + return capabilities; + } + const [activeSpace, startServices] = await Promise.all([ spacesService.getActiveSpace(request), core.getStartServices(), From 6e5cc69e203254a7a05b70934daeafdbeb7ea381 Mon Sep 17 00:00:00 2001 From: Larry Gregory Date: Fri, 14 Feb 2020 14:26:15 -0500 Subject: [PATCH 06/11] reintroduce exception handling --- .../capabilities_switcher.test.ts | 43 +++++++++++++++---- .../capabilities/capabilities_switcher.ts | 24 +++++++---- .../spaces/server/capabilities/index.ts | 7 +-- x-pack/plugins/spaces/server/plugin.ts | 2 +- 4 files changed, 55 insertions(+), 21 deletions(-) diff --git a/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.test.ts b/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.test.ts index cdee177bcb4f4..866108826ea97 100644 --- a/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.test.ts +++ b/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.test.ts @@ -8,7 +8,7 @@ import { Feature } from '../../../../plugins/features/server'; import { Space } from '../../common/model/space'; import { setupCapabilitiesSwitcher } from './capabilities_switcher'; import { Capabilities, CoreSetup } from 'src/core/server'; -import { coreMock, httpServerMock } from 'src/core/server/mocks'; +import { coreMock, httpServerMock, loggingServiceMock } from 'src/core/server/mocks'; import { featuresPluginMock } from '../../../features/server/mocks'; import { spacesServiceMock } from '../spaces_service/spaces_service.mock'; import { PluginsSetup } from '../plugin'; @@ -109,10 +109,15 @@ const setup = (space: Space) => { const spacesService = spacesServiceMock.createSetupContract(); spacesService.getActiveSpace.mockResolvedValue(space); - return setupCapabilitiesSwitcher( + const logger = loggingServiceMock.createLogger(); + + const switcher = setupCapabilitiesSwitcher( (coreSetup as unknown) as CoreSetup, - spacesService + spacesService, + logger ); + + return { switcher, logger, spacesService }; }; describe('capabilitiesSwitcher', () => { @@ -125,7 +130,7 @@ describe('capabilitiesSwitcher', () => { const capabilities = buildCapabilities(); - const switcher = setup(space); + const { switcher } = setup(space); const request = httpServerMock.createKibanaRequest(); const result = await switcher(request, capabilities); @@ -141,7 +146,7 @@ describe('capabilitiesSwitcher', () => { const capabilities = buildCapabilities(); - const switcher = setup(space); + const { switcher } = setup(space); const request = httpServerMock.createKibanaRequest({ routeAuthRequired: false }); const result = await switcher(request, capabilities); @@ -149,6 +154,28 @@ describe('capabilitiesSwitcher', () => { expect(result).toEqual(buildCapabilities()); }); + it('logs a warning, and does not toggle capabilities if an error is encountered', async () => { + const space: Space = { + id: 'space', + name: '', + disabledFeatures: ['feature_1', 'feature_2', 'feature_3'], + }; + + const capabilities = buildCapabilities(); + + const { switcher, logger, spacesService } = setup(space); + const request = httpServerMock.createKibanaRequest(); + + spacesService.getActiveSpace.mockRejectedValue(new Error('Something terrible happened')); + + const result = await switcher(request, capabilities); + + expect(result).toEqual(buildCapabilities()); + expect(logger.warn).toHaveBeenCalledWith( + `Error toggling capabilities for request to /path: Error: Something terrible happened` + ); + }); + it('ignores unknown disabledFeatures', async () => { const space: Space = { id: 'space', @@ -158,7 +185,7 @@ describe('capabilitiesSwitcher', () => { const capabilities = buildCapabilities(); - const switcher = setup(space); + const { switcher } = setup(space); const request = httpServerMock.createKibanaRequest(); const result = await switcher(request, capabilities); @@ -174,7 +201,7 @@ describe('capabilitiesSwitcher', () => { const capabilities = buildCapabilities(); - const switcher = setup(space); + const { switcher } = setup(space); const request = httpServerMock.createKibanaRequest(); const result = await switcher(request, capabilities); @@ -198,7 +225,7 @@ describe('capabilitiesSwitcher', () => { const capabilities = buildCapabilities(); - const switcher = setup(space); + const { switcher } = setup(space); const request = httpServerMock.createKibanaRequest(); const result = await switcher(request, capabilities); diff --git a/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.ts b/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.ts index ae4a5bd73ddb8..7dbf1a70aeab2 100644 --- a/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.ts +++ b/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.ts @@ -4,7 +4,7 @@ * you may not use this file except in compliance with the Elastic License. */ import _ from 'lodash'; -import { Capabilities, CapabilitiesSwitcher, CoreSetup } from 'src/core/server'; +import { Capabilities, CapabilitiesSwitcher, CoreSetup, Logger } from 'src/core/server'; import { Feature } from '../../../../plugins/features/server'; import { Space } from '../../common/model/space'; import { SpacesServiceSetup } from '../spaces_service'; @@ -12,7 +12,8 @@ import { PluginsSetup } from '../plugin'; export function setupCapabilitiesSwitcher( core: CoreSetup, - spacesService: SpacesServiceSetup + spacesService: SpacesServiceSetup, + logger: Logger ): CapabilitiesSwitcher { return async (request, capabilities) => { const isAnonymousRequest = !request.route.options.authRequired; @@ -21,16 +22,21 @@ export function setupCapabilitiesSwitcher( return capabilities; } - const [activeSpace, startServices] = await Promise.all([ - spacesService.getActiveSpace(request), - core.getStartServices(), - ]); + try { + const [activeSpace, startServices] = await Promise.all([ + spacesService.getActiveSpace(request), + core.getStartServices(), + ]); - const [, pluginsStart] = startServices; + const [, pluginsStart] = startServices; - const features = pluginsStart.features.getFeatures(); + const features = pluginsStart.features.getFeatures(); - return toggleCapabilities(features, capabilities, activeSpace); + return toggleCapabilities(features, capabilities, activeSpace); + } catch (e) { + logger.warn(`Error toggling capabilities for request to ${request.url.pathname}: ${e}`); + return capabilities; + } }; } diff --git a/x-pack/plugins/spaces/server/capabilities/index.ts b/x-pack/plugins/spaces/server/capabilities/index.ts index 2a7117f3a58b3..1989ceaf3d172 100644 --- a/x-pack/plugins/spaces/server/capabilities/index.ts +++ b/x-pack/plugins/spaces/server/capabilities/index.ts @@ -4,7 +4,7 @@ * you may not use this file except in compliance with the Elastic License. */ -import { CoreSetup } from 'src/core/server'; +import { CoreSetup, Logger } from 'src/core/server'; import { capabilitiesProvider } from './capabilities_provider'; import { setupCapabilitiesSwitcher } from './capabilities_switcher'; import { PluginsSetup } from '../plugin'; @@ -12,8 +12,9 @@ import { SpacesServiceSetup } from '../spaces_service'; export const setupCapabilities = ( core: CoreSetup, - spacesService: SpacesServiceSetup + spacesService: SpacesServiceSetup, + logger: Logger ) => { core.capabilities.registerProvider(capabilitiesProvider); - core.capabilities.registerSwitcher(setupCapabilitiesSwitcher(core, spacesService)); + core.capabilities.registerSwitcher(setupCapabilitiesSwitcher(core, spacesService, logger)); }; diff --git a/x-pack/plugins/spaces/server/plugin.ts b/x-pack/plugins/spaces/server/plugin.ts index 766c8d9f2815f..a3771df07b797 100644 --- a/x-pack/plugins/spaces/server/plugin.ts +++ b/x-pack/plugins/spaces/server/plugin.ts @@ -139,7 +139,7 @@ export class Plugin { features: plugins.features, }); - setupCapabilities(core, spacesService); + setupCapabilities(core, spacesService, this.log); registerSpacesUsageCollector(plugins.usageCollection, { kibanaIndexConfig: this.kibanaIndexConfig, From 09cec11c5858abf9b4b7bb4f9828552a04896638 Mon Sep 17 00:00:00 2001 From: Larry Gregory Date: Fri, 14 Feb 2020 15:39:47 -0500 Subject: [PATCH 07/11] pipe dat config --- .../spaces/server/usage_collection/spaces_usage_collector.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugins/spaces/server/usage_collection/spaces_usage_collector.ts b/x-pack/plugins/spaces/server/usage_collection/spaces_usage_collector.ts index c169b36a42d59..8b7afa027b2e6 100644 --- a/x-pack/plugins/spaces/server/usage_collection/spaces_usage_collector.ts +++ b/x-pack/plugins/spaces/server/usage_collection/spaces_usage_collector.ts @@ -135,7 +135,7 @@ export function getSpacesUsageCollector( const license = await deps.licensing.license$.pipe(take(1)).toPromise(); const available = license.isAvailable; // some form of spaces is available for all valid licenses - const kibanaIndex = (await deps.kibanaIndexConfig.toPromise()).kibana.index; + const kibanaIndex = (await deps.kibanaIndexConfig.pipe(take(1)).toPromise()).kibana.index; const usageStats = await getSpacesUsage(callCluster, kibanaIndex, deps.features, available); From 993c0bb841e73c074c46c489ce6d77a71ccb99b1 Mon Sep 17 00:00:00 2001 From: Larry Gregory Date: Mon, 24 Feb 2020 14:05:24 -0500 Subject: [PATCH 08/11] start addressing PR feedback --- src/plugins/usage_collection/server/mocks.ts | 7 ++----- .../server/capabilities/capabilities_switcher.ts | 8 +++----- .../on_post_auth_interceptor.test.ts | 1 - .../on_post_auth_interceptor.ts | 4 +--- x-pack/plugins/spaces/server/plugin.ts | 16 +++++++++------- .../spaces_usage_collector.test.ts | 6 +++--- .../usage_collection/spaces_usage_collector.ts | 9 +++------ 7 files changed, 21 insertions(+), 30 deletions(-) diff --git a/src/plugins/usage_collection/server/mocks.ts b/src/plugins/usage_collection/server/mocks.ts index 91f643738083a..2194b1fb83f6e 100644 --- a/src/plugins/usage_collection/server/mocks.ts +++ b/src/plugins/usage_collection/server/mocks.ts @@ -17,13 +17,12 @@ * under the License. */ -// eslint-disable-next-line @kbn/eslint/no-restricted-paths -import { loggingServiceMock } from 'src/core/server/mocks'; +import { loggingServiceMock } from '../../../core/server/mocks'; import { UsageCollectionSetup } from './plugin'; import { CollectorSet } from './collector'; const createSetupContract = () => { - const setupContract = { + return { ...new CollectorSet({ logger: loggingServiceMock.createLogger(), maximumWaitTimeForAllCollectorsInS: 1, @@ -32,8 +31,6 @@ const createSetupContract = () => { UsageCollectionSetup['registerLegacySavedObjects'] >, } as UsageCollectionSetup; - - return setupContract; }; export const usageCollectionPluginMock = { diff --git a/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.ts b/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.ts index 7dbf1a70aeab2..118a33a055952 100644 --- a/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.ts +++ b/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.ts @@ -23,16 +23,14 @@ export function setupCapabilitiesSwitcher( } try { - const [activeSpace, startServices] = await Promise.all([ + const [activeSpace, [, { features }]] = await Promise.all([ spacesService.getActiveSpace(request), core.getStartServices(), ]); - const [, pluginsStart] = startServices; + const registeredFeatures = features.getFeatures(); - const features = pluginsStart.features.getFeatures(); - - return toggleCapabilities(features, capabilities, activeSpace); + return toggleCapabilities(registeredFeatures, capabilities, activeSpace); } catch (e) { logger.warn(`Error toggling capabilities for request to ${request.url.pathname}: ${e}`); return capabilities; diff --git a/x-pack/plugins/spaces/server/lib/request_interceptors/on_post_auth_interceptor.test.ts b/x-pack/plugins/spaces/server/lib/request_interceptors/on_post_auth_interceptor.test.ts index a9fff5098c1d3..61157a9318781 100644 --- a/x-pack/plugins/spaces/server/lib/request_interceptors/on_post_auth_interceptor.test.ts +++ b/x-pack/plugins/spaces/server/lib/request_interceptors/on_post_auth_interceptor.test.ts @@ -205,7 +205,6 @@ describe('onPostAuthInterceptor', () => { }); initSpacesOnPostAuthRequestInterceptor({ - getLegacyAPI: () => legacyAPI, http: (http as unknown) as CoreSetup['http'], log: loggingMock, features: featuresPlugin, diff --git a/x-pack/plugins/spaces/server/lib/request_interceptors/on_post_auth_interceptor.ts b/x-pack/plugins/spaces/server/lib/request_interceptors/on_post_auth_interceptor.ts index 4674f3641084a..b07ff11f6efc6 100644 --- a/x-pack/plugins/spaces/server/lib/request_interceptors/on_post_auth_interceptor.ts +++ b/x-pack/plugins/spaces/server/lib/request_interceptors/on_post_auth_interceptor.ts @@ -7,13 +7,12 @@ import { Logger, CoreSetup } from 'src/core/server'; import { Space } from '../../../common/model/space'; import { wrapError } from '../errors'; import { SpacesServiceSetup } from '../../spaces_service/spaces_service'; -import { LegacyAPI, PluginsSetup } from '../../plugin'; +import { PluginsSetup } from '../../plugin'; import { getSpaceSelectorUrl } from '../get_space_selector_url'; import { DEFAULT_SPACE_ID, ENTER_SPACE_PATH } from '../../../common/constants'; import { addSpaceIdToPath } from '../../../common'; export interface OnPostAuthInterceptorDeps { - getLegacyAPI(): LegacyAPI; http: CoreSetup['http']; features: PluginsSetup['features']; spacesService: SpacesServiceSetup; @@ -22,7 +21,6 @@ export interface OnPostAuthInterceptorDeps { export function initSpacesOnPostAuthRequestInterceptor({ features, - getLegacyAPI, spacesService, log, http, diff --git a/x-pack/plugins/spaces/server/plugin.ts b/x-pack/plugins/spaces/server/plugin.ts index a3771df07b797..0b51fb2096e5b 100644 --- a/x-pack/plugins/spaces/server/plugin.ts +++ b/x-pack/plugins/spaces/server/plugin.ts @@ -67,7 +67,7 @@ export class Plugin { private readonly config$: Observable; - private readonly kibanaIndexConfig: Observable<{ kibana: { index: string } }>; + private readonly kibanaIndexConfig$: Observable<{ kibana: { index: string } }>; private readonly log: Logger; @@ -91,7 +91,7 @@ export class Plugin { constructor(initializerContext: PluginInitializerContext) { this.config$ = initializerContext.config.create(); - this.kibanaIndexConfig = initializerContext.config.legacy.globalConfig$; + this.kibanaIndexConfig$ = initializerContext.config.legacy.globalConfig$; this.log = initializerContext.logger.get(); } @@ -141,11 +141,13 @@ export class Plugin { setupCapabilities(core, spacesService, this.log); - registerSpacesUsageCollector(plugins.usageCollection, { - kibanaIndexConfig: this.kibanaIndexConfig, - features: plugins.features, - licensing: plugins.licensing, - }); + if (plugins.usageCollection) { + registerSpacesUsageCollector(plugins.usageCollection, { + kibanaIndexConfig$: this.kibanaIndexConfig$, + features: plugins.features, + licensing: plugins.licensing, + }); + } if (plugins.security) { plugins.security.registerSpacesService(spacesService); diff --git a/x-pack/plugins/spaces/server/usage_collection/spaces_usage_collector.test.ts b/x-pack/plugins/spaces/server/usage_collection/spaces_usage_collector.test.ts index aa1b38d009825..834fea75b4905 100644 --- a/x-pack/plugins/spaces/server/usage_collection/spaces_usage_collector.test.ts +++ b/x-pack/plugins/spaces/server/usage_collection/spaces_usage_collector.test.ts @@ -74,7 +74,7 @@ describe('with a basic license', () => { license: { isAvailable: true, type: 'basic' }, }); const { fetch: getSpacesUsage } = getSpacesUsageCollector(usageCollecion as any, { - kibanaIndexConfig: pluginInitializerContextConfigMock({}).legacy.globalConfig$, + kibanaIndexConfig$: pluginInitializerContextConfigMock({}).legacy.globalConfig$, features, licensing, }); @@ -122,7 +122,7 @@ describe('with no license', () => { beforeAll(async () => { const { features, licensing, usageCollecion } = setup({ license: { isAvailable: false } }); const { fetch: getSpacesUsage } = getSpacesUsageCollector(usageCollecion as any, { - kibanaIndexConfig: pluginInitializerContextConfigMock({}).legacy.globalConfig$, + kibanaIndexConfig$: pluginInitializerContextConfigMock({}).legacy.globalConfig$, features, licensing, }); @@ -153,7 +153,7 @@ describe('with platinum license', () => { license: { isAvailable: true, type: 'platinum' }, }); const { fetch: getSpacesUsage } = getSpacesUsageCollector(usageCollecion as any, { - kibanaIndexConfig: pluginInitializerContextConfigMock({}).legacy.globalConfig$, + kibanaIndexConfig$: pluginInitializerContextConfigMock({}).legacy.globalConfig$, features, licensing, }); diff --git a/x-pack/plugins/spaces/server/usage_collection/spaces_usage_collector.ts b/x-pack/plugins/spaces/server/usage_collection/spaces_usage_collector.ts index 8b7afa027b2e6..5f6cd04308159 100644 --- a/x-pack/plugins/spaces/server/usage_collection/spaces_usage_collector.ts +++ b/x-pack/plugins/spaces/server/usage_collection/spaces_usage_collector.ts @@ -115,7 +115,7 @@ export interface UsageStats { } interface CollectorDeps { - kibanaIndexConfig: Observable<{ kibana: { index: string } }>; + kibanaIndexConfig$: Observable<{ kibana: { index: string } }>; features: PluginsSetup['features']; licensing: PluginsSetup['licensing']; } @@ -135,7 +135,7 @@ export function getSpacesUsageCollector( const license = await deps.licensing.license$.pipe(take(1)).toPromise(); const available = license.isAvailable; // some form of spaces is available for all valid licenses - const kibanaIndex = (await deps.kibanaIndexConfig.pipe(take(1)).toPromise()).kibana.index; + const kibanaIndex = (await deps.kibanaIndexConfig$.pipe(take(1)).toPromise()).kibana.index; const usageStats = await getSpacesUsage(callCluster, kibanaIndex, deps.features, available); @@ -165,12 +165,9 @@ export function getSpacesUsageCollector( } export function registerSpacesUsageCollector( - usageCollection: UsageCollectionSetup | undefined, + usageCollection: UsageCollectionSetup, deps: CollectorDeps ) { - if (!usageCollection) { - return; - } const collector = getSpacesUsageCollector(usageCollection, deps); usageCollection.registerCollector(collector); } From 64089cea595f05ba9973e160f4f792b7ce984a05 Mon Sep 17 00:00:00 2001 From: Larry Gregory Date: Tue, 25 Feb 2020 09:11:55 -0500 Subject: [PATCH 09/11] fix CoreSetup's generic type --- x-pack/plugins/features/server/index.ts | 2 +- .../capabilities/capabilities_switcher.test.ts | 4 ++-- .../server/capabilities/capabilities_switcher.ts | 4 ++-- x-pack/plugins/spaces/server/capabilities/index.ts | 4 ++-- x-pack/plugins/spaces/server/plugin.ts | 12 +++++++++--- 5 files changed, 16 insertions(+), 10 deletions(-) diff --git a/x-pack/plugins/features/server/index.ts b/x-pack/plugins/features/server/index.ts index 2b4f85aa04f04..48ef97a494f7e 100644 --- a/x-pack/plugins/features/server/index.ts +++ b/x-pack/plugins/features/server/index.ts @@ -14,7 +14,7 @@ import { Plugin } from './plugin'; export { uiCapabilitiesRegex } from './feature_schema'; export { Feature, FeatureWithAllOrReadPrivileges, FeatureKibanaPrivileges } from '../common'; -export { PluginSetupContract } from './plugin'; +export { PluginSetupContract, PluginStartContract } from './plugin'; export const plugin = (initializerContext: PluginInitializerContext) => new Plugin(initializerContext); diff --git a/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.test.ts b/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.test.ts index 866108826ea97..3f7b93c754aef 100644 --- a/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.test.ts +++ b/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.test.ts @@ -11,7 +11,7 @@ import { Capabilities, CoreSetup } from 'src/core/server'; import { coreMock, httpServerMock, loggingServiceMock } from 'src/core/server/mocks'; import { featuresPluginMock } from '../../../features/server/mocks'; import { spacesServiceMock } from '../spaces_service/spaces_service.mock'; -import { PluginsSetup } from '../plugin'; +import { PluginsStart } from '../plugin'; const features: Feature[] = [ { @@ -112,7 +112,7 @@ const setup = (space: Space) => { const logger = loggingServiceMock.createLogger(); const switcher = setupCapabilitiesSwitcher( - (coreSetup as unknown) as CoreSetup, + (coreSetup as unknown) as CoreSetup, spacesService, logger ); diff --git a/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.ts b/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.ts index 118a33a055952..317cc7fe0e3c3 100644 --- a/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.ts +++ b/x-pack/plugins/spaces/server/capabilities/capabilities_switcher.ts @@ -8,10 +8,10 @@ import { Capabilities, CapabilitiesSwitcher, CoreSetup, Logger } from 'src/core/ import { Feature } from '../../../../plugins/features/server'; import { Space } from '../../common/model/space'; import { SpacesServiceSetup } from '../spaces_service'; -import { PluginsSetup } from '../plugin'; +import { PluginsStart } from '../plugin'; export function setupCapabilitiesSwitcher( - core: CoreSetup, + core: CoreSetup, spacesService: SpacesServiceSetup, logger: Logger ): CapabilitiesSwitcher { diff --git a/x-pack/plugins/spaces/server/capabilities/index.ts b/x-pack/plugins/spaces/server/capabilities/index.ts index 1989ceaf3d172..56a72a2eeaf19 100644 --- a/x-pack/plugins/spaces/server/capabilities/index.ts +++ b/x-pack/plugins/spaces/server/capabilities/index.ts @@ -7,11 +7,11 @@ import { CoreSetup, Logger } from 'src/core/server'; import { capabilitiesProvider } from './capabilities_provider'; import { setupCapabilitiesSwitcher } from './capabilities_switcher'; -import { PluginsSetup } from '../plugin'; +import { PluginsStart } from '../plugin'; import { SpacesServiceSetup } from '../spaces_service'; export const setupCapabilities = ( - core: CoreSetup, + core: CoreSetup, spacesService: SpacesServiceSetup, logger: Logger ) => { diff --git a/x-pack/plugins/spaces/server/plugin.ts b/x-pack/plugins/spaces/server/plugin.ts index 0b51fb2096e5b..d125e0f54e9c1 100644 --- a/x-pack/plugins/spaces/server/plugin.ts +++ b/x-pack/plugins/spaces/server/plugin.ts @@ -13,7 +13,10 @@ import { Logger, PluginInitializerContext, } from '../../../../src/core/server'; -import { PluginSetupContract as FeaturesPluginSetup } from '../../features/server'; +import { + PluginSetupContract as FeaturesPluginSetup, + PluginStartContract as FeaturesPluginStart, +} from '../../features/server'; import { SecurityPluginSetup } from '../../security/server'; import { LicensingPluginSetup } from '../../licensing/server'; import { createDefaultSpace } from './lib/create_default_space'; @@ -51,6 +54,10 @@ export interface PluginsSetup { home?: HomeServerPluginSetup; } +export interface PluginsStart { + features: FeaturesPluginStart; +} + export interface SpacesPluginSetup { spacesService: SpacesServiceSetup; __legacyCompat: { @@ -98,7 +105,7 @@ export class Plugin { public async start() {} public async setup( - core: CoreSetup, + core: CoreSetup, plugins: PluginsSetup ): Promise { const service = new SpacesService(this.log, this.getLegacyAPI); @@ -134,7 +141,6 @@ export class Plugin { initSpacesRequestInterceptors({ http: core.http, log: this.log, - getLegacyAPI: this.getLegacyAPI, spacesService, features: plugins.features, }); From 4c792bcd989e91db44ee3ca08d0983ab61fe748d Mon Sep 17 00:00:00 2001 From: Larry Gregory Date: Tue, 25 Feb 2020 11:23:38 -0500 Subject: [PATCH 10/11] fix usage collector tests --- .../server/usage_collection/spaces_usage_collector.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/x-pack/plugins/spaces/server/usage_collection/spaces_usage_collector.test.ts b/x-pack/plugins/spaces/server/usage_collection/spaces_usage_collector.test.ts index 22a0cad344c7c..57ec688ab70e8 100644 --- a/x-pack/plugins/spaces/server/usage_collection/spaces_usage_collector.test.ts +++ b/x-pack/plugins/spaces/server/usage_collection/spaces_usage_collector.test.ts @@ -73,7 +73,7 @@ describe('error handling', () => { license: { isAvailable: true, type: 'basic' }, }); const { fetch: getSpacesUsage } = getSpacesUsageCollector(usageCollecion as any, { - kibanaIndex: '.kibana', + kibanaIndexConfig$: Rx.of({ kibana: { index: '.kibana' } }), features, licensing, }); @@ -86,7 +86,7 @@ describe('error handling', () => { license: { isAvailable: true, type: 'basic' }, }); const { fetch: getSpacesUsage } = getSpacesUsageCollector(usageCollecion as any, { - kibanaIndex: '.kibana', + kibanaIndexConfig$: Rx.of({ kibana: { index: '.kibana' } }), features, licensing, }); From 0003e4d42cf9db7ecc12258d692d25abc9f32328 Mon Sep 17 00:00:00 2001 From: Larry Gregory Date: Thu, 27 Feb 2020 13:34:12 -0500 Subject: [PATCH 11/11] PR review updates --- x-pack/plugins/features/server/plugin.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/x-pack/plugins/features/server/plugin.ts b/x-pack/plugins/features/server/plugin.ts index 755d8d9cfad06..e77fa218c0681 100644 --- a/x-pack/plugins/features/server/plugin.ts +++ b/x-pack/plugins/features/server/plugin.ts @@ -49,7 +49,7 @@ export interface LegacyAPI { export class Plugin { private readonly logger: Logger; - private featureRegistry!: FeatureRegistry; + private readonly featureRegistry: FeatureRegistry = new FeatureRegistry(); private legacyAPI?: LegacyAPI; private readonly getLegacyAPI = () => { @@ -67,8 +67,6 @@ export class Plugin { core: CoreSetup, { timelion }: { timelion?: TimelionSetupContract } ): Promise> { - this.featureRegistry = new FeatureRegistry(); - defineRoutes({ router: core.http.createRouter(), featureRegistry: this.featureRegistry, @@ -96,9 +94,9 @@ export class Plugin { public start(): RecursiveReadonly { this.logger.debug('Starting plugin'); - return { + return deepFreeze({ getFeatures: this.featureRegistry.getAll.bind(this.featureRegistry), - }; + }); } public stop() {