Backport: Removing hidden/reserved roles added via rolesmapping

src/main/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/InternalUsersApiAction.java

@@ -115,18 +115,23 @@ protected void handlePut(RestChannel channel, final RestRequest request, final C
         final ObjectNode contentAsNode = (ObjectNode) content;
         final SecurityJsonNode securityJsonNode = new SecurityJsonNode(contentAsNode);
 
-        // Don't allow user to add hidden, reserved or non-existent role
+        // Don't allow user to add hidden, reserved or non-existent rolesmapping
         final List<String> opendistroSecurityRoles = securityJsonNode.get("opendistro_security_roles").asList();
         if (opendistroSecurityRoles != null) {
             final SecurityDynamicConfiguration<?> rolesConfiguration = load(CType.ROLES, false);
+            final SecurityDynamicConfiguration<?> rolesmappingConfiguration = load(CType.ROLESMAPPING, false);
             for (final String role: opendistroSecurityRoles) {
 
-                if (!rolesConfiguration.exists(role) || isHidden(rolesConfiguration, role)) {
-                    notFound(channel, "Role '"+role+"' is not found.");
+                if (rolesConfiguration.getCEntry(role) == null) {
+                    notFound(channel, "Role '"+role+"' is not available.");
+                    return;
+                }
+                if (isHidden(rolesmappingConfiguration, role)) {
+                    notFound(channel, "Role '"+role+"' is not available.");
                     return;
                 }
 
-                if (isReadOnly(rolesConfiguration, role)) {
+                if (isReadOnly(rolesmappingConfiguration, role)) {
                     forbidden(channel, "Role '" + role + "' is read-only.");
                     return;
                 }

src/main/java/com/amazon/opendistroforelasticsearch/security/securityconf/DynamicConfigFactory.java

@@ -199,7 +199,9 @@ public void onChange(Map<CType, SecurityDynamicConfiguration<?>> typeToConfig) {
 
             //rebuild v7 Models
             dcm = new DynamicConfigModelV7(getConfigV7(config), esSettings, configPath, iab);
-            ium = new InternalUsersModelV7((SecurityDynamicConfiguration<InternalUserV7>) internalusers, (SecurityDynamicConfiguration<RoleV7>) roles);
+            ium = new InternalUsersModelV7((SecurityDynamicConfiguration<InternalUserV7>) internalusers,
+                (SecurityDynamicConfiguration<RoleV7>) roles,
+                (SecurityDynamicConfiguration<RoleMappingsV7>) rolesmapping);
             cm = new ConfigModelV7((SecurityDynamicConfiguration<RoleV7>) roles,(SecurityDynamicConfiguration<RoleMappingsV7>)rolesmapping, (SecurityDynamicConfiguration<ActionGroupsV7>)actionGroups, (SecurityDynamicConfiguration<TenantV7>) tenants,dcm, esSettings);
 
         } else {
@@ -253,12 +255,17 @@ private static class InternalUsersModelV7 extends InternalUsersModel {
 
         private final SecurityDynamicConfiguration<InternalUserV7> internalUserV7SecurityDynamicConfiguration;
 
-        private final SecurityDynamicConfiguration<RoleV7> roleV7SecurityDynamicConfiguration;
+        private final SecurityDynamicConfiguration<RoleV7> rolesV7SecurityDynamicConfiguration;
+
+        private final SecurityDynamicConfiguration<RoleMappingsV7> rolesMappingsV7SecurityDynamicConfiguration;
 
-        public InternalUsersModelV7(SecurityDynamicConfiguration<InternalUserV7> internalUserV7SecurityDynamicConfiguration, SecurityDynamicConfiguration<RoleV7> roleV7SecurityDynamicConfiguration) {
+        public InternalUsersModelV7(SecurityDynamicConfiguration<InternalUserV7> internalUserV7SecurityDynamicConfiguration,
+                                    SecurityDynamicConfiguration<RoleV7> rolesV7SecurityDynamicConfiguration,
+                                    SecurityDynamicConfiguration<RoleMappingsV7> rolesMappingsV7SecurityDynamicConfiguration) {
             super();
             this.internalUserV7SecurityDynamicConfiguration = internalUserV7SecurityDynamicConfiguration;
-            this.roleV7SecurityDynamicConfiguration = roleV7SecurityDynamicConfiguration;
+            this.rolesV7SecurityDynamicConfiguration = rolesV7SecurityDynamicConfiguration;
+            this.rolesMappingsV7SecurityDynamicConfiguration = rolesMappingsV7SecurityDynamicConfiguration;
         }
 
         @Override
@@ -293,23 +300,23 @@ public String getHash(String user) {
         public List<String> getOpenDistroSecurityRoles(String user) {
             InternalUserV7 tmp = internalUserV7SecurityDynamicConfiguration.getCEntry(user);
 
-            // Filtering out hidden and non-existent roles for existing users
+            // Opendistro security roles should only contain roles that exist in the roles dynamic config.
+            // We should filter out any roles that have hidden rolesmapping.
             return tmp == null ? ImmutableList.of() :
-                tmp.getOpendistro_security_roles().stream().filter(role -> !isHiddenOrNonExistent(role)).collect(ImmutableList.toImmutableList());
+                tmp.getOpendistro_security_roles().stream().filter(role -> !isRolesMappingHidden(role) && rolesV7SecurityDynamicConfiguration.exists(role)).collect(ImmutableList.toImmutableList());
         }
 
-        // We will remove opendistro security mapping for roles that are hidden or non-existent in the roles configuration
-        private boolean isHiddenOrNonExistent(String rolename) {
-            final RoleV7 role = roleV7SecurityDynamicConfiguration.getCEntry(rolename);
-            return role == null || role.isHidden();
+        // Remove any hidden rolesmapping from the opendistro security roles
+        private boolean isRolesMappingHidden(String rolename) {
+            final RoleMappingsV7 roleMapping = rolesMappingsV7SecurityDynamicConfiguration.getCEntry(rolename);
+            return roleMapping!=null && roleMapping.isHidden();
         }
     }
 
     private static class InternalUsersModelV6 extends InternalUsersModel {
 
         SecurityDynamicConfiguration<InternalUserV6> configuration;
 
-
 
         public InternalUsersModelV6(SecurityDynamicConfiguration<InternalUserV6> configuration) {
             super();

src/test/java/com/amazon/opendistroforelasticsearch/security/OpendistroSecurityRolesTests.java

@@ -70,6 +70,7 @@ public void testOpenDistroSecurityRolesAnon() throws Exception {
 	public void testOpenDistroSecurityRoles() throws Exception {
 
 		setup(Settings.EMPTY, new DynamicSecurityConfig()
+				.setSecurityRolesMapping("roles_mapping.yml")
 				.setSecurityInternalUsers("internal_users_sr.yml"), Settings.EMPTY, true);
 
 		RestHelper rh = nonSslRestHelper();
@@ -79,11 +80,15 @@ public void testOpenDistroSecurityRoles() throws Exception {
 		Assert.assertTrue(resc.getBody().contains("sr_user"));
 		Assert.assertTrue(resc.getBody().contains("xyz_sr"));
 
-		// Ensure hidden reserved and non-existent roles are not available
+		// Opendistro_security_roles cannot contain roles that don't exist.
 		Assert.assertFalse(resc.getBody().contains("xyz_sr_non_existent"));
-		Assert.assertFalse(resc.getBody().contains("xyz_sr_hidden"));
+
+		// Opendistro_security_roles can contain reserved roles.
 		Assert.assertTrue(resc.getBody().contains("xyz_sr_reserved"));
 
+		// Opendistro_security_roles cannot contain roles that are hidden in rolesmapping.yml.
+		Assert.assertFalse(resc.getBody().contains("xyz_sr_hidden"));
+
 		Assert.assertTrue(resc.getBody().contains("backend_roles=[abc_ber]"));
 		Assert.assertEquals(HttpStatus.SC_OK, resc.getStatusCode());
 	}

src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/UserApiTest.java

@@ -123,7 +123,7 @@ public void testUserApi() throws Exception {
             new Header[0]);
         Assert.assertEquals(HttpStatus.SC_NOT_FOUND, response.getStatusCode());
         settings = Settings.builder().loadFromSource(response.getBody(), XContentType.JSON).build();
-        Assert.assertEquals(settings.get("message"), "Role 'opendistro_security_hidden' is not found.");
+        Assert.assertEquals(settings.get("message"), "Role 'opendistro_security_hidden' is not available.");
 
         // Associating with reserved role is allowed (for superamdin)
         response = rh.executePutRequest("/_opendistro/_security/api/internalusers/test", "{ \"opendistro_security_roles\": [\"opendistro_security_reserved\"], " +
@@ -136,7 +136,7 @@ public void testUserApi() throws Exception {
             new Header[0]);
         Assert.assertEquals(HttpStatus.SC_NOT_FOUND, response.getStatusCode());
         settings = Settings.builder().loadFromSource(response.getBody(), XContentType.JSON).build();
-        Assert.assertEquals(settings.get("message"), "Role 'non_existent' is not found.");
+        Assert.assertEquals(settings.get("message"), "Role 'non_existent' is not available.");
 
         // Wrong config keys
         response = rh.executePutRequest("/_opendistro/_security/api/internalusers/nagilum", "{\"some\": \"thing\", \"other\": \"thing\"}",

src/test/resources/restapi/roles_mapping.yml

@@ -11,6 +11,27 @@ opendistro_security_unittest_1:
   - "CN=spock,OU=client,O=client,L=Test,C=DE"
   and_backend_roles: []
   description: "Migrated from v6"
+
+opendistro_security_hidden:
+  reserved: false
+  hidden: true
+  backend_roles:
+  - "hidden*"
+  hosts: []
+  users: []
+  and_backend_roles: []
+  description: "Migrated from v6"
+
+opendistro_security_reserved:
+  reserved: true
+  hidden: false
+  backend_roles:
+  - "reserved*"
+  hosts: []
+  users: ["nagilum"]
+  and_backend_roles: []
+  description: "Migrated from v6"
+
 opendistro_security_role_starfleet_library:
   reserved: true
   hidden: false

src/test/resources/roles.yml

@@ -1049,9 +1049,10 @@ xyz_sr:
         - "*"
   tenant_permissions: []
 
+# This role is hidden in rolesmapping
 xyz_sr_hidden:
   reserved: false
-  hidden: true
+  hidden: false
   description: "Migrated from v6 (all types mapped)"
   cluster_permissions:
     - "OPENDISTRO_SECURITY_CLUSTER_COMPOSITE_OPS_RO"

src/test/resources/roles_mapping.yml

@@ -376,4 +376,12 @@ role_foo_index:
 role_foo_all:
   users:
     - foo_all
-
+xyz_sr_hidden:
+  reserved: false
+  hidden: true
+  backend_roles: []
+  hosts: []
+  users:
+    - "test_user"
+  and_backend_roles: []
+  description: "Migrated from v6"