[Fleet] Rename force to revoke agent unenroll APIs (#97041)

## Summary

 - fcbc9d9 Rename `force` param to `revoke` for `/agents/{agent_id}/unenroll` & `/agents/bulk_unenroll`
 - 03b9b90 Add new `force` param

See https://github.com/elastic/kibana/issues/96873 for background

<table>
  <thead>
    <tr>
      <td rowspan="2"></td><td colspan="2">Unenroll Agent</td><td rowspan="2">Revoke API Keys</td>
    </tr>
    <tr>
      <td>Regular</td><td>Hosted</td></td>
    </tr>
  </thead>
  <tr><td colspan="4"><strong>Rename <code>force</code> to <code>revoke</code></strong></td></tr>
  <tr><td>Current <code>force=false|undefined</code></td><td>✅</td><td>❌</td><td>❌</td></tr>
  <tr><td>Proposed <code>revoke=false|undefined</code></td><td>✅</td><td>❌</td><td>❌</td></tr>
  <tr><td>Current <code>force=true</code></td><td>✅</td><td>❌</td><td>✅</td></tr>
  <tr><td>Proposed <code>revoke=true</code></td><td>✅</td><td>❌</td><td>✅</td></tr>
  <tr><td colspan="4"><strong>Change <code>force</code> param </strong></td></tr>
  <tr><td>Proposed <code>force=false|undefined</code></td><td>✅</td><td>❌</td><td>❌</td></tr>
  <tr><td>Proposed <code>force=true</code></td><td>✅</td><td>✅</td><td>❌</td></tr>
  <tr><td>Proposed <code>force=true</code> & <code>revoke=true</code></td><td>✅</td><td>✅</td><td>✅</td></tr>
</table>

### Checklist

- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

### Changes required for consumers
Any call to `/agents/{agent_id}/unenroll` & `/agents/bulk_unenroll` which passes the `force` param should change to `revoke` to maintain the current behavior.

x-pack/plugins/fleet/server/routes/agent/unenroll_handler.ts

@@ -28,11 +28,10 @@ export const postAgentUnenrollHandler: RequestHandler<
   const soClient = context.core.savedObjects.client;
   const esClient = context.core.elasticsearch.client.asInternalUser;
   try {
-    if (request.body?.force === true) {
-      await AgentService.forceUnenrollAgent(soClient, esClient, request.params.agentId);
-    } else {
-      await AgentService.unenrollAgent(soClient, esClient, request.params.agentId);
-    }
+    await AgentService.unenrollAgent(soClient, esClient, request.params.agentId, {
+      force: request.body?.force,
+      revoke: request.body?.revoke,
+    });
 
     const body: PostAgentUnenrollResponse = {};
     return response.ok({ body });
@@ -62,6 +61,7 @@ export const postBulkAgentsUnenrollHandler: RequestHandler<
   try {
     const results = await AgentService.unenrollAgents(soClient, esClient, {
       ...agentOptions,
+      revoke: request.body?.revoke,
       force: request.body?.force,
     });
     const body = results.items.reduce<PostBulkAgentUnenrollResponse>((acc, so) => {

x-pack/plugins/fleet/server/services/agents/acks.ts

@@ -81,7 +81,7 @@ export async function acknowledgeAgentActions(
 
   const isAgentUnenrolled = actions.some((action) => action.type === 'UNENROLL');
   if (isAgentUnenrolled) {
-    await forceUnenrollAgent(soClient, esClient, agent.id);
+    await forceUnenrollAgent(soClient, esClient, agent);
   }
 
   const upgradeAction = actions.find((action) => action.type === 'UPGRADE');

x-pack/plugins/fleet/server/services/agents/unenroll.test.ts

@@ -46,7 +46,7 @@ describe('unenrollAgent (singular)', () => {
     expect(calledWith[0]?.body).toHaveProperty('doc.unenrollment_started_at');
   });
 
-  it('cannot unenroll from managed policy', async () => {
+  it('cannot unenroll from managed policy by default', async () => {
     const { soClient, esClient } = createClientMock();
     await expect(unenrollAgent(soClient, esClient, agentInManagedDoc._id)).rejects.toThrowError(
       AgentUnenrollmentError
@@ -54,6 +54,35 @@ describe('unenrollAgent (singular)', () => {
     // does not call ES update
     expect(esClient.update).toBeCalledTimes(0);
   });
+
+  it('cannot unenroll from managed policy with revoke=true', async () => {
+    const { soClient, esClient } = createClientMock();
+    await expect(
+      unenrollAgent(soClient, esClient, agentInManagedDoc._id, { revoke: true })
+    ).rejects.toThrowError(AgentUnenrollmentError);
+    // does not call ES update
+    expect(esClient.update).toBeCalledTimes(0);
+  });
+
+  it('can unenroll from managed policy with force=true', async () => {
+    const { soClient, esClient } = createClientMock();
+    await unenrollAgent(soClient, esClient, agentInManagedDoc._id, { force: true });
+    // calls ES update with correct values
+    expect(esClient.update).toBeCalledTimes(1);
+    const calledWith = esClient.update.mock.calls[0];
+    expect(calledWith[0]?.id).toBe(agentInManagedDoc._id);
+    expect(calledWith[0]?.body).toHaveProperty('doc.unenrollment_started_at');
+  });
+
+  it('can unenroll from managed policy with force=true and revoke=true', async () => {
+    const { soClient, esClient } = createClientMock();
+    await unenrollAgent(soClient, esClient, agentInManagedDoc._id, { force: true, revoke: true });
+    // calls ES update with correct values
+    expect(esClient.update).toBeCalledTimes(1);
+    const calledWith = esClient.update.mock.calls[0];
+    expect(calledWith[0]?.id).toBe(agentInManagedDoc._id);
+    expect(calledWith[0]?.body).toHaveProperty('doc.unenrolled_at');
+  });
 });
 
 describe('unenrollAgents (plural)', () => {
@@ -68,13 +97,12 @@ describe('unenrollAgents (plural)', () => {
       .filter((i: any) => i.update !== undefined)
       .map((i: any) => i.update._id);
     const docs = calledWith?.body.filter((i: any) => i.doc).map((i: any) => i.doc);
-    expect(ids).toHaveLength(2);
     expect(ids).toEqual(idsToUnenroll);
     for (const doc of docs) {
       expect(doc).toHaveProperty('unenrollment_started_at');
     }
   });
-  it('cannot unenroll from a managed policy', async () => {
+  it('cannot unenroll from a managed policy by default', async () => {
     const { soClient, esClient } = createClientMock();
 
     const idsToUnenroll = [
@@ -91,12 +119,116 @@ describe('unenrollAgents (plural)', () => {
       .filter((i: any) => i.update !== undefined)
       .map((i: any) => i.update._id);
     const docs = calledWith?.body.filter((i: any) => i.doc).map((i: any) => i.doc);
-    expect(ids).toHaveLength(onlyUnmanaged.length);
     expect(ids).toEqual(onlyUnmanaged);
     for (const doc of docs) {
       expect(doc).toHaveProperty('unenrollment_started_at');
     }
   });
+
+  it('cannot unenroll from a managed policy with revoke=true', async () => {
+    const { soClient, esClient } = createClientMock();
+
+    const idsToUnenroll = [
+      agentInUnmanagedDoc._id,
+      agentInManagedDoc._id,
+      agentInUnmanagedDoc2._id,
+    ];
+
+    const unenrolledResponse = await unenrollAgents(soClient, esClient, {
+      agentIds: idsToUnenroll,
+      revoke: true,
+    });
+
+    expect(unenrolledResponse.items).toMatchObject([
+      {
+        id: 'agent-in-unmanaged-policy',
+        success: true,
+      },
+      {
+        id: 'agent-in-managed-policy',
+        success: false,
+      },
+      {
+        id: 'agent-in-unmanaged-policy2',
+        success: true,
+      },
+    ]);
+
+    // calls ES update with correct values
+    const onlyUnmanaged = [agentInUnmanagedDoc._id, agentInUnmanagedDoc2._id];
+    const calledWith = esClient.bulk.mock.calls[0][0];
+    const ids = calledWith?.body
+      .filter((i: any) => i.update !== undefined)
+      .map((i: any) => i.update._id);
+    const docs = calledWith?.body.filter((i: any) => i.doc).map((i: any) => i.doc);
+    expect(ids).toEqual(onlyUnmanaged);
+    for (const doc of docs) {
+      expect(doc).toHaveProperty('unenrolled_at');
+    }
+  });
+
+  it('can unenroll from managed policy with force=true', async () => {
+    const { soClient, esClient } = createClientMock();
+    const idsToUnenroll = [
+      agentInUnmanagedDoc._id,
+      agentInManagedDoc._id,
+      agentInUnmanagedDoc2._id,
+    ];
+    await unenrollAgents(soClient, esClient, { agentIds: idsToUnenroll, force: true });
+
+    // calls ES update with correct values
+    const calledWith = esClient.bulk.mock.calls[1][0];
+    const ids = calledWith?.body
+      .filter((i: any) => i.update !== undefined)
+      .map((i: any) => i.update._id);
+    const docs = calledWith?.body.filter((i: any) => i.doc).map((i: any) => i.doc);
+    expect(ids).toEqual(idsToUnenroll);
+    for (const doc of docs) {
+      expect(doc).toHaveProperty('unenrollment_started_at');
+    }
+  });
+
+  it('can unenroll from managed policy with force=true and revoke=true', async () => {
+    const { soClient, esClient } = createClientMock();
+
+    const idsToUnenroll = [
+      agentInUnmanagedDoc._id,
+      agentInManagedDoc._id,
+      agentInUnmanagedDoc2._id,
+    ];
+
+    const unenrolledResponse = await unenrollAgents(soClient, esClient, {
+      agentIds: idsToUnenroll,
+      revoke: true,
+      force: true,
+    });
+
+    expect(unenrolledResponse.items).toMatchObject([
+      {
+        id: 'agent-in-unmanaged-policy',
+        success: true,
+      },
+      {
+        id: 'agent-in-managed-policy',
+        success: true,
+      },
+      {
+        id: 'agent-in-unmanaged-policy2',
+        success: true,
+      },
+    ]);
+
+    // calls ES update with correct values
+    const calledWith = esClient.bulk.mock.calls[0][0];
+    const ids = calledWith?.body
+      .filter((i: any) => i.update !== undefined)
+      .map((i: any) => i.update._id);
+    const docs = calledWith?.body.filter((i: any) => i.doc).map((i: any) => i.doc);
+    expect(ids).toEqual(idsToUnenroll);
+    for (const doc of docs) {
+      expect(doc).toHaveProperty('unenrolled_at');
+    }
+  });
 });
 
 function createClientMock() {

x-pack/plugins/fleet/server/services/agents/unenroll.ts

@@ -39,10 +39,18 @@ async function unenrollAgentIsAllowed(
 export async function unenrollAgent(
   soClient: SavedObjectsClientContract,
   esClient: ElasticsearchClient,
-  agentId: string
+  agentId: string,
+  options?: {
+    force?: boolean;
+    revoke?: boolean;
+  }
 ) {
-  await unenrollAgentIsAllowed(soClient, esClient, agentId);
-
+  if (!options?.force) {
+    await unenrollAgentIsAllowed(soClient, esClient, agentId);
+  }
+  if (options?.revoke) {
+    return forceUnenrollAgent(soClient, esClient, agentId);
+  }
   const now = new Date().toISOString();
   await createAgentAction(soClient, esClient, {
     agent_id: agentId,
@@ -57,15 +65,17 @@ export async function unenrollAgent(
 export async function unenrollAgents(
   soClient: SavedObjectsClientContract,
   esClient: ElasticsearchClient,
-  options: GetAgentsOptions & { force?: boolean }
+  options: GetAgentsOptions & {
+    force?: boolean;
+    revoke?: boolean;
+  }
 ): Promise<{ items: BulkActionResult[] }> {
   // start with all agents specified
   const givenAgents = await getAgents(esClient, options);
-  const outgoingErrors: Record<Agent['id'], Error> = {};
 
   // Filter to those not already unenrolled, or unenrolling
   const agentsEnrolled = givenAgents.filter((agent) => {
-    if (options.force) {
+    if (options.revoke) {
       return !agent.unenrolled_at;
     }
     return !agent.unenrollment_started_at && !agent.unenrolled_at;
@@ -76,34 +86,23 @@ export async function unenrollAgents(
       unenrollAgentIsAllowed(soClient, esClient, agent.id).then((_) => agent)
     )
   );
-  const agentsToUpdate = agentResults.reduce<Agent[]>((agents, result, index) => {
-    if (result.status === 'fulfilled') {
-      agents.push(result.value);
-    } else {
-      const id = givenAgents[index].id;
-      outgoingErrors[id] = result.reason;
-    }
-    return agents;
-  }, []);
+  const outgoingErrors: Record<Agent['id'], Error> = {};
+  const agentsToUpdate = options.force
+    ? agentsEnrolled
+    : agentResults.reduce<Agent[]>((agents, result, index) => {
+        if (result.status === 'fulfilled') {
+          agents.push(result.value);
+        } else {
+          const id = givenAgents[index].id;
+          outgoingErrors[id] = result.reason;
+        }
+        return agents;
+      }, []);
 
   const now = new Date().toISOString();
-  if (options.force) {
+  if (options.revoke) {
     // Get all API keys that need to be invalidated
-    const apiKeys = agentsToUpdate.reduce<string[]>((keys, agent) => {
-      if (agent.access_api_key_id) {
-        keys.push(agent.access_api_key_id);
-      }
-      if (agent.default_api_key_id) {
-        keys.push(agent.default_api_key_id);
-      }
-
-      return keys;
-    }, []);
-
-    // Invalidate all API keys
-    if (apiKeys.length) {
-      await APIKeyService.invalidateAPIKeys(apiKeys);
-    }
+    await invalidateAPIKeysForAgents(agentsToUpdate);
   } else {
     // Create unenroll action for each agent
     await bulkCreateAgentActions(
@@ -118,7 +117,7 @@ export async function unenrollAgents(
   }
 
   // Update the necessary agents
-  const updateData = options.force
+  const updateData = options.revoke
     ? { unenrolled_at: now, active: false }
     : { unenrollment_started_at: now };
 
@@ -127,39 +126,52 @@ export async function unenrollAgents(
     agentsToUpdate.map(({ id }) => ({ agentId: id, data: updateData }))
   );
 
-  const out = {
-    items: givenAgents.map((agent, index) => {
-      const hasError = agent.id in outgoingErrors;
-      const result: BulkActionResult = {
-        id: agent.id,
-        success: !hasError,
-      };
-      if (hasError) {
-        result.error = outgoingErrors[agent.id];
-      }
-      return result;
-    }),
+  const getResultForAgent = (agent: Agent) => {
+    const hasError = agent.id in outgoingErrors;
+    const result: BulkActionResult = {
+      id: agent.id,
+      success: !hasError,
+    };
+    if (hasError) {
+      result.error = outgoingErrors[agent.id];
+    }
+    return result;
   };
-  return out;
+
+  return {
+    items: givenAgents.map(getResultForAgent),
+  };
+}
+
+export async function invalidateAPIKeysForAgents(agents: Agent[]) {
+  const apiKeys = agents.reduce<string[]>((keys, agent) => {
+    if (agent.access_api_key_id) {
+      keys.push(agent.access_api_key_id);
+    }
+    if (agent.default_api_key_id) {
+      keys.push(agent.default_api_key_id);
+    }
+
+    return keys;
+  }, []);
+
+  if (apiKeys.length) {
+    await APIKeyService.invalidateAPIKeys(apiKeys);
+  }
 }
 
 export async function forceUnenrollAgent(
   soClient: SavedObjectsClientContract,
   esClient: ElasticsearchClient,
-  agentId: string
+  agentIdOrAgent: string | Agent
 ) {
-  const agent = await getAgentById(esClient, agentId);
+  const agent =
+    typeof agentIdOrAgent === 'string'
+      ? await getAgentById(esClient, agentIdOrAgent)
+      : agentIdOrAgent;
 
-  await Promise.all([
-    agent.access_api_key_id
-      ? APIKeyService.invalidateAPIKeys([agent.access_api_key_id])
-      : undefined,
-    agent.default_api_key_id
-      ? APIKeyService.invalidateAPIKeys([agent.default_api_key_id])
-      : undefined,
-  ]);
-
-  await updateAgent(esClient, agentId, {
+  await invalidateAPIKeysForAgents([agent]);
+  await updateAgent(esClient, agent.id, {
     active: false,
     unenrolled_at: new Date().toISOString(),
   });

x-pack/plugins/fleet/server/types/rest_spec/agent.ts

@@ -172,7 +172,8 @@ export const PostAgentUnenrollRequestSchema = {
   }),
   body: schema.nullable(
     schema.object({
-      force: schema.boolean(),
+      force: schema.maybe(schema.boolean()),
+      revoke: schema.maybe(schema.boolean()),
     })
   ),
 };
@@ -181,6 +182,7 @@ export const PostBulkAgentUnenrollRequestSchema = {
   body: schema.object({
     agents: schema.oneOf([schema.arrayOf(schema.string()), schema.string()]),
     force: schema.maybe(schema.boolean()),
+    revoke: schema.maybe(schema.boolean()),
   }),
 };
 

x-pack/test/fleet_api_integration/apis/agents/reassign.ts

@@ -181,7 +181,7 @@ export default function (providerContext: FtrProviderContext) {
           .post(`/api/fleet/agents/bulk_reassign`)
           .set('kbn-xsrf', 'xxx')
           .send({
-            agents: 'fleet-agents.active: true',
+            agents: 'active: true',
             policy_id: 'policy2',
           })
           .expect(200);

x-pack/test/fleet_api_integration/apis/agents/unenroll.ts

@@ -94,12 +94,12 @@ export default function (providerContext: FtrProviderContext) {
       await supertest.post(`/api/fleet/agents/agent1/unenroll`).set('kbn-xsrf', 'xxx').expect(200);
     });
 
-    it('/agents/{agent_id}/unenroll { force: true } should invalidate related API keys', async () => {
+    it('/agents/{agent_id}/unenroll { revoke: true } should invalidate related API keys', async () => {
       await supertest
         .post(`/api/fleet/agents/agent1/unenroll`)
         .set('kbn-xsrf', 'xxx')
         .send({
-          force: true,
+          revoke: true,
         })
         .expect(200);
 
@@ -116,7 +116,7 @@ export default function (providerContext: FtrProviderContext) {
       expect(outputAPIKeys[0].invalidated).eql(true);
     });
 
-    it('/agents/{agent_id}/bulk_unenroll should not allow unenroll from managed policy', async () => {
+    it('/agents/bulk_unenroll should not allow unenroll from managed policy', async () => {
       // set policy to managed
       await supertest
         .put(`/api/fleet/agent_policies/policy1`)
@@ -157,7 +157,7 @@ export default function (providerContext: FtrProviderContext) {
       expect(agent2data.body.item.active).to.eql(true);
     });
 
-    it('/agents/{agent_id}/bulk_unenroll should allow to unenroll multiple agents by id from an unmanaged policy', async () => {
+    it('/agents/bulk_unenroll should allow to unenroll multiple agents by id from an unmanaged policy', async () => {
       // set policy to unmanaged
       await supertest
         .put(`/api/fleet/agent_policies/policy1`)
@@ -187,13 +187,13 @@ export default function (providerContext: FtrProviderContext) {
       expect(agent2data.body.item.active).to.eql(true);
     });
 
-    it('/agents/{agent_id}/bulk_unenroll should allow to unenroll multiple agents by kuery', async () => {
+    it('/agents/bulk_unenroll should allow to unenroll multiple agents by kuery', async () => {
       await supertest
         .post(`/api/fleet/agents/bulk_unenroll`)
         .set('kbn-xsrf', 'xxx')
         .send({
-          agents: 'fleet-agents.active: true',
-          force: true,
+          agents: 'active: true',
+          revoke: true,
         })
         .expect(200);
 

x-pack/test/fleet_api_integration/apis/agents/upgrade.ts

@@ -167,7 +167,7 @@ export default function (providerContext: FtrProviderContext) {
       it('should respond 400 if trying to upgrade an agent that is unenrolling', async () => {
         const kibanaVersion = await kibanaServer.version.get();
         await supertest.post(`/api/fleet/agents/agent1/unenroll`).set('kbn-xsrf', 'xxx').send({
-          force: true,
+          revoke: true,
         });
         await supertest
           .post(`/api/fleet/agents/agent1/upgrade`)
@@ -331,7 +331,7 @@ export default function (providerContext: FtrProviderContext) {
       it('should not upgrade an unenrolling agent during bulk_upgrade', async () => {
         const kibanaVersion = await kibanaServer.version.get();
         await supertest.post(`/api/fleet/agents/agent1/unenroll`).set('kbn-xsrf', 'xxx').send({
-          force: true,
+          revoke: true,
         });
         await es.update({
           id: 'agent1',