-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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][Endpoint][Response Actions] Add license check to actions log management RBAC #142482
[Security Solution][Endpoint][Response Actions] Add license check to actions log management RBAC #142482
Conversation
Pinging @elastic/security-onboarding-and-lifecycle-mgt (Team:Onboarding and Lifecycle Mgt) |
SecurityPageName.responseActionsHistory | ||
); | ||
|
||
if (!canAccessResponseActionsHistoryNavLink) { |
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.
redirecting silently is not the best UX here. If the user does not have access to it, they should see a "not found" or "permission denied" type of message.
Should we instead change the Management page's index file to NOT register the route instead? Which would result in the user getting a 404
instead:
kibana/x-pack/plugins/security_solution/public/management/pages/index.tsx
Lines 96 to 114 in f336734
<Switch> | |
<Route path={MANAGEMENT_ROUTING_ENDPOINTS_PATH} component={EndpointTelemetry} /> | |
<Route path={MANAGEMENT_ROUTING_POLICIES_PATH} component={PolicyTelemetry} /> | |
<Route path={MANAGEMENT_ROUTING_TRUSTED_APPS_PATH} component={TrustedAppTelemetry} /> | |
<Route path={MANAGEMENT_ROUTING_EVENT_FILTERS_PATH} component={EventFilterTelemetry} /> | |
<Route | |
path={MANAGEMENT_ROUTING_HOST_ISOLATION_EXCEPTIONS_PATH} | |
component={HostIsolationExceptionsTelemetry} | |
/> | |
<Route path={MANAGEMENT_ROUTING_BLOCKLIST_PATH} component={BlocklistContainer} /> | |
<Route | |
path={MANAGEMENT_ROUTING_RESPONSE_ACTIONS_HISTORY_PATH} | |
component={ResponseActionsTelemetry} | |
/> | |
<Route path={MANAGEMENT_PATH} exact> | |
<Redirect to={getEndpointListPath({ name: 'endpointList' })} /> | |
</Route> | |
<Route path="*" component={NotFoundPage} /> | |
</Switch> |
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 like that idea as well. A 404 page is better. What do you think @kevinlog
Although, HIE page also silently redirects. Maybe change that too?
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 like that idea as well. A 404 page is better. What do you think @kevinlog
Although, HIE page also silently redirects. Maybe change that too?
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.
* Checks if the current user should be able to see the response actions history | ||
* menu item based on their current privileges | ||
*/ | ||
export function useCanSeeResponseActionsHistoryMenu(): boolean { |
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.
(Optional) Seems like this hook is really only used once. should you just access:
useUserPrivileges().endpointPrivileges.canReadActionsLogManagement
?
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.
* menu item based on their current privileges | ||
*/ | ||
export function useCanSeeResponseActionsHistoryMenu(): boolean { | ||
const privileges = useEndpointPrivileges(); |
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.
Can you use useUserPrivileges().endpointPrivileges
instead?
The endpoint hook is really meant to be used internally by useUserPrivileges()
hook
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.
review changes (@paul-tavares)
review changes (@paul-tavares)
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.
Approving, but left a minor comment (optional)
@@ -117,6 +122,23 @@ describe('useSecuritySolutionNavigation', () => { | |||
).toBeUndefined(); | |||
}); | |||
|
|||
it('should omit response actions history if hook reports 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.
being that the useUserPrivileges
is being mocked via jest.mock()
, should you add a afterEach()
that resets the mock back to its default?
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.
oh yes. Missed that. Will add it.
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.
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!
review changes (@paul-tavares)
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: cc @ashokaditya |
…actions log management RBAC (elastic#142482) * Add license check to actions log management RBAC fixes elastic/security-team/issues/5118 refs elastic/pull/142470 * useUSerPrivileges instead review changes (@paul-tavares) * Don't register route if no access review changes (@paul-tavares) * reset mocked privilege review changes (@paul-tavares)
…actions log management RBAC (elastic#142482) * Add license check to actions log management RBAC fixes elastic/security-team/issues/5118 refs elastic/pull/142470 * useUSerPrivileges instead review changes (@paul-tavares) * Don't register route if no access review changes (@paul-tavares) * reset mocked privilege review changes (@paul-tavares)
Summary
Allows access to the response actions history page for platinum licenses and up.
iteration of /pull/142470 for
>v8.6
Checklist