From e057ee0750443f5c9c65ab1f920d26e56888d7be Mon Sep 17 00:00:00 2001 From: Max Bowsher Date: Tue, 25 Jul 2023 16:22:33 +0100 Subject: [PATCH] OpenAPI: Define default response structure for ListOperations (#21934) * OpenAPI: Define default response structure for ListOperations Almost all Vault ListOperation responses have an identical response schema. Update the OpenAPI generator to know this, and remove a few instances where that standard response schema had been manually copy/pasted into place in individual endpoints. * changelog * Only render StandardListResponse schema, if an operation uses it * Teach the response schema validation test helper about the default list schema too --------- Co-authored-by: Anton Averchenkov <84287187+averche@users.noreply.github.com> --- builtin/credential/approle/path_role.go | 24 +---------------- builtin/logical/pki/path_fetch.go | 12 --------- builtin/logical/pki/path_revoke.go | 12 --------- builtin/logical/pki/path_roles.go | 12 --------- changelog/21934.txt | 3 +++ sdk/framework/openapi.go | 26 +++++++++++++++++++ .../testhelpers/schema/response_validation.go | 12 +++++++++ vault/logical_raw.go | 11 -------- vault/logical_system.go | 8 ++++++ vault/logical_system_paths.go | 13 +--------- vault/logical_system_quotas.go | 11 -------- 11 files changed, 51 insertions(+), 93 deletions(-) create mode 100644 changelog/21934.txt diff --git a/builtin/credential/approle/path_role.go b/builtin/credential/approle/path_role.go index 78867a5527..b7b46a69fd 100644 --- a/builtin/credential/approle/path_role.go +++ b/builtin/credential/approle/path_role.go @@ -13,7 +13,7 @@ import ( "github.com/hashicorp/go-secure-stdlib/parseutil" "github.com/hashicorp/go-secure-stdlib/strutil" - uuid "github.com/hashicorp/go-uuid" + "github.com/hashicorp/go-uuid" "github.com/hashicorp/vault/helper/parseip" "github.com/hashicorp/vault/sdk/framework" "github.com/hashicorp/vault/sdk/helper/cidrutil" @@ -311,17 +311,6 @@ can only be set during role creation and once set, it can't be reset later.`, Operations: map[logical.Operation]framework.OperationHandler{ logical.ListOperation: &framework.PathOperation{ Callback: b.pathRoleList, - Responses: map[int][]framework.Response{ - http.StatusOK: {{ - Description: "OK", - Fields: map[string]*framework.FieldSchema{ - "keys": { - Type: framework.TypeStringSlice, - Required: true, - }, - }, - }}, - }, }, }, HelpSynopsis: strings.TrimSpace(roleHelp["role-list"][0]), @@ -984,17 +973,6 @@ Overrides secret_id_ttl role option when supplied. May not be longer than role's DisplayAttrs: &framework.DisplayAttributes{ OperationSuffix: "secret-ids", }, - Responses: map[int][]framework.Response{ - http.StatusOK: {{ - Description: "OK", - Fields: map[string]*framework.FieldSchema{ - "keys": { - Required: true, - Type: framework.TypeStringSlice, - }, - }, - }}, - }, }, }, HelpSynopsis: strings.TrimSpace(roleHelp["role-secret-id"][0]), diff --git a/builtin/logical/pki/path_fetch.go b/builtin/logical/pki/path_fetch.go index b255cee51d..a532967cc7 100644 --- a/builtin/logical/pki/path_fetch.go +++ b/builtin/logical/pki/path_fetch.go @@ -239,18 +239,6 @@ func pathFetchListCerts(b *backend) *framework.Path { Operations: map[logical.Operation]framework.OperationHandler{ logical.ListOperation: &framework.PathOperation{ Callback: b.pathFetchCertList, - Responses: map[int][]framework.Response{ - http.StatusOK: {{ - Description: "OK", - Fields: map[string]*framework.FieldSchema{ - "keys": { - Type: framework.TypeStringSlice, - Description: `A list of keys`, - Required: true, - }, - }, - }}, - }, }, }, diff --git a/builtin/logical/pki/path_revoke.go b/builtin/logical/pki/path_revoke.go index 2decd50cff..42a56270d2 100644 --- a/builtin/logical/pki/path_revoke.go +++ b/builtin/logical/pki/path_revoke.go @@ -37,18 +37,6 @@ func pathListCertsRevoked(b *backend) *framework.Path { Operations: map[logical.Operation]framework.OperationHandler{ logical.ListOperation: &framework.PathOperation{ Callback: b.pathListRevokedCertsHandler, - Responses: map[int][]framework.Response{ - http.StatusOK: {{ - Description: "OK", - Fields: map[string]*framework.FieldSchema{ - "keys": { - Type: framework.TypeStringSlice, - Description: `List of Keys`, - Required: false, - }, - }, - }}, - }, }, }, diff --git a/builtin/logical/pki/path_roles.go b/builtin/logical/pki/path_roles.go index 5b7d37e998..44d2fa2bf9 100644 --- a/builtin/logical/pki/path_roles.go +++ b/builtin/logical/pki/path_roles.go @@ -32,18 +32,6 @@ func pathListRoles(b *backend) *framework.Path { Operations: map[logical.Operation]framework.OperationHandler{ logical.ListOperation: &framework.PathOperation{ Callback: b.pathRoleList, - Responses: map[int][]framework.Response{ - http.StatusOK: {{ - Description: "OK", - Fields: map[string]*framework.FieldSchema{ - "keys": { - Type: framework.TypeStringSlice, - Description: "List of roles", - Required: true, - }, - }, - }}, - }, }, }, diff --git a/changelog/21934.txt b/changelog/21934.txt new file mode 100644 index 0000000000..d4ce0b08a6 --- /dev/null +++ b/changelog/21934.txt @@ -0,0 +1,3 @@ +```release-note:improvement +openapi: Fix response definitions for list operations +``` diff --git a/sdk/framework/openapi.go b/sdk/framework/openapi.go index 1bb3b7361a..92b2243108 100644 --- a/sdk/framework/openapi.go +++ b/sdk/framework/openapi.go @@ -197,6 +197,29 @@ var OASStdRespNoContent = &OASResponse{ Description: "empty body", } +var OASStdRespListOK = &OASResponse{ + Description: "OK", + Content: OASContent{ + "application/json": &OASMediaTypeObject{ + Schema: &OASSchema{ + Ref: "#/components/schemas/StandardListResponse", + }, + }, + }, +} + +var OASStdSchemaStandardListResponse = &OASSchema{ + Type: "object", + Properties: map[string]*OASSchema{ + "keys": { + Type: "array", + Items: &OASSchema{ + Type: "string", + }, + }, + }, +} + // Regex for handling fields in paths, and string cleanup. // Predefined here to avoid substantial recompilation. @@ -456,6 +479,9 @@ func documentPath(p *Path, backend *Backend, requestResponsePrefix string, doc * if len(props.Responses) == 0 { if opType == logical.DeleteOperation { op.Responses[204] = OASStdRespNoContent + } else if opType == logical.ListOperation { + op.Responses[200] = OASStdRespListOK + doc.Components.Schemas["StandardListResponse"] = OASStdSchemaStandardListResponse } else { op.Responses[200] = OASStdRespOK } diff --git a/sdk/helper/testhelpers/schema/response_validation.go b/sdk/helper/testhelpers/schema/response_validation.go index 430d1754a5..8085b042b6 100644 --- a/sdk/helper/testhelpers/schema/response_validation.go +++ b/sdk/helper/testhelpers/schema/response_validation.go @@ -150,6 +150,18 @@ func GetResponseSchema(t *testing.T, path *framework.Path, operation logical.Ope } if len(schemaResponses) == 0 { + // ListOperations have a default response schema that is implicit unless overridden + if operation == logical.ListOperation { + return &framework.Response{ + Description: "OK", + Fields: map[string]*framework.FieldSchema{ + "keys": { + Type: framework.TypeStringSlice, + }, + }, + } + } + t.Fatalf( "could not find response schema: %s: %q operation: no responses found", path.Pattern, diff --git a/vault/logical_raw.go b/vault/logical_raw.go index ba221327a8..b8e709209a 100644 --- a/vault/logical_raw.go +++ b/vault/logical_raw.go @@ -377,17 +377,6 @@ func rawPaths(prefix string, r *RawBackend) []*framework.Path { OperationPrefix: "raw", OperationVerb: "list", }, - Responses: map[int][]framework.Response{ - http.StatusOK: {{ - Description: "OK", - Fields: map[string]*framework.FieldSchema{ - "keys": { - Type: framework.TypeStringSlice, - Required: true, - }, - }, - }}, - }, Summary: "Return a list keys for a given path prefix.", }, }, diff --git a/vault/logical_system.go b/vault/logical_system.go index e19ced430e..bd73f5524e 100644 --- a/vault/logical_system.go +++ b/vault/logical_system.go @@ -4679,6 +4679,14 @@ func (b *SystemBackend) pathInternalOpenAPI(ctx context.Context, req *logical.Re doc.CreateOperationIDs(context) + // Every backend that includes a ListOperation that uses the default response schema will have supplied its own + // version of that schema, on a last writer wins basis. To ensure an external plugin doesn't end up being the last + // writer, we now override with the version within the code of the hosting Vault instance, if the document now + // being generated contains any version of this: + if _, ok := doc.Components.Schemas["StandardListResponse"]; ok { + doc.Components.Schemas["StandardListResponse"] = framework.OASStdSchemaStandardListResponse + } + buf, err := json.Marshal(doc) if err != nil { return nil, err diff --git a/vault/logical_system_paths.go b/vault/logical_system_paths.go index 700581fe65..8724360b9d 100644 --- a/vault/logical_system_paths.go +++ b/vault/logical_system_paths.go @@ -3894,18 +3894,7 @@ func (b *SystemBackend) policyPaths() []*framework.Path { Operations: map[logical.Operation]framework.OperationHandler{ logical.ListOperation: &framework.PathOperation{ Callback: b.handlePoliciesPasswordList, - Responses: map[int][]framework.Response{ - http.StatusOK: {{ - Description: "OK", - Fields: map[string]*framework.FieldSchema{ - "keys": { - Type: framework.TypeStringSlice, - Required: false, - }, - }, - }}, - }, - Summary: "List the existing password policies.", + Summary: "List the existing password policies.", }, }, }, diff --git a/vault/logical_system_quotas.go b/vault/logical_system_quotas.go index 1a9dbbf608..a7acca6506 100644 --- a/vault/logical_system_quotas.go +++ b/vault/logical_system_quotas.go @@ -92,17 +92,6 @@ func (b *SystemBackend) quotasPaths() []*framework.Path { Operations: map[logical.Operation]framework.OperationHandler{ logical.ListOperation: &framework.PathOperation{ Callback: b.handleRateLimitQuotasList(), - Responses: map[int][]framework.Response{ - http.StatusOK: {{ - Description: "OK", - Fields: map[string]*framework.FieldSchema{ - "keys": { - Type: framework.TypeStringSlice, - Required: true, - }, - }, - }}, - }, }, }, HelpSynopsis: strings.TrimSpace(quotasHelp["rate-limit-list"][0]),