From 77a4642b02b7616240a904a2239c835090c9520b Mon Sep 17 00:00:00 2001 From: criamico Date: Thu, 23 Mar 2023 12:49:09 +0100 Subject: [PATCH 1/2] [Fleet] Create and update package policy API return 409 conflict when names are not unique --- .../plugins/fleet/server/errors/handlers.ts | 4 ++ x-pack/plugins/fleet/server/errors/index.ts | 1 + .../fleet/server/services/package_policy.ts | 47 ++++++++++--------- .../apis/package_policy/create.ts | 8 ++-- .../apis/package_policy/update.ts | 8 ++-- 5 files changed, 38 insertions(+), 30 deletions(-) diff --git a/x-pack/plugins/fleet/server/errors/handlers.ts b/x-pack/plugins/fleet/server/errors/handlers.ts index 89811ca2bc7aad..6a86f9b9f2c619 100644 --- a/x-pack/plugins/fleet/server/errors/handlers.ts +++ b/x-pack/plugins/fleet/server/errors/handlers.ts @@ -31,6 +31,7 @@ import { PackageFailedVerificationError, PackagePolicyNotFoundError, FleetUnauthorizedError, + PackagePolicyNameExistsError, } from '.'; type IngestErrorHandler = ( @@ -78,6 +79,9 @@ const getHTTPResponseCode = (error: FleetError): number => { if (error instanceof FleetUnauthorizedError) { return 403; // Unauthorized } + if (error instanceof PackagePolicyNameExistsError) { + return 409; // Conflict + } return 400; // Bad Request }; diff --git a/x-pack/plugins/fleet/server/errors/index.ts b/x-pack/plugins/fleet/server/errors/index.ts index 4856ff5294e446..84cf87d799f4af 100644 --- a/x-pack/plugins/fleet/server/errors/index.ts +++ b/x-pack/plugins/fleet/server/errors/index.ts @@ -50,6 +50,7 @@ export class ConcurrentInstallOperationError extends FleetError {} export class AgentReassignmentError extends FleetError {} export class PackagePolicyIneligibleForUpgradeError extends FleetError {} export class PackagePolicyValidationError extends FleetError {} +export class PackagePolicyNameExistsError extends FleetError {} export class PackagePolicyNotFoundError extends FleetError {} export class BundledPackageNotFoundError extends FleetError {} export class HostedAgentPolicyRestrictionRelatedError extends FleetError { diff --git a/x-pack/plugins/fleet/server/services/package_policy.ts b/x-pack/plugins/fleet/server/services/package_policy.ts index 8561b14dd9ded9..fe044f012dd115 100644 --- a/x-pack/plugins/fleet/server/services/package_policy.ts +++ b/x-pack/plugins/fleet/server/services/package_policy.ts @@ -71,6 +71,7 @@ import { PackagePolicyNotFoundError, HostedAgentPolicyRestrictionRelatedError, FleetUnauthorizedError, + PackagePolicyNameExistsError, } from '../errors'; import { NewPackagePolicySchema, PackagePolicySchema, UpdatePackagePolicySchema } from '../types'; import type { @@ -166,17 +167,7 @@ class PackagePolicyClientImpl implements PackagePolicyClient { // trailing whitespace causes issues creating API keys enrichedPackagePolicy.name = enrichedPackagePolicy.name.trim(); if (!options?.skipUniqueNameVerification) { - const existingPoliciesWithName = await this.list(soClient, { - perPage: 1, - kuery: `${PACKAGE_POLICY_SAVED_OBJECT_TYPE}.name: "${enrichedPackagePolicy.name}"`, - }); - - // Check that the name does not exist already - if (existingPoliciesWithName.items.length > 0) { - throw new FleetError( - `An integration policy with the name ${enrichedPackagePolicy.name} already exists. Please rename it or choose a different name.` - ); - } + await requireUniqueName(soClient, enrichedPackagePolicy); } let elasticsearchPrivileges: NonNullable['privileges']; @@ -548,17 +539,7 @@ class PackagePolicyClientImpl implements PackagePolicyClient { packagePolicy.name !== oldPackagePolicy.name && !options?.skipUniqueNameVerification ) { - // Check that the name does not exist already but exclude the current package policy - const existingPoliciesWithName = await this.list(soClient, { - perPage: SO_SEARCH_LIMIT, - kuery: `${PACKAGE_POLICY_SAVED_OBJECT_TYPE}.name:"${packagePolicy.name}"`, - }); - const filtered = (existingPoliciesWithName?.items || []).filter((p) => p.id !== id); - if (filtered.length > 0) { - throw new FleetError( - `An integration policy with the name ${packagePolicy.name} already exists. Please rename it or choose a different name.` - ); - } + await requireUniqueName(soClient, enrichedPackagePolicy, id); } let inputs = restOfPackagePolicy.inputs.map((input) => @@ -2251,3 +2232,25 @@ function deepMergeVars(original: any, override: any, keepOriginalValue = false): return result; } + +async function requireUniqueName( + soClient: SavedObjectsClientContract, + packagePolicy: UpdatePackagePolicy | NewPackagePolicy, + id?: string +) { + const existingPoliciesWithName = await packagePolicyService.list(soClient, { + perPage: SO_SEARCH_LIMIT, + kuery: `${PACKAGE_POLICY_SAVED_OBJECT_TYPE}.name:"${packagePolicy.name}"`, + }); + + const policiesToCheck = id + ? // Check that the name does not exist already but exclude the current package policy + (existingPoliciesWithName?.items || []).filter((p) => p.id !== id) + : existingPoliciesWithName?.items; + + if (policiesToCheck.length > 0) { + throw new PackagePolicyNameExistsError( + `An integration policy with the name ${packagePolicy.name} already exists. Please rename it or choose a different name.` + ); + } +} 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 7def6a1801bfde..82777e41eb5e1b 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 @@ -245,7 +245,7 @@ export default function (providerContext: FtrProviderContext) { .expect(400); }); - it('should return a 400 if there is another package policy with the same name', async function () { + it('should return a 409 if there is another package policy with the same name', async function () { await supertest .post(`/api/fleet/package_policies`) .set('kbn-xsrf', 'xxxx') @@ -279,10 +279,10 @@ export default function (providerContext: FtrProviderContext) { version: '0.1.0', }, }) - .expect(400); + .expect(409); }); - it('should return a 400 if there is a package policy with the same name on a different policy', async function () { + it('should return a 409 if there is a package policy with the same name on a different policy', async function () { const { body: agentPolicyResponse } = await supertest .post(`/api/fleet/agent_policies`) .set('kbn-xsrf', 'xxxx') @@ -325,7 +325,7 @@ export default function (providerContext: FtrProviderContext) { version: '0.1.0', }, }) - .expect(400); + .expect(409); }); it('should return a 400 with required variables not provided', async function () { diff --git a/x-pack/test/fleet_api_integration/apis/package_policy/update.ts b/x-pack/test/fleet_api_integration/apis/package_policy/update.ts index 2206527667a410..2cc2302a42b6a4 100644 --- a/x-pack/test/fleet_api_integration/apis/package_policy/update.ts +++ b/x-pack/test/fleet_api_integration/apis/package_policy/update.ts @@ -368,7 +368,7 @@ export default function (providerContext: FtrProviderContext) { }); }); - it('should return a 400 if there is another package policy with the same name', async function () { + it('should return a 409 if there is another package policy with the same name', async function () { await supertest .put(`/api/fleet/package_policies/${packagePolicyId2}`) .set('kbn-xsrf', 'xxxx') @@ -385,10 +385,10 @@ export default function (providerContext: FtrProviderContext) { version: '0.1.0', }, }) - .expect(400); + .expect(409); }); - it('should return a 400 if there is another package policy with the same name on a different policy', async function () { + it('should return a 409 if there is another package policy with the same name on a different policy', async function () { const { body: agentPolicyResponse } = await supertest .post(`/api/fleet/agent_policies`) .set('kbn-xsrf', 'xxxx') @@ -414,7 +414,7 @@ export default function (providerContext: FtrProviderContext) { version: '0.1.0', }, }) - .expect(400); + .expect(409); }); it('should work with frozen input vars', async function () { From f0c240fa3471e77a0e43e5d4655b58ff2a3fcb1d Mon Sep 17 00:00:00 2001 From: criamico Date: Thu, 23 Mar 2023 12:57:22 +0100 Subject: [PATCH 2/2] Update openapi specs --- x-pack/plugins/fleet/common/openapi/bundled.json | 6 ++++++ x-pack/plugins/fleet/common/openapi/bundled.yaml | 4 ++++ .../fleet/common/openapi/paths/package_policies.yaml | 2 ++ .../common/openapi/paths/package_policies@upgrade.yaml | 2 ++ 4 files changed, 14 insertions(+) diff --git a/x-pack/plugins/fleet/common/openapi/bundled.json b/x-pack/plugins/fleet/common/openapi/bundled.json index 9b9e9a64f2acf0..90e0459cb2e54c 100644 --- a/x-pack/plugins/fleet/common/openapi/bundled.json +++ b/x-pack/plugins/fleet/common/openapi/bundled.json @@ -3858,6 +3858,9 @@ }, "400": { "$ref": "#/components/responses/error" + }, + "409": { + "$ref": "#/components/responses/error" } }, "requestBody": { @@ -4066,6 +4069,9 @@ }, "400": { "$ref": "#/components/responses/error" + }, + "409": { + "$ref": "#/components/responses/error" } } } diff --git a/x-pack/plugins/fleet/common/openapi/bundled.yaml b/x-pack/plugins/fleet/common/openapi/bundled.yaml index 8f5436703f6f77..e50525db886d0a 100644 --- a/x-pack/plugins/fleet/common/openapi/bundled.yaml +++ b/x-pack/plugins/fleet/common/openapi/bundled.yaml @@ -2406,6 +2406,8 @@ paths: - item '400': $ref: '#/components/responses/error' + '409': + $ref: '#/components/responses/error' requestBody: description: >- You should use inputs as an object and not use the deprecated inputs @@ -2537,6 +2539,8 @@ paths: - success '400': $ref: '#/components/responses/error' + '409': + $ref: '#/components/responses/error' /package_policies/upgrade/dryrun: post: summary: Dry run package policy upgrade diff --git a/x-pack/plugins/fleet/common/openapi/paths/package_policies.yaml b/x-pack/plugins/fleet/common/openapi/paths/package_policies.yaml index 0fe987e1727def..89a54608d2310a 100644 --- a/x-pack/plugins/fleet/common/openapi/paths/package_policies.yaml +++ b/x-pack/plugins/fleet/common/openapi/paths/package_policies.yaml @@ -47,6 +47,8 @@ post: - item '400': $ref: ../components/responses/error.yaml + '409': + $ref: ../components/responses/error.yaml requestBody: description: You should use inputs as an object and not use the deprecated inputs array. content: diff --git a/x-pack/plugins/fleet/common/openapi/paths/package_policies@upgrade.yaml b/x-pack/plugins/fleet/common/openapi/paths/package_policies@upgrade.yaml index 2b6e69d49c44e4..1837675a15f22c 100644 --- a/x-pack/plugins/fleet/common/openapi/paths/package_policies@upgrade.yaml +++ b/x-pack/plugins/fleet/common/openapi/paths/package_policies@upgrade.yaml @@ -36,3 +36,5 @@ post: - success '400': $ref: ../components/responses/error.yaml + '409': + $ref: ../components/responses/error.yaml