Skip to content
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.

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

[Security Solution][Endpoint] Add validation to artifact create/update APIs for management of ownerSpaceId #211325

Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
f63eaef
Add new `canManageGlobalArtifacts` endpoint authz property
paul-tavares Feb 14, 2025
cd80b4a
Add new privilege to Fleet authz
paul-tavares Feb 14, 2025
03baa08
Add validators for ownerSpaceId for update/create operations
paul-tavares Feb 14, 2025
92f8f33
Add validation for ownerSpaceId to `update` operation
paul-tavares Feb 14, 2025
08a8f19
Fix word-wrapping css class name
paul-tavares Feb 14, 2025
b87f818
Fix UI artifact forms to display submit errors on the UI (bug fix)
paul-tavares Feb 14, 2025
2e72a18
Add validation for owner space id to create operation
paul-tavares Feb 14, 2025
1a8401c
Fix artifact forms so that they preserve non-assignment tags
paul-tavares Feb 14, 2025
79c677d
add additional debug to response actions base class `writeActionReque…
paul-tavares Feb 19, 2025
56deafc
Fix logic for `validateCreateOwnerSpaceIds()`
paul-tavares Feb 19, 2025
ae25877
tests for base validator new methods for space owner tags
paul-tavares Feb 19, 2025
10dd807
Unit tests for new authz property
paul-tavares Feb 19, 2025
7cdfaec
Add new `isOwnerSpaceIdTag()` utility
paul-tavares Feb 19, 2025
4797cf8
Update `useGetUpdatedTags()` hook to only update tag types that it is…
paul-tavares Feb 19, 2025
51dd985
Update all artifact forms to use `useGetUpdatedTags()` hook
paul-tavares Feb 19, 2025
d8e04db
Merge remote-tracking branch 'upstream/main' into task/olm-11873-spac…
paul-tavares Feb 20, 2025
0717a1a
Tests for blocklist form
paul-tavares Feb 20, 2025
9f17a7e
Tests for event filters form
paul-tavares Feb 20, 2025
a38a031
tests for trusted apps form
paul-tavares Feb 20, 2025
c3f53a7
use `createHttpFetchError()`
paul-tavares Feb 20, 2025
46bedfe
Update tests for `useGetUpdatedTags()`
paul-tavares Feb 20, 2025
275203e
test for host isolation exceptions (not working)
paul-tavares Feb 20, 2025
1da69e8
Merge remote-tracking branch 'upstream/main' into task/olm-11873-spac…
paul-tavares Feb 20, 2025
a34fe87
Fix test failures
paul-tavares Feb 21, 2025
2a8b696
Merge remote-tracking branch 'upstream/main' into task/olm-11873-spac…
paul-tavares Feb 21, 2025
fd0f780
Moved artifact spaces API FTR test to its own file
paul-tavares Feb 21, 2025
a857083
Added test to ensure active space id is added on create even if paylo…
paul-tavares Feb 21, 2025
b057e8e
add additional methods to the role and user loader script module
paul-tavares Feb 21, 2025
70a4692
Refactored artifacts space tests to include testing with and without …
paul-tavares Feb 21, 2025
a756416
Merge branch 'main' into task/olm-11873-spaces-restrict-artifact-spac…
paul-tavares Feb 21, 2025
6cc224a
[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…
kibanamachine Feb 21, 2025
55a9d00
Merge remote-tracking branch 'upstream/main' into task/olm-11873-spac…
paul-tavares Feb 24, 2025
d515cbc
Skip ftr test runs on serverless for now
paul-tavares Feb 24, 2025
dbb8c77
Skip test (again)
paul-tavares Feb 24, 2025
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
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,12 @@ export const ENDPOINT_PRIVILEGES: Record<string, PrivilegeMapObject> = deepFreez
privilegeType: 'api',
privilegeName: 'readEventFilters',
},
writeGlobalArtifacts: {
appId: DEFAULT_APP_CATEGORIES.security.id,
privilegeSplit: '-',
privilegeType: 'api',
privilegeName: 'writeGlobalArtifacts',
},
writePolicyManagement: {
appId: DEFAULT_APP_CATEGORIES.security.id,
privilegeSplit: '-',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ export const getPolicyIdsFromArtifact = (item: Pick<ExceptionListItemSchema, 'ta
return policyIds;
};

/**
* Given an Artifact tag value, utility will return a boolean indicating if that tag is
* tracking artifact assignment (global/per-policy)
*/
export const isPolicySelectionTag: TagFilter = (tag) =>
tag.startsWith(BY_POLICY_ARTIFACT_TAG_PREFIX) || tag === GLOBAL_ARTIFACT_TAG;

Expand Down Expand Up @@ -79,13 +83,16 @@ export const getEffectedPolicySelectionByTags = (
};
}
const selected: PolicyData[] = tags.reduce((acc, tag) => {
// edge case: a left over tag with a non-existed policy
// will be removed by verifying the policy exists
const id = tag.split(':')[1];
const foundPolicy = policies.find((policy) => policy.id === id);
if (foundPolicy !== undefined) {
acc.push(foundPolicy);
if (tag.startsWith(BY_POLICY_ARTIFACT_TAG_PREFIX)) {
const id = tag.split(':')[1];
const foundPolicy = policies.find((policy) => policy.id === id);

// edge case: a left over tag with a non-existed policy will be removed by verifying the policy exists
if (foundPolicy !== undefined) {
acc.push(foundPolicy);
}
}

return acc;
}, [] as PolicyData[]);

Expand Down Expand Up @@ -120,14 +127,22 @@ export const createExceptionListItemForCreate = (listId: string): CreateExceptio
};
};

/**
* Checks the provided `tag` string to see if it is an owner apace ID tag
* @param tag
*/
export const isOwnerSpaceIdTag = (tag: string): boolean => {
return tag.startsWith(OWNER_SPACE_ID_TAG_PREFIX);
};

/**
* Returns an array with all owner space IDs for the artifact
*/
export const getArtifactOwnerSpaceIds = (
item: Partial<Pick<ExceptionListItemSchema, 'tags'>>
): string[] => {
return (item.tags ?? []).reduce((acc, tag) => {
if (tag.startsWith(OWNER_SPACE_ID_TAG_PREFIX)) {
if (isOwnerSpaceIdTag(tag)) {
acc.push(tag.substring(OWNER_SPACE_ID_TAG_PREFIX.length));
}

Expand Down Expand Up @@ -176,5 +191,5 @@ export const setArtifactOwnerSpaceId = (
export const hasArtifactOwnerSpaceId = (
item: Partial<Pick<ExceptionListItemSchema, 'tags'>>
): boolean => {
return (item.tags ?? []).some((tag) => tag.startsWith(OWNER_SPACE_ID_TAG_PREFIX));
return (item.tags ?? []).some((tag) => isOwnerSpaceIdTag(tag));
};
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ describe('Endpoint Authz service', () => {
['canReadEventFilters', 'readEventFilters'],
['canReadWorkflowInsights', 'readWorkflowInsights'],
['canWriteWorkflowInsights', 'writeWorkflowInsights'],
['canManageGlobalArtifacts', 'writeGlobalArtifacts'],
])('%s should be true if `packagePrivilege.%s` is `true`', (auth) => {
const authz = calculateEndpointAuthz(licenseService, fleetAuthz, userRoles);
expect(authz[auth]).toBe(true);
Expand Down Expand Up @@ -220,6 +221,7 @@ describe('Endpoint Authz service', () => {
['canReadEventFilters', ['readEventFilters']],
['canWriteWorkflowInsights', ['writeWorkflowInsights']],
['canReadWorkflowInsights', ['readWorkflowInsights']],
['canManageGlobalArtifacts', ['writeGlobalArtifacts']],
// all dependent privileges are false and so it should be false
['canAccessResponseConsole', responseConsolePrivileges],
])('%s should be false if `packagePrivilege.%s` is `false`', (auth, privileges) => {
Expand Down Expand Up @@ -271,6 +273,7 @@ describe('Endpoint Authz service', () => {
['canReadEventFilters', ['readEventFilters']],
['canWriteWorkflowInsights', ['writeWorkflowInsights']],
['canReadWorkflowInsights', ['readWorkflowInsights']],
['canManageGlobalArtifacts', ['writeGlobalArtifacts']],
// all dependent privileges are false and so it should be false
['canAccessResponseConsole', responseConsolePrivileges],
])(
Expand Down Expand Up @@ -339,6 +342,7 @@ describe('Endpoint Authz service', () => {
canWriteExecuteOperations: false,
canWriteScanOperations: false,
canWriteFileOperations: false,
canManageGlobalArtifacts: false,
canWriteTrustedApplications: false,
canWriteWorkflowInsights: false,
canReadTrustedApplications: false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,8 @@ export const calculateEndpointAuthz = (
const canReadEndpointExceptions = hasAuth('showEndpointExceptions');
const canWriteEndpointExceptions = hasAuth('crudEndpointExceptions');

const canManageGlobalArtifacts = hasAuth('writeGlobalArtifacts');

const canReadWorkflowInsights = hasAuth('readWorkflowInsights');
const canWriteWorkflowInsights = hasAuth('writeWorkflowInsights');

Expand Down Expand Up @@ -156,6 +158,7 @@ export const calculateEndpointAuthz = (
canReadEventFilters,
canReadEndpointExceptions,
canWriteEndpointExceptions,
canManageGlobalArtifacts,
};

// Response console is only accessible when license is Enterprise and user has access to any
Expand Down Expand Up @@ -212,6 +215,7 @@ export const getEndpointAuthzInitialState = (): EndpointAuthz => {
canReadEventFilters: false,
canReadEndpointExceptions: false,
canWriteEndpointExceptions: false,
canManageGlobalArtifacts: false,
canReadWorkflowInsights: false,
canWriteWorkflowInsights: false,
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,9 @@ export interface EndpointAuthz {
canReadEndpointExceptions: boolean;
/** if the user has read permissions for endpoint exceptions */
canWriteEndpointExceptions: boolean;
/** If user is allowed to manage global artifacts. Introduced support for spaces feature */
canManageGlobalArtifacts: boolean;

/** if the user has write permissions for workflow insights */
canWriteWorkflowInsights: boolean;
/** if the user has read permissions for workflow insights */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ export const ObjectContent = memo<ObjectContentProps>(({ data }) => {
<EuiText size="s">
{Object.entries(data).map(([key, value]) => {
return (
<div key={key} className="eui-textBreakAll">
<div key={key} className="eui-textBreakWord">
<strong>{key}</strong>
{': '}
{value}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ describe('useGetUpdatedTags hook', () => {
// add first
rerender({ exception: { tags }, filters: getFiltersInOrder() });
tags = result.current.getTagsUpdatedBy('first', ['first:brie']);
expect(tags).toStrictEqual(['first:brie', 'special_second', 'third:spaghetti']);
expect(tags).toStrictEqual(['special_second', 'third:spaghetti', 'first:brie']);
});

it('should update category order on any change if filter is changed (although it should not)', () => {
Expand All @@ -155,11 +155,9 @@ describe('useGetUpdatedTags hook', () => {
expect(tags).toStrictEqual([
'first:mozzarella',
'first:roquefort',

'second:shiraz',

'third:tagliatelle',
'third:penne',
'second:shiraz',
]);

const newFilterOrder = {
Expand All @@ -172,12 +170,10 @@ describe('useGetUpdatedTags hook', () => {
tags = result.current.getTagsUpdatedBy('third', ['third:spaghetti']);

expect(tags).toStrictEqual([
'third:spaghetti',

'first:mozzarella',
'first:roquefort',

'second:shiraz',
'third:spaghetti',
]);
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,20 +8,31 @@
import type { ExceptionListItemSchema } from '@kbn/securitysolution-io-ts-list-types';
import { useCallback } from 'react';
import type { TagFilter } from '../../../../common/endpoint/service/artifacts/utils';
import {
isOwnerSpaceIdTag,
isFilterProcessDescendantsTag,
isPolicySelectionTag,
} from '../../../../common/endpoint/service/artifacts/utils';

type TagFiltersType = {
[tagCategory in string]: TagFilter;
};
interface TagFiltersType {
[tagCategory: string]: TagFilter;
}

type GetTagsUpdatedBy<TagFilters> = (tagType: keyof TagFilters, newTags: string[]) => string[];

type GetTagsUpdatedBy<TagFilters> = (tagsToUpdate: keyof TagFilters, newTags: string[]) => string[];
const DEFAULT_FILTERS = Object.freeze({
policySelection: isPolicySelectionTag,
processDescendantsFiltering: isFilterProcessDescendantsTag,
ownerSpaceId: isOwnerSpaceIdTag,
} as const);

/**
* A hook to be used to generate a new `tags` array that contains multiple 'categories' of tags,
* e.g. policy assignment, some special settings, in a desired order.
* A hook that returns a callback for using in updating the complete list of `tags` on an artifact.
* The callback will replace a given type of tag with new set of values - example: update the list
* of tags on an artifact with a new list of policy assignment tags.
*
* The hook excepts a `filter` object that contain a simple filter function for every tag
* category. The `filter` object should contain the filters in the same ORDER as the categories
* should appear in the `tags` array.
* The hook uses a `filter` object (can be overwritten on input) that contain a simple filter
* function that is used to identify tags for that category.
*
* ```
* const FILTERS_IN_ORDER = { // preferably defined out of the component
Expand All @@ -42,25 +53,21 @@ type GetTagsUpdatedBy<TagFilters> = (tagsToUpdate: keyof TagFilters, newTags: st
* @param filters
* @returns `getTagsUpdatedBy(tagCategory, ['new', 'tags'])`
*/
export const useGetUpdatedTags = <TagFilters extends TagFiltersType>(
export const useGetUpdatedTags = <TagFilters extends TagFiltersType = typeof DEFAULT_FILTERS>(
exception: Partial<Pick<ExceptionListItemSchema, 'tags'>>,
filters: TagFilters
): {
filters: TagFilters = DEFAULT_FILTERS as unknown as TagFilters
): Readonly<{
getTagsUpdatedBy: GetTagsUpdatedBy<TagFilters>;
} => {
}> => {
const getTagsUpdatedBy: GetTagsUpdatedBy<TagFilters> = useCallback(
(tagsToUpdate, newTags) => {
const tagCategories = Object.keys(filters);

const arrayOfTagArrays: string[][] = tagCategories.map((category) => {
if (tagsToUpdate === category) {
return newTags;
}

return (exception.tags ?? []).filter(filters[category]);
});
(tagType, newTags) => {
if (!filters[tagType]) {
throw new Error(
`getTagsUpdateBy() was called with an unknown tag type: ${String(tagType)}`
);
}

return arrayOfTagArrays.flat();
return (exception.tags ?? []).filter((tag) => !filters[tagType](tag)).concat(...newTags);
},
[exception, filters]
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import type { PolicyData } from '../../../../../../common/endpoint/types';
import { GLOBAL_ARTIFACT_TAG } from '../../../../../../common/endpoint/service/artifacts';
import { ListOperatorEnum, ListOperatorTypeEnum } from '@kbn/securitysolution-io-ts-list-types';
import { ENDPOINT_ARTIFACT_LISTS } from '@kbn/securitysolution-list-constants';
import { createHttpFetchError } from '@kbn/core-http-browser-mocks';

jest.mock('../../../../../common/hooks/use_license', () => {
const licenseServiceInstance = {
Expand Down Expand Up @@ -491,7 +492,7 @@ describe('blocklist form', () => {
expect(screen.getByTestId('blocklist-form-effectedPolicies-global')).toBeEnabled();
});

it('should correctly edit policies', async () => {
it('should correctly edit policies and retain all other tags', async () => {
const policies: PolicyData[] = [
{
id: 'policy-id-123',
Expand All @@ -502,18 +503,18 @@ describe('blocklist form', () => {
name: 'some-policy-456',
},
] as PolicyData[];
render(createProps({ policies }));
render(createProps({ policies, item: createItem({ tags: ['some:random_tag'] }) }));
const byPolicyButton = screen.getByTestId('blocklist-form-effectedPolicies-perPolicy');
await user.click(byPolicyButton);
expect(byPolicyButton).toBeEnabled();

await user.click(screen.getByText(policies[1].name));
const expected = createOnChangeArgs({
item: createItem({
tags: [`policy:${policies[1].id}`],
tags: ['some:random_tag', `policy:${policies[1].id}`],
}),
});
expect(onChangeSpy).toHaveBeenCalledWith(expected);
expect(onChangeSpy).toHaveBeenLastCalledWith(expected);
});

it('should correctly retain selected policies when toggling between global/by policy', async () => {
Expand Down Expand Up @@ -567,4 +568,11 @@ describe('blocklist form', () => {
});
expect(onChangeSpy).toHaveBeenCalledWith(expected);
});

it('should display submit errors', async () => {
const message = 'foo - something went wrong';
const { getByTestId } = render(createProps({ error: createHttpFetchError(message) }));

expect(getByTestId('blocklist-form-submitError').textContent).toMatch(message);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import { isOneOfOperator, isOperator } from '@kbn/securitysolution-list-utils';
import { uniq } from 'lodash';

import { ListOperatorEnum, ListOperatorTypeEnum } from '@kbn/securitysolution-io-ts-list-types';
import { FormattedError } from '../../../../components/formatted_error';
import { OS_TITLES } from '../../../../common/translations';
import type {
ArtifactFormComponentOnChangeCallbackProps,
Expand Down Expand Up @@ -62,6 +63,7 @@ import {
} from '../../../../../../common/endpoint/service/artifacts';
import type { PolicyData } from '../../../../../../common/endpoint/types';
import { useTestIdGenerator } from '../../../../hooks/use_test_id_generator';
import { useGetUpdatedTags } from '../../../../hooks/artifacts';

const testIdPrefix = 'blocklist-form';

Expand Down Expand Up @@ -113,7 +115,7 @@ function isValid(itemValidation: ItemValidation): boolean {

// eslint-disable-next-line react/display-name
export const BlockListForm = memo<ArtifactFormComponentProps>(
({ item, policies, policiesIsLoading, onChange, mode }) => {
({ item, policies, policiesIsLoading, onChange, mode, error: submitError }) => {
const [nameVisited, setNameVisited] = useState(false);
const [valueVisited, setValueVisited] = useState({ value: false }); // Use object to trigger re-render
const warningsRef = useRef<ItemValidation>({ name: {}, value: {} });
Expand All @@ -123,6 +125,7 @@ export const BlockListForm = memo<ArtifactFormComponentProps>(
const isGlobal = useMemo(() => isArtifactGlobal(item), [item]);
const [wasByPolicy, setWasByPolicy] = useState(!isArtifactGlobal(item));
const [hasFormChanged, setHasFormChanged] = useState(false);
const { getTagsUpdatedBy } = useGetUpdatedTags(item);

const showAssignmentSection = useMemo(() => {
return (
Expand Down Expand Up @@ -546,8 +549,7 @@ export const BlockListForm = memo<ArtifactFormComponentProps>(

const handleOnPolicyChange = useCallback(
(change: EffectedPolicySelection) => {
const tags = getArtifactTagsByPolicySelection(change);

const tags = getTagsUpdatedBy('policySelection', getArtifactTagsByPolicySelection(change));
const nextItem = { ...item, tags };

// Preserve old selected policies when switching to global
Expand All @@ -561,11 +563,19 @@ export const BlockListForm = memo<ArtifactFormComponentProps>(
});
setHasFormChanged(true);
},
[validateValues, onChange, item]
[getTagsUpdatedBy, item, validateValues, onChange]
);

return (
<EuiForm component="div">
<EuiForm
component="div"
error={
submitError ? (
<FormattedError error={submitError} data-test-subj={getTestId('submitError')} />
) : undefined
}
isInvalid={!!submitError}
>
<EuiTitle size="xs">
<h3>{DETAILS_HEADER}</h3>
</EuiTitle>
Expand Down
Loading