-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Cases] [Security Solution] New cases subfeatures, add comments and reopen cases #194898
base: main
Are you sure you want to change the base?
[Cases] [Security Solution] New cases subfeatures, add comments and reopen cases #194898
Conversation
Pinging @elastic/obs-ux-management-team (Team:obs-ux-management) |
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Focussed on features plugin and left a few nits that would be nice to address before merging.
createComment?: readonly string[]; | ||
reopenCases?: readonly string[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: missing doc comments like other interface members
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will update, although I think the grammar for all of these doc blocks is a little unclear. Proposed change for all of them: "List of case owners whose users should have settings access when granted this privilege.", "List of case owners whose users should have comment creation access when granted this privilege." etc. cc @cnasikas
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me. Also could you please add a comment on top of the update
to mention that it does not include reopening a case?
@@ -151,6 +151,14 @@ function mergeWithSubFeatures( | |||
mergedConfig.cases?.settings ?? [], | |||
subFeaturePrivilege.cases?.settings ?? [] | |||
), | |||
createComment: mergeArrays( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: would you mind including these in the relevant unit test in x-pack/plugins/features/server/feature_privilege_iterator/feature_privilege_iterator.test.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ya will update, although that seems like something that typescript should catch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for adding unit tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obs shared LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks okay, will also try to test it later today
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey! I did a first pass of the PR focusing mostly on the backend code. I left some comments.
@@ -171,6 +171,8 @@ export const DELETE_CASES_CAPABILITY = 'delete_cases' as const; | |||
export const PUSH_CASES_CAPABILITY = 'push_cases' as const; | |||
export const CASES_SETTINGS_CAPABILITY = 'cases_settings' as const; | |||
export const CASES_CONNECTORS_CAPABILITY = 'cases_connectors' as const; | |||
export const REOPEN_CASES_CAPABILITY = 'reopen_cases' as const; | |||
export const COMMENT_CASES_CAPABILITY = 'create_comment' as const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super nit: COMMENT_CASES_CAPABILITY
-> CREATE_COMMENT_CAPABILITY
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
createComment?: readonly string[]; | ||
reopenCases?: readonly string[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me. Also could you please add a comment on top of the update
to mention that it does not include reopening a case?
@@ -151,6 +151,14 @@ function mergeWithSubFeatures( | |||
mergedConfig.cases?.settings ?? [], | |||
subFeaturePrivilege.cases?.settings ?? [] | |||
), | |||
createComment: mergeArrays( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for adding unit tests.
@@ -148,6 +148,8 @@ export enum SecuritySubFeatureId { | |||
export enum CasesSubFeatureId { | |||
deleteCases = 'deleteCasesSubFeature', | |||
casesSettings = 'casesSettingsSubFeature', | |||
addComment = 'addCommentSubFeature', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Let's stick with the createComment*
terminology.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
const createCommentOperations = ['createComment'] as const; | ||
const reopenOperations = ['reopenCases'] as const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Because they are single operations I think it is better if we do const createComment = 'createComment'
etc. Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer actually if we left this as is for consistency in the getCasesPrivilege
fn and the way allOperations
is built. Also the naming paradigm operations
maintains that consistency as well. Willing to make the change if you'd still like us to, but think this way is a bit simpler
@@ -51,15 +51,15 @@ describe('useStatusAction', () => { | |||
}, | |||
Object { | |||
"data-test-subj": "cases-bulk-action-status-in-progress", | |||
"disabled": false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep the test with disabled: false
and then add some tests that test the disabled behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -52,47 +52,6 @@ describe('useBulkActions', () => { | |||
Object { | |||
"id": 0, | |||
"items": Array [ | |||
Object { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep the test without removing the actions and then add some tests that test the disabled behavior.
const canChangeStatus = useMemo(() => { | ||
// User has full permissions | ||
if (permissions.update && permissions.reopenCases) { | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we return true
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, had to rethink this a bit
// User has full permissions | ||
if (permissions.update && permissions.reopenCases) { | ||
return false; | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: What do you think of escaping early the if statements?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rethought this file a bit. Hopefully it makes more sense now
import { useCasesContext } from '../cases_context/use_cases_context'; | ||
import { CaseStatuses } from '../../../common/types/domain'; | ||
|
||
export const useUserPermissions = ({ status }: { status?: CaseStatuses }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: status
-> statusToAuthorize
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
obs-ux-management changes LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security Threat Hunting Explore code LGTM
8ec412d
to
10d7756
Compare
@@ -46,7 +46,7 @@ viewer: | |||
- feature_siem.read | |||
- feature_siem.read_alerts | |||
- feature_siem.endpoint_list_read | |||
- feature_securitySolutionCases.read | |||
- feature_securitySolutionCasesV2.read |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these new privileges for all these roles also updated in the elasticsearch-controller repo? The privileges in this file here should be a copy of those. If not, I'd suggest you create a PR on the other repo first, to update the privileges before merging this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The privileges in roles.yml
, security_roles.json
, roles.yml
and project_controller_security_roles.yml
need to be in sync with the ones in elasticsearch-controller
for our testing needs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If not, I'd suggest you create a PR on the other repo first, to update the privileges before merging this PR.
As I explained in https://github.com/elastic/kibana/pull/194898/files#r1801715345, it might be easier to do it the other way around: first merge the PR, wait until the changes reach production, and then drop legacy privileges from the predefined roles. Alternatively, you could update the predefined roles first, but you’d need to keep both old and new privileges since we’ll have Kibana pods running on different versions simultaneously, and later update predefined roles once again to remove legacy privileges.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM from the AppEx Platform Security side (code review only). I’ve left a few minor suggestions and a few items I consider important to address or discuss before merging.
@@ -46,7 +46,7 @@ viewer: | |||
- feature_siem.read | |||
- feature_siem.read_alerts | |||
- feature_siem.endpoint_list_read | |||
- feature_securitySolutionCases.read | |||
- feature_securitySolutionCasesV2.read |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If not, I'd suggest you create a PR on the other repo first, to update the privileges before merging this PR.
As I explained in https://github.com/elastic/kibana/pull/194898/files#r1801715345, it might be easier to do it the other way around: first merge the PR, wait until the changes reach production, and then drop legacy privileges from the predefined roles. Alternatively, you could update the predefined roles first, but you’d need to keep both old and new privileges since we’ll have Kibana pods running on different versions simultaneously, and later update predefined roles once again to remove legacy privileges.
replacedBy: [ | ||
{ | ||
feature: CASES_FEATURE_ID_V2, | ||
privileges: ['minimal_all', 'create_comment', 'case_reopen'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
I believe we agreed in #194898 (comment) to use "extended" syntax to have separate definition for default
and minimal
privilege variants (for both all
and read
)?
catalogue: [APP_ID], | ||
cases: [APP_ID], | ||
privileges: { | ||
all: { | ||
api: apiTags.all, | ||
app: [CASES_FEATURE_ID, 'kibana'], | ||
app: [SECURITY_SOLUTION_CASES_APP_ID, 'kibana'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Warning
Did you intentionally update app
only in all
and keep the old value for read
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, thanks for catching
api: apiTags.all, | ||
id: 'case_reopen', | ||
name: i18n.translate( | ||
'securitySolutionPackages.features.featureRegistry.reopenCaseubFeatureDetails', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tip
Typo
'securitySolutionPackages.features.featureRegistry.reopenCaseubFeatureDetails', | |
'securitySolutionPackages.features.featureRegistry.reopenCaseSubFeatureDetails', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@azasypkin I believe this has been addressed
}; | ||
const casesreopenCaseubFeature: SubFeatureConfig = { | ||
name: i18n.translate( | ||
'securitySolutionPackages.features.featureRegistry.reopenCaseubFeatureName', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tip
Typo (here and in other plugins)
'securitySolutionPackages.features.featureRegistry.reopenCaseubFeatureName', | |
'securitySolutionPackages.features.featureRegistry.reopenCaseSubFeatureName', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this has been addressed
@@ -0,0 +1,178 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note
/rant mode on
Having the sub-feature definitions separated from the feature definitions makes auditing ("what is granting what") so much harder!
/rant mode off
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, this decision predates this work, but completely understand 😅
read: [...filesSavedObjectTypes], | ||
}, | ||
ui: casesCapabilities.all, | ||
replacedBy: [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Same comment here: I believe we agreed in #194898 (comment) to use "extended" syntax to have separate definition for default
and minimal
privilege variants (for both all
and read
)?
@@ -72,6 +72,8 @@ export default function catalogueTests({ getService }: FtrProviderContext) { | |||
uiCapabilities.value!.catalogue, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note
I believe you’ll be able to revert all changes in x-pack/test/ui_capabilities
once I merge #198656. If I merge it before your PR, you’ll need to revert these changes; otherwise, I’ll revert them in my PR before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#198656 is merged, can you please try to revert changes in x-pack/test/ui_capabilities
and see if the tests pass?
uiCapabilities.value!.navLinks; | ||
const { | ||
kibana: _, | ||
securitySolutionCases: __, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be addressed in #198656 and all changes in x-pack/test/ui_capabilities
made in this PR can be reverted.
}); | ||
}); | ||
|
||
it('cases permissions are properly handled for deprecated privileges', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: would it be possible to add another test that validates that a user with the deprecated all
privilege (v1) can still reopen the case and comment on it, as well as a user with the non-deprecated all
privilege? /cc @cnasikas
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to that. This is the most important scenario we would like to test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did another pass on the PR. I hope this is the final one 🙂. I noticed that there are unaddressed comments from the previous reviews. Also, the following files are missing unit tests:
x-pack/plugins/cases/server/authorization/audit_logger.ts
x-pack/plugins/cases/public/components/user_actions/index.tsx
We also missing integration tests for the reopen sub privilege like in x-pack/test/cases_api_integration/security_and_spaces/tests/trial/create_comment_sub_privilege.ts
.
Finally, it would be nice if we could add some tests for the files (you cannot create files if you do not have the create comment subfeature privilege) in x-pack/test/api_integration/apis/cases/files.ts
@@ -40,5 +41,6 @@ export const getApiTags = (owner: Owner): CasesApiTags => { | |||
read, | |||
] as const, | |||
delete: [deleteTag] as const, | |||
createComment: [create, CREATE_COMMENT_API_TAG] as const, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not need the CREATE_COMMENT_API_TAG
, only the create
. The Cases API will perform its own authorization inside, so it is fine not to define any tags for the Cases APIs. For the File Cases API, though, which is not controlled by the cases code, we should have the create
(tag only for files) as you have.
createComment: [create, CREATE_COMMENT_API_TAG] as const, | |
createComment: [create] as const, |
}) | ||
).resolves.not.toThrow(); | ||
|
||
expect(mockLogger.log.mock.calls).toMatchSnapshot(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add a check that we call the checkPrivileges
with the correct requiredPrivileges
? You can do it expect(checkRequestReturningHasAllAsTrue.mock.calls[0]).toMatchInlineSnapshot(...)
. Also, I think is better to have two tests one for checking the operations and one for checking the logging.
}) { | ||
const ownerMsg = owners.length <= 0 ? 'of any owner' : `with owners: "${owners.join(', ')}"`; | ||
const operations = Array.isArray(operation) ? operation : [operation]; | ||
const operationVerbs = [...new Set(operations.map((op) => op.verbs.present))].join(', '); | ||
const operationDocTypes = [...new Set(operations.map((op) => op.docType))].join(', '); | ||
/** | ||
* This will take the form: | ||
* `Unauthorized to create case with owners: "securitySolution, observability"` | ||
* `Unauthorized to access cases of any owner` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Could we add an example for multiple operations?
@@ -96,6 +113,7 @@ export class ProductFeaturesService { | |||
|
|||
const casesProductFeaturesConfig = configurator.cases(); | |||
this.casesProductFeatures.setConfig(casesProductFeaturesConfig); | |||
this.casesProductV2Features.setConfig(casesProductFeaturesConfig); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it ok if the two features have the same configuration (casesProductFeaturesConfig
)? Not sure how the ProductFeaturesService
works tbh 🙂.
@@ -488,6 +488,8 @@ soc_manager: | |||
- application: "kibana-.kibana" | |||
privileges: | |||
- feature_ml.read | |||
- feature_generalCases.all | |||
- feature_generalCasesV2.all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generalCases
and generalCasesV2
features are used for Cases in the stack management page. Stack cases are not available in the Security project. Do we need it here?
}); | ||
}); | ||
|
||
it('cases permissions are properly handled for deprecated privileges', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to that. This is the most important scenario we would like to test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the tests should be owned by ResponseOps and not the Kibana security team as solutions use the framework to deprecate their features. You could add tests in x-pack/test/api_integration/apis/cases/privileges.ts
, create dedicated users (x-pack/test/api_integration/apis/cases/common/users.ts
) and roles (x-pack/test/api_integration/apis/cases/common/roles.ts
) for the new features, and use the old users for the old features. This way you could avoid creating roles on the fly and also test all solutions at the same time. Finally, you can use a lot of our utility functions exported from cases_api_integration/common/lib/api
.
}); | ||
}); | ||
|
||
// Delete |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: The test is for updating.
groupType: 'independent', | ||
privileges: [ | ||
{ | ||
api: apiTags.all, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should remove the tags here.
kibana: [ | ||
{ | ||
feature: { | ||
securitySolutionFixture: ['minimal_all'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be cases_delete
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no_delete right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think is better to keep the old privileges. This way we can be sure that the tests in x-pack/test/api_integration/apis/cases/privileges.ts
still working as expected with the old deprecated features.
@cnasikas for x-pack/plugins/cases/public/components/user_actions/index.tsx , there were no logic changes at all, merely moving some logic to a shared hook that has tests, and renamed a variable. Any new tests would be testing cases that did not exist before, and the existing tests passing without needing any changes should be proof enough of this. |
…es-subfeatures-main
⏳ Build in-progress, with failures
Failed CI Steps
Test Failures
History
|
Summary
This pr adds 2 new sub feature permissions to the cases plugin in stack/security/observability, that behave as follows. The first is for controlling the ability to reopen cases. When Cases has the read permission, and the reopen permission is not enabled, users have permissions as before. When enabled, users can move cases from closed to open/in progress, but nothing else. If a user has all and this permission, they can do anything as before, if the option is unselected, they can change case properties, and change a case from open to anything, in progress to anything, but if the case is closed, are unable to reopen it.
The 2nd permission is 'Add comment'. When enabled and the user has case read permissions, users can add comments, but not make any other changes to the case. When the user has read and this deselected, read functions as before. When a user has this permission and cases is all, this functions as all. When they have all but this permission is deselected, the user can do everything normally, except add cases comments.
Checklist