CliTool: Fixed adding of roles for existing users

The roles file had been checked instead of the users file when checking
if the user already exists. This lead to wrong "user not found" error
messages.

Original commit: elastic/x-pack-elasticsearch@6fc5646ce7

src/main/java/org/elasticsearch/shield/authc/esusers/tool/ESUsersTool.java

@@ -295,7 +295,6 @@ public class ESUsersTool extends CliTool {
                 return new ListUsersAndRoles(terminal, username).execute(settings, env);
             }
 
-
             // check for roles if they match
             String[] allRoles = ObjectArrays.concat(addRoles, removeRoles, String.class);
             for (String role : allRoles) {
@@ -305,15 +304,20 @@ public class ESUsersTool extends CliTool {
                 }
             }
 
-            Path file = FileUserRolesStore.resolveFile(settings, env);
+            Path path = FileUserPasswdStore.resolveFile(settings, env);
-            Map<String, String[]> userRoles = FileUserRolesStore.parseFile(file, null);
+            Map<String, char[]> usersMap = FileUserPasswdStore.parseFile(path, null);
-
+            if (!usersMap.containsKey(username)) {
-            if (!userRoles.containsKey(username)) {
                 terminal.println("User [%s] doesn't exist", username);
                 return ExitStatus.NO_USER;
             }
 
-            List<String> roles = Lists.newArrayList(userRoles.get(username));;
+            Path file = FileUserRolesStore.resolveFile(settings, env);
+            Map<String, String[]> userRoles = FileUserRolesStore.parseFile(file, null);
+
+            List<String> roles = Lists.newArrayList();
+            if (userRoles.get(username) != null) {
+                roles.addAll(Arrays.asList(userRoles.get(username)));
+            }
             roles.addAll(Arrays.asList(addRoles));
             roles.removeAll(Arrays.asList(removeRoles));
 

src/test/java/org/elasticsearch/shield/authc/esusers/tool/ESUsersToolTests.java

@@ -358,8 +358,10 @@ public class ESUsersToolTests extends CliToolTestCase {
 
     @Test
     public void testRoles_Cmd_addingRoleWorks() throws Exception {
-        File usersRoleFile = writeFile("admin: admin\nuser:user\n");
+        File usersFile = writeFile("admin:hash\nuser:hash");
+        File usersRoleFile = writeFile("admin: admin\nuser: user\n");
         Settings settings = ImmutableSettings.builder()
+                .put("shield.authc.esusers.files.users", usersFile)
                 .put("shield.authc.esusers.files.users_roles", usersRoleFile)
                 .build();
 
@@ -377,8 +379,10 @@ public class ESUsersToolTests extends CliToolTestCase {
 
     @Test
     public void testRoles_Cmd_removingRoleWorks() throws Exception {
-        File usersRoleFile = writeFile("admin: admin\nuser:user,foo,bar\n");
+        File usersFile = writeFile("admin:hash\nuser:hash");
+        File usersRoleFile = writeFile("admin: admin\nuser: user,foo,bar\n");
         Settings settings = ImmutableSettings.builder()
+                .put("shield.authc.esusers.files.users", usersFile)
                 .put("shield.authc.esusers.files.users_roles", usersRoleFile)
                 .build();
 
@@ -396,8 +400,10 @@ public class ESUsersToolTests extends CliToolTestCase {
 
     @Test
     public void testRoles_Cmd_addingAndRemovingRoleWorks() throws Exception {
+        File usersFile = writeFile("admin:hash\nuser:hash");
         File usersRoleFile = writeFile("admin: admin\nuser:user,foo,bar\n");
         Settings settings = ImmutableSettings.builder()
+                .put("shield.authc.esusers.files.users", usersFile)
                 .put("shield.authc.esusers.files.users_roles", usersRoleFile)
                 .build();
 
@@ -415,8 +421,10 @@ public class ESUsersToolTests extends CliToolTestCase {
 
     @Test
     public void testRoles_Cmd_userNotFound() throws Exception {
-        File usersRoleFile = writeFile("admin: admin\nuser:user,foo,bar\n");
+        File usersFile = writeFile("admin:hash\nuser:hash");
+        File usersRoleFile = writeFile("admin: admin\nuser: user,foo,bar\n");
         Settings settings = ImmutableSettings.builder()
+                .put("shield.authc.esusers.files.users", usersFile)
                 .put("shield.authc.esusers.files.users_roles", usersRoleFile)
                 .build();
 
@@ -443,6 +451,26 @@ public class ESUsersToolTests extends CliToolTestCase {
         assertThat(catchTerminalOutput.getTerminalOutput(), hasItem(allOf(containsString("user"), containsString("user,foo,bar"))));
     }
 
+    @Test
+    public void testRoles_cmd_testRoleCanBeAddedWhenUserIsNotInRolesFile() throws Exception {
+        File usersFile = writeFile("admin:hash\nuser:hash");
+        File usersRoleFile = writeFile("admin: admin\n");
+        Settings settings = ImmutableSettings.builder()
+                .put("shield.authc.esusers.files.users", usersFile)
+                .put("shield.authc.esusers.files.users_roles", usersRoleFile)
+                .build();
+
+        CaptureOutputTerminal catchTerminalOutput = new CaptureOutputTerminal();
+        ESUsersTool.Roles cmd = new ESUsersTool.Roles(catchTerminalOutput, "user", new String[]{"myrole"}, Strings.EMPTY_ARRAY);
+        CliTool.ExitStatus status = execute(cmd, settings);
+
+        assertThat(status, is(CliTool.ExitStatus.OK));
+        Map<String, String[]> userRoles = FileUserRolesStore.parseFile(usersRoleFile.toPath(), logger);
+        assertThat(userRoles.keySet(), hasSize(2));
+        assertThat(userRoles.keySet(), hasItems("admin", "user"));
+        assertThat(userRoles.get("user"), arrayContaining("myrole"));
+    }
+
     @Test
     public void testListUsersAndRoles_Cmd_parsingWorks() throws Exception {
         ESUsersTool tool = new ESUsersTool();