-
Notifications
You must be signed in to change notification settings - Fork 24.6k
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
Add the new 'maintenance' privilege containing 4 actions (#29998) #50643
Conversation
Pinging @elastic/es-security (:Security/Authorization) |
@amirhmd One of your "fix merge conflict" commits has introduced uninentional changes. Can you please have a look at the diff, and have a go at cleaning it up. Let us know if you need help - we may need to perform a little bit of git surgery. |
hi @tvernum |
Thanks @amirhmd the diff looks good. |
@elasticmachine ok to test. |
hi @albertzaharovits one test is failing as I was explained in the comment. |
...core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/IndexPrivilege.java
Outdated
Show resolved
Hide resolved
I left a comment to hint at why the test is failing, I'll take a closer look in the morning. |
@@ -201,15 +201,18 @@ Privilege to delete an index. | |||
Privilege to index and update documents. Also grants access to the update | |||
mapping action. | |||
|
|||
`maintenance`:: | |||
Privilege to refresh, flush, synced_flush, force merge |
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.
Privilege to refresh, flush, synced_flush, force merge | |
Permits refresh, flush, synced flush and force merge operations. No privilege to read or write index data or otherwise manage the index. |
x-pack/plugin/src/test/resources/rest-api-spec/test/privileges/11_builtin.yml
Outdated
Show resolved
Hide resolved
assertAccessIsAllowed("u15", | ||
"GET", "/" + randomIndex() + "/_msearch", "{}\n{ \"query\" : { \"match_all\" : {} } }\n"); | ||
assertAccessIsAllowed("u15", "POST", "/" + randomIndex() + "/_mget", "{ \"ids\" : [ \"1\", \"2\" ] } "); | ||
assertAccessIsDenied("u15", "PUT", | ||
"/" + randomIndex() + "/_bulk", "{ \"index\" : { \"_id\" : \"123\" } }\n{ \"foo\" : \"bar\" }\n"); | ||
assertAccessIsAllowed("u15", | ||
"GET", "/" + randomIndex() + "/_mtermvectors", "{ \"docs\" : [ { \"_id\": \"1\" }, { \"_id\": \"2\" } ] }"); |
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 don't think we need this fluff. This is testing the authorization of composite requests but refresh and flush are not part of those composite type of requests.
In addition to assertUserIsAllowed("u15", "maintenance", "a");
I would also check that crud
is not allowed, and we should also test that other users in this class don't have the maintenance
priv (right now it only checks the positive case).
assertAccessIsDenied(user, "PUT", "/" + index); | ||
assertAccessIsDenied(user, "POST", "/" + index + "/_refresh"); | ||
assertAccessIsDenied(user, "POST", "/" + index + "/_flush"); | ||
assertAccessIsAllowed(user, "POST", "/" + index + "/_flush/synced"); |
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.
Should be assertAccessIsDenied
, right?
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 left a few comments. I think this is close and after you address the comments it should be OK.
Thank you for your contribution @amirhmd and apologies for the delay. |
Hi @albertzaharovits , thanks for checking my code. As soon as I get home I will apply the comment. |
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've left two minor comments, please address, otherwise LGTM.
We'll wait for Tim's review and then merge. Thank you for your effort Amir!
assertUserIsAllowed("u15", "maintenance", "a"); | ||
assertUserIsDenied("u15", "crud", "a"); | ||
|
||
assertUserIsDenied("u11", "maintenance", "a"); |
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.
Please move this to the testUser11
function and do it for the b
and c
indices, i.e.
assertUserIsDenied("u11", "maintenance", "b");
, assertUserIsDenied("u11", "maintenance", "c");
.
@@ -204,15 +204,19 @@ Privilege to delete an index. | |||
Privilege to index and update documents. Also grants access to the update | |||
mapping action. | |||
|
|||
`maintenance`:: | |||
Permits refresh, flush, synced flush, force merge operations. No privilege |
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.
You missed an and
. We try to be sharp wrt the docs.
Permits refresh, flush, synced flush, force merge operations. No privilege | |
Permits refresh, flush, synced flush and force merge index administration operations. |
Thanks @albertzaharovits for the comment. I will commit in 3 hours. |
hi @albertzaharovits |
Hi @albertzaharovits may I know if I should do anything in this regard #50643 (comment)? |
@amirhmd you don't have to worry about those tests. There's no action to take on your part. |
@elasticmachine update branch |
@elasticmachine update branch |
@@ -102,7 +105,8 @@ | |||
entry("read_cross_cluster", READ_CROSS_CLUSTER), | |||
entry("manage_follow_index", MANAGE_FOLLOW_INDEX), | |||
entry("manage_leader_index", MANAGE_LEADER_INDEX), | |||
entry("manage_ilm", MANAGE_ILM)); | |||
entry("manage_ilm", MANAGE_ILM), | |||
entry("maintenance",MAINTENANCE)); |
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.
Minor:
entry("maintenance",MAINTENANCE)); | |
entry("maintenance", MAINTENANCE)); |
@@ -419,6 +432,22 @@ private void assertUserExecutes(String user, String action, String index, boolea | |||
} | |||
break; | |||
|
|||
case "maintenance" : | |||
if (userIsAllowed) { | |||
assertUserIsDenied(user, "crud", index); |
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 think this this is what we want.
A user being allowed maintenance
should not imply anything about whether they are/aren't allowed crud
.
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.
@tvernum the first and second statement made me a bit confuse. but it looks like the explanation has precedence. so I will remove them.
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.
Sorry, somehow I dropped a word. It should have said:
I don't think this is what we want.
Your fix is correct.
assertAccessIsAllowed(user, "POST", "/" + index + "/_flush/synced"); | ||
assertAccessIsAllowed(user, "POST", "/" + index + "/_forcemerge"); | ||
} else { | ||
assertUserIsDenied(user, "crud", index); |
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.
Per above.
Thank you for your contribution, @amirhmd ! |
This commit creates a new index privilege named `maintenance`. The privilege grants the following actions: `refresh`, `flush` (also synced-`flush`), and `force-merge`. Previously the actions were only under the `manage` privilege which in some situations was too permissive. Co-authored-by: Amir H Movahed <arhd83@gmail.com>
gradle check
?