Skip to content

[Security Solution] UI trusted applications RBAC #145593

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
gergoabraham merged 6 commits into from
Nov 23, 2022
Merged

[Security Solution] UI trusted applications RBAC #145593

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4ed8187
hide Trusted Apps from manage nav
gergoabraham Nov 17, 2022
c20c08e
some refactor on `links.test.ts`
gergoabraham Nov 18, 2022
763893a
disable add/edit/delete for Trusted Applications for `READ` privilege
gergoabraham Nov 21, 2022
4814c22
Merge branch 'main' into feature/olm-4913-ui-trusted-applications-rbac
gergoabraham Nov 22, 2022
0c09f1a
fix falsely passing unit test
gergoabraham Nov 23, 2022
dd367cc
reset mock after each test case
gergoabraham Nov 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 61 additions & 23 deletions x-pack/plugins/security_solution/public/management/links.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@ describe('links', () => {
let getPlugins: (roles: string[]) => StartPlugins;
let fakeHttpServices: jest.Mocked<HttpSetup>;

const getLinksWithout = (...excludedLinks: SecurityPageName[]) => ({
Copy link
Copy Markdown
Contributor

@dasansol92 dasansol92 Nov 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be worth using the without function from lodash?

Copy link
Copy Markdown
Contributor Author

@gergoabraham gergoabraham Nov 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't familiar with without so thanks for the suggestion! However, it looks like that without uses equality comparison on objects, so I don't think it's possible to filter out objects based on their attributes, link.id here.

...links,
links: links.links?.filter((link) => !excludedLinks.includes(link.id)),
});

Comment on lines +35 to +39
Copy link
Copy Markdown
Member

@ashokaditya ashokaditya Nov 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice 🚀

beforeAll(() => {
ExperimentalFeaturesService.init({
experimentalFeatures: { ...allowedExperimentalValues },
Expand Down Expand Up @@ -103,10 +108,7 @@ describe('links', () => {
coreMockStarted,
getPlugins(['superuser'])
);
expect(filteredLinks).toEqual({
...links,
links: links.links?.filter((link) => link.id !== SecurityPageName.hostIsolationExceptions),
});
expect(filteredLinks).toEqual(getLinksWithout(SecurityPageName.hostIsolationExceptions));
});

it('should return all but HIE when NO isolation permission due to license and NO host isolation exceptions entry', async () => {
Expand All @@ -123,10 +125,7 @@ describe('links', () => {
coreMockStarted,
getPlugins(['superuser'])
);
expect(filteredLinks).toEqual({
...links,
links: links.links?.filter((link) => link.id !== SecurityPageName.hostIsolationExceptions),
});
expect(filteredLinks).toEqual(getLinksWithout(SecurityPageName.hostIsolationExceptions));
});

it('should return all but HIE when HAS isolation permission AND has HIE entry but not superuser', async () => {
Expand All @@ -143,10 +142,7 @@ describe('links', () => {
coreMockStarted,
getPlugins(['superuser'])
);
expect(filteredLinks).toEqual({
...links,
links: links.links?.filter((link) => link.id !== SecurityPageName.hostIsolationExceptions),
});
expect(filteredLinks).toEqual(getLinksWithout(SecurityPageName.hostIsolationExceptions));
});

it('should return all when NO isolation permission due to license but HAS at least one host isolation exceptions entry', async () => {
Expand Down Expand Up @@ -177,10 +173,7 @@ describe('links', () => {
coreMockStarted,
getPlugins(['superuser'])
);
expect(filteredLinks).toEqual({
...links,
links: links.links?.filter((link) => link.id !== SecurityPageName.hostIsolationExceptions),
});
expect(filteredLinks).toEqual(getLinksWithout(SecurityPageName.hostIsolationExceptions));
});

it('should not affect hiding Action Log if getting from HIE API throws error', async () => {
Expand All @@ -196,15 +189,60 @@ describe('links', () => {
coreMockStarted,
getPlugins(['superuser'])
);
expect(filteredLinks).toEqual({
...links,
links: links.links?.filter(
(link) =>
link.id !== SecurityPageName.hostIsolationExceptions &&
link.id !== SecurityPageName.responseActionsHistory
),
expect(filteredLinks).toEqual(
getLinksWithout(
SecurityPageName.hostIsolationExceptions,
SecurityPageName.responseActionsHistory
)
);
});
});

// this can be removed after removing endpointRbacEnabled feature flag
describe('without endpointRbacEnabled', () => {
beforeAll(() => {
ExperimentalFeaturesService.init({
experimentalFeatures: { ...allowedExperimentalValues, endpointRbacEnabled: false },
});
});

it('shows Trusted Applications for non-superuser, too', async () => {
(calculateEndpointAuthz as jest.Mock).mockReturnValue(getEndpointAuthzInitialStateMock());

const filteredLinks = await getManagementFilteredLinks(coreMockStarted, getPlugins([]));

expect(filteredLinks).toEqual(links);
});
});

// this can be the default after removing endpointRbacEnabled feature flag
describe('with endpointRbacEnabled', () => {
beforeAll(() => {
ExperimentalFeaturesService.init({
experimentalFeatures: { ...allowedExperimentalValues, endpointRbacEnabled: true },
});
});

it('hides Trusted Applications for user without privilege', async () => {
(calculateEndpointAuthz as jest.Mock).mockReturnValue(
getEndpointAuthzInitialStateMock({
canReadTrustedApplications: false,
canReadHostIsolationExceptions: true,
})
);

const filteredLinks = await getManagementFilteredLinks(coreMockStarted, getPlugins([]));

expect(filteredLinks).toEqual(getLinksWithout(SecurityPageName.trustedApps));
});

it('shows Trusted Applications for user with privilege', async () => {
(calculateEndpointAuthz as jest.Mock).mockReturnValue(getEndpointAuthzInitialStateMock());

const filteredLinks = await getManagementFilteredLinks(coreMockStarted, getPlugins([]));

expect(filteredLinks).toEqual(links);
});
});
describe('Endpoint List', () => {
it('should return all but endpoints link when no Endpoint List READ access', async () => {
Expand Down
30 changes: 19 additions & 11 deletions x-pack/plugins/security_solution/public/management/links.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,17 +273,21 @@ export const getManagementFilteredLinks = async (
);
}

const { canReadActionsLogManagement, canReadHostIsolationExceptions, canReadEndpointList } =
fleetAuthz
? calculateEndpointAuthz(
licenseService,
fleetAuthz,
currentUser.roles,
isEndpointRbacEnabled,
endpointPermissions,
hasHostIsolationExceptions
)
: getEndpointAuthzInitialState();
const {
canReadActionsLogManagement,
canReadHostIsolationExceptions,
canReadEndpointList,
canReadTrustedApplications,
} = fleetAuthz
? calculateEndpointAuthz(
licenseService,
fleetAuthz,
currentUser.roles,
isEndpointRbacEnabled,
endpointPermissions,
hasHostIsolationExceptions
)
: getEndpointAuthzInitialState();

if (!canReadEndpointList) {
linksToExclude.push(SecurityPageName.endpoints);
Expand All @@ -297,5 +301,9 @@ export const getManagementFilteredLinks = async (
linksToExclude.push(SecurityPageName.hostIsolationExceptions);
}

if (endpointRbacEnabled && !canReadTrustedApplications) {
Copy link
Copy Markdown
Member

@ashokaditya ashokaditya Nov 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

endpointRbacEnabled is redundant here, as canReadTrustedApplications is already calculated with isEndpointRbacEnabled which is true if either FF (endpointRbacEnabled, endpointRbacV1Enabled) is true.

Copy link
Copy Markdown
Contributor Author

@gergoabraham gergoabraham Nov 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a slight difference in the operation with and without the feature flag:

  • if RBAC is enabled, we should hide the Trusted Apps from nav,
  • if disabled, Trusted Apps should be visible to non-superusers, too, but they are shown the Privileges Required screen when clicking on it.

So this condition here is to ensure that the operation stays exactly the same if the feature flag is disabled.

Copy link
Copy Markdown
Contributor

@paul-tavares paul-tavares Nov 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gergoabraham ,
I agree with @ashokaditya here. The fact that the TA menu items appear before, even if the user did not have permission to it, was actually not the desired behaviour and is something we been meaning to correct for a while.

Copy link
Copy Markdown
Contributor Author

@gergoabraham gergoabraham Nov 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paul-tavares
Oh, I see, didn't know that it was a not desired behaviour. I'll clean it up in a next RBAC UI PR then. 👍 Another benefit is that there will be less to clean up when removing the feature flag.

Copy link
Copy Markdown
Contributor Author

@gergoabraham gergoabraham Nov 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's here: 8ecd2ba

linksToExclude.push(SecurityPageName.trustedApps);
}

return excludeLinks(linksToExclude);
};
67 changes: 67 additions & 0 deletions ...ns/security_solution/public/management/pages/trusted_apps/view/trusted_apps_list.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,19 @@ import { TrustedAppsList } from './trusted_apps_list';
import { exceptionsListAllHttpMocks } from '../../../mocks/exceptions_list_http_mocks';
import { SEARCHABLE_FIELDS } from '../constants';
import { parseQueryFilterToKQL } from '../../../common/utils';
import { useUserPrivileges } from '../../../../common/components/user_privileges';
import type { EndpointPrivileges } from '../../../../../common/endpoint/types';

jest.mock('../../../../common/components/user_privileges');
const mockUserPrivileges = useUserPrivileges as jest.Mock;

describe('When on the trusted applications page', () => {
let render: () => ReturnType<AppContextTestRender['render']>;
let renderResult: ReturnType<typeof render>;
let history: AppContextTestRender['history'];
let mockedContext: AppContextTestRender;
let apiMocks: ReturnType<typeof exceptionsListAllHttpMocks>;
let mockedEndpointPrivileges: Partial<EndpointPrivileges>;

beforeEach(() => {
mockedContext = createAppRootMockRenderer();
Expand All @@ -35,6 +39,13 @@ describe('When on the trusted applications page', () => {
act(() => {
history.push(TRUSTED_APPS_PATH);
});

mockedEndpointPrivileges = { canWriteTrustedApplications: true };
mockUserPrivileges.mockReturnValue({ endpointPrivileges: mockedEndpointPrivileges });
Comment on lines +43 to +44
Copy link
Copy Markdown
Member

@ashokaditya ashokaditya Nov 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider resetting mockUserPrivileges after each test

Copy link
Copy Markdown
Contributor Author

@gergoabraham gergoabraham Nov 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice catch, thanks!

Copy link
Copy Markdown
Contributor Author

@gergoabraham gergoabraham Nov 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dd367cc

});

afterEach(() => {
mockUserPrivileges.mockReset();
});

it('should search using expected exception item fields', async () => {
Expand All @@ -59,4 +70,60 @@ describe('When on the trusted applications page', () => {
})
);
});

describe('RBAC Trusted Applications', () => {
describe('ALL privilege', () => {
beforeEach(() => {
mockedEndpointPrivileges.canWriteTrustedApplications = true;
});

it('should enable adding entries', async () => {
render();

await waitFor(() =>
expect(renderResult.queryByTestId('trustedAppsListPage-pageAddButton')).toBeTruthy()
);
});

it('should enable modifying/deleting entries', async () => {
render();

const actionsButton = await waitFor(
() => renderResult.getAllByTestId('trustedAppsListPage-card-header-actions-button')[0]
);
userEvent.click(actionsButton);

expect(renderResult.getByTestId('trustedAppsListPage-card-cardEditAction')).toBeTruthy();
expect(renderResult.getByTestId('trustedAppsListPage-card-cardDeleteAction')).toBeTruthy();
});
});

describe('READ privilege', () => {
beforeEach(() => {
mockedEndpointPrivileges.canWriteTrustedApplications = false;
});

it('should disable adding entries', async () => {
render();

await waitFor(() =>
expect(renderResult.queryByTestId('trustedAppsListPage-container')).toBeTruthy()
);

expect(renderResult.queryByTestId('trustedAppsListPage-pageAddButton')).toBeNull();
});

it('should disable modifying/deleting entries', async () => {
render();

await waitFor(() =>
expect(renderResult.queryByTestId('trustedAppsListPage-container')).toBeTruthy()
);

expect(
renderResult.queryByTestId('trustedAppsListPage-card-header-actions-button')
).toBeNull();
});
});
});
});
5 changes: 5 additions & 0 deletions ...plugins/security_solution/public/management/pages/trusted_apps/view/trusted_apps_list.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { FormattedMessage } from '@kbn/i18n-react';
import type { DocLinks } from '@kbn/doc-links';
import { EuiLink } from '@elastic/eui';

import { useUserPrivileges } from '../../../../common/components/user_privileges';
import { useHttp } from '../../../../common/lib/kibana';
import type { ArtifactListPageProps } from '../../../components/artifact_list_page';
import { ArtifactListPage } from '../../../components/artifact_list_page';
Expand Down Expand Up @@ -108,6 +109,7 @@ const TRUSTED_APPS_PAGE_LABELS: ArtifactListPageProps['labels'] = {
};

export const TrustedAppsList = memo(() => {
const { canWriteTrustedApplications } = useUserPrivileges().endpointPrivileges;
const http = useHttp();
const trustedAppsApiClient = TrustedAppsApiClient.getInstance(http);

Expand All @@ -119,6 +121,9 @@ export const TrustedAppsList = memo(() => {
data-test-subj="trustedAppsListPage"
searchableFields={SEARCHABLE_FIELDS}
secondaryPageInfo={<TrustedAppsArtifactsDocsLink />}
allowCardDeleteAction={canWriteTrustedApplications}
allowCardEditAction={canWriteTrustedApplications}
allowCardCreateAction={canWriteTrustedApplications}
/>
);
});
Expand Down