[Entity Analytics] New API endpoint to configure risk engine saved object#194545
[Entity Analytics] New API endpoint to configure risk engine saved object#194545abhishekbhatia1710 wants to merge 8 commits intoelastic:mainfrom
Conversation
💔 Build FailedFailed CI Steps
Test Failures
Metrics [docs]
HistoryTo update your PR or re-run it, just comment with: |
…thub.com/abhishekbhatia1710/kibana into ea-new-api-to-allow-user-to-configure-SO
|
Pinging @elastic/security-solution (Team: SecuritySolution) |
|
Pinging @elastic/security-entity-analytics (Team:Entity Analytics) |
💔 Build Failed
Failed CI StepsTest Failures
Metrics [docs]
History
|
| riskEngineDataClient = new RiskEngineDataClient(options); | ||
| const attributes = { | ||
| enabled: true, | ||
| excludeAlertStatuses: ['closed'], | ||
| excludeAlertTags: ['Duplicate'], | ||
| }; | ||
| mockSavedObjectClient.find.mockResolvedValueOnce(getSavedObjectConfiguration()); | ||
|
|
||
| mockSavedObjectClient.update.mockResolvedValueOnce({ | ||
| attributes, | ||
| } as unknown as SavedObject<RiskEngineConfiguration>); | ||
|
|
||
| const result = await riskEngineDataClient.updateSavedObjectConfiguration({ attributes }); | ||
| expect(result.attributes).toEqual(attributes); | ||
|
|
||
| // Check for the saved object configuration in the non-default space | ||
|
|
||
| options.namespace = namespaces.custom; | ||
| const riskEngineDataClient2 = new RiskEngineDataClient(options); | ||
| const attributes2 = { | ||
| enabled: true, | ||
| excludeAlertStatuses: ['open', 'closed'], | ||
| excludeAlertTags: ['False Positive', 'Duplicate'], | ||
| }; | ||
| mockSavedObjectClient.find.mockResolvedValueOnce(getSavedObjectConfiguration()); | ||
| mockSavedObjectClient.update.mockResolvedValueOnce({ | ||
| attributes, | ||
| } as unknown as SavedObject<RiskEngineConfiguration>); | ||
|
|
||
| const result2 = await riskEngineDataClient2.updateSavedObjectConfiguration({ | ||
| attributes: attributes2, | ||
| }); | ||
| expect(result2.attributes).toEqual(attributes2); |
There was a problem hiding this comment.
Could you split it into 2 different tests?
| enabled?: boolean; | ||
| dataViewId?: string; | ||
| filter?: object; | ||
| identifierType?: string; | ||
| interval?: string; | ||
| pageSize?: number; | ||
| alertSampleSizePerShard?: number; | ||
| range?: { | ||
| start?: string; | ||
| end?: string; | ||
| }; | ||
| excludeAlertStatuses?: Array<'open' | 'closed' | 'in-progress' | 'acknowledged'>; | ||
| excludeAlertTags?: Array<'Duplicate' | 'False Positive' | 'Futher investigation required'>; |
There was a problem hiding this comment.
Please reuse the RiskEngineConfiguration type as much as possible so we have a single source of truth.
| type: riskEngineConfigurationTypeName, | ||
| space: spaceName, | ||
| }); | ||
| if (soId.saved_objects.length !== 0) { | ||
| await kibanaServer.savedObjects.delete({ | ||
| type: riskEngineConfigurationTypeName, | ||
| space: spaceName, | ||
| id: soId.saved_objects[0].id, | ||
| }); | ||
| } | ||
| const soId2 = await kibanaServer.savedObjects.find({ | ||
| type: riskEngineConfigurationTypeName, | ||
| }); | ||
| if (soId2.saved_objects.length !== 0) { | ||
| await kibanaServer.savedObjects.delete({ | ||
| type: riskEngineConfigurationTypeName, | ||
| id: soId2.saved_objects[0].id, | ||
| }); | ||
| } | ||
| await esArchiver.load('x-pack/test/functional/es_archives/security_solution/ecs_compliant'); | ||
| }); | ||
|
|
||
| after(async () => { | ||
| const soId = await kibanaServer.savedObjects.find({ | ||
| type: riskEngineConfigurationTypeName, | ||
| space: spaceName, | ||
| }); | ||
| if (soId.saved_objects.length !== 0) { | ||
| await kibanaServer.savedObjects.delete({ | ||
| type: riskEngineConfigurationTypeName, | ||
| space: spaceName, | ||
| id: soId.saved_objects[0].id, | ||
| }); | ||
| } | ||
| const soId2 = await kibanaServer.savedObjects.find({ | ||
| type: riskEngineConfigurationTypeName, | ||
| }); | ||
| if (soId2.saved_objects.length !== 0) { | ||
| await kibanaServer.savedObjects.delete({ | ||
| type: riskEngineConfigurationTypeName, | ||
| id: soId2.saved_objects[0].id, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Could you please simplify this duplicated code? Maybe extracting a helper is enough to clarify it.
| await riskEngineRoutes.init(); | ||
| await waitForRiskEngineRun; | ||
|
|
||
| const currentSoConfig = await getRiskEngineConfigSO({ kibanaServer }); | ||
|
|
||
| expect(currentSoConfig.attributes).to.not.have.property('excludeAlertTags'); | ||
| expect(currentSoConfig.attributes).to.not.have.property('excludeAlertStatuses'); | ||
|
|
||
| const updatedSoBody = { | ||
| excludeAlertTags: ['False Positive'], | ||
| excludeAlertStatuses: ['open'], | ||
| }; | ||
|
|
||
| await riskEngineRoutes.soConfig(updatedSoBody, 200); | ||
| const currentSoConfig2 = await getRiskEngineConfigSO({ kibanaServer }); | ||
|
|
||
| expect(currentSoConfig2.attributes).to.have.property('excludeAlertTags'); | ||
| expect(currentSoConfig2.attributes).to.have.property('excludeAlertStatuses'); | ||
|
|
||
| await riskEngineRoutes.disable(); | ||
| await waitForRiskEngineTaskToBeGone; | ||
|
|
||
| updatedSoBody.excludeAlertStatuses = []; | ||
|
|
||
| await riskEngineRoutes.soConfig(updatedSoBody, 200); | ||
|
|
||
| await riskEngineRoutes.enable(); | ||
| await waitForRiskEngineRun; | ||
|
|
||
| const currentSoConfig3 = await getRiskEngineConfigSO({ kibanaServer }); | ||
| expect(JSON.stringify(currentSoConfig3.attributes.excludeAlertStatuses)).to.equal( | ||
| JSON.stringify(updatedSoBody.excludeAlertStatuses) |
There was a problem hiding this comment.
Here, you are doing several tests inside the "it" block, and the test name is generic, which makes It hard to follow.
Could you break it down into several tests?
If you can't break down the test into small logical steps like the GIVEN - WHEN - THEN pattern. Please leave comments explaining the test intention.
Why did you install, disable and then enable the risk engine?
|
|
||
| it('should include the right keys as per the update', async () => { | ||
| await riskEngineRoutes.init(); | ||
| await waitForRiskEngineRun; |
There was a problem hiding this comment.
This doesn't wait because you are calling the function
.
| await waitForRiskEngineRun; | |
| await waitForRiskEngineRun({ log, supertest }); |
CAWilson94
left a comment
There was a problem hiding this comment.
Looking good! Thanks for adding :D
|
Closing this as it is a duplicate of : #201344 |
Summary
The changes in the PR includes a new API endpoint to allow user to configure the risk engine saved object themselves.
Unit and integration tests are added considering the
spacesas well.Examples :
Risk Engine enabled for the first time :
Risk Engine disabled, SO is updated and risk engine is enabled again :
Risk Engine disabled, SO is updated and risk engine is enabled again :
Checklist
Delete any items that are not applicable to this PR.
For maintainers