From f6d917e553e979321e78d9c6f18b045af30fe588 Mon Sep 17 00:00:00 2001 From: akhilailla Date: Tue, 21 Jan 2025 14:49:33 -0800 Subject: [PATCH 1/8] Skip validating MSI in PatchBodyParametersSchema --- docs/patch-body-parameters-schema.md | 4 - .../rulesets/generated/spectral/az-arm.js | 11 ++- .../functions/patch-body-parameters.ts | 13 ++- .../test/patch-body-parameters.test.ts | 80 ++++++++++++++++++- 4 files changed, 97 insertions(+), 11 deletions(-) diff --git a/docs/patch-body-parameters-schema.md b/docs/patch-body-parameters-schema.md index 5681ff873..f3a4b2591 100644 --- a/docs/patch-body-parameters-schema.md +++ b/docs/patch-body-parameters-schema.md @@ -12,10 +12,6 @@ ARM and Data plane OpenAPI(swagger) specs - RPC-Patch-V1-10 -## Output Message - -Properties of a PATCH request body must not be {0}. PATCH operation: '{1}' Model Definition: '{2}' Property: '{3}' - ## Description A request parameter of the Patch Operation must not have a required/default/'x-ms-mutability:"create"' value. diff --git a/packages/rulesets/generated/spectral/az-arm.js b/packages/rulesets/generated/spectral/az-arm.js index 3acf8b5d3..a0eedbcb3 100644 --- a/packages/rulesets/generated/spectral/az-arm.js +++ b/packages/rulesets/generated/spectral/az-arm.js @@ -28,16 +28,16 @@ const avoidMsdnReferences = (swaggerObj, _opts, paths) => { if (swaggerObj === null) { return []; } - if (typeof swaggerObj === "string" && !swaggerObj.includes("https://msdn.microsoft.com")) + if (typeof swaggerObj === "string" && !(swaggerObj.includes("https://msdn.microsoft.com") || swaggerObj.includes("https://docs.microsoft.com"))) return []; if (typeof swaggerObj === "object") { const docUrl = swaggerObj.url; - if (docUrl === undefined || !docUrl.startsWith("https://msdn.microsoft.com")) + if (docUrl === undefined || !(docUrl.startsWith("https://msdn.microsoft.com") || docUrl.startsWith("https://docs.microsoft.com"))) return []; } const path = paths.path || []; return [{ - message: 'For better generated code quality, remove all references to "msdn.microsoft.com".', + message: 'For better generated code quality, remove all references to "msdn.microsoft.com" and "docs.microsoft.com".', path, }]; }; @@ -265,7 +265,7 @@ function getRequiredProperties(schema) { }); } if (schema.required) { - requires = [...schema.required, requires]; + requires = [...schema.required, ...requires]; } return requires; } @@ -2040,6 +2040,9 @@ const patchBodyParameters = (parameters, _opts, paths) => { if (parameters === null || parameters.schema === undefined || parameters.in !== "body") { return []; } + if (parameters.schema.description && parameters.schema.description.includes("Managed service identity")) { + return []; + } const path = paths.path || []; const properties = getProperties(parameters.schema); const requiredProperties = getRequiredProperties(parameters.schema); diff --git a/packages/rulesets/src/spectral/functions/patch-body-parameters.ts b/packages/rulesets/src/spectral/functions/patch-body-parameters.ts index d7eec132c..71dbe9c39 100644 --- a/packages/rulesets/src/spectral/functions/patch-body-parameters.ts +++ b/packages/rulesets/src/spectral/functions/patch-body-parameters.ts @@ -6,6 +6,13 @@ const patchBodyParameters = (parameters: any, _opts: any, paths: any): IFunction if (parameters === null || parameters.schema === undefined || parameters.in !== "body") { return [] } + + // skip validation for MSI(managed service identity), + // as it is being referenced from common-types + if (parameters.schema.description && parameters.schema.description.includes("Managed service identity")) { + return [] + } + const path = paths.path || [] const properties: object = getProperties(parameters.schema) @@ -18,12 +25,14 @@ const patchBodyParameters = (parameters: any, _opts: any, paths: any): IFunction path: [...path, "schema"], }) } + if (requiredProperties.includes(prop)) { errors.push({ message: `Properties of a PATCH request body must not be required, property:${prop}.`, path: [...path, "schema"], }) } + const xmsMutability = properties[prop]["x-ms-mutability"] if (xmsMutability && xmsMutability.length === 1 && xmsMutability[0] === "create") { errors.push({ @@ -40,8 +49,8 @@ const patchBodyParameters = (parameters: any, _opts: any, paths: any): IFunction in: "body", }, _opts, - { path: [...path, "schema", "properties", prop] } - ) + { path: [...path, "schema", "properties", prop] }, + ), ) } } diff --git a/packages/rulesets/src/spectral/test/patch-body-parameters.test.ts b/packages/rulesets/src/spectral/test/patch-body-parameters.test.ts index 4d73fd7a5..04793ac39 100644 --- a/packages/rulesets/src/spectral/test/patch-body-parameters.test.ts +++ b/packages/rulesets/src/spectral/test/patch-body-parameters.test.ts @@ -108,6 +108,84 @@ test("PatchBodyParametersSchema should find errors for required/create value", ( }) }) +test("PatchBodyParametersSchema should skip errors for MSI though has required value", () => { + const oasDoc = { + swagger: "2.0", + paths: { + "/foo": { + patch: { + tags: ["SampleTag"], + operationId: "Foo_Update", + description: "Test Description", + parameters: [ + { + name: "foo_patch", + in: "body", + schema: { + $ref: "#/definitions/OpenShiftClusterUpdate", + }, + }, + ], + responses: {}, + }, + }, + }, + definitions: { + OpenShiftClusterUpdate: { + description: "OpenShiftCluster represents an Azure Red Hat OpenShift cluster.", + type: "object", + properties: { + tags: { + $ref: "#/definitions/Tags", + description: "The resource tags.", + }, + identity: { + $ref: "#/definitions/ManagedServiceIdentity", + description: "Identity stores information about the cluster MSI(s) in a workload identity cluster.", + }, + }, + }, + ManagedServiceIdentity: { + description: "Managed service identity (system assigned and/or user assigned identities)", + type: "object", + properties: { + principalId: { + readOnly: true, + format: "uuid", + type: "string", + description: + "The service principal ID of the system assigned identity. This property will only be provided for a system assigned identity.", + }, + tenantId: { + readOnly: true, + format: "uuid", + type: "string", + description: + "The tenant ID of the system assigned identity. This property will only be provided for a system assigned identity.", + }, + type: { + description: "Type of managed service identity (where both SystemAssigned and UserAssigned types are allowed).", + type: "string", + }, + userAssignedIdentities: { + description: + "The set of user assigned identities associated with the resource. The userAssignedIdentities dictionary keys will be ARM resource ids in the form: '/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.ManagedIdentity/userAssignedIdentities/{identityName}. The dictionary values can be empty objects ({}) in requests.", + type: "object", + }, + }, + required: ["type"], + }, + Tags: { + description: "Tags represents an OpenShift cluster's tags.", + type: "object", + }, + }, + } + return linter.run(oasDoc).then((results) => { + expect(results.length).toBe(0) + }) +}) + test("PatchBodyParametersSchema should find errors for default value in nested body parameter", () => { const oasDoc = { swagger: "2.0", @@ -178,7 +256,7 @@ test("PatchBodyParametersSchema should find errors for default value in nested b } return linter.run(oasDoc).then((results) => { expect(results.length).toBe(2) - results.sort((a, b) => a.path.join('.').localeCompare(b.path.join('.'))); + results.sort((a, b) => a.path.join(".").localeCompare(b.path.join("."))) expect(results[0].path.join(".")).toBe("paths./bar.patch.parameters.0.schema.properties.properties") expect(results[0].message).toContain("Properties of a PATCH request body must not have default value, property:prop0.") expect(results[1].path.join(".")).toBe("paths./foo.patch.parameters.0.schema.properties.properties") From 6574bebad523111eb38db784f2d65570759d11a9 Mon Sep 17 00:00:00 2001 From: akhilailla Date: Thu, 23 Jan 2025 16:24:45 -0600 Subject: [PATCH 2/8] Update logic to check for the property name and not description --- packages/rulesets/generated/spectral/az-arm.js | 8 ++++---- .../src/spectral/functions/patch-body-parameters.ts | 12 ++++++------ 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/packages/rulesets/generated/spectral/az-arm.js b/packages/rulesets/generated/spectral/az-arm.js index a0eedbcb3..ed7ae74da 100644 --- a/packages/rulesets/generated/spectral/az-arm.js +++ b/packages/rulesets/generated/spectral/az-arm.js @@ -265,7 +265,7 @@ function getRequiredProperties(schema) { }); } if (schema.required) { - requires = [...schema.required, ...requires]; + requires = [...schema.required, requires]; } return requires; } @@ -2040,14 +2040,14 @@ const patchBodyParameters = (parameters, _opts, paths) => { if (parameters === null || parameters.schema === undefined || parameters.in !== "body") { return []; } - if (parameters.schema.description && parameters.schema.description.includes("Managed service identity")) { - return []; - } const path = paths.path || []; const properties = getProperties(parameters.schema); const requiredProperties = getRequiredProperties(parameters.schema); const errors = []; for (const prop of Object.keys(properties)) { + if (prop.toLowerCase() === "identity") { + continue; + } if (properties[prop].default) { errors.push({ message: `Properties of a PATCH request body must not have default value, property:${prop}.`, diff --git a/packages/rulesets/src/spectral/functions/patch-body-parameters.ts b/packages/rulesets/src/spectral/functions/patch-body-parameters.ts index 71dbe9c39..9550525af 100644 --- a/packages/rulesets/src/spectral/functions/patch-body-parameters.ts +++ b/packages/rulesets/src/spectral/functions/patch-body-parameters.ts @@ -7,18 +7,18 @@ const patchBodyParameters = (parameters: any, _opts: any, paths: any): IFunction return [] } - // skip validation for MSI(managed service identity), - // as it is being referenced from common-types - if (parameters.schema.description && parameters.schema.description.includes("Managed service identity")) { - return [] - } - const path = paths.path || [] const properties: object = getProperties(parameters.schema) const requiredProperties = getRequiredProperties(parameters.schema) const errors = [] for (const prop of Object.keys(properties)) { + // skip validation for identity property + // as it refers MSI(managed service identity) from common-types + if (prop.toLowerCase() === "identity") { + continue + } + if (properties[prop].default) { errors.push({ message: `Properties of a PATCH request body must not have default value, property:${prop}.`, From a592c07fb4cf0da38c1ecfd44509f0954ed8e599 Mon Sep 17 00:00:00 2001 From: akhilailla Date: Mon, 27 Jan 2025 10:20:14 -0600 Subject: [PATCH 3/8] Add change log --- ...fix_PatchBodyParametersSchema_2025-01-23-22-23.json | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 common/changes/@microsoft.azure/openapi-validator-rulesets/akhilailla-fix_PatchBodyParametersSchema_2025-01-23-22-23.json diff --git a/common/changes/@microsoft.azure/openapi-validator-rulesets/akhilailla-fix_PatchBodyParametersSchema_2025-01-23-22-23.json b/common/changes/@microsoft.azure/openapi-validator-rulesets/akhilailla-fix_PatchBodyParametersSchema_2025-01-23-22-23.json new file mode 100644 index 000000000..8f124e8b7 --- /dev/null +++ b/common/changes/@microsoft.azure/openapi-validator-rulesets/akhilailla-fix_PatchBodyParametersSchema_2025-01-23-22-23.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@microsoft.azure/openapi-validator-rulesets", + "comment": "Update PatchBodyParametersSchema to skip validation for MSI(managed service identity) as it is being referenced from common-types and has the required field & is being referenced in patch body parameter schema by several RP's who are having to get an exception.", + "type": "none" + } + ], + "packageName": "@microsoft.azure/openapi-validator-rulesets" +} \ No newline at end of file From e12cd54f3cf6064b3db621a5f65d5af3c61e3422 Mon Sep 17 00:00:00 2001 From: Mike Harder Date: Mon, 27 Jan 2025 18:54:10 +0000 Subject: [PATCH 4/8] Revert "Add change log" This reverts commit a592c07fb4cf0da38c1ecfd44509f0954ed8e599. --- ...fix_PatchBodyParametersSchema_2025-01-23-22-23.json | 10 ---------- 1 file changed, 10 deletions(-) delete mode 100644 common/changes/@microsoft.azure/openapi-validator-rulesets/akhilailla-fix_PatchBodyParametersSchema_2025-01-23-22-23.json diff --git a/common/changes/@microsoft.azure/openapi-validator-rulesets/akhilailla-fix_PatchBodyParametersSchema_2025-01-23-22-23.json b/common/changes/@microsoft.azure/openapi-validator-rulesets/akhilailla-fix_PatchBodyParametersSchema_2025-01-23-22-23.json deleted file mode 100644 index 8f124e8b7..000000000 --- a/common/changes/@microsoft.azure/openapi-validator-rulesets/akhilailla-fix_PatchBodyParametersSchema_2025-01-23-22-23.json +++ /dev/null @@ -1,10 +0,0 @@ -{ - "changes": [ - { - "packageName": "@microsoft.azure/openapi-validator-rulesets", - "comment": "Update PatchBodyParametersSchema to skip validation for MSI(managed service identity) as it is being referenced from common-types and has the required field & is being referenced in patch body parameter schema by several RP's who are having to get an exception.", - "type": "none" - } - ], - "packageName": "@microsoft.azure/openapi-validator-rulesets" -} \ No newline at end of file From a3c5a00e2234470e9323944fe76e695d50620d87 Mon Sep 17 00:00:00 2001 From: Mike Harder Date: Mon, 27 Jan 2025 18:55:52 +0000 Subject: [PATCH 5/8] Update changelog.md --- packages/rulesets/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/rulesets/CHANGELOG.md b/packages/rulesets/CHANGELOG.md index 3596c6084..82a039d77 100644 --- a/packages/rulesets/CHANGELOG.md +++ b/packages/rulesets/CHANGELOG.md @@ -5,6 +5,7 @@ ### Patches - [AllTrackedResourcesMustHaveDelete][TrackedResourcePatchOperation] Skip the api paths if it has PrivateEndpointConnectionProxy +- [PatchBodyParametersSchema] Skip validation for MSI (managed service identity) as it is being referenced from common-types and has the required field & is being referenced in patch body parameter schema by several RP's who are having to get an exception. ### Patches From 3f7d2d202a03e8bcb5f8f0ec037b73faad71e608 Mon Sep 17 00:00:00 2001 From: akhilailla Date: Sun, 2 Feb 2025 14:21:09 -0600 Subject: [PATCH 6/8] Updates based on comments --- .../rulesets/generated/spectral/az-arm.js | 6 +- .../functions/patch-body-parameters.ts | 7 +- .../test/patch-body-parameters.test.ts | 103 +++++++++++++++++- 3 files changed, 109 insertions(+), 7 deletions(-) diff --git a/packages/rulesets/generated/spectral/az-arm.js b/packages/rulesets/generated/spectral/az-arm.js index f696b467a..9088d6bf9 100644 --- a/packages/rulesets/generated/spectral/az-arm.js +++ b/packages/rulesets/generated/spectral/az-arm.js @@ -2036,7 +2036,7 @@ const parametersInPost = (postParameters, _opts, ctx) => { return errors; }; -const patchBodyParameters = (parameters, _opts, paths) => { +const patchBodyParameters = (parameters, _opts, paths, isTopLevel = true) => { if (parameters === null || parameters.schema === undefined || parameters.in !== "body") { return []; } @@ -2045,7 +2045,7 @@ const patchBodyParameters = (parameters, _opts, paths) => { const requiredProperties = getRequiredProperties(parameters.schema); const errors = []; for (const prop of Object.keys(properties)) { - if (prop.toLowerCase() === "identity") { + if (isTopLevel && prop.toLowerCase() === "identity") { continue; } if (properties[prop].default) { @@ -2071,7 +2071,7 @@ const patchBodyParameters = (parameters, _opts, paths) => { errors.push(...patchBodyParameters({ schema: properties[prop], in: "body", - }, _opts, { path: [...path, "schema", "properties", prop] })); + }, _opts, { path: [...path, "schema", "properties", prop] }, false)); } } return errors; diff --git a/packages/rulesets/src/spectral/functions/patch-body-parameters.ts b/packages/rulesets/src/spectral/functions/patch-body-parameters.ts index 9550525af..6a464831f 100644 --- a/packages/rulesets/src/spectral/functions/patch-body-parameters.ts +++ b/packages/rulesets/src/spectral/functions/patch-body-parameters.ts @@ -2,7 +2,7 @@ import type { IFunctionResult } from "@stoplight/spectral-core" import { getProperties, getRequiredProperties } from "./utils" //This rule appears if in the patch body parameters have properties which is marked as required or x-ms-mutability:["create"] or have default -const patchBodyParameters = (parameters: any, _opts: any, paths: any): IFunctionResult[] => { +const patchBodyParameters = (parameters: any, _opts: any, paths: any, isTopLevel: boolean = true): IFunctionResult[] => { if (parameters === null || parameters.schema === undefined || parameters.in !== "body") { return [] } @@ -13,9 +13,9 @@ const patchBodyParameters = (parameters: any, _opts: any, paths: any): IFunction const requiredProperties = getRequiredProperties(parameters.schema) const errors = [] for (const prop of Object.keys(properties)) { - // skip validation for identity property + // skip validation for identity property only at the top level // as it refers MSI(managed service identity) from common-types - if (prop.toLowerCase() === "identity") { + if (isTopLevel && prop.toLowerCase() === "identity") { continue } @@ -50,6 +50,7 @@ const patchBodyParameters = (parameters: any, _opts: any, paths: any): IFunction }, _opts, { path: [...path, "schema", "properties", prop] }, + false, ), ) } diff --git a/packages/rulesets/src/spectral/test/patch-body-parameters.test.ts b/packages/rulesets/src/spectral/test/patch-body-parameters.test.ts index 04793ac39..fc4928994 100644 --- a/packages/rulesets/src/spectral/test/patch-body-parameters.test.ts +++ b/packages/rulesets/src/spectral/test/patch-body-parameters.test.ts @@ -1,5 +1,6 @@ import { Spectral } from "@stoplight/spectral-core" import linterForRule from "./utils" +//import { identity } from "lodash" let linter: Spectral @@ -108,7 +109,7 @@ test("PatchBodyParametersSchema should find errors for required/create value", ( }) }) -test("PatchBodyParametersSchema should skip errors for MSI though has required value", () => { +test("PatchBodyParametersSchema should skip errors for MSI though has required value only at top level", () => { const oasDoc = { swagger: "2.0", paths: { @@ -186,6 +187,106 @@ test("PatchBodyParametersSchema should skip errors for MSI though has required v }) }) +test("PatchBodyParametersSchema should not skip validation for non top level identity property", () => { + const oasDoc = { + swagger: "2.0", + paths: { + "/foo": { + patch: { + tags: ["SampleTag"], + operationId: "Foo_Update", + description: "Test Description", + parameters: [ + { + name: "foo_patch", + in: "body", + schema: { + $ref: "#/definitions/OpenShiftClusterUpdate", + }, + }, + ], + responses: {}, + }, + }, + }, + definitions: { + OpenShiftClusterUpdate: { + description: "OpenShiftCluster represents an Azure Red Hat OpenShift cluster.", + type: "object", + properties: { + identity: { + $ref: "#/definitions/ManagedServiceIdentity", + description: "Identity stores information about the cluster MSI(s) in a workload identity cluster.", + }, + testIdentity: { + $ref: "#/definitions/foo", + }, + }, + }, + ManagedServiceIdentity: { + description: "Managed service identity (system assigned and/or user assigned identities)", + type: "object", + properties: { + principalId: { + readOnly: true, + format: "uuid", + type: "string", + description: + "The service principal ID of the system assigned identity. This property will only be provided for a system assigned identity.", + }, + tenantId: { + readOnly: true, + format: "uuid", + type: "string", + description: + "The tenant ID of the system assigned identity. This property will only be provided for a system assigned identity.", + }, + type: { + description: "Type of managed service identity (where both SystemAssigned and UserAssigned types are allowed).", + type: "string", + }, + userAssignedIdentities: { + description: + "The set of user assigned identities associated with the resource. The userAssignedIdentities dictionary keys will be ARM resource ids in the form: '/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.ManagedIdentity/userAssignedIdentities/{identityName}. The dictionary values can be empty objects ({}) in requests.", + type: "object", + }, + }, + required: ["type"], + }, + TestManagedServiceIdentity: { + type: "object", + properties: { + type: { + description: "Type of managed service identity (where both SystemAssigned and UserAssigned types are allowed).", + type: "string", + }, + userAssignedIdentities: { + description: + "The set of user assigned identities associated with the resource. The userAssignedIdentities dictionary keys will be ARM resource ids in the form: '/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.ManagedIdentity/userAssignedIdentities/{identityName}. The dictionary values can be empty objects ({}) in requests.", + type: "object", + }, + }, + required: ["type"], + }, + foo: { + description: "OpenShiftCluster represents an Azure Red Hat OpenShift cluster.", + type: "object", + properties: { + identity: { + $ref: "#/definitions/TestManagedServiceIdentity", + description: "Identity stores information about the cluster MSI(s) in a workload identity cluster.", + }, + }, + }, + }, + } + return linter.run(oasDoc).then((results) => { + expect(results.length).toBe(1) + expect(results[0].path.join(".")).toBe("paths./foo.patch.parameters.0.schema.properties.testIdentity") + expect(results[0].message).toContain("Properties of a PATCH request body must not be required, property:type.") + }) +}) + test("PatchBodyParametersSchema should find errors for default value in nested body parameter", () => { const oasDoc = { swagger: "2.0", From 5825ef8e41472a1b68185aac5308798eb7122f84 Mon Sep 17 00:00:00 2001 From: akhilailla Date: Sun, 2 Feb 2025 14:30:28 -0600 Subject: [PATCH 7/8] revert comment --- .../rulesets/src/spectral/test/patch-body-parameters.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/rulesets/src/spectral/test/patch-body-parameters.test.ts b/packages/rulesets/src/spectral/test/patch-body-parameters.test.ts index fc4928994..fe2856669 100644 --- a/packages/rulesets/src/spectral/test/patch-body-parameters.test.ts +++ b/packages/rulesets/src/spectral/test/patch-body-parameters.test.ts @@ -1,6 +1,6 @@ import { Spectral } from "@stoplight/spectral-core" import linterForRule from "./utils" -//import { identity } from "lodash" +import { identity } from "lodash" let linter: Spectral From ed677c9e38c53996d9f52524763745fb659142fa Mon Sep 17 00:00:00 2001 From: akhilailla Date: Mon, 3 Feb 2025 16:21:21 -0600 Subject: [PATCH 8/8] Fix build --- .../rulesets/src/spectral/test/patch-body-parameters.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/rulesets/src/spectral/test/patch-body-parameters.test.ts b/packages/rulesets/src/spectral/test/patch-body-parameters.test.ts index fe2856669..5f4b3ff4e 100644 --- a/packages/rulesets/src/spectral/test/patch-body-parameters.test.ts +++ b/packages/rulesets/src/spectral/test/patch-body-parameters.test.ts @@ -1,6 +1,5 @@ import { Spectral } from "@stoplight/spectral-core" import linterForRule from "./utils" -import { identity } from "lodash" let linter: Spectral