-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
Changes from 3 commits
363f1ff
3421673
22f7588
73f5763
0ddc277
844c4b1
e1a6208
cecd284
8a9779a
746d4d7
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 | ||||
---|---|---|---|---|---|---|
|
@@ -66,6 +66,8 @@ public final class IndexPrivilege extends Privilege { | |||||
CloseIndexAction.NAME + "*"); | ||||||
private static final Automaton MANAGE_LEADER_INDEX_AUTOMATON = patterns(ForgetFollowerAction.NAME + "*"); | ||||||
private static final Automaton MANAGE_ILM_AUTOMATON = patterns("indices:admin/ilm/*"); | ||||||
private static final Automaton MAINTENANCE_AUTOMATON = patterns("indices:admin/refresh", "indices:admin/flush", | ||||||
albertzaharovits marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
"indices:admin/synced_flush", "indices:admin/forcemerge"); | ||||||
|
||||||
public static final IndexPrivilege NONE = new IndexPrivilege("none", Automatons.EMPTY); | ||||||
public static final IndexPrivilege ALL = new IndexPrivilege("all", ALL_AUTOMATON); | ||||||
|
@@ -83,7 +85,8 @@ public final class IndexPrivilege extends Privilege { | |||||
public static final IndexPrivilege VIEW_METADATA = new IndexPrivilege("view_index_metadata", VIEW_METADATA_AUTOMATON); | ||||||
public static final IndexPrivilege MANAGE_FOLLOW_INDEX = new IndexPrivilege("manage_follow_index", MANAGE_FOLLOW_INDEX_AUTOMATON); | ||||||
public static final IndexPrivilege MANAGE_LEADER_INDEX = new IndexPrivilege("manage_leader_index", MANAGE_LEADER_INDEX_AUTOMATON); | ||||||
public static final IndexPrivilege MANAGE_ILM = new IndexPrivilege("manage_ilm", MANAGE_ILM_AUTOMATON); | ||||||
public static final IndexPrivilege MANAGE_ILM = new IndexPrivilege("manage_ilm", MANAGE_ILM_AUTOMATON); | ||||||
public static final IndexPrivilege MAINTENANCE = new IndexPrivilege("maintenance", MAINTENANCE_AUTOMATON); | ||||||
|
||||||
private static final Map<String, IndexPrivilege> VALUES = Map.ofEntries( | ||||||
entry("none", NONE), | ||||||
|
@@ -102,7 +105,8 @@ public final class IndexPrivilege extends Privilege { | |||||
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 commentThe reason will be displayed to describe this comment to others. Learn more. Minor:
Suggested change
|
||||||
|
||||||
public static final Predicate<String> ACTION_MATCHER = ALL.predicate(); | ||||||
public static final Predicate<String> CREATE_INDEX_MATCHER = CREATE_INDEX.predicate(); | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -69,6 +69,10 @@ public class IndexPrivilegeTests extends AbstractPrivilegeTestCase { | |
" indices:\n" + | ||
" - names: 'b'\n" + | ||
" privileges: [ monitor ]\n" + | ||
"maintenance_a_role:\n" + | ||
" indices:\n" + | ||
" - names: 'a'\n" + | ||
" privileges: [ maintenance ]\n" + | ||
"read_write_a_role:\n" + | ||
" indices:\n" + | ||
" - names: 'a'\n" + | ||
|
@@ -96,6 +100,7 @@ public class IndexPrivilegeTests extends AbstractPrivilegeTestCase { | |
"read_write_all_role:u12\n" + | ||
"create_c_role:u11\n" + | ||
"monitor_b_role:u14\n" + | ||
"maintenance_a_role:u15\n" + | ||
"read_write_a_role:u12\n" + | ||
"delete_b_role:u11\n" + | ||
"index_a_role:u13\n"; | ||
|
@@ -129,7 +134,8 @@ protected String configUsers() { | |
"u11:" + usersPasswdHashed + "\n" + | ||
"u12:" + usersPasswdHashed + "\n" + | ||
"u13:" + usersPasswdHashed + "\n" + | ||
"u14:" + usersPasswdHashed + "\n"; | ||
"u14:" + usersPasswdHashed + "\n" + | ||
"u15:" + usersPasswdHashed + "\n" ; | ||
} | ||
|
||
@Override | ||
|
@@ -385,6 +391,18 @@ public void testUserU14() throws Exception { | |
"GET", "/" + randomIndex() + "/_mtermvectors", "{ \"docs\" : [ { \"_id\": \"1\" }, { \"_id\": \"2\" } ] }"); | ||
} | ||
|
||
public void testUser15() throws Exception { | ||
assertUserIsAllowed("u15", "maintenance", "a"); | ||
|
||
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 commentThe 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. |
||
} | ||
|
||
public void testThatUnknownUserIsRejectedProperly() throws Exception { | ||
try { | ||
Request request = new Request("GET", "/"); | ||
|
@@ -419,6 +437,23 @@ 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 commentThe reason will be displayed to describe this comment to others. Learn more. I think this this is what we want. 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. @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 commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, somehow I dropped a word. It should have said:
Your fix is correct. |
||
assertAccessIsAllowed(user, "POST", "/" + index + "/_refresh"); | ||
assertAccessIsAllowed(user, "POST", "/" + index + "/_flush"); | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. Per above. |
||
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 commentThe reason will be displayed to describe this comment to others. Learn more. Should be |
||
assertAccessIsDenied(user, "POST", "/" + index + "/_forcemerge"); | ||
} | ||
break; | ||
|
||
case "manage" : | ||
if (userIsAllowed) { | ||
assertAccessIsAllowed(user, "DELETE", "/" + 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.