Add the new 'maintenance' privilege containing 4 actions (#29998) #50643

amirhmd · 2020-01-06T09:23:28Z

Have you signed the contributor license agreement?
Have you followed the contributor guidelines?
If submitting code, have you built your formula locally prior to submission with gradle check?
If submitting code, is your pull request against master? Unless there is a good reason otherwise, we prefer pull requests against master and will backport as needed.
If submitting code, have you checked that your submission is for an OS and architecture that we support?
If you are submitting this code for a class then read our policy for that.

elasticmachine · 2020-01-06T10:42:52Z

Pinging @elastic/es-security (:Security/Authorization)

tvernum · 2020-01-06T23:40:10Z

@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.

amirhmd · 2020-01-07T00:09:28Z

hi @tvernum
could you please confirm if it sounds good now?

tvernum · 2020-01-07T01:11:32Z

Thanks @amirhmd the diff looks good.
However, it still shows a merge conflit in GitHub, so I think there's still some work to do before we get to merging this.

tvernum · 2020-01-07T01:11:42Z

@elasticmachine ok to test.

amirhmd · 2020-01-07T20:46:01Z

hi @albertzaharovits one test is failing as I was explained in the comment.
IndexPrivilegeTests/testUser15/
The other one which is failing XPackRestIT.test {p0=privileges/11_builtin/Test get builtin privileges} is because there is 33 Cluster Priviledge, I didnt increase this one in 11_builtin but at the time of commit it shows a conflict and I forced to change it.
could you please advise on this?

...core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/IndexPrivilege.java

albertzaharovits · 2020-01-07T22:53:13Z

I left a comment to hint at why the test is failing, I'll take a closer look in the morning.

albertzaharovits · 2020-01-09T09:21:02Z

x-pack/docs/en/security/authorization/privileges.asciidoc

@@ -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


Suggested change

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

albertzaharovits · 2020-01-09T10:23:38Z

x-pack/plugin/security/src/test/java/org/elasticsearch/integration/IndexPrivilegeTests.java

+        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\" } ] }");


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).

albertzaharovits · 2020-01-09T10:24:08Z

x-pack/plugin/security/src/test/java/org/elasticsearch/integration/IndexPrivilegeTests.java

+                    assertAccessIsDenied(user, "PUT", "/" + index);
+                    assertAccessIsDenied(user, "POST", "/" + index + "/_refresh");
+                    assertAccessIsDenied(user, "POST", "/" + index + "/_flush");
+                    assertAccessIsAllowed(user, "POST", "/" + index + "/_flush/synced");


Should be assertAccessIsDenied, right?

albertzaharovits

I left a few comments. I think this is close and after you address the comments it should be OK.

albertzaharovits · 2020-01-09T10:28:26Z

Thank you for your contribution @amirhmd and apologies for the delay.
I'll have another pass after we settle the comments.

amirhmd · 2020-01-09T14:41:43Z

Hi @albertzaharovits , thanks for checking my code. As soon as I get home I will apply the comment.

albertzaharovits

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!

albertzaharovits · 2020-01-10T17:26:29Z

x-pack/plugin/security/src/test/java/org/elasticsearch/integration/IndexPrivilegeTests.java

+        assertUserIsAllowed("u15", "maintenance", "a");
+        assertUserIsDenied("u15", "crud", "a");
+
+        assertUserIsDenied("u11", "maintenance", "a");


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"); .

albertzaharovits · 2020-01-10T17:31:30Z

x-pack/docs/en/security/authorization/privileges.asciidoc

@@ -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


You missed an and. We try to be sharp wrt the docs.

Suggested change

Permits refresh, flush, synced flush, force merge operations. No privilege

Permits refresh, flush, synced flush and force merge index administration operations.

amirhmd · 2020-01-10T17:50:39Z

Thanks @albertzaharovits for the comment. I will commit in 3 hours.

amirhmd · 2020-01-10T22:48:55Z

hi @albertzaharovits elasticsearch-ci/2 is failing because as part of #50454 and #50781 Wait For Snapshot is added. I think thats why this test (testWaitForSnapshotSlmExecutedBefore) is failing. could you please advise?

amirhmd · 2020-01-15T21:17:05Z

Hi @albertzaharovits may I know if I should do anything in this regard #50643 (comment)?

albertzaharovits · 2020-01-16T12:00:09Z

@amirhmd you don't have to worry about those tests. There's no action to take on your part.
@elasticmachine update branch
ping @tvernum

albertzaharovits · 2020-01-16T21:31:01Z

@elasticmachine update branch

albertzaharovits · 2020-01-27T11:30:54Z

@elasticmachine update branch

tvernum · 2020-01-28T00:39:56Z

...core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/IndexPrivilege.java

@@ -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));


Minor:

Suggested change

entry("maintenance",MAINTENANCE));

entry("maintenance", MAINTENANCE));

tvernum · 2020-01-28T00:46:29Z

x-pack/plugin/security/src/test/java/org/elasticsearch/integration/IndexPrivilegeTests.java

@@ -419,6 +432,22 @@ private void assertUserExecutes(String user, String action, String index, boolea
                }
                break;

+            case "maintenance" :
+                if (userIsAllowed) {
+                    assertUserIsDenied(user, "crud", index);


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.

@tvernum the first and second statement made me a bit confuse. but it looks like the explanation has precedence. so I will remove them.

Sorry, somehow I dropped a word. It should have said:

I don't think this is what we want.

Your fix is correct.

tvernum · 2020-01-28T00:46:32Z

x-pack/plugin/security/src/test/java/org/elasticsearch/integration/IndexPrivilegeTests.java

+                    assertAccessIsAllowed(user, "POST", "/" + index + "/_flush/synced");
+                    assertAccessIsAllowed(user, "POST", "/" + index + "/_forcemerge");
+                } else {
+                    assertUserIsDenied(user, "crud", index);


albertzaharovits · 2020-01-30T08:39:59Z

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>

Add the new 'maintenance' privilege containing 4 actions (#29998)

363f1ff

amirhmd mentioned this pull request Jan 6, 2020

Add index privilege for refresh/flush separate to manage #29998

Closed

cbuescher added the :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC label Jan 6, 2020

tvernum assigned albertzaharovits Jan 6, 2020

tvernum requested review from albertzaharovits and tvernum January 6, 2020 23:33

tvernum added >enhancement v7.6.0 v8.0.0 labels Jan 6, 2020

Fix merge conflict

3421673

Fix merge conflict

22f7588

albertzaharovits reviewed Jan 7, 2020

View reviewed changes

...core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/IndexPrivilege.java Outdated Show resolved Hide resolved

albertzaharovits reviewed Jan 9, 2020

View reviewed changes

x-pack/plugin/src/test/resources/rest-api-spec/test/privileges/11_builtin.yml Outdated Show resolved Hide resolved

albertzaharovits reviewed Jan 9, 2020

View reviewed changes

albertzaharovits requested changes Jan 9, 2020

View reviewed changes

amirhmd added 3 commits January 9, 2020 22:23

Apply code review - fixed failed test (#29998)

73f5763

Fix merge conflict - increase cluster (#29998)

0ddc277

Merge branch 'master' into master

844c4b1

albertzaharovits approved these changes Jan 10, 2020

View reviewed changes

albertzaharovits mentioned this pull request Jan 10, 2020

Unit tests for index and cluster privileges #50867

Open

Polish message and test (#29998)

e1a6208

$@polyfractal$ polyfractal added v7.7.0 and removed v7.6.0 labels Jan 15, 2020

Merge branch 'master' into master

cecd284

Merge branch 'master' into master

8a9779a

tvernum requested changes Jan 28, 2020

View reviewed changes

Polish and remove unnecessary crud check in test (#29998)

746d4d7

amirhmd requested a review from tvernum January 28, 2020 09:59

tvernum approved these changes Jan 28, 2020

View reviewed changes

albertzaharovits merged commit 5712246 into elastic:master Jan 30, 2020

albertzaharovits added the backport pending label Jan 30, 2020

albertzaharovits mentioned this pull request Jan 30, 2020

BACKPORT Add new 'maintenance' index privilege (#50643) #51661

Merged

albertzaharovits removed the backport pending label Jan 30, 2020

codebrain mentioned this pull request Apr 1, 2020

7.7.0 meta ticket (Part 2) elastic/elasticsearch-net#4533

Closed

albertzaharovits mentioned this pull request Jun 10, 2020

Add index-level privilege for force merge #29702

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the new 'maintenance' privilege containing 4 actions (#29998) #50643

Add the new 'maintenance' privilege containing 4 actions (#29998) #50643

amirhmd commented Jan 6, 2020

elasticmachine commented Jan 6, 2020

tvernum commented Jan 6, 2020

amirhmd commented Jan 7, 2020

tvernum commented Jan 7, 2020

tvernum commented Jan 7, 2020

amirhmd commented Jan 7, 2020

albertzaharovits commented Jan 7, 2020

albertzaharovits Jan 9, 2020

albertzaharovits Jan 9, 2020

albertzaharovits Jan 9, 2020

albertzaharovits left a comment

albertzaharovits commented Jan 9, 2020 •

edited

Loading

amirhmd commented Jan 9, 2020

albertzaharovits left a comment

albertzaharovits Jan 10, 2020

albertzaharovits Jan 10, 2020

amirhmd commented Jan 10, 2020

amirhmd commented Jan 10, 2020 •

edited

Loading

amirhmd commented Jan 15, 2020

albertzaharovits commented Jan 16, 2020

albertzaharovits commented Jan 16, 2020

albertzaharovits commented Jan 27, 2020

tvernum Jan 28, 2020

tvernum Jan 28, 2020 •

edited

Loading

amirhmd Jan 28, 2020

tvernum Jan 28, 2020

tvernum Jan 28, 2020

albertzaharovits commented Jan 30, 2020

	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.

	Permits refresh, flush, synced flush, force merge operations. No privilege
	Permits refresh, flush, synced flush and force merge index administration operations.

	entry("maintenance",MAINTENANCE));
	entry("maintenance", MAINTENANCE));

Add the new 'maintenance' privilege containing 4 actions (#29998) #50643

Add the new 'maintenance' privilege containing 4 actions (#29998) #50643

Conversation

amirhmd commented Jan 6, 2020

elasticmachine commented Jan 6, 2020

tvernum commented Jan 6, 2020

amirhmd commented Jan 7, 2020

tvernum commented Jan 7, 2020

tvernum commented Jan 7, 2020

amirhmd commented Jan 7, 2020

albertzaharovits commented Jan 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertzaharovits left a comment

Choose a reason for hiding this comment

albertzaharovits commented Jan 9, 2020 • edited Loading

amirhmd commented Jan 9, 2020

albertzaharovits left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirhmd commented Jan 10, 2020

amirhmd commented Jan 10, 2020 • edited Loading

amirhmd commented Jan 15, 2020

albertzaharovits commented Jan 16, 2020

albertzaharovits commented Jan 16, 2020

albertzaharovits commented Jan 27, 2020

Choose a reason for hiding this comment

tvernum Jan 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertzaharovits commented Jan 30, 2020

albertzaharovits commented Jan 9, 2020 •

edited

Loading

amirhmd commented Jan 10, 2020 •

edited

Loading

tvernum Jan 28, 2020 •

edited

Loading