[Security Solution] add new permission properties for endpoint rbac #142243
Conversation
joeypoon
added
release_note:skip
|
Pinging @elastic/security-onboarding-and-lifecycle-mgt (Team:Onboarding and Lifecycle Mgt) |
x-pack/plugins/security_solution/common/endpoint/service/authz/authz.ts
Outdated
Show resolved
Hide resolved
| isEndpointRbacEnabled, | ||
| hasEndpointManagementAccess, | ||
| 'readEventFilters' | ||
| ); | ||
|
|
||
| return { | ||
| canAccessFleet: fleetAuthz?.fleet.all ?? userRoles.includes('superuser'), | ||
| canAccessEndpointManagement: hasEndpointManagementAccess, |
There was a problem hiding this comment.
we're still missing readEndpointList and writeEndpointList. I'm still unsure of the interaction these will have with canAccessEndpointManagement so I left them off for now.
There was a problem hiding this comment.
I think canAccessEndpointManagment will be deleted once we move over to RBAC. It made sense only when everthing was tied to superuser.
My suggestion is for you to add the properties for endpoint list and just set them to hasEndpointManagementAccess here for now. I think we had said that for the endpoint metadata api we will likely tie access to ti if hte user has access to security solution, so we'll have to add that in at some point.
There was a problem hiding this comment.
Yeah, deleting canAccessEndpointManagment definitely seems most likely. I can add them in first.
| hasEndpointManagementAccess, | ||
| 'writePolicyManagement' | ||
| ); | ||
| const canReadPolicyManagement = hasPermission( |
There was a problem hiding this comment.
This comment applies for all of the *Read* properties.
I assume the Read properties is what we will check to ensure a user can visit a page or if we display a link in menus to allow the user to visit a given page. If that is correct, should they all be renamed to canAccess* instead? These types of authz properties would indicate that the user has at least read permissions to the page. The write properties would indicate if they are able to create/delete/edit entries in that page and would only further limit the expose of the UI's functionality once they have access to it.
The other change here is on the assignment of its value. There should never be a case where read would be false, but write is true. So I would suggest that when assigning the value to the read properties, we instead do:
const canReadPolicyManagement = canWritePolicyManagement || hasPermission(
There was a problem hiding this comment.
btw - we can also just keep the *Read* properties if that makes it clearer to the team and maybe helps tie it back to the Kibana feature control UI
There was a problem hiding this comment.
Not a big deal to me either way but I find read to be clearer than access.
👍 on the suggestion for checking write first, will update that.
| hasEndpointManagementAccess, | ||
| 'readPolicyManagement' | ||
| ); | ||
| const canWriteActionsLogManagement = hasPermission( |
There was a problem hiding this comment.
Confused about this one based on its name. What would be the write functionality?
There was a problem hiding this comment.
I believe this is one of the ones where we preemptively did a all | read | none even though all and read are functionally the same due to migration constraints. @kevinlog correct me if I'm wrong.
| isEndpointRbacEnabled, | ||
| hasEndpointManagementAccess, | ||
| 'readEventFilters' | ||
| ); | ||
|
|
||
| return { | ||
| canAccessFleet: fleetAuthz?.fleet.all ?? userRoles.includes('superuser'), | ||
| canAccessEndpointManagement: hasEndpointManagementAccess, |
There was a problem hiding this comment.
I think canAccessEndpointManagment will be deleted once we move over to RBAC. It made sense only when everthing was tied to superuser.
My suggestion is for you to add the properties for endpoint list and just set them to hasEndpointManagementAccess here for now. I think we had said that for the endpoint metadata api we will likely tie access to ti if hte user has access to security solution, so we'll have to add that in at some point.
joeypoon
force-pushed
the
chore/rbac-properties
branch
from
9bc6d0d to
59db0ab
Compare
…-ref HEAD~1..HEAD --fix'
💚 Build Succeeded
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
…lastic#142243) Co-authored-by: kibanamachine <[email protected]>
Summary
Add properties for new permissions into authz service for endpoint rbac.
Checklist
For maintainers