Skip to content

Commit

Permalink
fix(security, features): do not expose UI capabilities of the depreca…
Browse files Browse the repository at this point in the history
…ted features (elastic#198656)

## Summary

This PR ensures that we don’t expose UI capabilities for deprecated
features since they’re unnecessary, and the code should rely on the UI
capabilities of the replacement features instead.

Additionally, this PR transforms the `disabledFeatures` property of
Space objects returned from our programmatic and HTTP APIs to replace
any deprecated feature IDs with the IDs of their replacement features,
ensuring that feature visibility toggles work for deprecated features as
well.

## How to test

1. Run Kibana FTR server with the following config (registers test
deprecated features):
```shell
node scripts/functional_tests_server.js --config x-pack/test/security_api_integration/features.config.ts
```
2. Once server is up and running create Space with the
`case_1_feature_a` **deprecated** feature disabled:
```shell
curl 'http://localhost:5620/api/spaces/space' -u elastic:changeme \
  -X POST -H 'Content-Type: application/json' -H 'kbn-version: 9.0.0' \
  --data-raw '{"name":"space-alpha","id":"space-alpha","initials":"s","color":"#D6BF57","disabledFeatures":["case_1_feature_a"],"imageUrl":""}'
```
3. Log in to Kibana and [navigate to a Space
`space-alpha`](http://localhost:5620/app/management/kibana/spaces/edit/space-alpha)
you've just created. Observe that deprecated `Case #1 feature A`
(`case_1_feature_a`) isn't displayed, and instead you should see that
replaces deprecated one - `Case #1 feature B` (`case_1_feature_b`):

![Screen Shot 2024-11-01 at 17 40
59](https://github.com/user-attachments/assets/5b91e71c-7d46-4ff1-bf73-d148622e8ec4)

Co-authored-by: Elastic Machine <[email protected]>
  • Loading branch information
azasypkin and elasticmachine authored Nov 6, 2024
1 parent de46e7f commit deeb9fe
Show file tree
Hide file tree
Showing 15 changed files with 301 additions and 42 deletions.
4 changes: 2 additions & 2 deletions x-pack/plugins/features/server/mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ const createSetup = (): jest.Mocked<FeaturesPluginSetup> => {

const createStart = (): jest.Mocked<FeaturesPluginStart> => {
return {
getKibanaFeatures: jest.fn(),
getElasticsearchFeatures: jest.fn(),
getKibanaFeatures: jest.fn().mockReturnValue([]),
getElasticsearchFeatures: jest.fn().mockReturnValue([]),
};
};

Expand Down
3 changes: 2 additions & 1 deletion x-pack/plugins/features/server/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,8 @@ export class FeaturesPlugin
this.featureRegistry.validateFeatures();

this.capabilities = uiCapabilitiesForFeatures(
this.featureRegistry.getAllKibanaFeatures(),
// Don't expose capabilities of the deprecated features.
this.featureRegistry.getAllKibanaFeatures({ omitDeprecated: true }),
this.featureRegistry.getAllElasticsearchFeatures()
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -145,12 +145,9 @@ describe('#start', () => {
customBranding: mockCoreSetup.customBranding,
});

const featuresStart = featuresPluginMock.createStart();
featuresStart.getKibanaFeatures.mockReturnValue([]);

authorizationService.start({
clusterClient: mockClusterClient,
features: featuresStart,
features: featuresPluginMock.createStart(),
online$: statusSubject.asObservable(),
});

Expand Down Expand Up @@ -217,12 +214,9 @@ it('#stop unsubscribes from license and ES updates.', async () => {
customBranding: mockCoreSetup.customBranding,
});

const featuresStart = featuresPluginMock.createStart();
featuresStart.getKibanaFeatures.mockReturnValue([]);

authorizationService.start({
clusterClient: mockClusterClient,
features: featuresStart,
features: featuresPluginMock.createStart(),
online$: statusSubject.asObservable(),
});

Expand Down
4 changes: 1 addition & 3 deletions x-pack/plugins/security/server/plugin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,8 @@ describe('Security Plugin', () => {

mockCoreStart = coreMock.createStart();

const mockFeaturesStart = featuresPluginMock.createStart();
mockFeaturesStart.getKibanaFeatures.mockReturnValue([]);
mockStartDependencies = {
features: mockFeaturesStart,
features: featuresPluginMock.createStart(),
licensing: licensingMock.createStart(),
taskManager: taskManagerMock.createStart(),
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ const features = [
category: { id: 'securitySolution' },
},
{
// feature 4 intentionally delcares the same items as feature 3
// feature 4 intentionally declares the same items as feature 3
id: 'feature_4',
name: 'Feature 4',
app: ['feature3', 'feature3_app'],
Expand All @@ -87,6 +87,32 @@ const features = [
},
category: { id: 'observability' },
},
{
deprecated: { notice: 'It was a mistake.' },
id: 'deprecated_feature',
name: 'Deprecated Feature',
// Expose the same `app` and `catalogue` entries as `feature_2` to make sure they are disabled
// when `feature_2` is disabled even if the deprecated feature isn't explicitly disabled.
app: ['feature2'],
catalogue: ['feature2Entry'],
category: { id: 'deprecated', label: 'deprecated' },
privileges: {
all: {
savedObject: { all: [], read: [] },
ui: ['ui_deprecated_all'],
app: ['feature2'],
catalogue: ['feature2Entry'],
replacedBy: [{ feature: 'feature_2', privileges: ['all'] }],
},
read: {
savedObject: { all: [], read: [] },
ui: ['ui_deprecated_read'],
app: ['feature2'],
catalogue: ['feature2Entry'],
replacedBy: [{ feature: 'feature_2', privileges: ['all'] }],
},
},
},
] as unknown as KibanaFeature[];

const buildCapabilities = () =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ function toggleDisabledFeatures(
(acc, feature) => {
if (disabledFeatureKeys.includes(feature.id)) {
acc.disabledFeatures.push(feature);
} else {
} else if (!feature.deprecated) {
acc.enabledFeatures.push(feature);
}
return acc;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ const enabledFeaturesPerSolution: Record<SolutionId, string[]> = {
* This function takes the current space's disabled features and the space solution and returns
* the updated array of disabled features.
*
* @param features The list of all Kibana registered features.
* @param spaceDisabledFeatures The current space's disabled features
* @param spaceSolution The current space's solution (es, oblt, security or classic)
* @returns The updated array of disabled features
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,9 @@ describe('Spaces Public API', () => {
basePath: httpService.basePath,
});

const featuresPluginMockStart = featuresPluginMock.createStart();

featuresPluginMockStart.getKibanaFeatures.mockReturnValue([]);

const usageStatsServicePromise = Promise.resolve(usageStatsServiceMock.createSetupContract());

const clientServiceStart = clientService.start(coreStart, featuresPluginMockStart);
const clientServiceStart = clientService.start(coreStart, featuresPluginMock.createStart());

const spacesServiceStart = service.start({
basePath: coreStart.http.basePath,
Expand Down
6 changes: 1 addition & 5 deletions x-pack/plugins/spaces/server/routes/api/external/put.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,9 @@ describe('PUT /api/spaces/space', () => {
basePath: httpService.basePath,
});

const featuresPluginMockStart = featuresPluginMock.createStart();

featuresPluginMockStart.getKibanaFeatures.mockReturnValue([]);

const usageStatsServicePromise = Promise.resolve(usageStatsServiceMock.createSetupContract());

const clientServiceStart = clientService.start(coreStart, featuresPluginMockStart);
const clientServiceStart = clientService.start(coreStart, featuresPluginMock.createStart());

const spacesServiceStart = service.start({
basePath: coreStart.http.basePath,
Expand Down
48 changes: 48 additions & 0 deletions x-pack/plugins/spaces/server/spaces_client/spaces_client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,37 @@ const features = [
catalogue: ['feature3Entry'],
category: { id: 'securitySolution' },
},
{
deprecated: { notice: 'It was a mistake.' },
id: 'feature_4_deprecated',
name: '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 @@ -103,6 +134,17 @@ describe('#getAll', () => {
bar: 'baz-bar', // an extra attribute that will be ignored during conversion
},
},
{
// alpha only has deprecated disabled features
id: 'alpha',
type: 'space',
references: [],
attributes: {
name: 'alpha-name',
description: 'alpha-description',
disabledFeatures: ['feature_1', 'feature_4_deprecated'],
},
},
];

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

test(`finds spaces using callWithRequestRepository`, async () => {
Expand Down
59 changes: 55 additions & 4 deletions x-pack/plugins/spaces/server/spaces_client/spaces_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import type {
SavedObject,
} from '@kbn/core/server';
import type { LegacyUrlAliasTarget } from '@kbn/core-saved-objects-common';
import type { KibanaFeature } from '@kbn/features-plugin/common';
import { KibanaFeatureScope } from '@kbn/features-plugin/common';
import type { FeaturesPluginStart } from '@kbn/features-plugin/server';

Expand Down Expand Up @@ -84,7 +85,13 @@ export interface ISpacesClient {
* Client for interacting with spaces.
*/
export class SpacesClient implements ISpacesClient {
private isServerless = false;
private readonly isServerless: boolean;

/**
* A map of deprecated feature IDs to the feature IDs that replace them used to transform the disabled features
* of a space to make sure they only reference non-deprecated features.
*/
private readonly deprecatedFeaturesReferences: Map<string, Set<string>>;

constructor(
private readonly debugLogger: (message: string) => void,
Expand All @@ -95,6 +102,9 @@ export class SpacesClient implements ISpacesClient {
private readonly features: FeaturesPluginStart
) {
this.isServerless = this.buildFlavour === 'serverless';
this.deprecatedFeaturesReferences = this.collectDeprecatedFeaturesReferences(
features.getKibanaFeatures()
);
}

public async getAll(options: v1.GetAllSpacesOptions = {}): Promise<v1.GetSpaceResult[]> {
Expand Down Expand Up @@ -247,6 +257,8 @@ export class SpacesClient implements ISpacesClient {
};

private transformSavedObjectToSpace = (savedObject: SavedObject<any>): v1.Space => {
// Solution isn't supported in the serverless offering.
const solution = !this.isServerless ? savedObject.attributes.solution : undefined;
return {
id: savedObject.id,
name: savedObject.attributes.name ?? '',
Expand All @@ -256,11 +268,13 @@ export class SpacesClient implements ISpacesClient {
imageUrl: savedObject.attributes.imageUrl,
disabledFeatures: withSpaceSolutionDisabledFeatures(
this.features.getKibanaFeatures(),
savedObject.attributes.disabledFeatures ?? [],
!this.isServerless ? savedObject.attributes.solution : undefined
savedObject.attributes.disabledFeatures?.flatMap((featureId: string) =>
Array.from(this.deprecatedFeaturesReferences.get(featureId) ?? [featureId])
) ?? [],
solution
),
_reserved: savedObject.attributes._reserved,
...(!this.isServerless ? { solution: savedObject.attributes.solution } : {}),
...(solution ? { solution } : {}),
} as v1.Space;
};

Expand All @@ -275,4 +289,41 @@ export class SpacesClient implements ISpacesClient {
...(!this.isServerless && space.solution ? { solution: space.solution } : {}),
};
};

/**
* Collects a map of all deprecated feature IDs and the feature IDs that replace them.
* @param features A list of all available Kibana features including deprecated ones.
*/
private collectDeprecatedFeaturesReferences(features: KibanaFeature[]) {
const deprecatedFeatureReferences = new Map();
for (const feature of features) {
if (!feature.deprecated || !feature.scope?.includes(KibanaFeatureScope.Spaces)) {
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) =>
subFeature.privilegeGroups.flatMap(({ privileges }) => privileges)
) ?? []
);

// Collect all features IDs that are referenced by the deprecated feature privileges.
const referencedFeaturesIds = new Set();
for (const privilege of allPrivileges) {
const replacedBy = privilege.replacedBy
? 'default' in privilege.replacedBy
? privilege.replacedBy.default.concat(privilege.replacedBy.minimal)
: privilege.replacedBy
: [];
for (const privilegeReference of replacedBy) {
referencedFeaturesIds.add(privilegeReference.feature);
}
}

deprecatedFeatureReferences.set(feature.id, referencedFeaturesIds);
}

return deprecatedFeatureReferences;
}
}
Loading

0 comments on commit deeb9fe

Please sign in to comment.