-
Notifications
You must be signed in to change notification settings - Fork 8.6k
Beginning to use the ES APIs to insert/check privileges #18645
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
Changes from all commits
d243802
d9c93b7
5aaf5bc
05c594f
4d60ca1
03c429a
1d2b564
a9fda18
80494b8
6567941
d93b758
442ee73
3019006
5a16dea
92ab282
bb87576
6f83da2
1ea7cb9
dddccb6
f0f705e
98c9d8c
19dd2bc
c35e759
4385765
e621440
4096be9
22a22ac
9d73c4b
ab2d9ff
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License; | ||
| * you may not use this file except in compliance with the Elastic License. | ||
| */ | ||
|
|
||
| export const DEFAULT_RESOURCE = 'default'; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,10 @@ | ||
| <kbn-management-app section="security" omit-breadcrumb-pages="['edit']"> | ||
| <!-- This content gets injected below the localNav. --> | ||
| <div class="kuiViewContent kuiViewContent--constrainedWidth kuiViewContentItem"> | ||
|
|
||
| <!-- Subheader --> | ||
| <div class="kuiBar kuiVerticalRhythm"> | ||
|
|
||
| <div class="kuiBarSection"> | ||
| <!-- Title --> | ||
| <h1 class="kuiTitle"> | ||
|
|
@@ -39,6 +41,18 @@ <h1 class="kuiTitle"> | |
| </div> | ||
| </div> | ||
|
|
||
| <div class="kuiBar kuiVerticalRhythm" ng-if="otherApplications.length > 0"> | ||
| <div class="kuiInfoPanel kuiInfoPanel--warning"> | ||
| <div class="kuiInfoPanelHeader"> | ||
| <span class="kuiInfoPanelHeader__icon kuiIcon kuiIcon--warning fa-warning"></span> | ||
| <span class="kuiInfoPanelHeader__title"> | ||
| This role contains application privileges for the {{ otherApplications.join(', ') }} application(s) that can't be edited. | ||
| If they are for other instances of Kibana, you must manage those privileges on that Kibana. | ||
| </span> | ||
| </div> | ||
| </div> | ||
| </div> | ||
|
|
||
| <!-- Form --> | ||
| <form name="form" novalidate class="kuiVerticalRhythm"> | ||
| <!-- Name --> | ||
|
|
@@ -56,7 +70,7 @@ <h1 class="kuiTitle"> | |
| ng-model="role.name" | ||
| required | ||
| pattern="[a-zA-Z_][a-zA-Z0-9_@\-\$\.]*" | ||
| maxlength="30" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like ES supports role names up to 1024 characters. Is there a reason we don't mirror that on the Kibana side?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a good one! I have an excuse but I won't bore you with it. |
||
| maxlength="1024" | ||
| data-test-subj="roleFormNameInput" | ||
| /> | ||
|
|
||
|
|
@@ -102,20 +116,15 @@ <h1 class="kuiTitle"> | |
| Kibana Privileges | ||
| </label> | ||
|
|
||
| <div class="kuiInputNote kuiInputNote--warning" ng-if="role.hasUnsupportedCustomPrivileges"> | ||
| Changes to this section are not supported: this role contains application privileges that do not belong to this instance of Kibana. | ||
| </div> | ||
|
|
||
| <div ng-repeat="privilege in kibanaPrivileges track by privilege.name"> | ||
| <div ng-repeat="(key, value) in kibanaPrivileges"> | ||
| <label> | ||
| <input | ||
| class="kuiCheckBox" | ||
| type="checkbox" | ||
| ng-checked="includesPermission(role, privilege)" | ||
| ng-click="togglePermission(role, privilege)" | ||
| ng-disabled="role.metadata._reserved || !isRoleEnabled(role) || role.hasUnsupportedCustomPrivileges" | ||
| ng-model="kibanaPrivileges[key]" | ||
| ng-disabled="role.metadata._reserved || !isRoleEnabled(role)" | ||
| /> | ||
| <span class="kuiOptionLabel">{{privilege.metadata.displayName}}</span> | ||
| <span class="kuiOptionLabel">{{key}}</span> | ||
| </label> | ||
| </div> | ||
| </div> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| // Jest Snapshot v1, https://goo.gl/fbAQLP | ||
|
|
||
| exports[`throws error if missing version privilege and has login privilege 1`] = `"Multiple versions of Kibana are running against the same Elasticsearch cluster, unable to authorize user."`; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License; | ||
| * you may not use this file except in compliance with the Elastic License. | ||
| */ | ||
|
|
||
| import { getClient } from '../../../../../server/lib/get_client_shield'; | ||
| import { DEFAULT_RESOURCE } from '../../../common/constants'; | ||
| import { getVersionPrivilege, getLoginPrivilege } from '../privileges'; | ||
|
|
||
| const getMissingPrivileges = (resource, application, privilegeCheck) => { | ||
| const privileges = privilegeCheck.application[application][resource]; | ||
| return Object.keys(privileges).filter(key => privileges[key] === false); | ||
| }; | ||
|
|
||
| export function hasPrivilegesWithServer(server) { | ||
| const callWithRequest = getClient(server).callWithRequest; | ||
|
|
||
| const config = server.config(); | ||
| const kibanaVersion = config.get('pkg.version'); | ||
| const application = config.get('xpack.security.rbac.application'); | ||
|
|
||
| return function hasPrivilegesWithRequest(request) { | ||
| return async function hasPrivileges(privileges) { | ||
|
|
||
| const versionPrivilege = getVersionPrivilege(kibanaVersion); | ||
| const loginPrivilege = getLoginPrivilege(); | ||
|
|
||
| const privilegeCheck = await callWithRequest(request, 'shield.hasPrivileges', { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to add exception handling here? If this call throws, then I think the error will get thrown out of the SavedObjectsClient uninspected, which condradicts its documentation
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch, I'll make sure we're throwing the appropriate wrapped errors.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this will be used outside of the context of the SavedObjectClient, I'm going to have the SavedObjectClient's usage wrap these errors instead of modifying it directly in the hasPrivileges service. |
||
| body: { | ||
| applications: [{ | ||
| application, | ||
| resources: [DEFAULT_RESOURCE], | ||
| privileges: [versionPrivilege, loginPrivilege, ...privileges] | ||
| }] | ||
| } | ||
| }); | ||
|
|
||
| const success = privilegeCheck.has_all_requested; | ||
| const missingPrivileges = getMissingPrivileges(DEFAULT_RESOURCE, application, privilegeCheck); | ||
|
|
||
| // We include the login privilege on all privileges, so the existence of it and not the version privilege | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could very well be YAGNI, but I'll ask anyway: Do we envision having service accounts in the future, for reporting or other background tasks? In other words, could we have accounts that have privileges within Kibana, but without the ability to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't heard any discussion of service accounts in this form, and they'd all have to authenticate somehow (making the login action relevant). I'm having trouble envisioning a situation where we couldn't rely on the login privilege to be present. |
||
| // lets us know that we're running in an incorrect configuration. Without the login privilege check, we wouldn't | ||
| // know whether the user just wasn't authorized for this instance of Kibana in general | ||
| if (missingPrivileges.includes(versionPrivilege) && !missingPrivileges.includes(loginPrivilege)) { | ||
| throw new Error('Multiple versions of Kibana are running against the same Elasticsearch cluster, unable to authorize user.'); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess the corrective action would be "Stop doing that", but are there any steps we should document for users who find themselves in this situation? ("no" is a perfectly acceptable answer 😄)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is definitely something that we'll want to document. There isn't really a corrective action besides "don't do that" unfortunately. |
||
| } | ||
|
|
||
| return { | ||
| success, | ||
| missing: missingPrivileges | ||
| }; | ||
| }; | ||
| }; | ||
| } | ||
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.
Since we use the
rbacApplicationto derive the role name, what do you think about validating that this also conforms to the role naming rules?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.
Good call.
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 chose a more restrictive pattern because it'd look inconsistent to accept punctuation and spaces and then append
_rbac_userand_rbac_dashboard_only_userto the end.