[Cases] File improvements and other clean up (#154011)

This PR does some clean up and adds some validation.

Notable changes
- Removes unused operation code
- Adds check to ensure that only a single file can be contained in a
file attachment
- Adds some testing
- Modifies the LimitChecker to use the file service to determine how
many files are attached to a case
This commit is contained in:
Jonathan Buttner 2023-04-11 09:58:22 -04:00 committed by GitHub
parent 2459819c62
commit a8341d984a
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
15 changed files with 211 additions and 319 deletions

View file

@ -2184,90 +2184,6 @@ Object {
}
`;
exports[`audit_logger log function event structure creates the correct audit event for operation: "getUserActionStats" with an error and entity 1`] = `
Object {
"error": Object {
"code": "Error",
"message": "an error",
},
"event": Object {
"action": "case_user_action_get_stats",
"category": Array [
"database",
],
"outcome": "failure",
"type": Array [
"access",
],
},
"kibana": Object {
"saved_object": Object {
"id": "1",
"type": "cases-user-actions",
},
},
"message": "Failed attempt to access cases-user-actions [id=1] as owner \\"awesome\\"",
}
`;
exports[`audit_logger log function event structure creates the correct audit event for operation: "getUserActionStats" with an error but no entity 1`] = `
Object {
"error": Object {
"code": "Error",
"message": "an error",
},
"event": Object {
"action": "case_user_action_get_stats",
"category": Array [
"database",
],
"outcome": "failure",
"type": Array [
"access",
],
},
"message": "Failed attempt to access a user actions as any owners",
}
`;
exports[`audit_logger log function event structure creates the correct audit event for operation: "getUserActionStats" without an error but with an entity 1`] = `
Object {
"event": Object {
"action": "case_user_action_get_stats",
"category": Array [
"database",
],
"outcome": "success",
"type": Array [
"access",
],
},
"kibana": Object {
"saved_object": Object {
"id": "5",
"type": "cases-user-actions",
},
},
"message": "User has accessed cases-user-actions [id=5] as owner \\"super\\"",
}
`;
exports[`audit_logger log function event structure creates the correct audit event for operation: "getUserActionStats" without an error or entity 1`] = `
Object {
"event": Object {
"action": "case_user_action_get_stats",
"category": Array [
"database",
],
"outcome": "success",
"type": Array [
"access",
],
},
"message": "User has accessed a user actions as any owners",
}
`;
exports[`audit_logger log function event structure creates the correct audit event for operation: "getUserActionUsers" with an error and entity 1`] = `
Object {
"error": Object {

View file

@ -359,14 +359,6 @@ export const Operations: Record<ReadOperations | WriteOperations, OperationDetai
docType: 'user actions',
savedObjectType: CASE_USER_ACTION_SAVED_OBJECT,
},
[ReadOperations.GetUserActionStats]: {
ecsType: EVENT_TYPES.access,
name: ACCESS_USER_ACTION_OPERATION,
action: 'case_user_action_get_stats',
verbs: accessVerbs,
docType: 'user actions',
savedObjectType: CASE_USER_ACTION_SAVED_OBJECT,
},
[ReadOperations.GetUserActionUsers]: {
ecsType: EVENT_TYPES.access,
name: ACCESS_USER_ACTION_OPERATION,

View file

@ -47,7 +47,6 @@ export enum ReadOperations {
GetCaseMetrics = 'getCaseMetrics',
GetCasesMetrics = 'getCasesMetrics',
GetUserActionMetrics = 'getUserActionMetrics',
GetUserActionStats = 'getUserActionStats',
GetUserActionUsers = 'getUserActionUsers',
}

View file

@ -5,11 +5,7 @@
* 2.0.
*/
import type * as estypes from '@elastic/elasticsearch/lib/api/typesWithBodyKey';
import type { KueryNode } from '@kbn/es-query';
import { CASE_COMMENT_SAVED_OBJECT } from '../../../common/constants';
import type { CommentRequest, CommentType } from '../../../common/api';
import type { AttachmentService } from '../../services';
import type { Limiter } from './types';
interface LimiterParams {
@ -17,46 +13,18 @@ interface LimiterParams {
attachmentType: CommentType;
field: string;
attachmentNoun: string;
filter?: KueryNode;
}
export abstract class BaseLimiter implements Limiter {
public readonly limit: number;
public readonly errorMessage: string;
private readonly limitAggregation: Record<string, estypes.AggregationsAggregationContainer>;
private readonly params: LimiterParams;
constructor(params: LimiterParams) {
this.params = params;
this.limit = params.limit;
this.errorMessage = makeErrorMessage(this.limit, params.attachmentNoun);
this.limitAggregation = {
limiter: {
value_count: {
field: `${CASE_COMMENT_SAVED_OBJECT}.attributes.${params.field}`,
},
},
};
}
public async countOfItemsWithinCase(
attachmentService: AttachmentService,
caseId: string
): Promise<number> {
const itemsAttachedToCase = await attachmentService.executeCaseAggregations<{
limiter: { value: number };
}>({
caseId,
aggregations: this.limitAggregation,
attachmentType: this.params.attachmentType,
filter: this.params.filter,
});
return itemsAttachedToCase?.limiter?.value ?? 0;
}
public abstract countOfItemsWithinCase(caseId: string): Promise<number>;
public abstract countOfItemsInRequest(requests: CommentRequest[]): number;
}

View file

@ -5,13 +5,15 @@
* 2.0.
*/
import { createFileServiceMock } from '@kbn/files-plugin/server/mocks';
import { AttachmentLimitChecker } from '.';
import { createAttachmentServiceMock } from '../../services/mocks';
import { createAlertRequests, createFileRequests, createUserRequests } from './test_utils';
describe('AttachmentLimitChecker', () => {
const mockAttachmentService = createAttachmentServiceMock();
const checker = new AttachmentLimitChecker(mockAttachmentService, 'id');
const mockFileService = createFileServiceMock();
const checker = new AttachmentLimitChecker(mockAttachmentService, mockFileService, 'id');
beforeEach(() => {
jest.clearAllMocks();
@ -23,6 +25,13 @@ describe('AttachmentLimitChecker', () => {
},
};
});
mockFileService.find.mockImplementation(async () => {
return {
files: [],
total: 5,
};
});
});
describe('validate', () => {

View file

@ -7,6 +7,7 @@
import Boom from '@hapi/boom';
import type { FileServiceStart } from '@kbn/files-plugin/server';
import type { CommentRequest } from '../../../common/api';
import type { AttachmentService } from '../../services';
import type { Limiter } from './types';
@ -14,12 +15,15 @@ import { AlertLimiter } from './limiters/alerts';
import { FileLimiter } from './limiters/files';
export class AttachmentLimitChecker {
private readonly limiters: Limiter[] = [new AlertLimiter(), new FileLimiter()];
private readonly limiters: Limiter[];
constructor(
private readonly attachmentService: AttachmentService,
attachmentService: AttachmentService,
fileService: FileServiceStart,
private readonly caseId: string
) {}
) {
this.limiters = [new AlertLimiter(attachmentService), new FileLimiter(fileService)];
}
public async validate(requests: CommentRequest[]) {
for (const limiter of this.limiters) {
@ -27,10 +31,7 @@ export class AttachmentLimitChecker {
const hasItemsInRequests = itemsWithinRequests > 0;
const totalAfterRequests = async () => {
const itemsWithinCase = await limiter.countOfItemsWithinCase(
this.attachmentService,
this.caseId
);
const itemsWithinCase = await limiter.countOfItemsWithinCase(this.caseId);
return itemsWithinRequests + itemsWithinCase;
};

View file

@ -10,7 +10,20 @@ import { AlertLimiter } from './alerts';
import { createAlertRequests, createUserRequests } from '../test_utils';
describe('AlertLimiter', () => {
const alert = new AlertLimiter();
const attachmentService = createAttachmentServiceMock();
attachmentService.executeCaseAggregations.mockImplementation(async () => {
return {
limiter: {
value: 5,
},
};
});
const alert = new AlertLimiter(attachmentService);
beforeEach(() => {
jest.clearAllMocks();
});
describe('public fields', () => {
it('sets the errorMessage to the 1k limit', () => {
@ -62,21 +75,8 @@ describe('AlertLimiter', () => {
});
describe('countOfItemsWithinCase', () => {
const attachmentService = createAttachmentServiceMock();
attachmentService.executeCaseAggregations.mockImplementation(async () => {
return {
limiter: {
value: 5,
},
};
});
beforeEach(() => {
jest.clearAllMocks();
});
it('calls the aggregation function with the correct arguments', async () => {
await alert.countOfItemsWithinCase(attachmentService, 'id');
await alert.countOfItemsWithinCase('id');
expect(attachmentService.executeCaseAggregations.mock.calls[0]).toMatchInlineSnapshot(`
Array [
@ -90,14 +90,13 @@ describe('AlertLimiter', () => {
},
"attachmentType": "alert",
"caseId": "id",
"filter": undefined,
},
]
`);
});
it('returns 5', async () => {
expect(await alert.countOfItemsWithinCase(attachmentService, 'id')).toBe(5);
expect(await alert.countOfItemsWithinCase('id')).toBe(5);
});
});
});

View file

@ -5,14 +5,15 @@
* 2.0.
*/
import type { AttachmentService } from '../../../services';
import { CommentType } from '../../../../common/api';
import type { CommentRequest, CommentRequestAlertType } from '../../../../common/api';
import { MAX_ALERTS_PER_CASE } from '../../../../common/constants';
import { CASE_COMMENT_SAVED_OBJECT, MAX_ALERTS_PER_CASE } from '../../../../common/constants';
import { isCommentRequestTypeAlert } from '../../utils';
import { BaseLimiter } from '../base_limiter';
export class AlertLimiter extends BaseLimiter {
constructor() {
constructor(private readonly attachmentService: AttachmentService) {
super({
limit: MAX_ALERTS_PER_CASE,
attachmentType: CommentType.alert,
@ -21,6 +22,26 @@ export class AlertLimiter extends BaseLimiter {
});
}
public async countOfItemsWithinCase(caseId: string): Promise<number> {
const limitAggregation = {
limiter: {
value_count: {
field: `${CASE_COMMENT_SAVED_OBJECT}.attributes.alertId`,
},
},
};
const itemsAttachedToCase = await this.attachmentService.executeCaseAggregations<{
limiter: { value: number };
}>({
caseId,
aggregations: limitAggregation,
attachmentType: CommentType.alert,
});
return itemsAttachedToCase?.limiter?.value ?? 0;
}
public countOfItemsInRequest(requests: CommentRequest[]): number {
const totalAlertsInReq = requests
.filter<CommentRequestAlertType>(isCommentRequestTypeAlert)

View file

@ -5,12 +5,24 @@
* 2.0.
*/
import { createAttachmentServiceMock } from '../../../services/mocks';
import { createFileServiceMock } from '@kbn/files-plugin/server/mocks';
import { FileLimiter } from './files';
import { createFileRequests, createUserRequests } from '../test_utils';
describe('FileLimiter', () => {
const file = new FileLimiter();
const mockFileService = createFileServiceMock();
mockFileService.find.mockImplementation(async () => {
return {
files: [],
total: 5,
};
});
const file = new FileLimiter(mockFileService);
beforeEach(() => {
jest.clearAllMocks();
});
describe('public fields', () => {
it('sets the errorMessage to the 100 limit', () => {
@ -62,57 +74,25 @@ describe('FileLimiter', () => {
});
describe('countOfItemsWithinCase', () => {
const attachmentService = createAttachmentServiceMock();
attachmentService.executeCaseAggregations.mockImplementation(async () => {
return {
limiter: {
value: 5,
},
};
});
beforeEach(() => {
jest.clearAllMocks();
});
it('calls the aggregation function with the correct arguments', async () => {
await file.countOfItemsWithinCase(attachmentService, 'id');
await file.countOfItemsWithinCase('id');
expect(attachmentService.executeCaseAggregations.mock.calls[0]).toMatchInlineSnapshot(`
expect(mockFileService.find.mock.calls[0]).toMatchInlineSnapshot(`
Array [
Object {
"aggregations": Object {
"limiter": Object {
"value_count": Object {
"field": "cases-comments.attributes.externalReferenceAttachmentTypeId",
},
},
},
"attachmentType": "externalReference",
"caseId": "id",
"filter": Object {
"arguments": Array [
Object {
"isQuoted": false,
"type": "literal",
"value": "cases-comments.attributes.externalReferenceAttachmentTypeId",
},
Object {
"isQuoted": false,
"type": "literal",
"value": ".files",
},
"meta": Object {
"caseIds": Array [
"id",
],
"function": "is",
"type": "function",
},
"perPage": 1,
},
]
`);
});
it('returns 5', async () => {
expect(await file.countOfItemsWithinCase(attachmentService, 'id')).toBe(5);
expect(await file.countOfItemsWithinCase('id')).toBe(5);
});
});
});

View file

@ -5,24 +5,34 @@
* 2.0.
*/
import { buildFilter } from '../../../client/utils';
import { CommentType, FILE_ATTACHMENT_TYPE } from '../../../../common/api';
import type { FileServiceStart } from '@kbn/files-plugin/server';
import { CommentType } from '../../../../common/api';
import type { CommentRequest } from '../../../../common/api';
import { CASE_COMMENT_SAVED_OBJECT, MAX_FILES_PER_CASE } from '../../../../common/constants';
import { MAX_FILES_PER_CASE } from '../../../../common/constants';
import { isFileAttachmentRequest } from '../../utils';
import { BaseLimiter } from '../base_limiter';
export class FileLimiter extends BaseLimiter {
constructor() {
constructor(private readonly fileService: FileServiceStart) {
super({
limit: MAX_FILES_PER_CASE,
attachmentType: CommentType.externalReference,
field: 'externalReferenceAttachmentTypeId',
filter: createFileFilter(),
attachmentNoun: 'files',
});
}
public async countOfItemsWithinCase(caseId: string): Promise<number> {
const files = await this.fileService.find({
perPage: 1,
meta: {
caseIds: [caseId],
},
});
return files.total;
}
public countOfItemsInRequest(requests: CommentRequest[]): number {
let fileRequests = 0;
@ -35,11 +45,3 @@ export class FileLimiter extends BaseLimiter {
return fileRequests;
}
}
const createFileFilter = () =>
buildFilter({
filters: FILE_ATTACHMENT_TYPE,
field: 'externalReferenceAttachmentTypeId',
operator: 'or',
type: CASE_COMMENT_SAVED_OBJECT,
});

View file

@ -6,11 +6,10 @@
*/
import type { CommentRequest } from '../../../common/api';
import type { AttachmentService } from '../../services';
export interface Limiter {
readonly limit: number;
readonly errorMessage: string;
countOfItemsWithinCase(attachmentService: AttachmentService, caseId: string): Promise<number>;
countOfItemsWithinCase(caseId: string): Promise<number>;
countOfItemsInRequest: (requests: CommentRequest[]) => number;
}

View file

@ -258,6 +258,7 @@ export class CaseCommentModel {
const limitChecker = new AttachmentLimitChecker(
this.params.services.attachmentService,
this.params.fileService,
this.caseInfo.id
);

View file

@ -24,5 +24,12 @@ export const registerInternalAttachments = (
};
const schemaValidator = (data: unknown): void => {
pipe(excess(FileAttachmentMetadataRt).decode(data), fold(throwErrors(badRequest), identity));
const fileMetadata = pipe(
excess(FileAttachmentMetadataRt).decode(data),
fold(throwErrors(badRequest), identity)
);
if (fileMetadata.files.length > 1) {
throw badRequest('Only a single file can be stored in an attachment');
}
};

View file

@ -39,7 +39,6 @@ import {
updateCase,
getCaseUserActions,
removeServerGeneratedPropertiesFromUserAction,
bulkCreateAttachments,
} from '../../../../common/lib/api';
import {
createSignalsIndex,
@ -180,13 +179,17 @@ export default ({ getService }: FtrProviderContext): void => {
expect(caseWithAttachments.totalComment).to.be(1);
expect(fileAttachment.externalReferenceMetadata).to.eql(fileAttachmentMetadata);
});
});
});
it('should create a single file attachment with multiple file objects within it', async () => {
describe('unhappy path', () => {
describe('files', () => {
it('400s when attempting to create a single file attachment with multiple file objects within it', async () => {
const postedCase = await createCase(supertest, getPostCaseRequest());
const files = [fileMetadata(), fileMetadata()];
const caseWithAttachments = await createComment({
await createComment({
supertest,
caseId: postedCase.id,
params: getFilesAttachmentReq({
@ -194,38 +197,10 @@ export default ({ getService }: FtrProviderContext): void => {
files,
},
}),
expectedHttpCode: 400,
});
const firstFileAttachment =
caseWithAttachments.comments![0] as CommentRequestExternalReferenceSOType;
expect(caseWithAttachments.totalComment).to.be(1);
expect(firstFileAttachment.externalReferenceMetadata).to.eql({ files });
});
it('should create a file attachments when there are 99 attachments already within the case', async () => {
const fileRequests = [...Array(99).keys()].map(() => getFilesAttachmentReq());
const postedCase = await createCase(supertest, postCaseReq);
await bulkCreateAttachments({
supertest,
caseId: postedCase.id,
params: fileRequests,
});
const caseWith100Files = await createComment({
supertest,
caseId: postedCase.id,
params: getFilesAttachmentReq(),
});
expect(caseWith100Files.comments?.length).to.be(100);
});
});
});
describe('unhappy path', () => {
describe('files', () => {
it('should return a 400 when attaching a file with metadata that is missing the file field', async () => {
const postedCase = await createCase(supertest, getPostCaseRequest());
@ -254,24 +229,6 @@ export default ({ getService }: FtrProviderContext): void => {
expectedHttpCode: 400,
});
});
it('should return a 400 when attempting to create a file attachment when the case already has 100 files attached', async () => {
const fileRequests = [...Array(100).keys()].map(() => getFilesAttachmentReq());
const postedCase = await createCase(supertest, postCaseReq);
await bulkCreateAttachments({
supertest,
caseId: postedCase.id,
params: fileRequests,
});
await createComment({
supertest,
caseId: postedCase.id,
params: getFilesAttachmentReq(),
expectedHttpCode: 400,
});
});
});
it('400s when attempting to create a comment with a different owner than the case', async () => {

View file

@ -38,6 +38,8 @@ import {
updateCase,
getCaseUserActions,
removeServerGeneratedPropertiesFromUserAction,
createAndUploadFile,
deleteAllFiles,
} from '../../../../common/lib/api';
import {
createSignalsIndex,
@ -64,6 +66,7 @@ import {
getAlertById,
} from '../../../../common/lib/alerts';
import { User } from '../../../../common/lib/authentication/types';
import { SECURITY_SOLUTION_FILE_KIND } from '../../../../common/lib/constants';
// eslint-disable-next-line import/no-default-export
export default ({ getService }: FtrProviderContext): void => {
@ -173,30 +176,6 @@ export default ({ getService }: FtrProviderContext): void => {
expect(secondFileAttachment.externalReferenceMetadata).to.eql(fileAttachmentMetadata);
});
it('should create a single file attachment with multiple file objects within it', async () => {
const postedCase = await createCase(supertest, getPostCaseRequest());
const files = [fileMetadata(), fileMetadata()];
const caseWithAttachments = await bulkCreateAttachments({
supertest,
caseId: postedCase.id,
params: [
getFilesAttachmentReq({
externalReferenceMetadata: {
files,
},
}),
],
});
const firstFileAttachment =
caseWithAttachments.comments![0] as CommentRequestExternalReferenceSOType;
expect(caseWithAttachments.totalComment).to.be(1);
expect(firstFileAttachment.externalReferenceMetadata).to.eql({ files });
});
it('should bulk create 100 file attachments', async () => {
const fileRequests = [...Array(100).keys()].map(() => getFilesAttachmentReq());
@ -235,11 +214,108 @@ export default ({ getService }: FtrProviderContext): void => {
params: [postExternalReferenceSOReq, ...fileRequests],
});
});
it('should bulk create 99 file attachments when the case has a file associated to it', async () => {
const postedCase = await createCase(
supertestWithoutAuth,
getPostCaseRequest({ owner: 'securitySolution' }),
200,
{ user: superUser, space: null }
);
await createAndUploadFile({
supertest: supertestWithoutAuth,
createFileParams: {
name: 'testfile',
kind: SECURITY_SOLUTION_FILE_KIND,
mimeType: 'text/plain',
meta: {
caseIds: [postedCase.id],
owner: [postedCase.owner],
},
},
data: 'abc',
auth: { user: superUser, space: null },
});
const fileRequests = [...Array(99).keys()].map(() =>
getFilesAttachmentReq({ owner: 'securitySolution' })
);
await bulkCreateAttachments({
supertest: supertestWithoutAuth,
caseId: postedCase.id,
params: fileRequests,
auth: { user: superUser, space: null },
expectedHttpCode: 200,
});
});
});
});
describe('errors', () => {
describe('files', () => {
afterEach(async () => {
await deleteAllFiles({
supertest,
});
});
it('should return a 400 when attempting to create 100 file attachments when a file associated to the case exists', async () => {
const postedCase = await createCase(
supertestWithoutAuth,
getPostCaseRequest({ owner: 'securitySolution' }),
200,
{ user: superUser, space: null }
);
await createAndUploadFile({
supertest: supertestWithoutAuth,
createFileParams: {
name: 'testfile',
kind: SECURITY_SOLUTION_FILE_KIND,
mimeType: 'text/plain',
meta: {
caseIds: [postedCase.id],
owner: [postedCase.owner],
},
},
data: 'abc',
auth: { user: superUser, space: null },
});
const fileRequests = [...Array(100).keys()].map(() =>
getFilesAttachmentReq({ owner: 'securitySolution' })
);
await bulkCreateAttachments({
supertest: supertestWithoutAuth,
caseId: postedCase.id,
params: fileRequests,
auth: { user: superUser, space: null },
expectedHttpCode: 400,
});
});
it('400s when attempting to create a single file attachment with multiple file objects within it', async () => {
const postedCase = await createCase(supertest, getPostCaseRequest());
const files = [fileMetadata(), fileMetadata()];
await bulkCreateAttachments({
supertest,
caseId: postedCase.id,
params: [
getFilesAttachmentReq({
externalReferenceMetadata: {
files,
},
}),
],
expectedHttpCode: 400,
});
});
it('400s when attaching a file with metadata that is missing the file field', async () => {
const postedCase = await createCase(supertest, getPostCaseRequest());
@ -284,41 +360,6 @@ export default ({ getService }: FtrProviderContext): void => {
expectedHttpCode: 400,
});
});
it('400s when attempting to add a file to a case that already has 100 files', async () => {
const fileRequests = [...Array(100).keys()].map(() => getFilesAttachmentReq());
const postedCase = await createCase(supertest, postCaseReq);
await bulkCreateAttachments({
supertest,
caseId: postedCase.id,
params: fileRequests,
});
await bulkCreateAttachments({
supertest,
caseId: postedCase.id,
params: [getFilesAttachmentReq()],
expectedHttpCode: 400,
});
});
it('400s when the case already has files and the sum of existing and new files exceed 100', async () => {
const fileRequests = [...Array(51).keys()].map(() => getFilesAttachmentReq());
const postedCase = await createCase(supertest, postCaseReq);
await bulkCreateAttachments({
supertest,
caseId: postedCase.id,
params: fileRequests,
});
await bulkCreateAttachments({
supertest,
caseId: postedCase.id,
params: fileRequests,
expectedHttpCode: 400,
});
});
});
it('400s when attempting to create a comment with a different owner than the case', async () => {