[Cases] Fix an issue with the reopen case permission, add integration tests for failing case #201517
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…or the failing case
kqualters-elastic
added
release_note:skip
|
Pinging @elastic/response-ops (Team:ResponseOps) |
|
Pinging @elastic/security-threat-hunting-investigations (Team:Threat Hunting:Investigations) |
cnasikas
added
the
bug
adcoelho
reviewed
Nov 25, 2024
| @@ -288,15 +288,18 @@ function partitionPatchRequest( | |||
| conflictedCases.push(reqCase); | |||
| // let's try to authorize the conflicted case even though we'll fail after afterwards just in case | |||
| casesToAuthorize.set(foundCase.id, { id: foundCase.id, owner: foundCase.attributes.owner }); | |||
| } else if ( | |||
| } else { | |||
There was a problem hiding this comment.
Could you please also update the tests in x-pack/plugins/cases/server/client/cases/bulk_update.test.ts?
I am thinking specifically about 'checks authorization for both reopenCase and updateCase operations when reopening a case'.
This test only checks that expect(clientArgs.authorization.ensureAuthorized).not.toThrow(); but if we had expect(clientArgs.authorization.ensureAuthorized).toHaveBeenCalledWith(...) we would have caught this bug.
Could you please assert it is called with the proper params?
adcoelho
requested changes
Nov 25, 2024
There was a problem hiding this comment.
The fix looks good but I left a comment about bulk_update.test.ts
nit: I think a smaller change would be to simply leave everything as is and add casesToAuthorize.set(foundCase.id, { id: foundCase.id, owner: foundCase.attributes.owner }); after reopenedCases.push in line 297(before the changes).
adcoelho
reviewed
Nov 25, 2024
| @@ -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 () => { | |||
There was a problem hiding this comment.
why was this completely removed?
There was a problem hiding this comment.
Because it did not add value, this test and the one above do the same thing.
There was a problem hiding this comment.
Ah I see they both do clientArgs.authorization.ensureAuthorized.mockRejectedValue(error); ok, makes sense.
adcoelho
approved these changes
Nov 25, 2024
|
Starting backport for target branches: 8.17 https://github.com/elastic/kibana/actions/runs/12008814245 |
💚 Build Succeeded
Metrics [docs]
History
|
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Nov 25, 2024
… tests for failing case (elastic#201517) ## Summary Currently, the partitionPatchRequest in x-pack/plugins/cases/server/client/cases/bulk_update.ts will not check a case properly if a case is being re-opened, instead of an else if in the loop comparing cases to cases in the request, the status logic should be outside of this set of if statements. If a case is being re-opened, the final else is never run, and casesToAuthorize is 0. This results in ensureAuthorized being called with an empty array of entities, and so the check always succeeds. To fix this, reopenedCases is still populated in the same loop, just not when casesToAuthorize logic is running as well. Also adds some missing tests for this and the create comment permission as requested in elastic#194898. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios (cherry picked from commit 1c3bcea)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
kibanamachine
added a commit
that referenced
this pull request
Nov 25, 2024
…gration tests for failing case (#201517) (#201554) # Backport This will backport the following commits from `main` to `8.17`: - [[Cases] Fix an issue with the reopen case permission, add integration tests for failing case (#201517)](#201517) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Kevin Qualters","email":"56408403+kqualters-elastic@users.noreply.github.com"},"sourceCommit":{"committedDate":"2024-11-25T11:12:43Z","message":"[Cases] Fix an issue with the reopen case permission, add integration tests for failing case (#201517)\n\n## Summary\r\n\r\nCurrently, the partitionPatchRequest in\r\nx-pack/plugins/cases/server/client/cases/bulk_update.ts will not check a\r\ncase properly if a case is being re-opened, instead of an else if in the\r\nloop comparing cases to cases in the request, the status logic should be\r\noutside of this set of if statements. If a case is being re-opened, the\r\nfinal else is never run, and casesToAuthorize is 0. This results in\r\nensureAuthorized being called with an empty array of entities, and so\r\nthe check always succeeds. To fix this, reopenedCases is still populated\r\nin the same loop, just not when casesToAuthorize logic is running as\r\nwell. Also adds some missing tests for this and the create comment\r\npermission as requested in\r\nhttps://github.com//pull/194898.\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios","sha":"1c3bceacc06f5f8a01a2ffde2ec24f114717b396","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:skip","Team:ResponseOps","v9.0.0","Team:Threat Hunting:Investigations","backport:version","v8.17.0"],"title":"[Cases] Fix an issue with the reopen case permission, add integration tests for failing case","number":201517,"url":"https://github.com/elastic/kibana/pull/201517","mergeCommit":{"message":"[Cases] Fix an issue with the reopen case permission, add integration tests for failing case (#201517)\n\n## Summary\r\n\r\nCurrently, the partitionPatchRequest in\r\nx-pack/plugins/cases/server/client/cases/bulk_update.ts will not check a\r\ncase properly if a case is being re-opened, instead of an else if in the\r\nloop comparing cases to cases in the request, the status logic should be\r\noutside of this set of if statements. If a case is being re-opened, the\r\nfinal else is never run, and casesToAuthorize is 0. This results in\r\nensureAuthorized being called with an empty array of entities, and so\r\nthe check always succeeds. To fix this, reopenedCases is still populated\r\nin the same loop, just not when casesToAuthorize logic is running as\r\nwell. Also adds some missing tests for this and the create comment\r\npermission as requested in\r\nhttps://github.com//pull/194898.\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios","sha":"1c3bceacc06f5f8a01a2ffde2ec24f114717b396"}},"sourceBranch":"main","suggestedTargetBranches":["8.17"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/201517","number":201517,"mergeCommit":{"message":"[Cases] Fix an issue with the reopen case permission, add integration tests for failing case (#201517)\n\n## Summary\r\n\r\nCurrently, the partitionPatchRequest in\r\nx-pack/plugins/cases/server/client/cases/bulk_update.ts will not check a\r\ncase properly if a case is being re-opened, instead of an else if in the\r\nloop comparing cases to cases in the request, the status logic should be\r\noutside of this set of if statements. If a case is being re-opened, the\r\nfinal else is never run, and casesToAuthorize is 0. This results in\r\nensureAuthorized being called with an empty array of entities, and so\r\nthe check always succeeds. To fix this, reopenedCases is still populated\r\nin the same loop, just not when casesToAuthorize logic is running as\r\nwell. Also adds some missing tests for this and the create comment\r\npermission as requested in\r\nhttps://github.com//pull/194898.\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios","sha":"1c3bceacc06f5f8a01a2ffde2ec24f114717b396"}},{"branch":"8.17","label":"v8.17.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Kevin Qualters <56408403+kqualters-elastic@users.noreply.github.com>
paulinashakirova
pushed a commit
to paulinashakirova/kibana
that referenced
this pull request
Nov 26, 2024
… tests for failing case (elastic#201517) ## Summary Currently, the partitionPatchRequest in x-pack/plugins/cases/server/client/cases/bulk_update.ts will not check a case properly if a case is being re-opened, instead of an else if in the loop comparing cases to cases in the request, the status logic should be outside of this set of if statements. If a case is being re-opened, the final else is never run, and casesToAuthorize is 0. This results in ensureAuthorized being called with an empty array of entities, and so the check always succeeds. To fix this, reopenedCases is still populated in the same loop, just not when casesToAuthorize logic is running as well. Also adds some missing tests for this and the create comment permission as requested in elastic#194898. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
CAWilson94
pushed a commit
to CAWilson94/kibana
that referenced
this pull request
Dec 12, 2024
… tests for failing case (elastic#201517) ## Summary Currently, the partitionPatchRequest in x-pack/plugins/cases/server/client/cases/bulk_update.ts will not check a case properly if a case is being re-opened, instead of an else if in the loop comparing cases to cases in the request, the status logic should be outside of this set of if statements. If a case is being re-opened, the final else is never run, and casesToAuthorize is 0. This results in ensureAuthorized being called with an empty array of entities, and so the check always succeeds. To fix this, reopenedCases is still populated in the same loop, just not when casesToAuthorize logic is running as well. Also adds some missing tests for this and the create comment permission as requested in elastic#194898. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Currently, the partitionPatchRequest in x-pack/plugins/cases/server/client/cases/bulk_update.ts will not check a case properly if a case is being re-opened, instead of an else if in the loop comparing cases to cases in the request, the status logic should be outside of this set of if statements. If a case is being re-opened, the final else is never run, and casesToAuthorize is 0. This results in
ensureAuthorized being called with an empty array of entities, and so the check always succeeds. To fix this, reopenedCases is still populated in the same loop, just not when casesToAuthorize logic is running as well. Also adds some missing tests for this and the create comment permission as requested in #194898.
Checklist