Massive refactoring to permission infrastructure

- The Permission class changed such that now there isn't a single `check` method that all permission types must implement. Instead, each permission type has its own (if at all) check method that is relevant to what the permssion is supposed to check.

- Moved the indices resolving logic outside of the indices permission class to the authorization service. Also, the authroization service has all the logic on how to check each one of the indices against a compound/merged permission view over all the user's roles. This fixes a critical bug where if a user had more than one role, its permission wouldn't be checked appropriately (they were checked separately which introduced invalid results)

- Cleaned up and got rid of unused code

- System role is no longer implementing Permission (no need for that)

- Additional tests were added with different users/roles configuration to try an capture such bugs

Fixes elastic/elasticsearch#304

Original commit: elastic/x-pack-elasticsearch@5c9a581019

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

@@ -348,7 +348,7 @@ public class ESUsersTool extends CliTool {
 
         @Override
         public ExitStatus execute(Settings settings, Environment env) throws Exception {
-            ImmutableMap<String, Permission.Global> knownRoles = loadRoles(terminal, settings, env);
+            ImmutableMap<String, Permission.Global.Role> knownRoles = loadRoles(terminal, settings, env);
             Path userRolesFilePath = FileUserRolesStore.resolveFile(settings, env);
             Map<String, String[]> userRoles = FileUserRolesStore.parseFile(userRolesFilePath, null);
             Path userFilePath = FileUserPasswdStore.resolveFile(settings, env);
@@ -402,7 +402,7 @@ public class ESUsersTool extends CliTool {
         }
     }
 
-    private static ImmutableMap<String, Permission.Global> loadRoles(Terminal terminal, Settings settings, Environment env) {
+    private static ImmutableMap<String, Permission.Global.Role> loadRoles(Terminal terminal, Settings settings, Environment env) {
         Path rolesFile = FileRolesStore.resolveFile(settings, env);
         try {
             return FileRolesStore.parseFile(rolesFile, null, new DummyAuthzService());
@@ -429,7 +429,7 @@ public class ESUsersTool extends CliTool {
     }
 
     private static void verifyRoles(Terminal terminal, Settings settings, Environment env, String[] roles) {
-        ImmutableMap<String, Permission.Global> knownRoles = loadRoles(terminal, settings, env);
+        ImmutableMap<String, Permission.Global.Role> knownRoles = loadRoles(terminal, settings, env);
         if (knownRoles == null) {
             return;
         }

src/main/java/org/elasticsearch/shield/authz/InternalAuthorizationService.java

@@ -5,6 +5,8 @@
  */
 package org.elasticsearch.shield.authz;
 
+import org.elasticsearch.action.CompositeIndicesRequest;
+import org.elasticsearch.action.IndicesRequest;
 import org.elasticsearch.cluster.ClusterService;
 import org.elasticsearch.cluster.metadata.MetaData;
 import org.elasticsearch.common.base.Predicate;
@@ -12,14 +14,17 @@ import org.elasticsearch.common.base.Predicates;
 import org.elasticsearch.common.collect.ImmutableList;
 import org.elasticsearch.common.component.AbstractComponent;
 import org.elasticsearch.common.inject.Inject;
-import org.elasticsearch.common.inject.internal.Nullable;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.shield.User;
 import org.elasticsearch.shield.audit.AuditTrail;
+import org.elasticsearch.shield.authz.indicesresolver.DefaultIndicesResolver;
+import org.elasticsearch.shield.authz.indicesresolver.IndicesResolver;
 import org.elasticsearch.shield.authz.store.RolesStore;
 import org.elasticsearch.transport.TransportRequest;
 
+import java.util.Collections;
 import java.util.Iterator;
+import java.util.Set;
 
 /**
  *
@@ -28,14 +33,18 @@ public class InternalAuthorizationService extends AbstractComponent implements A
 
     private final ClusterService clusterService;
     private final RolesStore rolesStore;
-    private final @Nullable AuditTrail auditTrail;
+    private final AuditTrail auditTrail;
+    private final IndicesResolver[] indicesResolvers;
 
     @Inject
-    public InternalAuthorizationService(Settings settings, RolesStore rolesStore, ClusterService clusterService, @Nullable AuditTrail auditTrail) {
+    public InternalAuthorizationService(Settings settings, RolesStore rolesStore, ClusterService clusterService, AuditTrail auditTrail) {
         super(settings);
         this.rolesStore = rolesStore;
         this.clusterService = clusterService;
         this.auditTrail = auditTrail;
+        this.indicesResolvers = new IndicesResolver[] {
+                new DefaultIndicesResolver(this)
+        };
     }
 
     @Override
@@ -46,7 +55,7 @@ public class InternalAuthorizationService extends AbstractComponent implements A
         }
         ImmutableList.Builder<Predicate<String>> predicates = ImmutableList.builder();
         for (String role : roles) {
-            Permission.Global global = rolesStore.permission(role);
+            Permission.Global.Role global = rolesStore.role(role);
             if (global != null) {
                 predicates.add(global.indices().allowedIndicesMatcher(action));
             }
@@ -72,43 +81,121 @@ public class InternalAuthorizationService extends AbstractComponent implements A
     @Override
     public void authorize(User user, String action, TransportRequest request) throws AuthorizationException {
 
+        // first we need to check if the user is the system. If it is, we'll just authorize the system access
         if (user.isSystem()) {
-            MetaData metaData = clusterService.state().metaData();
+            if (SystemRole.INSTANCE.check(action)) {
-            if (SystemRole.INSTANCE.check(user, action, request, metaData)) {
                 grant(user, action, request);
+                return;
+            }
+            throw denial(user, action, request);
+        }
+
+        String[] roleNames = user.roles();
+        if (roleNames.length == 0) {
+            throw denial(user, action, request);
+        }
+
+        Permission.Global permission;
+        if (roleNames.length == 1) {
+            permission = rolesStore.role(roleNames[0]);
         } else {
-                deny(user, action, request);
+
+            // we'll take all the roles and combine their associated permissions
+
+            Permission.Global.Compound.Builder roles = Permission.Global.Compound.builder();
+            for (String roleName : roleNames) {
+                Permission.Global role = rolesStore.role(roleName);
+                if (role != null) {
+                    roles.add(role);
                 }
-            return;
+            }
+            permission = roles.build();
         }
 
-        String[] roles = user.roles();
+        // permission can be null as it might be that the user's role
-        if (roles.length == 0) {
+        // is unknown
-            deny(user, action, request);
+        if (permission == null || permission.isEmpty()) {
+            throw denial(user, action, request);
         }
 
-        MetaData metaData = clusterService.state().metaData();
+        // first, we'll check if the action is a cluster action. If it is, we'll only check it
-        for (String role : roles) {
+        // agaist the cluster permissions
-            Permission permission = rolesStore.permission(role);
+        if (Privilege.Cluster.ACTION_MATCHER.apply(action)) {
-            if (permission != null && permission.check(user, action, request, metaData)) {
+            Permission.Cluster cluster = permission.cluster();
+            if (cluster != null && cluster.check(action)) {
                 grant(user, action, request);
                 return;
             }
+            throw denial(user, action, request);
         }
 
-        deny(user, action, request);
+        // ok... this is not a cluster action, let's verify it's an indices action
+        if (!Privilege.Index.ACTION_MATCHER.apply(action)) {
+            throw denial(user, action, request);
         }
 
-    private void deny(User user, String action, TransportRequest request) {
+        Permission.Indices indices = permission.indices();
-        if (auditTrail != null) {
+        if (indices == null || indices.isEmpty()) {
+            throw denial(user, action, request);
+        }
+
+        Set<String> indexNames = resolveIndices(user, action, request);
+        if (indexNames == null) {
+            // the only time this will be null, is for those requests that are
+            // categorized as indices request but they're actully not (for example, scroll)
+            // in these cases, we only grant/deny based on the action name (performed above)
+            grant(user, action, request);
+            return;
+        }
+
+        // now... every index that is associated with the request, must be granted
+        // by at least one indices permission group
+        for (String index : indexNames) {
+            boolean granted = false;
+            for (Permission.Indices.Group group : indices) {
+                if (group.check(action, index)) {
+                    granted = true;
+                    break;
+                }
+            }
+            if (!granted) {
+                throw denial(user, action, request);
+            }
+        }
+
+        grant(user, action, request);
+    }
+
+    private AuthorizationException denial(User user, String action, TransportRequest request) {
         auditTrail.accessDenied(user, action, request);
-        }
+        return new AuthorizationException("Action [" + action + "] is unauthorized for user [" + user.principal() + "]");
-        throw new AuthorizationException("Action [" + action + "] is unauthorized for user [" + user.principal() + "]");
     }
 
     private void grant(User user, String action, TransportRequest request) {
-        if (auditTrail != null) {
         auditTrail.accessGranted(user, action, request);
     }
+
+    private Set<String> resolveIndices(User user, String action, TransportRequest request) {
+        MetaData metaData = clusterService.state().metaData();
+
+        // some APIs are indices requests that are not actually associated with indices. For example,
+        // search scroll request, is categorized under the indices context, but doesn't hold indices names
+        // (in this case, the security check on the indices was done on the search request that initialized
+        // the scroll... and we rely on the signed scroll id to provide security over this request).
+
+        // so we only check indices if indeed the request is an actual IndicesRequest, if it's not, we only
+        // perform the check on the action name.
+        Set<String> indices = null;
+
+        if (request instanceof IndicesRequest || request instanceof CompositeIndicesRequest) {
+            indices = Collections.emptySet();
+            for (IndicesResolver resolver : indicesResolvers) {
+                if (resolver.requestType().isInstance(request)) {
+                    indices = resolver.resolve(user, action, request, metaData);
+                    break;
+                }
+            }
+        }
+        return indices;
     }
 }

src/main/java/org/elasticsearch/shield/authz/Permission.java

@@ -5,24 +5,18 @@
  */
 package org.elasticsearch.shield.authz;
 
-import org.elasticsearch.action.CompositeIndicesRequest;
-import org.elasticsearch.action.IndicesRequest;
-import org.elasticsearch.cluster.metadata.MetaData;
 import org.elasticsearch.common.base.Predicate;
 import org.elasticsearch.common.cache.CacheBuilder;
 import org.elasticsearch.common.cache.CacheLoader;
 import org.elasticsearch.common.cache.LoadingCache;
 import org.elasticsearch.common.collect.ImmutableList;
-import org.elasticsearch.shield.User;
+import org.elasticsearch.common.collect.Iterators;
-import org.elasticsearch.shield.authz.indicesresolver.DefaultIndicesResolver;
+import org.elasticsearch.common.collect.UnmodifiableIterator;
-import org.elasticsearch.shield.authz.indicesresolver.IndicesResolver;
 import org.elasticsearch.shield.support.AutomatonPredicate;
 import org.elasticsearch.shield.support.Automatons;
-import org.elasticsearch.transport.TransportRequest;
 
-import java.util.Collection;
 import java.util.Collections;
-import java.util.Set;
+import java.util.Iterator;
 
 /**
  * Represents a permission in the system. There are 3 types of permissions:
@@ -42,17 +36,13 @@ import java.util.Set;
  */
 public interface Permission {
 
-    boolean check(User user, String action, TransportRequest request, MetaData metaData);
+    boolean isEmpty();
 
-    static class Global implements Permission {
+    static abstract class Global implements Permission {
 
         private final Cluster cluster;
         private final Indices indices;
 
-        Global() {
-            this(null, null);
-        }
-
         Global(Cluster cluster, Indices indices) {
             this.cluster = cluster;
             this.indices = indices;
@@ -66,64 +56,117 @@ public interface Permission {
             return indices;
         }
 
-        public boolean check(User user, String action, TransportRequest request, MetaData metaData) {
+        @Override
-            if (Privilege.Cluster.ACTION_MATCHER.apply(action)) {
+        public boolean isEmpty() {
-                return cluster != null && cluster.check(user, action, request, metaData);
+            return (cluster == null || cluster.isEmpty()) && (indices == null || indices.isEmpty());
-            }
-            if (Privilege.Index.ACTION_MATCHER.apply(action)) {
-                return indices != null && indices.check(user, action, request, metaData);
-            }
-            return false;
         }
 
-        public static Builder builder(AuthorizationService authzService) {
+        public static class Role extends Global {
-            return new Builder(authzService);
+
+            private final String name;
+
+            private Role(String name, Cluster.Core cluster, Indices.Core indices) {
+                super(cluster, indices);
+                this.name = name;
+            }
+
+            public String name() {
+                return name;
+            }
+
+            @Override
+            public Cluster.Core cluster() {
+                return (Cluster.Core) super.cluster();
+            }
+
+            @Override
+            public Indices.Core indices() {
+                return (Indices.Core) super.indices();
+            }
+
+            public static Builder builder(String name) {
+                return new Builder(name);
             }
 
             public static class Builder {
 
-            private final AuthorizationService authzService;
+                private final String name;
+                private Cluster.Core cluster = Cluster.Core.NONE;
+                private ImmutableList.Builder<Indices.Group> groups = ImmutableList.builder();
 
-            private Cluster cluster = Cluster.NONE;
+                private Builder(String name) {
-            private ImmutableList.Builder<Indices.Group> groups;
+                    this.name = name;
-
-            private Builder(AuthorizationService authzService) {
-                this.authzService = authzService;
                 }
 
                 public Builder set(Privilege.Cluster privilege) {
-                cluster = new Cluster(privilege);
+                    cluster = new Cluster.Core(privilege);
                     return this;
                 }
 
                 public Builder add(Privilege.Index privilege, String... indices) {
-                if (groups == null) {
-                    groups = ImmutableList.builder();
-                }
                     groups.add(new Indices.Group(privilege, indices));
                     return this;
                 }
 
-            public Global build() {
+                public Role build() {
-                Indices indices = groups != null ? new Indices(authzService, groups.build()) : Indices.NONE;
+                    ImmutableList<Indices.Group> list = groups.build();
-                return new Global(cluster, indices);
+                    Indices.Core indices = list.isEmpty() ? Indices.Core.NONE : new Indices.Core(list.toArray(new Indices.Group[list.size()]));
+                    return new Role(name, cluster, indices);
                 }
             }
         }
 
-    static class Cluster implements Permission {
+        static class Compound extends Global {
 
-        public static final Cluster NONE = new Cluster(Privilege.Cluster.NONE) {
+            public Compound(ImmutableList<Global> globals) {
+                super(new Cluster.Globals(globals), new Indices.Globals(globals));
+            }
+
+            public static Builder builder() {
+                return new Builder();
+            }
+
+            public static class Builder {
+
+                private ImmutableList.Builder<Global> globals = ImmutableList.builder();
+
+                private Builder() {
+                }
+
+                public Builder add(Global global) {
+                    globals.add(global);
+                    return this;
+                }
+
+                public Compound build() {
+                    return new Compound(globals.build());
+                }
+            }
+        }
+    }
+
+    static interface Cluster extends Permission {
+
+        boolean check(String action);
+
+        public static class Core implements Cluster {
+
+            public static final Core NONE = new Core(Privilege.Cluster.NONE) {
                 @Override
-            public boolean check(User user, String action, TransportRequest request, MetaData metaData) {
+                public boolean check(String action) {
                     return false;
                 }
+
+                @Override
+                public boolean isEmpty() {
+                    return true;
+                }
             };
 
             private final Privilege.Cluster privilege;
             private final Predicate<String> predicate;
 
-        private Cluster(Privilege.Cluster privilege) {
+            private Core(Privilege.Cluster privilege) {
                 this.privilege = privilege;
                 this.predicate = privilege.predicate();
             }
@@ -132,19 +175,67 @@ public interface Permission {
                 return privilege;
             }
 
-        @Override
+            public boolean check(String action) {
-        public boolean check(User user, String action, TransportRequest request, MetaData metaData) {
                 return predicate.apply(action);
             }
+
+            @Override
+            public boolean isEmpty() {
+                return false;
+            }
         }
 
-    static class Indices implements Permission {
+        static class Globals implements Cluster {
+
+            private final ImmutableList<Global> globals;
+
+            public Globals(ImmutableList<Global> globals) {
+                this.globals = globals;
+            }
 
-        public static final Indices NONE = new Indices() {
             @Override
-            public boolean check(User user, String action, TransportRequest request, MetaData metaData) {
+            public boolean check(String action) {
+                if (globals == null) {
                     return false;
                 }
+                for (Global global : globals) {
+                    if (global.cluster().check(action)) {
+                        return true;
+                    }
+                }
+                return false;
+            }
+
+            @Override
+            public boolean isEmpty() {
+                if (globals == null || globals.isEmpty()) {
+                    return true;
+                }
+                for (Global global : globals) {
+                    if (!global.isEmpty()) {
+                        return false;
+                    }
+                }
+                return true;
+            }
+        }
+
+    }
+
+    static interface Indices extends Permission, Iterable<Indices.Group> {
+
+        public static class Core implements Indices {
+
+            public static final Core NONE = new Core() {
+                @Override
+                public Iterator<Group> iterator() {
+                    return Collections.emptyIterator();
+                }
+
+                @Override
+                public boolean isEmpty() {
+                    return true;
+                }
             };
 
             private final LoadingCache<String, Predicate<String>> allowedIndicesMatchersForAction = CacheBuilder.newBuilder()
@@ -161,46 +252,24 @@ public interface Permission {
                         }
                     });
 
-        private final LoadingCache<Privilege.Index, Predicate<String>> allowedIndicesMatchersForPrivilege = CacheBuilder.newBuilder()
-                .build(new CacheLoader<Privilege.Index, Predicate<String>>() {
-                    @Override
-                    public Predicate<String> load(Privilege.Index privilege) throws Exception {
-                        ImmutableList.Builder<String> indices = ImmutableList.builder();
-                        for (Group group : groups) {
-                            if (group.privilege.implies(privilege)) {
-                                indices.add(group.indices);
-                            }
-                        }
-                        return new AutomatonPredicate(Automatons.patterns(indices.build()));
-                    }
-                });
-
-        private final IndicesResolver[] indicesResolvers;
             private final Group[] groups;
 
-        private Indices() {
+            public Core(Group... groups) {
-            this.indicesResolvers = new IndicesResolver[0];
+                this.groups = groups;
-            this.groups = new Group[0];
             }
 
-        public Indices(AuthorizationService authzService, Collection<Group> groups) {
+            @Override
-            this.groups = groups.toArray(new Group[groups.size()]);
+            public Iterator<Group> iterator() {
-            this.indicesResolvers = new IndicesResolver[] {
+                return Iterators.forArray(groups);
-                    // add special resolvers here
-                    new DefaultIndicesResolver(authzService)
-            };
             }
 
             public Group[] groups() {
                 return groups;
             }
 
-        /**
+            @Override
-         * @return  A predicate that will match all the indices that this permission
+            public boolean isEmpty() {
-         *          has the given privilege for.
+                return groups == null || groups.length == 0;
-         */
-        public Predicate<String> allowedIndicesMatcher(Privilege.Index privilege) {
-            return allowedIndicesMatchersForPrivilege.getUnchecked(privilege);
             }
 
             /**
@@ -210,49 +279,72 @@ public interface Permission {
             public Predicate<String> allowedIndicesMatcher(String action) {
                 return allowedIndicesMatchersForAction.getUnchecked(action);
             }
-
-        @Override @SuppressWarnings("unchecked")
-        public boolean check(User user, String action, TransportRequest request, MetaData metaData) {
-
-            // some APIs are indices requests that are not actually associated with indices. For example,
-            // search scroll request, is categorized under the indices context, but doesn't hold indices names
-            // (in this case, the security check on the indices was done on the search request that initialized
-            // the scroll... and we rely on the signed scroll id to provide security over this request).
-            //
-            // so we only check indices if indeed the request is an actual IndicesRequest, if it's not, we only
-            // perform the check on the action name.
-            Set<String> indices = null;
-            if (request instanceof IndicesRequest || request instanceof CompositeIndicesRequest) {
-                indices = Collections.emptySet();
-                for (IndicesResolver resolver : indicesResolvers) {
-                    if (resolver.requestType().isInstance(request)) {
-                        indices = resolver.resolve(user, action, request, metaData);
-                        break;
-                    }
-                }
         }
 
-            if (indices == null) {
+        public static class Globals implements Indices {
+
+            private final ImmutableList<Global> globals;
+
+            public Globals(ImmutableList<Global> globals) {
+                this.globals = globals;
+            }
+
+            @Override
+            public Iterator<Group> iterator() {
+                return globals == null || globals.isEmpty() ?
+                        Collections.<Group>emptyIterator() :
+                        new Iter(globals);
+            }
+
+            @Override
+            public boolean isEmpty() {
+                if (globals == null || globals.isEmpty()) {
                     return true;
                 }
-
+                for (Global global : globals) {
-            // for every index, at least one group should match it... otherwise denied
+                    if (!global.indices().isEmpty()) {
-            for (String index : indices) {
-                boolean grant = false;
-                for (Group group : groups) {
-                    if (group.check(action, index)) {
-                        grant = true;
-                        break;
-                    }
-                }
-                if (!grant) {
                         return false;
                     }
                 }
-
                 return true;
             }
 
+            static class Iter extends UnmodifiableIterator<Group> {
+
+                private final Iterator<Global> globals;
+                private Iterator<Group> current;
+
+                Iter(ImmutableList<Global> globals) {
+                    this.globals = globals.iterator();
+                    advance();
+                }
+
+                @Override
+                public boolean hasNext() {
+                    return current != null && current.hasNext();
+                }
+
+                @Override
+                public Group next() {
+                    Group group = current.next();
+                    advance();
+                    return group;
+                }
+
+                private void advance() {
+                    if (current != null && current.hasNext()) {
+                        return;
+                    }
+                    if (!globals.hasNext()) {
+                        // we've reached the end of the globals array
+                        current = null;
+                        return;
+                    }
+                    current = globals.next().indices().iterator();
+                }
+            }
+        }
+
         public static class Group {
 
             private final Privilege.Index privilege;

src/main/java/org/elasticsearch/shield/authz/SystemRole.java

@@ -5,15 +5,12 @@
  */
 package org.elasticsearch.shield.authz;
 
-import org.elasticsearch.cluster.metadata.MetaData;
 import org.elasticsearch.common.base.Predicate;
-import org.elasticsearch.shield.User;
-import org.elasticsearch.transport.TransportRequest;
 
 /**
  *
  */
-public class SystemRole extends Permission.Global {
+public class SystemRole {
 
     public static final SystemRole INSTANCE = new SystemRole();
 
@@ -26,9 +23,4 @@ public class SystemRole extends Permission.Global {
     public boolean check(String action) {
         return PREDICATE.apply(action);
     }
-
-    @Override
-    public boolean check(User user, String action, TransportRequest request, MetaData metaData) {
-        return PREDICATE.apply(action);
-    }
 }

src/main/java/org/elasticsearch/shield/authz/store/FileRolesStore.java

@@ -50,7 +50,7 @@ public class FileRolesStore extends AbstractComponent implements RolesStore {
     private final Path file;
     private final Listener listener;
 
-    private volatile ImmutableMap<String, Permission.Global> permissions;
+    private volatile ImmutableMap<String, Permission.Global.Role> permissions;
 
     @Inject
     public FileRolesStore(Settings settings, Environment env, ResourceWatcherService watcherService, AuthorizationService authzService) {
@@ -68,7 +68,7 @@ public class FileRolesStore extends AbstractComponent implements RolesStore {
     }
 
     @Override
-    public Permission.Global permission(String role) {
+    public Permission.Global.Role role(String role) {
         return permissions.get(role);
     }
 
@@ -81,7 +81,7 @@ public class FileRolesStore extends AbstractComponent implements RolesStore {
         return Paths.get(location);
     }
 
-    public static ImmutableMap<String, Permission.Global> parseFile(Path path, ESLogger logger, AuthorizationService authzService) {
+    public static ImmutableMap<String, Permission.Global.Role> parseFile(Path path, ESLogger logger, AuthorizationService authzService) {
         if (logger != null) {
             logger.trace("Reading roles file located at [{}]", path);
         }
@@ -90,7 +90,7 @@ public class FileRolesStore extends AbstractComponent implements RolesStore {
             return ImmutableMap.of();
         }
 
-        ImmutableMap.Builder<String, Permission.Global> roles = ImmutableMap.builder();
+        ImmutableMap.Builder<String, Permission.Global.Role> roles = ImmutableMap.builder();
         try (InputStream input = Files.newInputStream(path, StandardOpenOption.READ)) {
             XContentParser parser = YamlXContent.yamlXContent.createParser(input);
             XContentParser.Token token;
@@ -100,7 +100,7 @@ public class FileRolesStore extends AbstractComponent implements RolesStore {
                     currentFieldName = parser.currentName();
                 } else if (token == XContentParser.Token.START_OBJECT && currentFieldName != null) {
                     String roleName = currentFieldName;
-                    Permission.Global.Builder permission = Permission.Global.builder(authzService);
+                    Permission.Global.Role.Builder permission = Permission.Global.Role.builder(roleName);
                     while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
                         if (token == XContentParser.Token.FIELD_NAME) {
                             currentFieldName = parser.currentName();
@@ -174,17 +174,17 @@ public class FileRolesStore extends AbstractComponent implements RolesStore {
         }
     }
 
-    public static void writeFile(Map<String, Permission.Global> roles, Path path) {
+    public static void writeFile(Map<String, Permission.Global.Role> roles, Path path) {
         try (OutputStream output = Files.newOutputStream(path, StandardOpenOption.CREATE, StandardOpenOption.TRUNCATE_EXISTING, StandardOpenOption.WRITE)) {
             XContentBuilder builder = XContentFactory.yamlBuilder(output);
-            for (Map.Entry<String, Permission.Global> entry : roles.entrySet()) {
+            for (Map.Entry<String, Permission.Global.Role> entry : roles.entrySet()) {
                 builder.startObject(entry.getKey());
-                Permission.Global permission = entry.getValue();
+                Permission.Global.Role permission = entry.getValue();
-                Permission.Cluster cluster = permission.cluster();
+                Permission.Cluster.Core cluster = permission.cluster();
                 if (cluster != null && cluster.privilege() != Privilege.Cluster.NONE) {
                     builder.field("cluster", cluster.privilege().name());
                 }
-                Permission.Indices indices = permission.indices();
+                Permission.Indices.Core indices = permission.indices();
                 if (indices != null) {
                     Permission.Global.Indices.Group[] groups = indices.groups();
                     if (groups != null && groups.length > 0) {

src/main/java/org/elasticsearch/shield/authz/store/RolesStore.java

@@ -6,26 +6,12 @@
 package org.elasticsearch.shield.authz.store;
 
 import org.elasticsearch.shield.authz.Permission;
-import org.elasticsearch.shield.authz.Privilege;
 
 /**
  *
  */
 public interface RolesStore {
 
-    Permission.Global permission(String role);
+    Permission.Global.Role role(String role);
-
-    static interface Writable extends RolesStore {
-
-        void set(String role, Privilege.Index privilege, String... indices);
-
-        void grant(String role, Privilege.Index privilege, String... indices);
-
-        void revoke(String role, Privilege.Index privileges, String... indices);
-
-        void grant(String role, Privilege.Cluster privilege);
-
-        void revoke(String role, Privilege.Cluster privileges);
-    }
 
 }

src/test/java/org/elasticsearch/shield/MultipleIndicesPermissionsTests.java

@@ -9,6 +9,8 @@ import org.elasticsearch.action.index.IndexResponse;
 import org.elasticsearch.action.search.MultiSearchResponse;
 import org.elasticsearch.action.search.SearchResponse;
 import org.elasticsearch.client.Client;
+import org.elasticsearch.shield.authc.support.SecuredStringTests;
+import org.elasticsearch.shield.authc.support.UsernamePasswordToken;
 import org.elasticsearch.shield.authz.AuthorizationException;
 import org.elasticsearch.shield.test.ShieldIntegrationTest;
 import org.junit.Test;
@@ -16,30 +18,60 @@ import org.junit.Test;
 import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder;
 import static org.elasticsearch.index.query.QueryBuilders.indicesQuery;
 import static org.elasticsearch.index.query.QueryBuilders.matchAllQuery;
+import static org.elasticsearch.shield.authc.support.UsernamePasswordToken.BASIC_AUTH_HEADER;
 import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount;
 import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertNoFailures;
-import static org.hamcrest.Matchers.*;
+import static org.hamcrest.Matchers.is;
 
 /**
  *
  */
 public class MultipleIndicesPermissionsTests extends ShieldIntegrationTest {
 
-    public static final String ROLES = "user:\n" +
+    public static final String ROLES =
+            DEFAULT_ROLE + ":\n" +
             "  cluster: all\n" +
             "  indices:\n" +
             "    '*': manage\n" +
             "    '/.*/': write\n" +
             "    'test': read\n" +
-            "    'test1': read\n";
+            "    'test1': read\n" +
+            "\n" +
+            "role_a:\n" +
+            "  indices:\n" +
+            "    'a': all\n" +
+            "\n" +
+            "role_b:\n" +
+            "  indices:\n" +
+            "    'b': all\n";
+
+    public static final String USERS =
+            CONFIG_STANDARD_USER +
+            "user_a:{plain}passwd\n" +
+            "user_b:{plain}passwd\n";
+
+    public static final String USERS_ROLES =
+            CONFIG_STANDARD_USER_ROLES +
+            "role_a:user_a,user_b\n" +
+            "role_b:user_b\n";
 
     @Override
     protected String configRole() {
         return ROLES;
     }
 
+    @Override
+    protected String configUsers() {
+        return USERS;
+    }
+
+    @Override
+    protected String configUsersRoles() {
+        return USERS_ROLES;
+    }
+
     @Test
-    public void testDifferetCombinationsOfIndices() throws Exception {
+    public void testSingleRole() throws Exception {
         IndexResponse indexResponse = index("test", "type", jsonBuilder()
                 .startObject()
                 .field("name", "value")
@@ -111,4 +143,78 @@ public class MultipleIndicesPermissionsTests extends ShieldIntegrationTest {
         assertNoFailures(searchResponse);
         assertHitCount(searchResponse, 1);
     }
+
+    @Test
+    public void testMultipleRoles() throws Exception {
+
+        IndexResponse indexResponse = index("a", "type", jsonBuilder()
+                .startObject()
+                .field("name", "value_a")
+                .endObject());
+        assertThat(indexResponse.isCreated(), is(true));
+
+
+        indexResponse = index("b", "type", jsonBuilder()
+                .startObject()
+                .field("name", "value_b")
+                .endObject());
+        assertThat(indexResponse.isCreated(), is(true));
+
+        refresh();
+
+        Client client = internalCluster().transportClient();
+
+        SearchResponse response = client.prepareSearch("a")
+                .putHeader(BASIC_AUTH_HEADER, userHeader("user_a", "passwd"))
+                .get();
+        assertNoFailures(response);
+        assertHitCount(response, 1);
+
+        String[] indices = randomDouble() < 0.3 ?
+                new String[] { "_all"} : randomBoolean() ?
+                new String[] { "*" } :
+                new String[] {};
+        response = client.prepareSearch(indices)
+                .putHeader(BASIC_AUTH_HEADER, userHeader("user_a", "passwd"))
+                .get();
+        assertNoFailures(response);
+        assertHitCount(response, 1);
+
+        try {
+            indices = randomBoolean() ? new String[] { "a", "b" } : new String[] { "b", "a" };
+            client.prepareSearch(indices)
+                    .putHeader(BASIC_AUTH_HEADER, userHeader("user_a", "passwd"))
+                    .get();
+            fail("expected an authorization excpetion when trying to search on multiple indices where there are no search permissions on one/some of them");
+        } catch (AuthorizationException ae) {
+            //expected
+        }
+
+        response = client.prepareSearch("b")
+                .putHeader(BASIC_AUTH_HEADER, userHeader("user_b", "passwd"))
+                .get();
+        assertNoFailures(response);
+        assertHitCount(response, 1);
+
+        indices = randomBoolean() ? new String[] { "a", "b" } : new String[] { "b", "a" };
+        response = client.prepareSearch(indices)
+                .putHeader(BASIC_AUTH_HEADER, userHeader("user_b", "passwd"))
+                .get();
+        assertNoFailures(response);
+        assertHitCount(response, 2);
+
+        indices = randomDouble() < 0.3 ?
+                new String[] { "_all"} : randomBoolean() ?
+                new String[] { "*" } :
+                new String[] {};
+        response = client.prepareSearch(indices)
+                .putHeader(BASIC_AUTH_HEADER, userHeader("user_b", "passwd"))
+                .get();
+        assertNoFailures(response);
+        assertHitCount(response, 2);
+    }
+
+    private static String userHeader(String username, String password) {
+        return UsernamePasswordToken.basicAuthHeaderValue(username, SecuredStringTests.build(password));
+    }
 }

src/test/java/org/elasticsearch/shield/authz/PermissionTests.java

@@ -13,36 +13,23 @@ import org.junit.Test;
 
 import static org.elasticsearch.shield.authz.Privilege.Index.*;
 import static org.hamcrest.Matchers.is;
-import static org.mockito.Mockito.mock;
 
 /**
  *
  */
 public class PermissionTests extends ElasticsearchTestCase {
 
-    private Permission.Global permission;
+    private Permission.Global.Role permission;
 
     @Before
     public void init() {
-        Permission.Global.Builder builder = Permission.Global.builder(mock(AuthorizationService.class));
+        Permission.Global.Role.Builder builder = Permission.Global.Role.builder("test");
         builder.add(union(SEARCH, MONITOR), "test_*", "/foo.*/");
         builder.add(union(READ), "baz_*foo", "/fool.*bar/");
         builder.add(union(MONITOR), "/bar.*/");
         permission = builder.build();
     }
 
-    @Test
-    public void testAllowedIndicesMatcher_Privilege() throws Exception {
-        testAllowedIndicesMatcher(permission.indices().allowedIndicesMatcher(GET));
-    }
-
-    @Test
-    public void testAllowedIndicesMatcher_Privilege_Caching() throws Exception {
-        Predicate<String> matcher1 = permission.indices().allowedIndicesMatcher(GET.plus(SEARCH).plus(WRITE));
-        Predicate<String> matcher2 = permission.indices().allowedIndicesMatcher(GET.plus(SEARCH).plus(WRITE));
-        assertThat(matcher1, is(matcher2));
-    }
-
     @Test
     public void testAllowedIndicesMatcher_Action() throws Exception {
         testAllowedIndicesMatcher(permission.indices().allowedIndicesMatcher(GetAction.NAME));

src/test/java/org/elasticsearch/shield/authz/PrivilegeTests.java

@@ -5,7 +5,6 @@
  */
 package org.elasticsearch.shield.authz;
 
-import com.carrotsearch.randomizedtesting.annotations.Repeat;
 import org.elasticsearch.ElasticsearchIllegalArgumentException;
 import org.elasticsearch.common.base.Predicate;
 import org.elasticsearch.shield.support.AutomatonPredicate;

src/test/java/org/elasticsearch/shield/authz/store/FileRolesStoreTests.java

@@ -7,7 +7,6 @@ package org.elasticsearch.shield.authz.store;
 
 import org.elasticsearch.ElasticsearchException;
 import org.elasticsearch.common.base.Charsets;
-import org.elasticsearch.common.base.Predicate;
 import org.elasticsearch.common.settings.ImmutableSettings;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.env.Environment;
@@ -45,19 +44,20 @@ public class FileRolesStoreTests extends ElasticsearchTestCase {
     @Test
     public void testParseFile() throws Exception {
         Path path = Paths.get(getClass().getResource("roles.yml").toURI());
-        Map<String, Permission.Global> roles = FileRolesStore.parseFile(path, logger, mock(AuthorizationService.class));
+        Map<String, Permission.Global.Role> roles = FileRolesStore.parseFile(path, logger, mock(AuthorizationService.class));
         assertThat(roles, notNullValue());
         assertThat(roles.size(), is(3));
 
-        Permission.Global permission = roles.get("role1");
+        Permission.Global.Role role = roles.get("role1");
-        assertThat(permission, notNullValue());
+        assertThat(role, notNullValue());
-        assertThat(permission.cluster(), notNullValue());
+        assertThat(role.name(), equalTo("role1"));
-        assertThat(permission.cluster().privilege(), is(Privilege.Cluster.ALL));
+        assertThat(role.cluster(), notNullValue());
-        assertThat(permission.indices(), notNullValue());
+        assertThat(role.cluster().privilege(), is(Privilege.Cluster.ALL));
-        assertThat(permission.indices().groups(), notNullValue());
+        assertThat(role.indices(), notNullValue());
-        assertThat(permission.indices().groups().length, is(2));
+        assertThat(role.indices().groups(), notNullValue());
+        assertThat(role.indices().groups().length, is(2));
 
-        Permission.Global.Indices.Group group = permission.indices().groups()[0];
+        Permission.Global.Indices.Group group = role.indices().groups()[0];
         assertThat(group.indices(), notNullValue());
         assertThat(group.indices().length, is(2));
         assertThat(group.indices()[0], equalTo("idx1"));
@@ -65,29 +65,31 @@ public class FileRolesStoreTests extends ElasticsearchTestCase {
         assertThat(group.privilege(), notNullValue());
         assertThat(group.privilege(), is(Privilege.Index.READ));
 
-        group = permission.indices().groups()[1];
+        group = role.indices().groups()[1];
         assertThat(group.indices(), notNullValue());
         assertThat(group.indices().length, is(1));
         assertThat(group.indices()[0], equalTo("idx3"));
         assertThat(group.privilege(), notNullValue());
         assertThat(group.privilege(), is(Privilege.Index.CRUD));
 
-        permission = roles.get("role2");
+        role = roles.get("role2");
-        assertThat(permission, notNullValue());
+        assertThat(role, notNullValue());
-        assertThat(permission.cluster(), notNullValue());
+        assertThat(role.name(), equalTo("role2"));
-        assertThat(permission.cluster().privilege(), is(Privilege.Cluster.ALL)); // MONITOR is collapsed into ALL
+        assertThat(role.cluster(), notNullValue());
-        assertThat(permission.indices(), notNullValue());
+        assertThat(role.cluster().privilege(), is(Privilege.Cluster.ALL)); // MONITOR is collapsed into ALL
-        assertThat(permission.indices(), is(Permission.Global.Indices.NONE));
+        assertThat(role.indices(), notNullValue());
+        assertThat(role.indices(), is(Permission.Indices.Core.NONE));
 
-        permission = roles.get("role3");
+        role = roles.get("role3");
-        assertThat(permission, notNullValue());
+        assertThat(role, notNullValue());
-        assertThat(permission.cluster(), notNullValue());
+        assertThat(role.name(), equalTo("role3"));
-        assertThat(permission.cluster(), is(Permission.Global.Cluster.NONE));
+        assertThat(role.cluster(), notNullValue());
-        assertThat(permission.indices(), notNullValue());
+        assertThat(role.cluster(), is(Permission.Cluster.Core.NONE));
-        assertThat(permission.indices().groups(), notNullValue());
+        assertThat(role.indices(), notNullValue());
-        assertThat(permission.indices().groups().length, is(1));
+        assertThat(role.indices().groups(), notNullValue());
+        assertThat(role.indices().groups().length, is(1));
 
-        group = permission.indices().groups()[0];
+        group = role.indices().groups()[0];
         assertThat(group.indices(), notNullValue());
         assertThat(group.indices().length, is(1));
         assertThat(group.indices()[0], equalTo("/.*_.*/"));
@@ -101,7 +103,7 @@ public class FileRolesStoreTests extends ElasticsearchTestCase {
     @Test
     public void testDefaultRolesFile() throws Exception {
         Path path = Paths.get(getClass().getResource("default_roles.yml").toURI());
-        Map<String, Permission.Global> roles = FileRolesStore.parseFile(path, logger, mock(AuthorizationService.class));
+        Map<String, Permission.Global.Role> roles = FileRolesStore.parseFile(path, logger, mock(AuthorizationService.class));
         assertThat(roles, notNullValue());
         assertThat(roles.size(), is(8));
 
@@ -140,10 +142,10 @@ public class FileRolesStoreTests extends ElasticsearchTestCase {
                 }
             });
 
-            Permission.Global permission = store.permission("role1");
+            Permission.Global.Role role = store.role("role1");
-            assertThat(permission, notNullValue());
+            assertThat(role, notNullValue());
-            permission = store.permission("role4");
+            role = store.role("role4");
-            assertThat(permission, nullValue());
+            assertThat(role, nullValue());
 
             watcherService.start();
 
@@ -159,10 +161,11 @@ public class FileRolesStoreTests extends ElasticsearchTestCase {
                 fail("Waited too long for the updated file to be picked up");
             }
 
-            permission = store.permission("role4");
+            role = store.role("role4");
-            assertThat(permission, notNullValue());
+            assertThat(role, notNullValue());
-            assertThat(permission.check(null, "cluster:monitor/foo/bar", null, null), is(true));
+            assertThat(role.name(), equalTo("role4"));
-            assertThat(permission.check(null, "cluster:admin/foo/bar", null, null), is(false));
+            assertThat(role.cluster().check("cluster:monitor/foo/bar"), is(true));
+            assertThat(role.cluster().check("cluster:admin/foo/bar"), is(false));
 
         } finally {
             if (watcherService != null) {
@@ -178,7 +181,7 @@ public class FileRolesStoreTests extends ElasticsearchTestCase {
     public void testThatEmptyFileDoesNotResultInLoop() throws Exception {
         File file = tempFolder.newFile();
         com.google.common.io.Files.write("#".getBytes(Charsets.UTF_8), file);
-        Map<String, Permission.Global> roles = FileRolesStore.parseFile(file.toPath(), logger, mock(AuthorizationService.class));
+        Map<String, Permission.Global.Role> roles = FileRolesStore.parseFile(file.toPath(), logger, mock(AuthorizationService.class));
         assertThat(roles.keySet(), is(empty()));
     }