-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Grant access to machine learning features when base privileges are used #115444
Conversation
Pinging @elastic/kibana-security (Team:Security) |
@XavierM could you please give a bit more detail on what user facing changes this will introduce? |
@jgowdyelastic sorry this should have been marked as draft for the time being. We opened this to see what the impact of the change would be on CI, so we could start addressing test failures. The intent here is for access to ML to be granted when the base |
💔 Build Failed
Failed CI Steps
Test FailuresKibana Pipeline / general / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/ml/permissions/no_ml_access·ts.machine learning permissions for user with no ML access (ft_ml_unauthorized) should not allow access to the ML appStandard Out
Stack Trace
Kibana Pipeline / general / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/ml/permissions/no_ml_access·ts.machine learning permissions for user with no ML access (ft_ml_unauthorized) should not allow access to the ML appStandard Out
Stack Trace
Kibana Pipeline / general / Chrome X-Pack UI Functional Tests Basic License.x-pack/test/functional_basic/apps/ml/permissions/no_ml_access·ts.apps machine learning basic license permissions for user with no ML access (ft_ml_unauthorized) should not allow access to the ML appStandard Out
Stack Trace
and 1 more failures, only showing the first 3. Metrics [docs]
History
To update your PR or re-run it, just comment with: |
@elasticmachine merge upstream |
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.
Nice job on this, I think all your changes look great!
I wonder if we should add some more tests though -- it looks like we effectively don't have any ML integration tests that make assertions for users that have base privileges.
we have tests for these authorized users:
ML_POWERUSER
(ft_ml_poweruser
) - access granted to ML via themachine_learning_admin
reserved roleML_POWERUSER_SPACES
(ft_ml_poweruser_spaces
) - explicit access to ML in the default spaceML_POWERUSER_SPACE1
(ft_ml_poweruser_space1
) - explicit access to ML in the space1 spaceML_POWERUSER_ALL_SPACES
(ft_ml_poweruser_all_spaces
) - explicit access to ML in all spacesML_VIEWER
(ft_ml_viewer
) - access granted via themachine_learning_user
reserved roleML_VIEWER_SPACES
(ft_ml_viewer_spaces
) - explicit access to ML in the default spacML_VIEWER_SPACE1
(ft_ml_viewer_space1
) - explicit access to ML in the space1 spaceML_VIEWER_ALL_SPACES
(ft_ml_viewer_all_spaces
) - explicit access to ML in all spaces
I don't think we should make two additional users to test implicit access (base privileges). Instead, I think we should change ML_POWERUSER_ALL_SPACES
and ML_VIEWER_ALL_SPACES
to use base privileges. E.g., change these two roles to the following:
{
name: 'ft_all_spaces_ml_all',
elasticsearch: { cluster: [], indices: [], run_as: [] },
kibana: [
{
base: ['all'],
feature: {},
spaces: ['*'],
},
],
},
...
{
name: 'ft_all_spaces_ml_read',
elasticsearch: { cluster: [], indices: [], run_as: [] },
kibana: [
{
base: ['read'],
feature: {},
spaces: ['*'],
},
],
},
WDYT?
@@ -103,5 +103,75 @@ export default function ({ getPageObjects, getService }: FtrProviderContext) { | |||
expect(navLinks).to.contain('Machine Learning'); | |||
}); | |||
}); | |||
|
|||
describe('ml read', () => { |
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 these tests that you added for explicit read / explicit none
I like your suggestion about these two roles |
cc @pheyos regarding tests and user changes |
I'm not sure what is causing the module integration test failures. It seems that the "ft_ml_poweruser" user does not have enough privileges to access the "apache_data_stream" and "nginx_data_stream" modules, but I'm not sure why. I attempted adding extra feature privileges to the "ft_all_space_ml_none" role ( |
To summarize the failing test investigation and slack discussion: |
There's another test failure (I think this was hidden by a different failure before). We also have a test where we modify an index pattern / data view. I haven't tested it, but I think this means we also need |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
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 on green CI.
💚 Build Succeeded
Metrics [docs]
History
To update your PR or re-run it, just comment with: |
…-migrate-away-from-injected-css-js * 'master' of github.com:elastic/kibana: (61 commits) [ML] Nodes overview for the Model Management page (elastic#116361) [Uptime] Uptime index config using kibana.yml (elastic#115775) [Controls] Dashboard Integration (elastic#115991) skip flaky suite (elastic#104260) Include Files in GitHub UI (elastic#115956) skip flaky suite (elastic#116060) [Canvas] By-Value Embeddables (elastic#113827) Skip failing test (elastic#115366) [Osquery] Fix live query search doesn't return relevant results for agents (elastic#116332) [Integrations] Added link in old Add Data description and fixed alignment in cards (elastic#116213) [Actions] Extended ActionTypeRegistry with connector validation to validate config with secrets (elastic#116079) skip flaky suite (elastic#109329) Grant access to machine learning features when base privileges are used (elastic#115444) Skipping failing test (elastic#84957) [RAC][Security Solution] Adds migration to new SecuritySolution rule types (elastic#112113) skip flaky suite (elastic#115366) [Fleet] Marking API spec as experimental (elastic#116331) [Docs] Cleaning up the versions in the upgrade paths. Closes elastic#116223 (elastic#116228) [Reporting] Suppress debug logs in the mock logger (elastic#116012) [Metrics UI] Clear threshold alert groups state when filterQuery changes (elastic#116205) ... # Conflicts: # src/plugins/dashboard/public/application/embeddable/dashboard_container.tsx # src/plugins/dashboard/public/types.ts
Friendly reminder: Looks like this PR hasn’t been backported yet. |
1 similar comment
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Summary
The Machine Learning feature should be included as part of the base privileges for the 8.0 release.
This is considered a breaking change since it would grant considerable privileges to existing users, and this may not be desirable for certain installations.
Resolves #71422
Checklist
Delete any items that are not applicable to this PR.