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.

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

[Cases] Fix an issue with the reopen case permission, add integration tests for failing case #201517

Merged
merged 4 commits into from
Nov 25, 2024
Merged

[Cases] Fix an issue with the reopen case permission, add integration tests for failing case #201517

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4d5b89c
Fix an issue with the reopen case permission, add integration tests f…
kqualters-elastic Nov 25, 2024
44ea3a3
Merge branch 'main' into fix-reopen-case-auth
cnasikas Nov 25, 2024
4d49fe3
Fix bug with SO error objects
cnasikas Nov 25, 2024
53a06e6
PR feedback
cnasikas Nov 25, 2024
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
91 changes: 58 additions & 33 deletions x-pack/plugins/cases/server/client/cases/bulk_update.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1512,6 +1512,59 @@ describe('update', () => {
);
});

it('throws an error if the case is not found', async () => {
clientArgsMock.services.caseService.getCases.mockResolvedValue({ saved_objects: [] });

await expect(
bulkUpdate(
{
cases: [
{
id: mockCases[0].id,
version: mockCases[0].version ?? '',
status: CaseStatuses.open,
},
],
},
clientArgsMock,
casesClientMock
)
).rejects.toThrow(
'Failed to update case, ids: [{"id":"mock-id-1","version":"WzAsMV0="}]: Error: These cases mock-id-1 do not exist. Please check you have the correct ids.'
);
});

it('throws an error if the case is not found and the SO clients returns an SO object', async () => {
clientArgsMock.services.caseService.getCases.mockResolvedValue({
saved_objects: [
{
type: 'cases',
id: 'mock-id-1',
references: [],
error: { error: 'Non found', message: 'Non found', statusCode: 404 },
},
],
});

await expect(
bulkUpdate(
{
cases: [
{
id: mockCases[0].id,
version: mockCases[0].version ?? '',
status: CaseStatuses.open,
},
],
},
clientArgsMock,
casesClientMock
)
).rejects.toThrow(
'Failed to update case, ids: [{"id":"mock-id-1","version":"WzAsMV0="}]: Error: These cases mock-id-1 do not exist. Please check you have the correct ids.'
);
});

describe('Validate max user actions per page', () => {
beforeEach(() => {
jest.clearAllMocks();
Expand Down Expand Up @@ -1681,6 +1734,7 @@ describe('update', () => {
status: CaseStatuses.closed,
},
};

clientArgs.services.caseService.getCases.mockResolvedValue({ saved_objects: [closedCase] });

clientArgs.services.caseService.patchCases.mockResolvedValue({
Expand All @@ -1701,7 +1755,10 @@ describe('update', () => {
casesClientMock
);

expect(clientArgs.authorization.ensureAuthorized).not.toThrow();
expect(clientArgs.authorization.ensureAuthorized).toHaveBeenCalledWith({
entities: [{ id: closedCase.id, owner: closedCase.attributes.owner }],
operation: [Operations.reopenCase, Operations.updateCase],
});
});

it('throws when user is not authorized to update case', async () => {
Expand All @@ -1726,38 +1783,6 @@ describe('update', () => {
`"Failed to update case, ids: [{\\"id\\":\\"mock-id-1\\",\\"version\\":\\"WzAsMV0=\\"}]: Error: Unauthorized"`
);
});

it('throws when user is not authorized to reopen case', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

why was this completely removed?

Copy link
Member

Choose a reason for hiding this comment

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

Because it did not add value, this test and the one above do the same thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see they both do clientArgs.authorization.ensureAuthorized.mockRejectedValue(error); ok, makes sense.

const closedCase = {
...mockCases[0],
attributes: {
...mockCases[0].attributes,
status: CaseStatuses.closed,
},
};
clientArgs.services.caseService.getCases.mockResolvedValue({ saved_objects: [closedCase] });

const error = new Error('Unauthorized to reopen case');
clientArgs.authorization.ensureAuthorized.mockRejectedValueOnce(error); // Reject reopenCase

await expect(
bulkUpdate(
{
cases: [
{
id: closedCase.id,
version: closedCase.version ?? '',
status: CaseStatuses.open,
},
],
},
clientArgs,
casesClientMock
)
).rejects.toThrowErrorMatchingInlineSnapshot(
`"Failed to update case, ids: [{\\"id\\":\\"mock-id-1\\",\\"version\\":\\"WzAsMV0=\\"}]: Error: Unauthorized to reopen case"`
);
});
});
});
});
3 changes: 2 additions & 1 deletion x-pack/plugins/cases/server/client/cases/bulk_update.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,8 +293,9 @@ function partitionPatchRequest(
}

if (
reqCase.status != null &&
foundCase != null &&
!isSOError(foundCase) &&
reqCase.status != null &&
foundCase.attributes.status !== reqCase.status &&
foundCase.attributes.status === CaseStatuses.closed
) {
Expand Down
2 changes: 1 addition & 1 deletion x-pack/test/api_integration/apis/cases/common/users.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ export const secCasesV2NoReopenWithCreateCommentUser: User = {
};

export const secCasesV2NoCreateCommentWithReopenUser: User = {
username: 'sec_cases_v2_create_comment_no_reopen_user_api_int',
username: 'sec_cases_v2_no_create_comment_with_reopen_user_api_int',
password: 'password',
roles: [secCasesV2NoCreateCommentWithReopen.name],
};
Expand Down