azure-rest-api-specs icon indicating copy to clipboard operation
azure-rest-api-specs copied to clipboard

Swagger ModelValidation erroneously fails when required x-ms-secret properties are omitted from responses in examples

Open erjms opened this issue 3 years ago • 2 comments

From the x-ms-secret documentation:

When a property is modeled as both "required" and "x-ms-secret": true, which means that this property must not exist in the response but has to be present in request.

The Swagger ModelValidation pipeline fails for this scenario, erroneously flagging properties that match this scenario that are missing from example responses as errors, e.g.: https://github.com/Azure/azure-rest-api-specs/pull/18645/checks?check_run_id=6586657182

erjms avatar Aug 15 '22 15:08 erjms

@keryul , can you check if this issue was fixed?

raych1 avatar Aug 18 '22 10:08 raych1

hi @erjms , from the example of x-ms-secret documentation

"SecretAuthInfo": {
      "type": "object",
      "description": "The authentication info when authType is secret",
      "properties": {
        "name": {
          "description": "Username or account name for secret auth.",
          "type": "string"
        },
        "secret": {
          "description": "Password or account key for secret auth.",
          "type": "string",
          "x-ms-secret": true
        }
    }
}

x-ms-secret will not work when property has "$ref"

I suggest the following changes to swagger and example swagger file

@@ -5337,12 +5337,7 @@
       "properties": {
         "secrets": {
           "description": "[Required] Storage account secrets.",
-          "$ref": "#/definitions/AccountKeyDatastoreSecrets",
-          "x-ms-mutability": [
-            "create",
-            "update"
-          ],
-          "x-ms-secret": true
+          "$ref": "#/definitions/AccountKeyDatastoreSecrets"
         }
       },
       "x-ms-discriminator-value": "AccountKey",
@@ -5360,6 +5355,11 @@
         "key": {
           "description": "Storage account key.",
           "type": "string",
+          "x-ms-mutability": [
+            "create",
+            "update"
+          ],
+          "x-ms-secret": true,
           "x-nullable": true
         }
       },

example file

@@ -31,7 +31,10 @@
               "isDefault": false,
               "properties": null,
               "credentials": {
-                "credentialsType": "AccountKey"
+                "credentialsType": "AccountKey",
+                "secrets": {
+                  "secretsType": "AccountKey"
+                }
               },
               "datastoreType": "AzureBlob",
               "accountName": "string",

ankhyk avatar Aug 19 '22 03:08 ankhyk

Hi @keryul, thanks for responding. We are unable to make the suggested changes because our service's swagger has already been published as a stable version and they would constitute breaking changes. But more generally, could you please point me to where in the documentation it forbids applying x-ms-secret to $ref properties? I don't see any such restriction called out, and I can imagine many scenarios (such as ours) where we would need to mark an entire complex object as a secret, not just individual string properties.

erjms avatar Aug 29 '22 21:08 erjms

Hi @erjms , we can find the relationship between x-ms-secret and x-ms-mutability in the fourth rule of x-ms-secret

"x-ms-secret": true is equivalent to "x-ms-mutability": ["create", "update"]

then we can find the last rule of x-ms-mutability

When this extension is applied on a collection (array, dictionary) then this will have effects on the mutability (adding/removing elements) of the collection. Mutability of the collection cannot be applied on its elements. The mutability of the element will be governed based on the mutability defined in the element's definition.

"The mutability of the element will be governed based on the mutability defined in the element's definition." and the example "Mutability of the object property; which is a collection of items"

"definitions": {
  "ResourceCollection": {
    "description": "Collection of Resource objects. Resource is defined in the above example.",
    "properties": {
      "value": {
        "type": "array",
        "description": "Array of Resource objects.",
        "x-ms-mutability": ["create", "read", "update"], //This means that the array is mutable
        "items": {
          "type": object,
          "x-ms-mutability": ["create", "read"] // X - Applying mutability on the itemType of the array or valueType of the dictionary is not allowed.
          "schema": {
            "$ref": "#/definitions/Resource" // The mutability of the properties of the Resource object is governed by the mutability defined in it's model definition.
          }
        }
      }
    }
  }
}

"X - Applying mutability on the itemType of the array or valueType of the dictionary is not allowed."

A more convenient way is to imagine every property has its own x-ms-secret, with a default value of false. the second rule of x-ms-secret:

When applying this extensions as: "x-ms-secret": false, it has same effect as not applying it.

ankhyk avatar Sep 07 '22 06:09 ankhyk