Skip to content

Commit

Permalink
[8.x] feat(security): extend `Feature` definition to suppor…
Browse files Browse the repository at this point in the history
…t explicit feature replacements (#206660) (#206925)

# Backport

This will backport the following commits from `main` to `8.x`:
- [feat(security): extend `Feature` definition to support
explicit feature replacements
(#206660)](#206660)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Aleh
Zasypkin","email":"[email protected]"},"sourceCommit":{"committedDate":"2025-01-16T11:35:32Z","message":"feat(security):
extend `Feature` definition to support explicit feature replacements
(#206660)\n\n## Summary\n\nToday, when a developer deprecates a feature
and replaces its privileges\nwith those of another feature, we
reasonably assume that the new feature\nfully replaces the old one in
all possible contexts - whether in role\nmanagement UIs or in the Spaces
feature toggles visibility UI. However,\nwhen deprecated privileges are
replaced by the privileges of multiple\nfeatures, such as in
[this\ncase](https://github.com/elastic/kibana/pull/202863#discussion_r1892672114)\nwhere
the Discover/Dashboard/Maps feature privileges are replaced by
the\nprivileges of Discover_v2/Dashboard_v2/Maps_v2, respectively,
**and**\nthe privileges of the Saved Query Management feature, the
choice is\nambiguous.\n\nWhich of these features should be treated as
the replacement for the\ndeprecated feature in contexts that deal with
entire features (like the\nSpaces feature toggles visibility UI) rather
than individual privileges\n(like in role management UIs)? Should all
referenced features be\nconsidered replacements? Or just a subset - or
even a single feature? If\nso, which one? Currently, we treat all
referenced features as\nreplacements for the deprecated feature, which
creates problems, as\ndescribed in detail in
[this\ndiscussion](https://github.com/elastic/kibana/pull/202863#discussion_r1892672114).\n\nThis
PR allows developers to customize this behavior by specifying
which\nfeatures Kibana should treat as direct successors to deprecated
features\nin contexts that deal with whole features rather than
individual\nprivileges:\n\n```ts\ndeps.features.registerKibanaFeature({\n
deprecated: {\n notice: 'The feature is deprecated because … well,
there’s a reason.',\n --> replacedBy: ['feature_id_v2'], <--\n },\n id:
'feature_id'\n name: `Case #4 feature ${suffix} (DEPRECATED)`,\n
…\n});\n```\n\n## How to test\n\n1. Run test server\n```bash\nnode
scripts/functional_tests_server.js --config
x-pack/test/security_api_integration/features.config.ts\n```\n\n2.
Execute the following request from the Dev Tools (`case_4_feature_a`\nis
a deprecated feature that is replaced by multiple features
and\n**doesn't use** `deprecated.replacedBy`)\n```http\nPUT
kbn:/api/spaces/space/default?overwrite=true\n{\n \"id\":\"default\",\n
\"name\":\"Default\",\n \"description\":\"This is your default
space!\",\n \"color\":\"#00bfb3\",\n
\"disabledFeatures\":[\"case_4_feature_a\"],\n \"_reserved\":true,\n
\"imageUrl\":\"\",\n \"initials\":\"D\"\n}\n```\n\n3. Observe that in
response deprecated `case_4_feature_a` is replaced by\ntwo features (you
can also
check\nhttp://localhost:5620/app/management/kibana/spaces/edit/default
to see\nhow it's reflected in UI)\n```http\n{\n \"id\": \"default\",\n
\"name\": \"Default\",\n \"description\": \"This is your default
space!\",\n \"color\": \"#00bfb3\",\n \"initials\": \"D\",\n
\"imageUrl\": \"\",\n \"disabledFeatures\": [\n
\"case_4_feature_a_v2\",\n \"case_4_feature_c\"\n ],\n \"_reserved\":
true\n}\n```\n\n4. Execute the following request from the Dev Tools
(`case_4_feature_b`\nis a deprecated feature that is replaced by
multiple features, but\n**uses** `deprecated.replacedBy` to set the
conceptual\nfeature-successor)\n```http\nPUT
kbn:/api/spaces/space/default?overwrite=true\n{\n \"id\":\"default\",\n
\"name\":\"Default\",\n \"description\":\"This is your default
space!\",\n \"color\":\"#00bfb3\",\n
\"disabledFeatures\":[\"case_4_feature_b\"],\n \"_reserved\":true,\n
\"imageUrl\":\"\",\n \"initials\":\"D\"\n}\n```\n\n5. Observe that in
response deprecated `case_4_feature_b` is replaced by\na single feature
(you can also
check\nhttp://localhost:5620/app/management/kibana/spaces/edit/default
to see\nhow it's reflected in UI)\n```http\n{\n \"id\": \"default\",\n
\"name\": \"Default\",\n \"description\": \"This is your default
space!\",\n \"color\": \"#00bfb3\",\n \"initials\": \"D\",\n
\"imageUrl\": \"\",\n \"disabledFeatures\": [\n
\"case_4_feature_b_v2\"\n ],\n \"_reserved\": true\n}\n```\n\n__Required
by:__\nhttps://github.com//pull/202863#discussion_r1892672114\n\n//cc
@davismcphee","sha":"dd3ce0e7f534279f48be8c125853c89aa92969e2","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:Security/Spaces","release_note:skip","Feature:Security/Authorization","v9.0.0","backport:prev-minor"],"title":"feat(security):
extend `Feature` definition to support explicit feature
replacements","number":206660,"url":"https://github.com/elastic/kibana/pull/206660","mergeCommit":{"message":"feat(security):
extend `Feature` definition to support explicit feature replacements
(#206660)\n\n## Summary\n\nToday, when a developer deprecates a feature
and replaces its privileges\nwith those of another feature, we
reasonably assume that the new feature\nfully replaces the old one in
all possible contexts - whether in role\nmanagement UIs or in the Spaces
feature toggles visibility UI. However,\nwhen deprecated privileges are
replaced by the privileges of multiple\nfeatures, such as in
[this\ncase](https://github.com/elastic/kibana/pull/202863#discussion_r1892672114)\nwhere
the Discover/Dashboard/Maps feature privileges are replaced by
the\nprivileges of Discover_v2/Dashboard_v2/Maps_v2, respectively,
**and**\nthe privileges of the Saved Query Management feature, the
choice is\nambiguous.\n\nWhich of these features should be treated as
the replacement for the\ndeprecated feature in contexts that deal with
entire features (like the\nSpaces feature toggles visibility UI) rather
than individual privileges\n(like in role management UIs)? Should all
referenced features be\nconsidered replacements? Or just a subset - or
even a single feature? If\nso, which one? Currently, we treat all
referenced features as\nreplacements for the deprecated feature, which
creates problems, as\ndescribed in detail in
[this\ndiscussion](https://github.com/elastic/kibana/pull/202863#discussion_r1892672114).\n\nThis
PR allows developers to customize this behavior by specifying
which\nfeatures Kibana should treat as direct successors to deprecated
features\nin contexts that deal with whole features rather than
individual\nprivileges:\n\n```ts\ndeps.features.registerKibanaFeature({\n
deprecated: {\n notice: 'The feature is deprecated because … well,
there’s a reason.',\n --> replacedBy: ['feature_id_v2'], <--\n },\n id:
'feature_id'\n name: `Case #4 feature ${suffix} (DEPRECATED)`,\n
…\n});\n```\n\n## How to test\n\n1. Run test server\n```bash\nnode
scripts/functional_tests_server.js --config
x-pack/test/security_api_integration/features.config.ts\n```\n\n2.
Execute the following request from the Dev Tools (`case_4_feature_a`\nis
a deprecated feature that is replaced by multiple features
and\n**doesn't use** `deprecated.replacedBy`)\n```http\nPUT
kbn:/api/spaces/space/default?overwrite=true\n{\n \"id\":\"default\",\n
\"name\":\"Default\",\n \"description\":\"This is your default
space!\",\n \"color\":\"#00bfb3\",\n
\"disabledFeatures\":[\"case_4_feature_a\"],\n \"_reserved\":true,\n
\"imageUrl\":\"\",\n \"initials\":\"D\"\n}\n```\n\n3. Observe that in
response deprecated `case_4_feature_a` is replaced by\ntwo features (you
can also
check\nhttp://localhost:5620/app/management/kibana/spaces/edit/default
to see\nhow it's reflected in UI)\n```http\n{\n \"id\": \"default\",\n
\"name\": \"Default\",\n \"description\": \"This is your default
space!\",\n \"color\": \"#00bfb3\",\n \"initials\": \"D\",\n
\"imageUrl\": \"\",\n \"disabledFeatures\": [\n
\"case_4_feature_a_v2\",\n \"case_4_feature_c\"\n ],\n \"_reserved\":
true\n}\n```\n\n4. Execute the following request from the Dev Tools
(`case_4_feature_b`\nis a deprecated feature that is replaced by
multiple features, but\n**uses** `deprecated.replacedBy` to set the
conceptual\nfeature-successor)\n```http\nPUT
kbn:/api/spaces/space/default?overwrite=true\n{\n \"id\":\"default\",\n
\"name\":\"Default\",\n \"description\":\"This is your default
space!\",\n \"color\":\"#00bfb3\",\n
\"disabledFeatures\":[\"case_4_feature_b\"],\n \"_reserved\":true,\n
\"imageUrl\":\"\",\n \"initials\":\"D\"\n}\n```\n\n5. Observe that in
response deprecated `case_4_feature_b` is replaced by\na single feature
(you can also
check\nhttp://localhost:5620/app/management/kibana/spaces/edit/default
to see\nhow it's reflected in UI)\n```http\n{\n \"id\": \"default\",\n
\"name\": \"Default\",\n \"description\": \"This is your default
space!\",\n \"color\": \"#00bfb3\",\n \"initials\": \"D\",\n
\"imageUrl\": \"\",\n \"disabledFeatures\": [\n
\"case_4_feature_b_v2\"\n ],\n \"_reserved\": true\n}\n```\n\n__Required
by:__\nhttps://github.com//pull/202863#discussion_r1892672114\n\n//cc
@davismcphee","sha":"dd3ce0e7f534279f48be8c125853c89aa92969e2"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/206660","number":206660,"mergeCommit":{"message":"feat(security):
extend `Feature` definition to support explicit feature replacements
(#206660)\n\n## Summary\n\nToday, when a developer deprecates a feature
and replaces its privileges\nwith those of another feature, we
reasonably assume that the new feature\nfully replaces the old one in
all possible contexts - whether in role\nmanagement UIs or in the Spaces
feature toggles visibility UI. However,\nwhen deprecated privileges are
replaced by the privileges of multiple\nfeatures, such as in
[this\ncase](https://github.com/elastic/kibana/pull/202863#discussion_r1892672114)\nwhere
the Discover/Dashboard/Maps feature privileges are replaced by
the\nprivileges of Discover_v2/Dashboard_v2/Maps_v2, respectively,
**and**\nthe privileges of the Saved Query Management feature, the
choice is\nambiguous.\n\nWhich of these features should be treated as
the replacement for the\ndeprecated feature in contexts that deal with
entire features (like the\nSpaces feature toggles visibility UI) rather
than individual privileges\n(like in role management UIs)? Should all
referenced features be\nconsidered replacements? Or just a subset - or
even a single feature? If\nso, which one? Currently, we treat all
referenced features as\nreplacements for the deprecated feature, which
creates problems, as\ndescribed in detail in
[this\ndiscussion](https://github.com/elastic/kibana/pull/202863#discussion_r1892672114).\n\nThis
PR allows developers to customize this behavior by specifying
which\nfeatures Kibana should treat as direct successors to deprecated
features\nin contexts that deal with whole features rather than
individual\nprivileges:\n\n```ts\ndeps.features.registerKibanaFeature({\n
deprecated: {\n notice: 'The feature is deprecated because … well,
there’s a reason.',\n --> replacedBy: ['feature_id_v2'], <--\n },\n id:
'feature_id'\n name: `Case #4 feature ${suffix} (DEPRECATED)`,\n
…\n});\n```\n\n## How to test\n\n1. Run test server\n```bash\nnode
scripts/functional_tests_server.js --config
x-pack/test/security_api_integration/features.config.ts\n```\n\n2.
Execute the following request from the Dev Tools (`case_4_feature_a`\nis
a deprecated feature that is replaced by multiple features
and\n**doesn't use** `deprecated.replacedBy`)\n```http\nPUT
kbn:/api/spaces/space/default?overwrite=true\n{\n \"id\":\"default\",\n
\"name\":\"Default\",\n \"description\":\"This is your default
space!\",\n \"color\":\"#00bfb3\",\n
\"disabledFeatures\":[\"case_4_feature_a\"],\n \"_reserved\":true,\n
\"imageUrl\":\"\",\n \"initials\":\"D\"\n}\n```\n\n3. Observe that in
response deprecated `case_4_feature_a` is replaced by\ntwo features (you
can also
check\nhttp://localhost:5620/app/management/kibana/spaces/edit/default
to see\nhow it's reflected in UI)\n```http\n{\n \"id\": \"default\",\n
\"name\": \"Default\",\n \"description\": \"This is your default
space!\",\n \"color\": \"#00bfb3\",\n \"initials\": \"D\",\n
\"imageUrl\": \"\",\n \"disabledFeatures\": [\n
\"case_4_feature_a_v2\",\n \"case_4_feature_c\"\n ],\n \"_reserved\":
true\n}\n```\n\n4. Execute the following request from the Dev Tools
(`case_4_feature_b`\nis a deprecated feature that is replaced by
multiple features, but\n**uses** `deprecated.replacedBy` to set the
conceptual\nfeature-successor)\n```http\nPUT
kbn:/api/spaces/space/default?overwrite=true\n{\n \"id\":\"default\",\n
\"name\":\"Default\",\n \"description\":\"This is your default
space!\",\n \"color\":\"#00bfb3\",\n
\"disabledFeatures\":[\"case_4_feature_b\"],\n \"_reserved\":true,\n
\"imageUrl\":\"\",\n \"initials\":\"D\"\n}\n```\n\n5. Observe that in
response deprecated `case_4_feature_b` is replaced by\na single feature
(you can also
check\nhttp://localhost:5620/app/management/kibana/spaces/edit/default
to see\nhow it's reflected in UI)\n```http\n{\n \"id\": \"default\",\n
\"name\": \"Default\",\n \"description\": \"This is your default
space!\",\n \"color\": \"#00bfb3\",\n \"initials\": \"D\",\n
\"imageUrl\": \"\",\n \"disabledFeatures\": [\n
\"case_4_feature_b_v2\"\n ],\n \"_reserved\": true\n}\n```\n\n__Required
by:__\nhttps://github.com//pull/202863#discussion_r1892672114\n\n//cc
@davismcphee","sha":"dd3ce0e7f534279f48be8c125853c89aa92969e2"}}]}]
BACKPORT-->

Co-authored-by: Aleh Zasypkin <[email protected]>
  • Loading branch information
kibanamachine and azasypkin authored Jan 16, 2025
1 parent 7aebf1b commit db491db
Show file tree
Hide file tree
Showing 13 changed files with 200 additions and 31 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,14 @@ export interface KibanaFeatureConfig {
* documentation.
*/
notice: string;
/**
* An optional list of feature IDs representing the features that should _conceptually_ replace this deprecated
* feature. This is used, for example, in the Spaces feature visibility toggles UI to display the replacement
* feature(s) instead of the deprecated one. By default, the list of replacement features is derived from the
* `replacedBy` fields of the feature privileges. However, if the feature privileges are replaced by the privileges
* of multiple features, this behavior is not always desired and can be overridden here.
*/
replacedBy?: readonly string[];
}>;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2702,10 +2702,12 @@ describe('FeatureRegistry', () => {
}

function createDeprecatedFeature({
deprecated,
all,
read,
subAlpha,
}: {
deprecated?: { notice: string; replacedBy?: string[] };
all?: FeatureKibanaPrivilegesReference[];
read?: {
minimal: FeatureKibanaPrivilegesReference[];
Expand All @@ -2714,7 +2716,7 @@ describe('FeatureRegistry', () => {
subAlpha?: FeatureKibanaPrivilegesReference[];
} = {}): KibanaFeatureConfig {
return {
deprecated: { notice: 'It was a mistake.' },
deprecated: deprecated ?? { notice: 'It was a mistake.' },
id: 'feature-alpha',
name: 'Feature Alpha',
app: [],
Expand Down Expand Up @@ -3240,6 +3242,50 @@ describe('FeatureRegistry', () => {
`"Cannot replace privilege \\"sub-alpha-1-1\\" of deprecated feature \\"feature-alpha\\" with disabled privilege \\"read\\" of feature \\"feature-delta\\"."`
);
});

it('requires correct list of feature IDs to be replaced by', () => {
// Case 1: empty list of feature IDs.
expect(() =>
createRegistry(
createDeprecatedFeature({ deprecated: { notice: 'some notice', replacedBy: [] } })
).validateFeatures()
).toThrowErrorMatchingInlineSnapshot(
`"Feature “feature-alpha” is deprecated and must have at least one feature ID added to the “replacedBy” property, or the property must be left out completely."`
);

// Case 2: invalid feature IDs.
expect(() =>
createRegistry(
createDeprecatedFeature({
deprecated: {
notice: 'some notice',
replacedBy: ['feature-beta', 'feature-gamma', 'feature-delta'],
},
})
).validateFeatures()
).toThrowErrorMatchingInlineSnapshot(
`"Cannot replace deprecated feature “feature-alpha” with the following features, as they aren’t used to replace feature privileges: feature-gamma, feature-delta."`
);

// Case 3: valid feature ID.
expect(() =>
createRegistry(
createDeprecatedFeature({
deprecated: { notice: 'some notice', replacedBy: ['feature-beta'] },
})
).validateFeatures()
).not.toThrow();

// Case 4: valid multiple feature IDs.
expect(() =>
createRegistry(
createDeprecatedFeature({
deprecated: { notice: 'some notice', replacedBy: ['feature-beta', 'feature-delta'] },
all: [{ feature: 'feature-delta', privileges: ['all'] }],
})
).validateFeatures()
).not.toThrow();
});
});
});

Expand Down
44 changes: 41 additions & 3 deletions x-pack/platform/plugins/shared/features/server/feature_registry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@ import { validateKibanaFeature, validateElasticsearchFeature } from './feature_s
import type { ConfigOverridesType } from './config';

/**
* Describes parameters used to retrieve all Kibana features.
* Describes parameters used to retrieve all Kibana features (for internal consumers).
*/
export interface GetKibanaFeaturesParams {
export interface GetKibanaFeaturesParamsInternal {
/**
* If provided, the license will be used to filter out features that require a license higher than the specified one.
* */
Expand All @@ -41,6 +41,17 @@ export interface GetKibanaFeaturesParams {
omitDeprecated?: boolean;
}

/**
* Describes parameters used to retrieve all Kibana features (for public consumers).
*/
export interface GetKibanaFeaturesParams {
/**
* If true, deprecated features will be omitted. For backward compatibility reasons, deprecated features are included
* in the result by default.
*/
omitDeprecated: boolean;
}

export class FeatureRegistry {
private locked = false;
private kibanaFeatures: Record<string, KibanaFeatureConfig> = {};
Expand Down Expand Up @@ -207,6 +218,7 @@ export class FeatureRegistry {

// Iterate over all top-level and sub-feature privileges.
const isFeatureDeprecated = !!feature.deprecated;
const replacementFeatureIds = new Set();
for (const [privilegeId, privilege] of [
...Object.entries(feature.privileges),
...collectSubFeaturesPrivileges(feature),
Expand Down Expand Up @@ -263,6 +275,32 @@ export class FeatureRegistry {
);
}
}

replacementFeatureIds.add(featureReference.feature);
}
}

const featureReplacedBy = feature.deprecated?.replacedBy;
if (featureReplacedBy) {
if (featureReplacedBy.length === 0) {
throw new Error(
`Feature “${feature.id}” is deprecated and must have at least one feature ID added to the “replacedBy” property, or the property must be left out completely.`
);
}

// The feature can be marked as replaced by another feature only if that feature is actually used to replace any
// of the deprecated feature’s privileges.
const invalidFeatureIds = featureReplacedBy.filter(
(featureId) => !replacementFeatureIds.has(featureId)
);
if (invalidFeatureIds.length > 0) {
throw new Error(
`Cannot replace deprecated feature “${
feature.id
}” with the following features, as they aren’t used to replace feature privileges: ${invalidFeatureIds.join(
', '
)}.`
);
}
}
}
Expand All @@ -272,7 +310,7 @@ export class FeatureRegistry {
license,
ignoreLicense = false,
omitDeprecated = false,
}: GetKibanaFeaturesParams = {}): KibanaFeature[] {
}: GetKibanaFeaturesParamsInternal = {}): KibanaFeature[] {
if (!this.locked) {
throw new Error('Cannot retrieve Kibana features while registration is still open');
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,12 @@ const kibanaFeatureSchema = schema.object({
),
})
),
deprecated: schema.maybe(schema.object({ notice: schema.string() })),
deprecated: schema.maybe(
schema.object({
notice: schema.string(),
replacedBy: schema.maybe(schema.arrayOf(schema.string())),
})
),
});

const elasticsearchPrivilegeSchema = schema.object({
Expand Down
1 change: 1 addition & 0 deletions x-pack/platform/plugins/shared/features/server/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ export type {
} from '../common';
export type { SubFeaturePrivilegeIterator } from './feature_privilege_iterator';
export { KibanaFeature, ElasticsearchFeature } from '../common';
export type { GetKibanaFeaturesParams } from './feature_registry';
export type { FeaturesPluginSetup, FeaturesPluginStart } from './plugin';

export const config: PluginConfigDescriptor<TypeOf<typeof ConfigSchema>> = { schema: ConfigSchema };
Expand Down
13 changes: 10 additions & 3 deletions x-pack/platform/plugins/shared/features/server/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import {
Capabilities as UICapabilities,
} from '@kbn/core/server';
import { ConfigType } from './config';
import { FeatureRegistry } from './feature_registry';
import { FeatureRegistry, GetKibanaFeaturesParams } from './feature_registry';
import { uiCapabilitiesForFeatures } from './ui_capabilities_for_features';
import { buildOSSFeatures } from './oss_features';
import { defineRoutes } from './routes';
Expand Down Expand Up @@ -84,7 +84,11 @@ export interface FeaturesPluginSetup {
export interface FeaturesPluginStart {
getElasticsearchFeatures(): ElasticsearchFeature[];

getKibanaFeatures(): KibanaFeature[];
/**
* Returns all registered Kibana features.
* @param params Optional parameters to filter features.
*/
getKibanaFeatures(params?: GetKibanaFeaturesParams): KibanaFeature[];
}

/**
Expand Down Expand Up @@ -147,7 +151,10 @@ export class FeaturesPlugin
getElasticsearchFeatures: this.featureRegistry.getAllElasticsearchFeatures.bind(
this.featureRegistry
),
getKibanaFeatures: this.featureRegistry.getAllKibanaFeatures.bind(this.featureRegistry),
getKibanaFeatures: (params) =>
this.featureRegistry.getAllKibanaFeatures(
params && { omitDeprecated: params.omitDeprecated }
),
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ describe.skip('onPostAuthInterceptor', () => {

const loggingMock = loggingSystemMock.create().asLoggerFactory().get('xpack', 'spaces');

const featuresPlugin = featuresPluginMock.createSetup();
const featuresPlugin = featuresPluginMock.createStart();
featuresPlugin.getKibanaFeatures.mockReturnValue([
{
id: 'feature-1',
Expand Down Expand Up @@ -163,7 +163,7 @@ describe.skip('onPostAuthInterceptor', () => {
initSpacesOnPostAuthRequestInterceptor({
http: http as unknown as CoreSetup['http'],
log: loggingMock,
features: featuresPlugin,
getFeatures: async () => featuresPlugin,
getSpacesService: () => spacesServiceStart,
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,25 +6,25 @@
*/

import type { CoreSetup, Logger } from '@kbn/core/server';
import type { FeaturesPluginStart } from '@kbn/features-plugin/server';

import type { Space } from '../../../common';
import { addSpaceIdToPath } from '../../../common';
import { DEFAULT_SPACE_ID, ENTER_SPACE_PATH } from '../../../common/constants';
import type { PluginsSetup } from '../../plugin';
import type { SpacesServiceStart } from '../../spaces_service/spaces_service';
import type { SpacesServiceStart } from '../../spaces_service';
import { wrapError } from '../errors';
import { getSpaceSelectorUrl } from '../get_space_selector_url';
import { withSpaceSolutionDisabledFeatures } from '../utils/space_solution_disabled_features';

export interface OnPostAuthInterceptorDeps {
http: CoreSetup['http'];
features: PluginsSetup['features'];
getFeatures: () => Promise<FeaturesPluginStart>;
getSpacesService: () => SpacesServiceStart;
log: Logger;
}

export function initSpacesOnPostAuthRequestInterceptor({
features,
getFeatures,
getSpacesService,
log,
http,
Expand All @@ -38,7 +38,7 @@ export function initSpacesOnPostAuthRequestInterceptor({

const spaceId = spacesService.getSpaceId(request);

// The root of kibana is also the root of the defaut space,
// The root of kibana is also the root of the default space,
// since the default space does not have a URL Identifier (i.e., `/s/foo`).
const isRequestingKibanaRoot = path === '/' && spaceId === DEFAULT_SPACE_ID;
const isRequestingSpaceRoot = path === '/' && spaceId !== DEFAULT_SPACE_ID;
Expand Down Expand Up @@ -106,7 +106,10 @@ export function initSpacesOnPostAuthRequestInterceptor({
}
}

const allFeatures = features.getKibanaFeatures();
// The Spaces client returns migrated feature IDs in `disabledFeatures`, so we need to omit
// deprecated features. Otherwise, apps granted by deprecated features will be considered
// available when they shouldn't be, since their IDs won't be present in `disabledFeatures`.
const allFeatures = (await getFeatures()).getKibanaFeatures({ omitDeprecated: true });
const disabledFeatureKeys = withSpaceSolutionDisabledFeatures(
allFeatures,
space.disabledFeatures,
Expand Down
2 changes: 1 addition & 1 deletion x-pack/platform/plugins/shared/spaces/server/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ export class SpacesPlugin
http: core.http,
log: this.log,
getSpacesService,
features: plugins.features,
getFeatures: async () => (await core.getStartServices())[1].features,
});

setupCapabilities(core, getSpacesService, this.log);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,37 @@ const features = [
},
},
},
{
deprecated: { notice: 'It was another mistake.', replacedBy: ['feature_2'] },
id: 'feature_5_deprecated',
name: 'Another deprecated Feature',
app: ['feature2', 'feature3'],
catalogue: ['feature2Entry', 'feature3Entry'],
category: { id: 'deprecated', label: 'deprecated' },
scope: ['spaces', 'security'],
privileges: {
all: {
savedObject: { all: [], read: [] },
ui: [],
app: ['feature2', 'feature3'],
catalogue: ['feature2Entry', 'feature3Entry'],
replacedBy: [
{ feature: 'feature_2', privileges: ['all'] },
{ feature: 'feature_3', privileges: ['all'] },
],
},
read: {
savedObject: { all: [], read: [] },
ui: [],
app: ['feature2', 'feature3'],
catalogue: ['feature2Entry', 'feature3Entry'],
replacedBy: [
{ feature: 'feature_2', privileges: ['read'] },
{ feature: 'feature_3', privileges: ['read'] },
],
},
},
},
] as unknown as KibanaFeature[];
const featuresStart = featuresPluginMock.createStart();

Expand Down Expand Up @@ -135,7 +166,7 @@ describe('#getAll', () => {
},
},
{
// alpha only has deprecated disabled features
// alpha has deprecated disabled features
id: 'alpha',
type: 'space',
references: [],
Expand All @@ -145,6 +176,17 @@ describe('#getAll', () => {
disabledFeatures: ['feature_1', 'feature_4_deprecated'],
},
},
{
// beta has deprecated disabled features with specified `replacedBy` on feature level
id: 'beta',
type: 'space',
references: [],
attributes: {
name: 'beta-name',
description: 'beta-description',
disabledFeatures: ['feature_1', 'feature_5_deprecated'],
},
},
];

const expectedSpaces: Space[] = [
Expand Down Expand Up @@ -178,6 +220,12 @@ describe('#getAll', () => {
description: 'alpha-description',
disabledFeatures: ['feature_1', 'feature_2', 'feature_3'],
},
{
id: 'beta',
name: 'beta-name',
description: 'beta-description',
disabledFeatures: ['feature_1', 'feature_2'],
},
];

test(`finds spaces using callWithRequestRepository`, async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,14 @@ export class SpacesClient implements ISpacesClient {
continue;
}

// If the feature is deprecated and replacement features are explicitly defined, use them.
// Otherwise, use the replacement features defined in the feature privileges.
const featureReplacedBy = feature.deprecated?.replacedBy;
if (featureReplacedBy) {
deprecatedFeatureReferences.set(feature.id, new Set(featureReplacedBy));
continue;
}

// Collect all feature privileges including the ones provided by sub-features, if any.
const allPrivileges = Object.values(feature.privileges ?? {}).concat(
feature.subFeatures?.flatMap((subFeature) =>
Expand Down
Loading

0 comments on commit db491db

Please sign in to comment.