Skip to content

Commit e1bbcc5

Browse files
semdelasticmachinekibanamachine
authored
[Security Solution][Navigation] Distinguish between unavailable and unauthorized (elastic#220552)
## Summary Implements different behaviour for unavailable vs unauthorised links. ### Challenge In serverless, the application links implementation did not discriminate between these 2 scenarios: - **unavailable**: The link can not be accessed because it's not available in the current payment plan (aka PLI scenario) - **unauthorized**: The link can not be accessed because the user does not have sufficient privileges (aka RBAC scenario) This happened because, in serverless, both scenarios are checked via `capabilities`, the behaviour for when the capabilities check did not pass was delegated to the components, so we were always doing the same fallback (show no privilege page or redirect to landing) for both scenarios. This is a problem for links that need to do different things in each scenario. ### Proposal The capability check is split to discriminate between both scenarios, and a new computed property is introduced: `unavailable`. Example with `securitySolutionAttackDiscovery` feature: - When all conditions are met, PLI is enabled, and the user has the right privileges, the capabilities are `true`: ![true](https://github.com/user-attachments/assets/5fc37b34-07e3-456c-a503-987d838d89ce) - When we are in the RBAC scenario, where the user role does not have the required privileges, the relevant capabilities are `false`: ![false](https://github.com/user-attachments/assets/f2352335-0105-4b3e-a79f-e1898b727a6d) - In the disabled PLI scenario, the capabilities do not even exist, because they were not registered in the Kibana feature privileges: ![missing](https://github.com/user-attachments/assets/e804e007-a7bf-4247-a254-58b378cc5409) We can distinguish between these scenarios and act accordingly, consistently across the app, without relying on each route component decision (`redirectOnMissing` prop). ### Scenarios - **Available and authorized**: - left nav: shown - global search: shown - content: the page Serverless ![ok_serverless](https://github.com/user-attachments/assets/2e5e8fe8-62cd-4512-807b-28f9743b4ac4) Classic ![ok_classic](https://github.com/user-attachments/assets/824918bc-99d6-4851-8f5d-37da32b2c290) - **Unauthorized**: - left nav: hidden - global search: hidden - content: the generic _NoPrivilege_ page. Before this PR, we were sometimes redirecting to the landing page (when using the `redirectOnMissing` flag). Serverless ![NoPrivilege_serverless](https://github.com/user-attachments/assets/3a0866d5-cee9-4ab5-b9be-7c299eaab06b) Classic ![NoPrivilege_ess](https://github.com/user-attachments/assets/3e32bbec-7a9d-41b0-a376-d7c08be64a51) - **Unavailable**: When the links needs a higher the payment plan - **With Upselling**: (same behaviour) - left nav: shown - global search: hidden - content: the registered upselling component Serverless ![upselling_serverless](https://github.com/user-attachments/assets/db98ca03-0e2b-464b-9dac-750eaeee4981) Classic ![upselling_ess](https://github.com/user-attachments/assets/3c9518da-9eaf-4e35-a9bc-439dc39953da) - **Without Upselling**: - left nav: hidden - global search: hidden - content: redirects to the landing page. Before this PR, we were sometimes showing the _NoPrivilege_ page (when the `redirectOnMissing` flag was missing). Serverless ![no_upselling_serverless](https://github.com/user-attachments/assets/b73853ba-8a9c-4b5e-90a2-d76ca9ee5e6d) Classic ![no_upselling_ess](https://github.com/user-attachments/assets/587b7562-9a5a-4903-992d-ad634ba8f83e) --------- Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: kibanamachine <[email protected]>
1 parent 800340b commit e1bbcc5

File tree

30 files changed

+587
-347
lines changed

30 files changed

+587
-347
lines changed

x-pack/solutions/security/plugins/security_solution/public/app/links/application_links_updater.test.ts

Lines changed: 103 additions & 85 deletions
Original file line numberDiff line numberDiff line change
@@ -10,19 +10,23 @@ import {
1010
type ApplicationLinksUpdateParams,
1111
} from './application_links_updater';
1212
import type { AppLinkItems, LinkItem } from '../../common/links/types';
13-
import { hasCapabilities as mockHasCapabilities } from '../../common/lib/capabilities';
13+
import { hasCapabilities, existCapabilities } from '../../common/lib/capabilities';
1414
import type { Capabilities, IUiSettingsClient } from '@kbn/core/public';
1515
import type { ExperimentalFeatures, SecurityPageName } from '../../../common';
1616
import type { ILicense } from '@kbn/licensing-plugin/public';
1717
import type { UpsellingService } from '@kbn/security-solution-upselling/service';
1818

1919
jest.mock('../../common/lib/capabilities', () => ({
2020
hasCapabilities: jest.fn(),
21+
existCapabilities: jest.fn(),
2122
}));
2223

24+
const mockHasCapabilities = hasCapabilities as jest.Mock;
25+
const mockExistCapabilities = existCapabilities as jest.Mock;
26+
2327
// Allow access to private method just for testing
2428
const appLinks = applicationLinksUpdater as unknown as {
25-
filterAppLinks: (links: AppLinkItems, params: ApplicationLinksUpdateParams) => LinkItem[];
29+
processAppLinks: (links: AppLinkItems, params: ApplicationLinksUpdateParams) => LinkItem[];
2630
};
2731

2832
const link: LinkItem = {
@@ -31,7 +35,7 @@ const link: LinkItem = {
3135
title: 'Test',
3236
};
3337

34-
describe('ApplicationLinks - filterAppLinks', () => {
38+
describe('ApplicationLinksUpdater', () => {
3539
const createMockParams = (
3640
overrides: Partial<ApplicationLinksUpdateParams> = {}
3741
): ApplicationLinksUpdateParams => ({
@@ -51,115 +55,129 @@ describe('ApplicationLinks - filterAppLinks', () => {
5155

5256
beforeEach(() => {
5357
jest.clearAllMocks();
58+
59+
mockHasCapabilities.mockReturnValue(true);
60+
mockExistCapabilities.mockReturnValue(true);
5461
});
5562

56-
it('should include a link when all links are allowed', () => {
57-
(mockHasCapabilities as jest.Mock).mockReturnValue(true);
63+
describe('processAppLinks', () => {
64+
it('should include a link when all links are allowed', () => {
65+
const params = createMockParams();
66+
const result = appLinks.processAppLinks([link], params);
5867

59-
const params = createMockParams();
60-
const result = appLinks.filterAppLinks([link], params);
68+
expect(result).toEqual([link]);
69+
});
6170

62-
expect(result).toEqual([link]);
63-
});
71+
it('should exclude a link when capabilities do not exist and not upsellable', () => {
72+
mockExistCapabilities.mockReturnValue(false);
6473

65-
it('should exclude a link when capabilities are not met and not upsellable', () => {
66-
(mockHasCapabilities as jest.Mock).mockReturnValue(false);
74+
const links: AppLinkItems = [{ ...link, capabilities: ['admin'] }];
6775

68-
const links: AppLinkItems = [{ ...link, capabilities: ['admin'] }];
76+
const params = createMockParams();
77+
const result = appLinks.processAppLinks(links, params);
6978

70-
const params = createMockParams();
71-
const result = appLinks.filterAppLinks(links, params);
79+
expect(result).toEqual([]);
80+
});
7281

73-
expect(result).toEqual([]);
74-
});
82+
it('should mark link as unavailable when capabilities do not exist and it is upsellable', () => {
83+
mockExistCapabilities.mockReturnValue(false);
84+
85+
const links: AppLinkItems = [{ ...link, capabilities: ['advanced_access'] }];
7586

76-
it('should mark a link as unauthorized if upsellable but capabilities fail', () => {
77-
(mockHasCapabilities as jest.Mock).mockReturnValue(false);
87+
const params = createMockParams({
88+
upselling: { isPageUpsellable: jest.fn(() => true) } as unknown as UpsellingService,
89+
});
7890

79-
const links: AppLinkItems = [{ ...link, capabilities: ['advanced_access'] }];
91+
const result = appLinks.processAppLinks(links, params);
8092

81-
const params = createMockParams({
82-
upselling: { isPageUpsellable: jest.fn(() => true) } as unknown as UpsellingService,
93+
expect(result).toEqual([expect.objectContaining({ ...link, unavailable: true })]);
8394
});
8495

85-
const result = appLinks.filterAppLinks(links, params);
96+
it('should mark a link as unauthorized if capabilities exist but requirements are not met', () => {
97+
mockExistCapabilities.mockReturnValue(true);
98+
mockHasCapabilities.mockReturnValue(false);
8699

87-
expect(result).toEqual([expect.objectContaining({ ...link, unauthorized: true })]);
88-
});
100+
const links: AppLinkItems = [{ ...link, capabilities: ['advanced_access'] }];
89101

90-
it('should filter out a link based on uiSettingsClient', () => {
91-
(mockHasCapabilities as jest.Mock).mockReturnValue(true);
102+
const params = createMockParams({
103+
upselling: { isPageUpsellable: jest.fn(() => true) } as unknown as UpsellingService,
104+
});
92105

93-
const links: AppLinkItems = [{ ...link, uiSettingRequired: 'showBeta' }];
106+
const result = appLinks.processAppLinks(links, params);
94107

95-
const params = createMockParams({
96-
uiSettingsClient: { get: jest.fn(() => false) } as unknown as IUiSettingsClient,
108+
expect(result).toEqual([expect.objectContaining({ ...link, unauthorized: true })]);
97109
});
98110

99-
const result = appLinks.filterAppLinks(links, params);
100-
101-
expect(result).toEqual([]);
102-
});
111+
it('should filter out a link based on uiSettingsClient', () => {
112+
const links: AppLinkItems = [{ ...link, uiSettingRequired: 'showBeta' }];
103113

104-
it('should filter out links based on experimental features', () => {
105-
(mockHasCapabilities as jest.Mock).mockReturnValue(true);
114+
const params = createMockParams({
115+
uiSettingsClient: { get: jest.fn(() => false) } as unknown as IUiSettingsClient,
116+
});
106117

107-
const links: AppLinkItems = [
108-
{ ...link, experimentalKey: 'labsEnabled' as keyof ExperimentalFeatures },
109-
];
118+
const result = appLinks.processAppLinks(links, params);
110119

111-
const params = createMockParams({
112-
experimentalFeatures: { labsEnabled: false } as unknown as ExperimentalFeatures,
120+
expect(result).toEqual([]);
113121
});
114122

115-
const result = appLinks.filterAppLinks(links, params);
123+
it('should filter out links based on experimental features', () => {
124+
const links: AppLinkItems = [
125+
{ ...link, experimentalKey: 'labsEnabled' as keyof ExperimentalFeatures },
126+
];
116127

117-
expect(result).toEqual([]);
118-
});
128+
const params = createMockParams({
129+
experimentalFeatures: { labsEnabled: false } as unknown as ExperimentalFeatures,
130+
});
131+
132+
const result = appLinks.processAppLinks(links, params);
119133

120-
it('should recurse and filter child links', () => {
121-
(mockHasCapabilities as jest.Mock).mockImplementation(
122-
(caps, required) => required?.includes('read') ?? true
123-
);
124-
125-
const links: AppLinkItems = [
126-
{
127-
id: 'parent' as SecurityPageName,
128-
path: '/app/parent',
129-
title: 'Parent',
130-
capabilities: ['read'],
131-
links: [
132-
{
133-
id: 'child-1' as SecurityPageName,
134-
title: 'Child 1',
135-
path: '/app/child-1',
136-
capabilities: ['read'],
137-
},
138-
{
139-
id: 'child-2' as SecurityPageName,
140-
title: 'Child 2',
141-
path: '/app/child-2',
142-
capabilities: ['admin'],
143-
},
144-
],
145-
},
146-
];
147-
148-
const params = createMockParams({
149-
upselling: { isPageUpsellable: jest.fn(() => false) } as unknown as UpsellingService,
134+
expect(result).toEqual([]);
150135
});
151136

152-
const result = appLinks.filterAppLinks(links, params);
153-
154-
expect(result).toEqual([
155-
expect.objectContaining({
156-
id: 'parent',
157-
links: [
158-
expect.objectContaining({
159-
id: 'child-1',
160-
}),
161-
],
162-
}),
163-
]);
137+
it('should recurse and filter child links', () => {
138+
mockExistCapabilities.mockImplementation(
139+
(_caps, required) => required?.includes('read') ?? true
140+
);
141+
142+
const links: AppLinkItems = [
143+
{
144+
id: 'parent' as SecurityPageName,
145+
path: '/app/parent',
146+
title: 'Parent',
147+
capabilities: ['read'],
148+
links: [
149+
{
150+
id: 'child-1' as SecurityPageName,
151+
title: 'Child 1',
152+
path: '/app/child-1',
153+
capabilities: ['read'],
154+
},
155+
{
156+
id: 'child-2' as SecurityPageName,
157+
title: 'Child 2',
158+
path: '/app/child-2',
159+
capabilities: ['admin'],
160+
},
161+
],
162+
},
163+
];
164+
165+
const params = createMockParams({
166+
upselling: { isPageUpsellable: jest.fn(() => false) } as unknown as UpsellingService,
167+
});
168+
169+
const result = appLinks.processAppLinks(links, params);
170+
171+
expect(result).toEqual([
172+
expect.objectContaining({
173+
id: 'parent',
174+
links: [
175+
expect.objectContaining({
176+
id: 'child-1',
177+
}),
178+
],
179+
}),
180+
]);
181+
});
164182
});
165183
});

x-pack/solutions/security/plugins/security_solution/public/app/links/application_links_updater.ts

Lines changed: 35 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ import type { UpsellingService } from '@kbn/security-solution-upselling/service'
1313
import type { IUiSettingsClient } from '@kbn/core/public';
1414
import type { ExperimentalFeatures } from '../../../common';
1515
import type { AppLinkItems, LinkItem, NormalizedLinks } from '../../common/links/types';
16-
import { hasCapabilities } from '../../common/lib/capabilities';
16+
import { hasCapabilities, existCapabilities } from '../../common/lib/capabilities';
1717

1818
/**
1919
* Dependencies to update the application links
@@ -51,9 +51,9 @@ class ApplicationLinksUpdater {
5151
* Updates the internal app links applying the filter by permissions
5252
*/
5353
public update(appLinksToUpdate: AppLinkItems, params: ApplicationLinksUpdateParams) {
54-
const filteredAppLinks = this.filterAppLinks(appLinksToUpdate, params);
55-
this.linksSubject$.next(Object.freeze(filteredAppLinks));
56-
this.normalizedLinksSubject$.next(Object.freeze(this.getNormalizedLinks(filteredAppLinks)));
54+
const processedAppLinks = this.processAppLinks(appLinksToUpdate, params);
55+
this.linksSubject$.next(Object.freeze(processedAppLinks));
56+
this.normalizedLinksSubject$.next(Object.freeze(this.getNormalizedLinks(processedAppLinks)));
5757
}
5858

5959
/**
@@ -92,36 +92,54 @@ class ApplicationLinksUpdater {
9292
/**
9393
* Filters the app links based on the links configuration
9494
*/
95-
private filterAppLinks(appLinks: AppLinkItems, params: ApplicationLinksUpdateParams): LinkItem[] {
95+
private processAppLinks(
96+
appLinks: AppLinkItems,
97+
params: ApplicationLinksUpdateParams,
98+
inheritedProps: Partial<LinkItem> = {}
99+
): LinkItem[] {
96100
const { experimentalFeatures, uiSettingsClient, capabilities, license, upselling } = params;
97101

98-
return appLinks.reduce<LinkItem[]>((acc, { links, ...appLinkInfo }) => {
102+
return appLinks.reduce<LinkItem[]>((acc, appLink) => {
103+
const { links, ...link } = appLink;
104+
// Check experimental flags and uiSettings, removing the link if any of them is defined and disabled
99105
if (
100-
!this.isLinkExperimentalKeyAllowed(appLinkInfo, experimentalFeatures) ||
101-
!this.isLinkUiSettingsAllowed(appLinkInfo, uiSettingsClient)
106+
!this.isLinkExperimentalKeyAllowed(link, experimentalFeatures) ||
107+
!this.isLinkUiSettingsAllowed(link, uiSettingsClient)
102108
) {
103109
return acc;
104110
}
105111

112+
// Extra props to be assigned to the current link and its children
113+
const extraProps: Partial<LinkItem> = { ...inheritedProps };
114+
115+
// Check link availability
106116
if (
107-
!hasCapabilities(capabilities, appLinkInfo.capabilities) ||
108-
!this.isLinkLicenseAllowed(appLinkInfo, license)
117+
!existCapabilities(capabilities, link.capabilities) ||
118+
!this.isLinkLicenseAllowed(link, license)
109119
) {
110-
if (upselling.isPageUpsellable(appLinkInfo.id)) {
111-
acc.push({ ...appLinkInfo, unauthorized: true });
120+
// The link is not available in the current product payment plan
121+
if (!upselling.isPageUpsellable(link.id)) {
122+
return acc; // no upselling registered for this link, just exclude it
112123
}
113-
return acc; // not adding sub-links for links that are not authorized
124+
extraProps.unavailable = true;
125+
126+
// Check link authorization only if it is available
127+
} else if (!hasCapabilities(capabilities, link.capabilities)) {
128+
// Required capabilities exist but are not granted
129+
extraProps.unauthorized = true;
114130
}
115131

116-
const resultAppLink: LinkItem = appLinkInfo;
132+
const processedAppLink: LinkItem = { ...link, ...extraProps };
133+
134+
// Process children links if they exist
117135
if (links) {
118-
const childrenLinks = this.filterAppLinks(links, params);
136+
const childrenLinks = this.processAppLinks(links, params, extraProps);
119137
if (childrenLinks.length > 0) {
120-
resultAppLink.links = childrenLinks;
138+
processedAppLink.links = childrenLinks;
121139
}
122140
}
123141

124-
acc.push(resultAppLink);
142+
acc.push(processedAppLink);
125143
return acc;
126144
}, []);
127145
}

0 commit comments

Comments
 (0)