From cb3c4e3a212255a9e9b8c89e784e0e452b661233 Mon Sep 17 00:00:00 2001 From: John Schulz Date: Mon, 12 Apr 2021 14:05:39 -0400 Subject: [PATCH] [Fleet] support force flag to add/remove package_policies (#96713) ## Summary Can now pass a `force=true` parameter to add & remove integrations on hosted policies as originally intended [1] & [2] * Add `force` param for `POST` `/api/fleet/package_policies` & `/api/fleet/package_policies/delete` to a policy. Update tests to confirm * Not strictly required, but "while I was in there" * Updated a few places to throw `IngestManagerError` vs `Error` for `400` response vs `500`. Updated tests. * removed a few unnecessary `await`s of sync function [1] https://github.com/elastic/kibana/issues/92426#issuecomment-785092670 [2] https://github.com/elastic/kibana/issues/90445 ### 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 Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> --- .../server/routes/package_policy/handlers.ts | 12 +- .../fleet/server/services/agent_policy.ts | 10 +- .../fleet/server/services/package_policy.ts | 14 +- .../server/types/models/package_policy.ts | 1 + .../server/types/rest_spec/package_policy.ts | 1 + .../apis/package_policy/create.ts | 422 +++++++++--------- .../apis/package_policy/delete.ts | 14 +- 7 files changed, 247 insertions(+), 227 deletions(-) diff --git a/x-pack/plugins/fleet/server/routes/package_policy/handlers.ts b/x-pack/plugins/fleet/server/routes/package_policy/handlers.ts index 5e8abd5966e3a..4427ba714ad6a 100644 --- a/x-pack/plugins/fleet/server/routes/package_policy/handlers.ts +++ b/x-pack/plugins/fleet/server/routes/package_policy/handlers.ts @@ -79,11 +79,12 @@ export const createPackagePolicyHandler: RequestHandler< > = async (context, request, response) => { const soClient = context.core.savedObjects.client; const esClient = context.core.elasticsearch.client.asCurrentUser; - const user = (await appContextService.getSecurity()?.authc.getCurrentUser(request)) || undefined; + const user = appContextService.getSecurity()?.authc.getCurrentUser(request) || undefined; + const { force, ...newPolicy } = request.body; try { const newData = await packagePolicyService.runExternalCallbacks( 'packagePolicyCreate', - { ...request.body }, + newPolicy, context, request ); @@ -91,6 +92,7 @@ export const createPackagePolicyHandler: RequestHandler< // Create package policy const packagePolicy = await packagePolicyService.create(soClient, esClient, newData, { user, + force, }); const body: CreatePackagePolicyResponse = { item: packagePolicy }; return response.ok({ @@ -114,7 +116,7 @@ export const updatePackagePolicyHandler: RequestHandler< > = async (context, request, response) => { const soClient = context.core.savedObjects.client; const esClient = context.core.elasticsearch.client.asCurrentUser; - const user = (await appContextService.getSecurity()?.authc.getCurrentUser(request)) || undefined; + const user = appContextService.getSecurity()?.authc.getCurrentUser(request) || undefined; const packagePolicy = await packagePolicyService.get(soClient, request.params.packagePolicyId); if (!packagePolicy) { @@ -155,13 +157,13 @@ export const deletePackagePolicyHandler: RequestHandler< > = async (context, request, response) => { const soClient = context.core.savedObjects.client; const esClient = context.core.elasticsearch.client.asCurrentUser; - const user = (await appContextService.getSecurity()?.authc.getCurrentUser(request)) || undefined; + const user = appContextService.getSecurity()?.authc.getCurrentUser(request) || undefined; try { const body: DeletePackagePoliciesResponse = await packagePolicyService.delete( soClient, esClient, request.body.packagePolicyIds, - { user } + { user, force: request.body.force } ); return response.ok({ body, diff --git a/x-pack/plugins/fleet/server/services/agent_policy.ts b/x-pack/plugins/fleet/server/services/agent_policy.ts index be61a70154b11..7f793a41ab985 100644 --- a/x-pack/plugins/fleet/server/services/agent_policy.ts +++ b/x-pack/plugins/fleet/server/services/agent_policy.ts @@ -466,7 +466,9 @@ class AgentPolicyService { esClient: ElasticsearchClient, id: string, packagePolicyIds: string[], - options: { user?: AuthenticatedUser; bumpRevision: boolean } = { bumpRevision: true } + options: { user?: AuthenticatedUser; bumpRevision: boolean; force?: boolean } = { + bumpRevision: true, + } ): Promise { const oldAgentPolicy = await this.get(soClient, id, false); @@ -474,7 +476,7 @@ class AgentPolicyService { throw new Error('Agent policy not found'); } - if (oldAgentPolicy.is_managed) { + if (oldAgentPolicy.is_managed && !options?.force) { throw new IngestManagerError(`Cannot update integrations of managed policy ${id}`); } @@ -497,7 +499,7 @@ class AgentPolicyService { esClient: ElasticsearchClient, id: string, packagePolicyIds: string[], - options?: { user?: AuthenticatedUser } + options?: { user?: AuthenticatedUser; force?: boolean } ): Promise { const oldAgentPolicy = await this.get(soClient, id, false); @@ -505,7 +507,7 @@ class AgentPolicyService { throw new Error('Agent policy not found'); } - if (oldAgentPolicy.is_managed) { + if (oldAgentPolicy.is_managed && !options?.force) { throw new IngestManagerError(`Cannot remove integrations of managed policy ${id}`); } diff --git a/x-pack/plugins/fleet/server/services/package_policy.ts b/x-pack/plugins/fleet/server/services/package_policy.ts index 210c9128b1ec7..7d12aad6f32b5 100644 --- a/x-pack/plugins/fleet/server/services/package_policy.ts +++ b/x-pack/plugins/fleet/server/services/package_policy.ts @@ -60,14 +60,14 @@ class PackagePolicyService { soClient: SavedObjectsClientContract, esClient: ElasticsearchClient, packagePolicy: NewPackagePolicy, - options?: { id?: string; user?: AuthenticatedUser; bumpRevision?: boolean } + options?: { id?: string; user?: AuthenticatedUser; bumpRevision?: boolean; force?: boolean } ): Promise { // Check that its agent policy does not have a package policy with the same name const parentAgentPolicy = await agentPolicyService.get(soClient, packagePolicy.policy_id); if (!parentAgentPolicy) { throw new Error('Agent policy not found'); } - if (parentAgentPolicy.is_managed) { + if (parentAgentPolicy.is_managed && !options?.force) { throw new IngestManagerError( `Cannot add integrations to managed policy ${parentAgentPolicy.id}` ); @@ -77,7 +77,9 @@ class PackagePolicyService { (siblingPackagePolicy) => siblingPackagePolicy.name === packagePolicy.name ) ) { - throw new Error('There is already a package with the same name on this agent policy'); + throw new IngestManagerError( + 'There is already a package with the same name on this agent policy' + ); } // Add ids to stream @@ -106,7 +108,7 @@ class PackagePolicyService { if (isPackageLimited(pkgInfo)) { const agentPolicy = await agentPolicyService.get(soClient, packagePolicy.policy_id, true); if (agentPolicy && doesAgentPolicyAlreadyIncludePackage(agentPolicy, pkgInfo.name)) { - throw new Error( + throw new IngestManagerError( `Unable to create package policy. Package '${pkgInfo.name}' already exists on this agent policy.` ); } @@ -140,6 +142,7 @@ class PackagePolicyService { { user: options?.user, bumpRevision: options?.bumpRevision ?? true, + force: options?.force, } ); @@ -367,7 +370,7 @@ class PackagePolicyService { soClient: SavedObjectsClientContract, esClient: ElasticsearchClient, ids: string[], - options?: { user?: AuthenticatedUser; skipUnassignFromAgentPolicies?: boolean } + options?: { user?: AuthenticatedUser; skipUnassignFromAgentPolicies?: boolean; force?: boolean } ): Promise { const result: DeletePackagePoliciesResponse = []; @@ -385,6 +388,7 @@ class PackagePolicyService { [packagePolicy.id], { user: options?.user, + force: options?.force, } ); } diff --git a/x-pack/plugins/fleet/server/types/models/package_policy.ts b/x-pack/plugins/fleet/server/types/models/package_policy.ts index 6248b375f8edb..1f39b3135cb3f 100644 --- a/x-pack/plugins/fleet/server/types/models/package_policy.ts +++ b/x-pack/plugins/fleet/server/types/models/package_policy.ts @@ -78,6 +78,7 @@ const PackagePolicyBaseSchema = { export const NewPackagePolicySchema = schema.object({ ...PackagePolicyBaseSchema, + force: schema.maybe(schema.boolean()), }); export const UpdatePackagePolicySchema = schema.object({ diff --git a/x-pack/plugins/fleet/server/types/rest_spec/package_policy.ts b/x-pack/plugins/fleet/server/types/rest_spec/package_policy.ts index 3c6f54177096e..6086d1f0e00fb 100644 --- a/x-pack/plugins/fleet/server/types/rest_spec/package_policy.ts +++ b/x-pack/plugins/fleet/server/types/rest_spec/package_policy.ts @@ -33,5 +33,6 @@ export const UpdatePackagePolicyRequestSchema = { export const DeletePackagePoliciesRequestSchema = { body: schema.object({ packagePolicyIds: schema.arrayOf(schema.string()), + force: schema.maybe(schema.boolean()), }), }; diff --git a/x-pack/test/fleet_api_integration/apis/package_policy/create.ts b/x-pack/test/fleet_api_integration/apis/package_policy/create.ts index 3764bdbc20d03..e2e1cc2f584bb 100644 --- a/x-pack/test/fleet_api_integration/apis/package_policy/create.ts +++ b/x-pack/test/fleet_api_integration/apis/package_policy/create.ts @@ -6,19 +6,18 @@ */ import expect from '@kbn/expect'; import { FtrProviderContext } from '../../../api_integration/ftr_provider_context'; -import { warnAndSkipTest } from '../../helpers'; +import { skipIfNoDockerRegistry } from '../../helpers'; -export default function ({ getService }: FtrProviderContext) { - const log = getService('log'); +export default function (providerContext: FtrProviderContext) { + const { getService } = providerContext; const supertest = getService('supertest'); - const dockerServers = getService('dockerServers'); - const server = dockerServers.get('registry'); // use function () {} and not () => {} here // because `this` has to point to the Mocha context // see https://mochajs.org/#arrow-functions describe('Package Policy - create', async function () { + skipIfNoDockerRegistry(providerContext); let agentPolicyId: string; before(async () => { await getService('esArchiver').load('empty_kibana'); @@ -47,230 +46,229 @@ export default function ({ getService }: FtrProviderContext) { .send({ agentPolicyId }); }); - it('should fail for managed agent policies', async function () { - if (server.enabled) { - // get a managed policy - const { - body: { item: managedPolicy }, - } = await supertest - .post(`/api/fleet/agent_policies`) - .set('kbn-xsrf', 'xxxx') - .send({ - name: `Managed policy from ${Date.now()}`, - namespace: 'default', - is_managed: true, - }); + it('can only add to managed agent policies using the force parameter', async function () { + // get a managed policy + const { + body: { item: managedPolicy }, + } = await supertest + .post(`/api/fleet/agent_policies`) + .set('kbn-xsrf', 'xxxx') + .send({ + name: `Managed policy from ${Date.now()}`, + namespace: 'default', + is_managed: true, + }); - // try to add an integration to the managed policy - const { body } = await supertest - .post(`/api/fleet/package_policies`) - .set('kbn-xsrf', 'xxxx') - .send({ - name: 'filetest-1', - description: '', - namespace: 'default', - policy_id: managedPolicy.id, - enabled: true, - output_id: '', - inputs: [], - package: { - name: 'filetest', - title: 'For File Tests', - version: '0.1.0', - }, - }) - .expect(400); + // try to add an integration to the managed policy + const { body: responseWithoutForce } = await supertest + .post(`/api/fleet/package_policies`) + .set('kbn-xsrf', 'xxxx') + .send({ + name: 'filetest-1', + description: '', + namespace: 'default', + policy_id: managedPolicy.id, + enabled: true, + output_id: '', + inputs: [], + package: { + name: 'filetest', + title: 'For File Tests', + version: '0.1.0', + }, + }) + .expect(400); - expect(body.statusCode).to.be(400); - expect(body.message).to.contain('Cannot add integrations to managed policy'); + expect(responseWithoutForce.statusCode).to.be(400); + expect(responseWithoutForce.message).to.contain('Cannot add integrations to managed policy'); - // delete policy we just made - await supertest.post(`/api/fleet/agent_policies/delete`).set('kbn-xsrf', 'xxxx').send({ - agentPolicyId: managedPolicy.id, - }); - } else { - warnAndSkipTest(this, log); - } + // try same request with `force: true` + const { body: responseWithForce } = await supertest + .post(`/api/fleet/package_policies`) + .set('kbn-xsrf', 'xxxx') + .send({ + force: true, + name: 'filetest-1', + description: '', + namespace: 'default', + policy_id: managedPolicy.id, + enabled: true, + output_id: '', + inputs: [], + package: { + name: 'filetest', + title: 'For File Tests', + version: '0.1.0', + }, + }) + .expect(200); + + expect(responseWithForce.item.name).to.eql('filetest-1'); + + // delete policy we just made + await supertest.post(`/api/fleet/agent_policies/delete`).set('kbn-xsrf', 'xxxx').send({ + agentPolicyId: managedPolicy.id, + }); }); it('should work with valid values', async function () { - if (server.enabled) { - await supertest - .post(`/api/fleet/package_policies`) - .set('kbn-xsrf', 'xxxx') - .send({ - name: 'filetest-1', - description: '', - namespace: 'default', - policy_id: agentPolicyId, - enabled: true, - output_id: '', - inputs: [], - package: { - name: 'filetest', - title: 'For File Tests', - version: '0.1.0', - }, - }) - .expect(200); - } else { - warnAndSkipTest(this, log); - } + await supertest + .post(`/api/fleet/package_policies`) + .set('kbn-xsrf', 'xxxx') + .send({ + name: 'filetest-1', + description: '', + namespace: 'default', + policy_id: agentPolicyId, + enabled: true, + output_id: '', + inputs: [], + package: { + name: 'filetest', + title: 'For File Tests', + version: '0.1.0', + }, + }) + .expect(200); }); it('should return a 400 with an empty namespace', async function () { - if (server.enabled) { - await supertest - .post(`/api/fleet/package_policies`) - .set('kbn-xsrf', 'xxxx') - .send({ - name: 'filetest-1', - description: '', - namespace: '', - policy_id: agentPolicyId, - enabled: true, - output_id: '', - inputs: [], - package: { - name: 'filetest', - title: 'For File Tests', - version: '0.1.0', - }, - }) - .expect(400); - } else { - warnAndSkipTest(this, log); - } + await supertest + .post(`/api/fleet/package_policies`) + .set('kbn-xsrf', 'xxxx') + .send({ + name: 'filetest-1', + description: '', + namespace: '', + policy_id: agentPolicyId, + enabled: true, + output_id: '', + inputs: [], + package: { + name: 'filetest', + title: 'For File Tests', + version: '0.1.0', + }, + }) + .expect(400); }); it('should return a 400 with an invalid namespace', async function () { - if (server.enabled) { - await supertest - .post(`/api/fleet/package_policies`) - .set('kbn-xsrf', 'xxxx') - .send({ - name: 'filetest-1', - description: '', - namespace: 'InvalidNamespace', - policy_id: agentPolicyId, - enabled: true, - output_id: '', - inputs: [], - package: { - name: 'filetest', - title: 'For File Tests', - version: '0.1.0', - }, - }) - .expect(400); - await supertest - .post(`/api/fleet/package_policies`) - .set('kbn-xsrf', 'xxxx') - .send({ - name: 'filetest-1', - description: '', - namespace: - 'testlengthπŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€', - policy_id: agentPolicyId, - enabled: true, - output_id: '', - inputs: [], - package: { - name: 'filetest', - title: 'For File Tests', - version: '0.1.0', - }, - }) - .expect(400); - } else { - warnAndSkipTest(this, log); - } + await supertest + .post(`/api/fleet/package_policies`) + .set('kbn-xsrf', 'xxxx') + .send({ + name: 'filetest-1', + description: '', + namespace: 'InvalidNamespace', + policy_id: agentPolicyId, + enabled: true, + output_id: '', + inputs: [], + package: { + name: 'filetest', + title: 'For File Tests', + version: '0.1.0', + }, + }) + .expect(400); + await supertest + .post(`/api/fleet/package_policies`) + .set('kbn-xsrf', 'xxxx') + .send({ + name: 'filetest-1', + description: '', + namespace: + 'testlengthπŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€πŸ˜€', + policy_id: agentPolicyId, + enabled: true, + output_id: '', + inputs: [], + package: { + name: 'filetest', + title: 'For File Tests', + version: '0.1.0', + }, + }) + .expect(400); }); it('should not allow multiple limited packages on the same agent policy', async function () { - if (server.enabled) { - await supertest - .post(`/api/fleet/package_policies`) - .set('kbn-xsrf', 'xxxx') - .send({ - name: 'endpoint-1', - description: '', - namespace: 'default', - policy_id: agentPolicyId, - enabled: true, - output_id: '', - inputs: [], - package: { - name: 'endpoint', - title: 'Endpoint', - version: '0.13.0', - }, - }) - .expect(200); - await supertest - .post(`/api/fleet/package_policies`) - .set('kbn-xsrf', 'xxxx') - .send({ - name: 'endpoint-2', - description: '', - namespace: 'default', - policy_id: agentPolicyId, - enabled: true, - output_id: '', - inputs: [], - package: { - name: 'endpoint', - title: 'Endpoint', - version: '0.13.0', - }, - }) - .expect(500); - } else { - warnAndSkipTest(this, log); - } + await supertest + .post(`/api/fleet/package_policies`) + .set('kbn-xsrf', 'xxxx') + .send({ + name: 'endpoint-1', + description: '', + namespace: 'default', + policy_id: agentPolicyId, + enabled: true, + output_id: '', + inputs: [], + package: { + name: 'endpoint', + title: 'Endpoint', + version: '0.13.0', + }, + }) + .expect(200); + await supertest + .post(`/api/fleet/package_policies`) + .set('kbn-xsrf', 'xxxx') + .send({ + name: 'endpoint-2', + description: '', + namespace: 'default', + policy_id: agentPolicyId, + enabled: true, + output_id: '', + inputs: [], + package: { + name: 'endpoint', + title: 'Endpoint', + version: '0.13.0', + }, + }) + .expect(400); }); - it('should return a 500 if there is another package policy with the same name', async function () { - if (server.enabled) { - await supertest - .post(`/api/fleet/package_policies`) - .set('kbn-xsrf', 'xxxx') - .send({ - name: 'same-name-test-1', - description: '', - namespace: 'default', - policy_id: agentPolicyId, - enabled: true, - output_id: '', - inputs: [], - package: { - name: 'filetest', - title: 'For File Tests', - version: '0.1.0', - }, - }) - .expect(200); - await supertest - .post(`/api/fleet/package_policies`) - .set('kbn-xsrf', 'xxxx') - .send({ - name: 'same-name-test-1', - description: '', - namespace: 'default', - policy_id: agentPolicyId, - enabled: true, - output_id: '', - inputs: [], - package: { - name: 'filetest', - title: 'For File Tests', - version: '0.1.0', - }, - }) - .expect(500); - } else { - warnAndSkipTest(this, log); - } + it('should return a 400 if there is another package policy with the same name', async function () { + await supertest + .post(`/api/fleet/package_policies`) + .set('kbn-xsrf', 'xxxx') + .send({ + name: 'same-name-test-1', + description: '', + namespace: 'default', + policy_id: agentPolicyId, + enabled: true, + output_id: '', + inputs: [], + package: { + name: 'filetest', + title: 'For File Tests', + version: '0.1.0', + }, + }) + .expect(200); + await supertest + .post(`/api/fleet/package_policies`) + .set('kbn-xsrf', 'xxxx') + .send({ + name: 'same-name-test-1', + description: '', + namespace: 'default', + policy_id: agentPolicyId, + enabled: true, + output_id: '', + inputs: [], + package: { + name: 'filetest', + title: 'For File Tests', + version: '0.1.0', + }, + }) + .expect(400); }); }); } diff --git a/x-pack/test/fleet_api_integration/apis/package_policy/delete.ts b/x-pack/test/fleet_api_integration/apis/package_policy/delete.ts index 85e6f5ab92b74..15aba758c85d0 100644 --- a/x-pack/test/fleet_api_integration/apis/package_policy/delete.ts +++ b/x-pack/test/fleet_api_integration/apis/package_policy/delete.ts @@ -80,7 +80,7 @@ export default function (providerContext: FtrProviderContext) { await supertest .post(`/api/fleet/package_policies/delete`) .set('kbn-xsrf', 'xxxx') - .send({ packagePolicyIds: [packagePolicy.id] }); + .send({ force: true, packagePolicyIds: [packagePolicy.id] }); }); after(async () => { await getService('esArchiver').unload('empty_kibana'); @@ -112,6 +112,18 @@ export default function (providerContext: FtrProviderContext) { expect(results[0].success).to.be(false); expect(results[0].body.message).to.contain('Cannot remove integrations of managed policy'); + // same, but with force + const { body: resultsWithForce } = await supertest + .post(`/api/fleet/package_policies/delete`) + .set('kbn-xsrf', 'xxxx') + .send({ force: true, packagePolicyIds: [packagePolicy.id] }) + .expect(200); + + // delete always succeeds (returns 200) with Array<{success: boolean}> + expect(Array.isArray(resultsWithForce)); + expect(resultsWithForce.length).to.be(1); + expect(resultsWithForce[0].success).to.be(true); + // revert existing policy to unmanaged await supertest .put(`/api/fleet/agent_policies/${agentPolicy.id}`)