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.

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

[Security Solution][Detections] Refactor ML calls for newest ML permissions #74582

Merged
merged 25 commits into from
Aug 13, 2020

Conversation

rylnd
Copy link
Contributor

@rylnd rylnd commented Aug 6, 2020

Summary

Addresses #73567.

ML Users (role: machine_learning_user) were previously able to invoke the ML Recognizer API, which we use to get not-yet-installed ML Jobs relevant to our index patterns. As of #64662 this is not true, and so we receive errors from components using the underlying hook, useSiemJobs.

To solve this I've created two separate hooks to replace useSiemJobs:

  • useSecurityJobs
    • used on ML Popover
    • includes uninstalled ML Jobs
    • checks (and returns) isMlAdmin before fetching data
  • useInstalledSecurityJobs
    • used on ML Jobs Dropdown and Anomalies Table
    • includes only installed ML Jobs
    • checks (and returns) isMlUser before fetching data

Note that we while we now receive the knowledge to do so, we do not always inform the user in the case of invalid permissions, and instead have the following behaviors:

User has insufficient license

  • ML Popover: shows an upgrade CTA
  • Anomalies Tables: show no data
  • Rule Creation: ML Rule option is disabled, shows upgrade CTA
  • Rule Details: ML Job Id is displayed as text

User is ML User

  • ML Popover: not shown
  • Anomalies Tables: show no data
  • Rule Creation: ML Rule option is disabled
  • Rule Details: ML Job Id is displayed as text

User is ML Admin

  • ML Popover: shown
  • Anomalies Tables: show data for installed ML Jobs
    • This is the same as previous logic, but worth calling out that you can't view historical anomalies
  • Rule Creation: ML Rule option is enabled, all ML Jobs available
  • Rule Details: ML Job Id is displayed as hyperlink, job status badge shown

Checklist

For maintainers

rylnd added 14 commits August 5, 2020 14:11
This is just some cleanup, this should be functionally equivalent. This
does use the new toast service, however.
This is incredibly similar to the existing useSiemJobs hook, with the
following exceptions:

* Does not fetch data if the user is not an ML admin
* Returns additional booleans: `isMlAdmin` and `isLicensed`
* uses new toasts service
Our jobs summary call returns all installed jobs regardless of group;
passing groups as jobIds does not perform group filtering.

This adds a helper predicate function on which to filter these results,
and updates tests accordingly.
This allows us to use this predicate on both JobSummary and MlSummaryJob
until we can consolidate to the former.
* Replaces existing API call with pure version
* Adds hook via useAsync
* Moves types to the more general ml/ folder out of ml_popover/

The JobSummary type can be retrieved from the ml plugin eventually, but
this is an intermediate step until I get there.
Unlike useSecurityJobs, which additionally:
* fetches uninstalled jobs
* requires ML Admin permissions

useInstalledSecurityJobs:
* fetches only installed jobs
* requires ml User permissions

This hook is a lightweight replacement for the more restrictive
useSiemJobs in places like the alerts table and the ML Jobs dropdown.
This was mostly a dropin replacement for the remaining use cases; there
were a few now-unnecessary `isInstalled` filters that were removed.
These are helpers for our new hook, not the old one.
This is no longer specific to the ml popover, nor has it been for quite
a while.
We should not be maintaining our own type here. There were two small
fixes needed to make this work:

* export the anomaly job types from ml/public
* change the auditMessage level value from a number to a string

The latter seems to have been an error, as `auditMessage.level` comes
back as a string, seemingly an enum. We treat it as an enum in our own
plugin, but I'll leave it as a string for now within ml.
 Conflicts:
	x-pack/plugins/security_solution/common/machine_learning/is_security_job.ts
@rylnd rylnd force-pushed the fix_unauthed_ml_calls branch from a8d6865 to 62b479f Compare August 6, 2020 19:13
rylnd added 5 commits August 6, 2020 19:25
* moves corresponding API mocks to new .mock format
  * the automock folder was actually getting in my way
* Adds new mocks for our useAppToasts hook
We have this info here, best to leverage it.
This abstracts away any dealing with our raw capabilities response.
There's already a translation for this, no need for extra work here.
For now, we don't want to filter our dropdown to just installed jobs;
that's a broader product decision that we'll circle back to.
@rylnd rylnd force-pushed the fix_unauthed_ml_calls branch from 0459dd6 to be0a06c Compare August 11, 2020 21:29
@rylnd
Copy link
Contributor Author

rylnd commented Aug 12, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
securitySolution 2035 +6 2029

async chunks size

id value diff baseline
ml 7.9MB -6.9KB 7.9MB
securitySolution 7.3MB +8.3KB 7.3MB
total +1.4KB

page load bundle size

id value diff baseline
ml 505.6KB +5.2KB 500.4KB
securitySolution 805.9KB +83.0B 805.8KB
total +5.3KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

ML changes LGTM

@@ -104,13 +100,13 @@ export const useAnomaliesTableData = ({
}
} catch (error) {
if (isSubscribed) {
errorToToaster({ title: i18n.SIEM_TABLE_FETCH_FAILURE, error, dispatchToaster });
addError(error, { title: i18n.SIEM_TABLE_FETCH_FAILURE });
Copy link
Member

Choose a reason for hiding this comment

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

++ for refactoring to leverage useAppToasts, thanks!

Comment on lines +10 to +12
const _getJobsSummary = withOptionalSignal(getJobsSummary);

export const useGetJobsSummary = () => useAsync(_getJobsSummary);
Copy link
Member

Choose a reason for hiding this comment

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

Really liking how the new composable hooks are turning out. Keeps the actual API request logic small and easy to grok, while making creating specific hooks like above super simple. Autocomplete is all 👍 too, so that's nice as well. Thanks for all the effort here @rylnd 🙂

Comment on lines +74 to +81
it('renders a toast error if the ML call fails', async () => {
(getJobsSummary as jest.Mock).mockRejectedValue('whoops');
const { waitForNextUpdate } = renderHook(() => useInstalledSecurityJobs());
await waitForNextUpdate();

expect(appToastsMock.addError).toHaveBeenCalledWith('whoops', {
title: 'Security job fetch failure',
});
Copy link
Member

Choose a reason for hiding this comment

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

We've had a gap in our coverage around the ML hooks for a long time, so awesome to see these tests! 🚀 🎉

@@ -31,7 +31,7 @@ export const mockGroupsResponse: Group[] = [
{ id: 'suricata', jobIds: ['suricata_alert_rate'], calendarIds: [] },
];

export const mockOpenedJob: JobSummary = {
export const mockOpenedJob: MlSummaryJob = {
Copy link
Member

Choose a reason for hiding this comment

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

++ for moving to use the ML types. More safety here now 🙂

* ml_module (whether installed or not). Use the corresponding helper functions to filter the job
* list as necessary. E.g. installed jobs, running jobs, etc.
*
* NOTE: If the user is not an ml admin, jobs will be empty and isMlAdmin will be false.
Copy link
Member

Choose a reason for hiding this comment

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

nit: May be worth referencing useInstalledSecurityJobs as an alternate here for when the user is not an ml admin and we still need to fetch relevant jobs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's an excellent point, will do.

const [jobs, setJobs] = useState<SecurityJob[]>([]);
const [loading, setLoading] = useState(true);
const mlCapabilities = useMlCapabilities();
const [siemDefaultIndex] = useUiSetting$<string[]>(DEFAULT_INDEX_KEY);
Copy link
Member

@spong spong Aug 12, 2020

Choose a reason for hiding this comment

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

Suggested change
const [siemDefaultIndex] = useUiSetting$<string[]>(DEFAULT_INDEX_KEY);
const [securitySolutionDefaultIndex] = useUiSetting$<string[]>(DEFAULT_INDEX_KEY);

And other references below.... 👋 SIEM

@@ -133,18 +111,18 @@ export interface CustomURL {
}

/**
* Representation of an ML Job as used by the SIEM App -- a composition of ModuleJob and JobSummary
* Representation of an ML Job as used by the SIEM App -- a composition of ModuleJob and MlSummaryJob
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Representation of an ML Job as used by the SIEM App -- a composition of ModuleJob and MlSummaryJob
* Representation of an ML Job as used by the Security Solution App -- a composition of ModuleJob and MlSummaryJob

Copy link
Member

@spong spong left a comment

Choose a reason for hiding this comment

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

Performed code review, and checked out locally and tested with multiple different user permissions from read_only, machine_learning_user and machine_learning_admin and all behavior has been as expected.

Left a couple nits for cleaning up some lingering siem references, but everything else looks great. Super clean code here, and love the new tests for the ML hooks -- really appreciate the attention to detail here @rylnd! LGTM! 👍 🎉

Note: In testing, @rylnd and I found an extra call to the getJobsSummary API on the Hosts page and it looks like there's a MatrixHistogram being rendered in the DOM above the Anomalies table, but it's not configured to the ML indices so it returns nothing and is not shown. Just some tech debt for us to clean up here it looks like.

@spong spong merged commit 4ca52e6 into elastic:master Aug 13, 2020
spong pushed a commit to spong/kibana that referenced this pull request Aug 13, 2020
…ssions (elastic#74582)

## Summary

Addresses elastic#73567.

ML Users (role: `machine_learning_user`) were previously able to invoke the ML Recognizer API, which we use to get not-yet-installed ML Jobs relevant to our index patterns. As of elastic#64662 this is not true, and so we receive errors from components using the underlying hook, `useSiemJobs`.

To solve this I've created two separate hooks to replace `useSiemJobs`:

* `useSecurityJobs`
  * used on ML Popover
  * includes uninstalled ML Jobs
  * checks (and returns) `isMlAdmin` before fetching data
* `useInstalledSecurityJobs`
  * used on ML Jobs Dropdown and Anomalies Table
  * includes only installed ML Jobs
  * checks (and returns) `isMlUser` before fetching data

Note that we while we now receive the knowledge to do so, we do not always inform the user in the case of invalid permissions, and instead have the following behaviors:

#### User has insufficient license
* ML Popover:  shows an upgrade CTA
* Anomalies Tables: show no data
* Rule Creation: ML Rule option is disabled, shows upgrade CTA
* Rule Details: ML Job Id is displayed as text
#### User is ML User
* ML Popover:  not shown
* Anomalies Tables: show no data
* Rule Creation: ML Rule option is disabled
* Rule Details: ML Job Id is displayed as text
#### User is ML Admin
* ML Popover:  shown
* Anomalies Tables: show data __for installed ML Jobs__
  * This is the same as previous logic, but worth calling out that you can't view historical anomalies
* Rule Creation: ML Rule option is enabled, all ML Jobs available
* Rule Details: ML Job Id is displayed as hyperlink, job status badge shown

### Checklist

- [x] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios

### For maintainers

- [ ] This was checked for breaking API changes and was [labeled appropriately](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#release-notes-process)
spong added a commit that referenced this pull request Aug 13, 2020
…ssions (#74582) (#74919)

## Summary

Addresses #73567.

ML Users (role: `machine_learning_user`) were previously able to invoke the ML Recognizer API, which we use to get not-yet-installed ML Jobs relevant to our index patterns. As of #64662 this is not true, and so we receive errors from components using the underlying hook, `useSiemJobs`.

To solve this I've created two separate hooks to replace `useSiemJobs`:

* `useSecurityJobs`
  * used on ML Popover
  * includes uninstalled ML Jobs
  * checks (and returns) `isMlAdmin` before fetching data
* `useInstalledSecurityJobs`
  * used on ML Jobs Dropdown and Anomalies Table
  * includes only installed ML Jobs
  * checks (and returns) `isMlUser` before fetching data

Note that we while we now receive the knowledge to do so, we do not always inform the user in the case of invalid permissions, and instead have the following behaviors:

#### User has insufficient license
* ML Popover:  shows an upgrade CTA
* Anomalies Tables: show no data
* Rule Creation: ML Rule option is disabled, shows upgrade CTA
* Rule Details: ML Job Id is displayed as text
#### User is ML User
* ML Popover:  not shown
* Anomalies Tables: show no data
* Rule Creation: ML Rule option is disabled
* Rule Details: ML Job Id is displayed as text
#### User is ML Admin
* ML Popover:  shown
* Anomalies Tables: show data __for installed ML Jobs__
  * This is the same as previous logic, but worth calling out that you can't view historical anomalies
* Rule Creation: ML Rule option is enabled, all ML Jobs available
* Rule Details: ML Job Id is displayed as hyperlink, job status badge shown

### Checklist

- [x] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios

### For maintainers

- [ ] This was checked for breaking API changes and was [labeled appropriately](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#release-notes-process)

Co-authored-by: Ryland Herrick <ryalnd@gmail.com>
@rylnd rylnd deleted the fix_unauthed_ml_calls branch August 13, 2020 03:22
gmmorris added a commit to gmmorris/kibana that referenced this pull request Aug 13, 2020
* master: (28 commits)
  [Task manager] Prevents edge case where already running tasks are reschedule every polling interval (elastic#74606)
  [Security Solution] Fix the status of timelines' bulk actions (elastic#74560)
  Data plugin: Suggested enhance pattern (elastic#74505)
  Use jest.useFakeTimers instead of hard coded timeout for tooltip tests. (elastic#74642)
  [Security Solution][lists] Adds tests for exception lists and items part 2 (elastic#74815)
  [Security Solution][Resolver] fix presentation role on edgeline (elastic#74869)
  [Security Solution][Detections] Refactor ML calls for newest ML permissions (elastic#74582)
  [bin/kibana-plugin] support KP plugins instead (elastic#74604)
  Reduce number of indexed fields in index pattern saved object (elastic#74817)
  [reporting] Pass along generic parameters in high-order route handler (elastic#74892)
  Migrated last pieces of legacy fixture code (elastic#74470)
  Empty index patterns page re-design  (elastic#68819)
  [babel] coalese some versions to prevent breaking yarn install (elastic#74864)
  [Dashboard First] Decouple Attribute Service and By Value Embeddables (elastic#74302)
  Revert "[reporting] Pass along generic parameters in high-order route handler" (elastic#74891)
  [reporting] Pass along generic parameters in high-order route handler (elastic#74879)
  [src/dev/build] implement a getBuildNumber() mock (elastic#74881)
  [Enterprise Search] Add solution-level side navigation (elastic#74705)
  [DOCS] Canvas docs 7.9 refresh (elastic#74000)
  [Security Solution][Resolver]Enzyme test related events closing (elastic#74811)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Aug 13, 2020
…le-buffer-with-update-of-same-id

* upstream/master: (37 commits)
  [Task manager] Prevents edge case where already running tasks are reschedule every polling interval (elastic#74606)
  [Security Solution] Fix the status of timelines' bulk actions (elastic#74560)
  Data plugin: Suggested enhance pattern (elastic#74505)
  Use jest.useFakeTimers instead of hard coded timeout for tooltip tests. (elastic#74642)
  [Security Solution][lists] Adds tests for exception lists and items part 2 (elastic#74815)
  [Security Solution][Resolver] fix presentation role on edgeline (elastic#74869)
  [Security Solution][Detections] Refactor ML calls for newest ML permissions (elastic#74582)
  [bin/kibana-plugin] support KP plugins instead (elastic#74604)
  Reduce number of indexed fields in index pattern saved object (elastic#74817)
  [reporting] Pass along generic parameters in high-order route handler (elastic#74892)
  Migrated last pieces of legacy fixture code (elastic#74470)
  Empty index patterns page re-design  (elastic#68819)
  [babel] coalese some versions to prevent breaking yarn install (elastic#74864)
  [Dashboard First] Decouple Attribute Service and By Value Embeddables (elastic#74302)
  Revert "[reporting] Pass along generic parameters in high-order route handler" (elastic#74891)
  [reporting] Pass along generic parameters in high-order route handler (elastic#74879)
  [src/dev/build] implement a getBuildNumber() mock (elastic#74881)
  [Enterprise Search] Add solution-level side navigation (elastic#74705)
  [DOCS] Canvas docs 7.9 refresh (elastic#74000)
  [Security Solution][Resolver]Enzyme test related events closing (elastic#74811)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Aug 13, 2020
* upstream/master: (45 commits)
  [Metrics UI] Fix inventory footer misalignment (elastic#74707)
  Remove legacy optimizer (elastic#73154)
  Update design-specific GH code-owners (elastic#74877)
  skip test Reporting paginates content elastic#74922
  [Metrics UI] Add Jest tests for alert previews (elastic#74890)
  Fixed tooltip (elastic#74074)
  [Ingest Pipelines] Processor forms for processors A-D (elastic#72849)
  [Observability] change ingest manager link (elastic#74928)
  [Task manager] Prevents edge case where already running tasks are reschedule every polling interval (elastic#74606)
  [Security Solution] Fix the status of timelines' bulk actions (elastic#74560)
  Data plugin: Suggested enhance pattern (elastic#74505)
  Use jest.useFakeTimers instead of hard coded timeout for tooltip tests. (elastic#74642)
  [Security Solution][lists] Adds tests for exception lists and items part 2 (elastic#74815)
  [Security Solution][Resolver] fix presentation role on edgeline (elastic#74869)
  [Security Solution][Detections] Refactor ML calls for newest ML permissions (elastic#74582)
  [bin/kibana-plugin] support KP plugins instead (elastic#74604)
  Reduce number of indexed fields in index pattern saved object (elastic#74817)
  [reporting] Pass along generic parameters in high-order route handler (elastic#74892)
  Migrated last pieces of legacy fixture code (elastic#74470)
  Empty index patterns page re-design  (elastic#68819)
  ...
spong pushed a commit to spong/kibana that referenced this pull request Aug 18, 2020
…ssions (elastic#74582)

## Summary

Addresses elastic#73567.

ML Users (role: `machine_learning_user`) were previously able to invoke the ML Recognizer API, which we use to get not-yet-installed ML Jobs relevant to our index patterns. As of elastic#64662 this is not true, and so we receive errors from components using the underlying hook, `useSiemJobs`.

To solve this I've created two separate hooks to replace `useSiemJobs`:

* `useSecurityJobs`
  * used on ML Popover
  * includes uninstalled ML Jobs
  * checks (and returns) `isMlAdmin` before fetching data
* `useInstalledSecurityJobs`
  * used on ML Jobs Dropdown and Anomalies Table
  * includes only installed ML Jobs
  * checks (and returns) `isMlUser` before fetching data

Note that we while we now receive the knowledge to do so, we do not always inform the user in the case of invalid permissions, and instead have the following behaviors:

#### User has insufficient license
* ML Popover:  shows an upgrade CTA
* Anomalies Tables: show no data
* Rule Creation: ML Rule option is disabled, shows upgrade CTA
* Rule Details: ML Job Id is displayed as text
#### User is ML User
* ML Popover:  not shown
* Anomalies Tables: show no data
* Rule Creation: ML Rule option is disabled
* Rule Details: ML Job Id is displayed as text
#### User is ML Admin
* ML Popover:  shown
* Anomalies Tables: show data __for installed ML Jobs__
  * This is the same as previous logic, but worth calling out that you can't view historical anomalies
* Rule Creation: ML Rule option is enabled, all ML Jobs available
* Rule Details: ML Job Id is displayed as hyperlink, job status badge shown

### Checklist

- [x] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios

### For maintainers

- [ ] This was checked for breaking API changes and was [labeled appropriately](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#release-notes-process)
spong added a commit that referenced this pull request Aug 18, 2020
…ssions (#74582) (#75287)

## Summary

Addresses #73567.

ML Users (role: `machine_learning_user`) were previously able to invoke the ML Recognizer API, which we use to get not-yet-installed ML Jobs relevant to our index patterns. As of #64662 this is not true, and so we receive errors from components using the underlying hook, `useSiemJobs`.

To solve this I've created two separate hooks to replace `useSiemJobs`:

* `useSecurityJobs`
  * used on ML Popover
  * includes uninstalled ML Jobs
  * checks (and returns) `isMlAdmin` before fetching data
* `useInstalledSecurityJobs`
  * used on ML Jobs Dropdown and Anomalies Table
  * includes only installed ML Jobs
  * checks (and returns) `isMlUser` before fetching data

Note that we while we now receive the knowledge to do so, we do not always inform the user in the case of invalid permissions, and instead have the following behaviors:

#### User has insufficient license
* ML Popover:  shows an upgrade CTA
* Anomalies Tables: show no data
* Rule Creation: ML Rule option is disabled, shows upgrade CTA
* Rule Details: ML Job Id is displayed as text
#### User is ML User
* ML Popover:  not shown
* Anomalies Tables: show no data
* Rule Creation: ML Rule option is disabled
* Rule Details: ML Job Id is displayed as text
#### User is ML Admin
* ML Popover:  shown
* Anomalies Tables: show data __for installed ML Jobs__
  * This is the same as previous logic, but worth calling out that you can't view historical anomalies
* Rule Creation: ML Rule option is enabled, all ML Jobs available
* Rule Details: ML Job Id is displayed as hyperlink, job status badge shown

### Checklist

- [x] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios

### For maintainers

- [ ] This was checked for breaking API changes and was [labeled appropriately](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#release-notes-process)

Co-authored-by: Ryland Herrick <ryalnd@gmail.com>
@spong spong added the v7.9.1 label Aug 18, 2020
@MindyRS MindyRS added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Sep 23, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:fix Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:SIEM v7.9.1 v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants