[ResponseOps][Cases] User suggestion API #137346
Conversation
jonathan-buttner
added
release_note:skip
| supertest: SuperTest.SuperTest<SuperTest.Test>; | ||
| users?: User[]; | ||
| }) => { | ||
| for (const user of users) { |
There was a problem hiding this comment.
This can't be done in a Promise.all because the order needs to be consistent for the results returned from the suggest API.
There was a problem hiding this comment.
Just to add more context why it works like that: by design suggest API, everything else being equal, is supposed to rank users who logged in recently higher than those who didn't login for a while.
|
Pinging @elastic/response-ops (Team:ResponseOps) |
|
Pinging @elastic/response-ops-cases (Feature:Cases) |
x-pack/plugins/cases/server/routes/api/internal/suggest_user_profiles.ts
Show resolved
Hide resolved
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
|
ACK: will review PR tomorrow! |
|
@elasticmachine merge upstream |
| name: rt.string, | ||
| owners: rt.array(rt.string), | ||
| }), | ||
| rt.partial({ size: rt.number }), |
There was a problem hiding this comment.
nit: would you mind limiting size here? The suggest API will throw if you pass size greater than 100 (to prevent DDoS since privileges check can be expensive) and your endpoint might end up throwing 500 Internal Server Error, but it's better to have controlled 400 Bad Request for this case (assuming io-ts validation results to 400 like built-in kbn-config-schema).
| const { spaces } = this.options; | ||
|
|
||
| const securityPluginFields = { | ||
| securityPluginSetup: this.options.securityPluginSetup, |
There was a problem hiding this comment.
note to myself: we need to expose license on the start contract too so that you can rely just on a single contract.
| securityPluginSetup?: SecurityPluginSetup; | ||
| securityPluginStart?: SecurityPluginStart; |
There was a problem hiding this comment.
question: I remember you were considering making security as a required dependency, is there any reason you decided not to do that?
There was a problem hiding this comment.
Yeah I found that the security plugin can still be disabled. The functionality to disable it was moved into Elasticsearch. When we initially developed the cases RBAC we intentionally allowed the security plugin to be disabled. It was something that Larry suggested we support. So I didn't want to deviate from that in this PR. I think some users want to play around with Kibana without security enable and it'd be nice for them to still have access to cases in that situation.
There was a problem hiding this comment.
Yeah I found that the security plugin can still be disabled. The functionality to disable it was moved into Elasticsearch.
Yeah, but you'll still get security contract even if Security is disabled in Elasticsearch and can replace securityPluginStart?: SecurityPluginStart; with securityPluginStart: SecurityPluginStart; to have less "undefined" checks (since you need to rely on the availabilty of the security feature via license check anyway). But it's minor, just wanted to point this out.
There was a problem hiding this comment.
ah ok, good to know. I'll create an issue to fix that up
| this.options = options; | ||
| } | ||
|
|
||
| public async suggest(request: KibanaRequest): ReturnType<UserProfileServiceStart['suggest']> { |
There was a problem hiding this comment.
nit: would be a little bit more readable (requires import type { UserProfile } from '@kbn/security-plugin/common';)
| public async suggest(request: KibanaRequest): ReturnType<UserProfileServiceStart['suggest']> { | |
| public async suggest(request: KibanaRequest): Promise<UserProfile[]> { |
Alternatively you can change line 65 to return [] as UserProfile[]; and remove return type signature completely since it can be inferred automatically, up to you.
| message: `Failed to retrieve suggested user profiles in service for name: ${name} owners: [${owners.join( | ||
| ',' | ||
| )}]`, |
There was a problem hiding this comment.
optional nit: join(',') is actually done automatically when array is used in the template string, we usually use join explicitly only if we want another separator (e.g. comma + space).
| message: `Failed to retrieve suggested user profiles in service for name: ${name} owners: [${owners.join( | |
| ',' | |
| )}]`, | |
| message: `Failed to retrieve suggested user profiles in service for name: ${name} owners: [${owners}]`, |
| ); | ||
| } | ||
|
|
||
| private static buildRequiredPrivileges(owners: string[], security: SecurityPluginStart) { |
There was a problem hiding this comment.
question: would you mind adding a JSDoc here explaining what privileges exactly we'd like potential assignees to have and why (who\what are these owners and why having only getCase isn't enough to be assigned to the case)? It'd help other Cases-noobs like me to quickly understand the "privilege model" here 🙂
| secAllUser, | ||
| } from './common/users'; | ||
|
|
||
| export default ({ getService }: FtrProviderContext): void => { |
There was a problem hiding this comment.
🏅 Thanks for adding all these API integration tests!
| // TODO: unskip when security plugin implements fix for size issue | ||
| it.skip('limits the results to one, when the size is specified as one', async () => { |
There was a problem hiding this comment.
nit: should be fixed now!
| // TODO: unskip when security plugin implements fix for size issue | |
| it.skip('limits the results to one, when the size is specified as one', async () => { | |
| it('limits the results to one, when the size is specified as one', async () => { |
| supertest: SuperTest.SuperTest<SuperTest.Test>; | ||
| users?: User[]; | ||
| }) => { | ||
| for (const user of users) { |
There was a problem hiding this comment.
Just to add more context why it works like that: by design suggest API, everything else being equal, is supposed to rank users who logged in recently higher than those who didn't login for a while.
💚 Build Succeeded
Metrics [docs]Module Count
Page load bundle
History
To update your PR or re-run it, just comment with: |
This PR adds the internal user profiles suggestion API to the cases backend. This API is mostly a pass through to the security plugin's API. It does construct the required privileges that a user profile must meet to be a valid result. I have this set to cases update and cases read (these are specific to cases, not comments etc). My thought process was that an assigned user should be able to change the status of a case (close it) and read it at a minimum.
It also adds the
api ['casesSuggestAssignees']tags to Stack Cases, Security Solution, and Observability.