-
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 Solutions] Add PLI authorisation for Threat Intelligence #162562
[Security Solutions] Add PLI authorisation for Threat Intelligence #162562
Conversation
9f3b3cf
to
fe410d5
Compare
fe410d5
to
13d9cf2
Compare
@@ -85,10 +85,10 @@ const ThreatIntelligence = memo(() => { | |||
}; | |||
|
|||
return ( | |||
<TrackApplicationView viewId="threat_intelligence"> | |||
<SecurityRoutePageWrapper pageName={SecurityPageName.threatIntelligence}> |
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.
SecurityRoutePageWrapper
is necessary for displaying the Upsell page.
Pinging @elastic/security-threat-hunting (Team:Threat Hunting) |
Pinging @elastic/security-solution (Team: SecuritySolution) |
Thank you Pablo, tested locally, LGTM! |
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.
tested LGTM, though I've got a couple of questions related to code consistency between ESS and Serverless
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'm not aware of how we're going to handle things for ESS and Serverless, but I'm wondering why do we duplicate this component here? We already have a paywal component in the Threat Intelligence plugin. Shouldn't we make that one smarter and handle both ESS and Serverless?
With the current approach, we basically make the paywall component within the Threat Intelligence plugin useless when loaded in serverless mode... but the component is still there.
It seems confusing to me that we'd have similar things in different places
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.
The approaches to authorization for ESS and Serverless are fundamentally different. ESS has several "if platinum" checks throughout the app, whereas Serverless has a centralized, privilege-based system. We can utilize the security_solutions_serverless plugin to register Upselling pages or components and retrieve them with hooks when the page is unauthorized.
While it's possible to repurpose the paywal component (if it's accessible on the serverless plugin) and make it more intelligent, the UX team is still working on the final design for serverless Upselling. It's conceivable that there will be a generic Upselling component for all features, so I don't believe it's necessary to refactor the paywal component at this time. But we need to revisit this decision after the designs are ready.
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 understand, thanks for explaining!
I think ultimately we should aim at having a similar way to manage these paywall between ESS and Servesless, but I guess this is a discussion for another time :D
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 am currently implementing your suggestion at the following PR: #163406.
.../plugins/security_solution_serverless/public/upselling/pages/threat_intelligence_paywall.tsx
Show resolved
Hide resolved
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsasync chunk count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @machadoum |
…lastic#162562) ## Summary Add PLI authorization checks for the Intelligence page. *This PR restricts access to the features* and creates a simplified Upselling page. * Rename `threat_intelligence-indicators` page name to `threat_intelligence` to simplify the code ### Not included * Final Upselling/PLG design ### How to test it? #### ESS `yarn start` * Run ESS with a basic license * It should not change * Run ESS with a platinum * It should not change #### Serverless `yarn serverless-security` * Run Serverless with security essentials (serverless.security.yml) * It should show the new Threat Intelligence Upsell ``` xpack.serverless.security.productTypes: [ { product_line: 'security', product_tier: 'essentials' } ] ``` * Run Serverless with security complete (kibana/config/serverless.security.yml) * It should show the Therat Intelligence page ``` xpack.serverless.security.productTypes: [ { product_line: 'security', product_tier: 'complete' }, ] ``` <img width="1785" alt="Screenshot 2023-07-26 at 15 59 52" src="https://github.com/elastic/kibana/assets/1490444/e8fb1bb4-ec26-477d-80e0-aecd4c15e7a2"> ### Checklist Delete any items that are not applicable to this PR. - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
Summary
Add PLI authorization checks for the Intelligence page.
This PR restricts access to the features and creates a simplified Upselling page.
threat_intelligence-indicators
page name tothreat_intelligence
to simplify the codeNot included
How to test it?
ESS
yarn start
Serverless
yarn serverless-security
Checklist
Delete any items that are not applicable to this PR.