Deleting no longer used privileges

x-pack/plugins/security/server/lib/authorization/register_privileges_with_cluster.js

@@ -51,12 +51,12 @@ export async function registerPrivilegesWithCluster(server) {
     });
   };
 
-  const shouldRemovePrivileges = (existingPrivileges, expectedPrivileges) => {
+  const getPrivilegesToDelete = (existingPrivileges, expectedPrivileges) => {
     if (isEmpty(existingPrivileges)) {
-      return false;
+      return [];
     }
 
-    return difference(Object.keys(existingPrivileges[application]), Object.keys(expectedPrivileges[application])).length > 0;
+    return difference(Object.keys(existingPrivileges[application]), Object.keys(expectedPrivileges[application]));
   };
 
   const privilegeMap = buildPrivilegeMap(savedObjectTypes, actions);
@@ -75,18 +75,24 @@ export async function registerPrivilegesWithCluster(server) {
       return;
     }
 
-    // The ES privileges POST endpoint only allows us to add new privileges, or update specified privileges; it doesn't
-    // remove unspecified privileges. We don't currently have a need to remove privileges, as this would be a
-    // backwards compatibility issue, and we'd have to figure out how to migrate roles, so we're throwing an Error if we
-    // unintentionally get ourselves in this position.
-    if (shouldRemovePrivileges(existingPrivileges, expectedPrivileges)) {
-      throw new Error(`Privileges are missing and can't be removed, currently.`);
+    const privilegesToDelete = getPrivilegesToDelete(existingPrivileges, expectedPrivileges);
+    for (const privilegeToDelete of privilegesToDelete) {
+      server.log(['security', 'debug'], `Deleting Kibana Privilege ${privilegeToDelete} from Elasticearch for ${application}`);
+      try {
+        await callCluster('shield.deletePrivilege', {
+          application,
+          privilege: privilegeToDelete
+        });
+      } catch (err) {
+        server.log(['security', 'error'], `Error deleting Kibana Privilege ${privilegeToDelete}`);
+        throw err;
+      }
     }
 
-    server.log(['security', 'debug'], `Updated Kibana Privileges with Elasticearch for ${application}`);
     await callCluster('shield.postPrivileges', {
       body: expectedPrivileges
     });
+    server.log(['security', 'debug'], `Updated Kibana Privileges with Elasticearch for ${application}`);
   } catch (err) {
     server.log(['security', 'error'], `Error registering Kibana Privileges with Elasticsearch for ${application}: ${err.message}`);
     throw err;

x-pack/plugins/security/server/lib/authorization/register_privileges_with_cluster.test.js

@@ -21,6 +21,8 @@ const registerPrivilegesWithClusterTest = (description, {
   savedObjectTypes,
   privilegeMap,
   existingPrivileges,
+  throwErrorWhenDeletingPrivileges,
+  errorDeletingPrivilegeName,
   throwErrorWhenGettingPrivileges,
   throwErrorWhenPuttingPrivileges,
   assert
@@ -67,9 +69,9 @@ const registerPrivilegesWithClusterTest = (description, {
   };
 
   const createExpectUpdatedPrivileges = (mockServer, mockCallWithInternalUser, error) => {
-    return (postPrivilegesBody) => {
+    return (postPrivilegesBody, deletedPrivileges = []) => {
       expect(error).toBeUndefined();
-      expect(mockCallWithInternalUser).toHaveBeenCalledTimes(2);
+      expect(mockCallWithInternalUser).toHaveBeenCalledTimes(2 + deletedPrivileges.length);
       expect(mockCallWithInternalUser).toHaveBeenCalledWith('shield.getPrivilege', {
         privilege: application,
       });
@@ -79,7 +81,19 @@ const registerPrivilegesWithClusterTest = (description, {
           body: postPrivilegesBody,
         }
       );
-
+      for (const deletedPrivilege of deletedPrivileges) {
+        expect(mockServer.log).toHaveBeenCalledWith(
+          ['security', 'debug'],
+          `Deleting Kibana Privilege ${deletedPrivilege} from Elasticearch for ${application}`
+        );
+        expect(mockCallWithInternalUser).toHaveBeenCalledWith(
+          'shield.deletePrivilege',
+          {
+            application,
+            privilege: deletedPrivilege
+          }
+        );
+      }
       expect(mockServer.log).toHaveBeenCalledWith(
         ['security', 'debug'],
         `Registering Kibana Privileges with Elasticsearch for ${application}`
@@ -116,6 +130,13 @@ const registerPrivilegesWithClusterTest = (description, {
       expect(actualError).toBeInstanceOf(Error);
       expect(actualError.message).toEqual(expectedErrorMessage);
 
+      if (throwErrorWhenDeletingPrivileges) {
+        expect(mockServer.log).toHaveBeenCalledWith(
+          ['security', 'error'],
+          `Error deleting Kibana Privilege ${errorDeletingPrivilegeName}`
+        );
+      }
+
       expect(mockServer.log).toHaveBeenCalledWith(
         ['security', 'error'],
         `Error registering Kibana Privileges with Elasticsearch for ${application}: ${expectedErrorMessage}`
@@ -126,21 +147,37 @@ const registerPrivilegesWithClusterTest = (description, {
   test(description, async () => {
     const mockServer = createMockServer();
     const mockCallWithInternalUser = registerMockCallWithInternalUser()
-      .mockImplementationOnce(async () => {
-        if (throwErrorWhenGettingPrivileges) {
-          throw throwErrorWhenGettingPrivileges;
-        }
+      .mockImplementation((api) => {
+        switch(api) {
+          case 'shield.getPrivilege': {
+            if (throwErrorWhenGettingPrivileges) {
+              throw throwErrorWhenGettingPrivileges;
+            }
 
-        // ES returns an empty object if we don't have any privileges
-        if (!existingPrivileges) {
-          return {};
-        }
+            // ES returns an empty object if we don't have any privileges
+            if (!existingPrivileges) {
+              return {};
+            }
 
-        return existingPrivileges;
-      })
-      .mockImplementationOnce(async () => {
-        if (throwErrorWhenPuttingPrivileges) {
-          throw throwErrorWhenPuttingPrivileges;
+            return existingPrivileges;
+          }
+          case 'shield.deletePrivilege': {
+            if (throwErrorWhenDeletingPrivileges) {
+              throw throwErrorWhenDeletingPrivileges;
+            }
+
+            break;
+          }
+          case 'shield.postPrivileges': {
+            if (throwErrorWhenPuttingPrivileges) {
+              throw throwErrorWhenPuttingPrivileges;
+            }
+
+            return;
+          }
+          default: {
+            expect(true).toBe(false);
+          }
         }
       });
 
@@ -208,7 +245,7 @@ registerPrivilegesWithClusterTest(`inserts privileges when we don't have any exi
   }
 });
 
-registerPrivilegesWithClusterTest(`throws error when we should be removing privilege`, {
+registerPrivilegesWithClusterTest(`deletes no-longer specified privileges`, {
   privilegeMap: {
     global: {
       foo: ['action:foo'],
@@ -236,11 +273,32 @@ registerPrivilegesWithClusterTest(`throws error when we should be removing privi
         name: 'space_bar',
         actions: ['action:not-bar'],
         metadata: {},
+      },
+      space_baz: {
+        application,
+        name: 'space_baz',
+        actions: ['action:not-baz'],
+        metadata: {},
       }
     }
   },
-  assert: ({ expectErrorThrown }) => {
-    expectErrorThrown(`Privileges are missing and can't be removed, currently.`);
+  assert: ({ expectUpdatedPrivileges }) => {
+    expectUpdatedPrivileges({
+      [application]: {
+        foo: {
+          application,
+          name: 'foo',
+          actions: ['action:foo'],
+          metadata: {},
+        },
+        space_bar: {
+          application,
+          name: 'space_bar',
+          actions: ['action:bar'],
+          metadata: {},
+        }
+      }
+    }, [ 'quz', 'space_baz' ]);
   }
 });
 
@@ -434,6 +492,28 @@ registerPrivilegesWithClusterTest(`throws and logs error when errors getting pri
   }
 });
 
+registerPrivilegesWithClusterTest(`throws and logs error when errors deleting privileges`, {
+  privilegeMap: {
+    global: {},
+    space: {}
+  },
+  existingPrivileges: {
+    [application]: {
+      foo: {
+        application,
+        name: 'foo',
+        actions: ['action:not-foo'],
+        metadata: {},
+      }
+    }
+  },
+  throwErrorWhenDeletingPrivileges: new Error('Error deleting privileges'),
+  errorDeletingPrivilegeName: 'foo',
+  assert: ({ expectErrorThrown }) => {
+    expectErrorThrown('Error deleting privileges');
+  }
+});
+
 registerPrivilegesWithClusterTest(`throws and logs error when errors putting privileges`, {
   privilegeMap: {
     global: {

x-pack/server/lib/esjs_shield_plugin.js

@@ -390,6 +390,23 @@
       }]
     });
 
+    shield.deletePrivilege = ca({
+      method: 'DELETE',
+      urls: [{
+        fmt: '/_xpack/security/privilege/<%=application%>/<%=privilege%>',
+        req: {
+          application: {
+            type: 'string',
+            required: true
+          },
+          privilege: {
+            type: 'string',
+            required: true
+          }
+        }
+      }]
+    });
+
     shield.postPrivileges = ca({
       method: 'POST',
       needBody: true,