From fe013a987a41662e9db39849b96e8838444d2a85 Mon Sep 17 00:00:00 2001 From: Max Bowsher Date: Fri, 21 Jul 2023 20:05:29 +0100 Subject: [PATCH] Add missing `Query: true` metadata to API definitions (#21949) * Add missing `Query: true` metadata to API definitions Also improve the documentation comment for `Query` to guide people better how they should be setting `Query` in the future. Endpoints affected: - auth/approle/role/{role_name}/secret-id/destroy - auth/approle/role/{role_name}/secret-id-accessor/destroy - auth/token/lookup - auth/token/lookup-self - sys/internal/specs/openapi - sys/wrapping/lookup - identity/oidc/provider/{name}/authorize There are also endpoints in the `aws` and `gcp` secrets engines which need the same treatment in their own PRs. When working on the `auth/token/lookup-self` path, I discovered that it had a parameter which was completely pointless - it was even documented as unused. It only existed because the `auth/token/lookup-self` code path was implemented by bodging the current token into the request data and passing control to the `auth/token/lookup` handler directly - instead of just factoring out the common code to a reusable function - so I fixed that whilst I was there. Note that two of the affected endpoints currently have one form of their OpenAPI operation ID set to something mentioning "with-parameters": - identity/oidc/provider/{name}/authorize - sys/internal/specs/openapi These operation IDs should be changed, as they perpetuate a misunderstanding - both read (GET) and update (POST/PUT) forms of these APIs are **equally** capable of being used with parameters. * I failed to spot that the aws plugin is in-repo! Update that too. * Remove code cleanup changes from this PR * Wording and wrapping adjustment as requested. --- builtin/credential/approle/path_role.go | 2 ++ builtin/logical/aws/path_user.go | 3 +++ sdk/framework/backend.go | 25 ++++++++++++++++++++++--- vault/identity_store_oidc_provider.go | 9 +++++++++ vault/logical_system_paths.go | 4 +++- vault/token_store.go | 3 ++- 6 files changed, 41 insertions(+), 5 deletions(-) diff --git a/builtin/credential/approle/path_role.go b/builtin/credential/approle/path_role.go index 112d2e0f13..78867a5527 100644 --- a/builtin/credential/approle/path_role.go +++ b/builtin/credential/approle/path_role.go @@ -1087,6 +1087,7 @@ Overrides secret_id_ttl role option when supplied. May not be longer than role's "secret_id": { Type: framework.TypeString, Description: "SecretID attached to the role.", + Query: true, }, }, Operations: map[logical.Operation]framework.OperationHandler{ @@ -1195,6 +1196,7 @@ Overrides secret_id_ttl role option when supplied. May not be longer than role's "secret_id_accessor": { Type: framework.TypeString, Description: "Accessor of the SecretID", + Query: true, }, }, Operations: map[logical.Operation]framework.OperationHandler{ diff --git a/builtin/logical/aws/path_user.go b/builtin/logical/aws/path_user.go index f368365c60..1b6a5cd3b0 100644 --- a/builtin/logical/aws/path_user.go +++ b/builtin/logical/aws/path_user.go @@ -35,15 +35,18 @@ func pathUser(b *backend) *framework.Path { "role_arn": { Type: framework.TypeString, Description: "ARN of role to assume when credential_type is " + assumedRoleCred, + Query: true, }, "ttl": { Type: framework.TypeDurationSecond, Description: "Lifetime of the returned credentials in seconds", Default: 3600, + Query: true, }, "role_session_name": { Type: framework.TypeString, Description: "Session name to use when assuming role. Max chars: 64", + Query: true, }, }, diff --git a/sdk/framework/backend.go b/sdk/framework/backend.go index 4da7d57149..0d05547d15 100644 --- a/sdk/framework/backend.go +++ b/sdk/framework/backend.go @@ -748,16 +748,35 @@ type FieldSchema struct { Required bool Deprecated bool - // Query indicates this field will be sent as a query parameter: + // Query indicates this field will be expected as a query parameter as part + // of ReadOperation, ListOperation or DeleteOperation requests: // // /v1/foo/bar?some_param=some_value // - // It doesn't affect handling of the value, but may be used for documentation. + // The field will still be expected as a request body parameter for + // CreateOperation or UpdateOperation requests! + // + // To put that another way, you should set Query for any non-path parameter + // you want to use in a read/list/delete operation. While setting the Query + // field to `true` is not required in such cases (Vault will expose the + // query parameters to you via req.Data regardless), it is highly + // recommended to do so in order to improve the quality of the generated + // OpenAPI documentation (as well as any code generation based on it), which + // will otherwise incorrectly omit the parameter. + // + // The reason for this design is historical: back at the start of 2018, + // query parameters were not mapped to fields at all, and it was implicit + // that all non-path fields were exclusively for the use of create/update + // operations. Since then, support for query parameters has gradually been + // extended to read, delete and list operations - and now this declarative + // metadata is needed, so that the OpenAPI generator can know which + // parameters are actually referred to, from within the code of + // read/delete/list operation handler functions. Query bool // AllowedValues is an optional list of permitted values for this field. // This constraint is not (yet) enforced by the framework, but the list is - // output as part of OpenAPI generation and may effect documentation and + // output as part of OpenAPI generation and may affect documentation and // dynamic UI generation. AllowedValues []interface{} diff --git a/vault/identity_store_oidc_provider.go b/vault/identity_store_oidc_provider.go index 612b166052..b08da04183 100644 --- a/vault/identity_store_oidc_provider.go +++ b/vault/identity_store_oidc_provider.go @@ -477,42 +477,51 @@ func oidcProviderPaths(i *IdentityStore) []*framework.Path { Type: framework.TypeString, Description: "The ID of the requesting client.", Required: true, + Query: true, }, "scope": { Type: framework.TypeString, Description: "A space-delimited, case-sensitive list of scopes to be requested. The 'openid' scope is required.", Required: true, + Query: true, }, "redirect_uri": { Type: framework.TypeString, Description: "The redirection URI to which the response will be sent.", Required: true, + Query: true, }, "response_type": { Type: framework.TypeString, Description: "The OIDC authentication flow to be used. The following response types are supported: 'code'", Required: true, + Query: true, }, "state": { Type: framework.TypeString, Description: "The value used to maintain state between the authentication request and client.", + Query: true, }, "nonce": { Type: framework.TypeString, Description: "The value that will be returned in the ID token nonce claim after a token exchange.", + Query: true, }, "max_age": { Type: framework.TypeInt, Description: "The allowable elapsed time in seconds since the last time the end-user was actively authenticated.", + Query: true, }, "code_challenge": { Type: framework.TypeString, Description: "The code challenge derived from the code verifier.", + Query: true, }, "code_challenge_method": { Type: framework.TypeString, Description: "The method that was used to derive the code challenge. The following methods are supported: 'S256', 'plain'. Defaults to 'plain'.", Default: codeChallengeMethodPlain, + Query: true, }, }, Operations: map[logical.Operation]framework.OperationHandler{ diff --git a/vault/logical_system_paths.go b/vault/logical_system_paths.go index ccb4d20819..700581fe65 100644 --- a/vault/logical_system_paths.go +++ b/vault/logical_system_paths.go @@ -2225,6 +2225,7 @@ func (b *SystemBackend) internalPaths() []*framework.Path { "context": { Type: framework.TypeString, Description: "Context string appended to every operationId", + Query: true, }, "generic_mount_paths": { Type: framework.TypeBool, @@ -4081,7 +4082,8 @@ func (b *SystemBackend) wrappingPaths() []*framework.Path { Fields: map[string]*framework.FieldSchema{ "token": { - Type: framework.TypeString, + Type: framework.TypeString, + Query: true, }, }, diff --git a/vault/token_store.go b/vault/token_store.go index df5c45990d..0d566a3219 100644 --- a/vault/token_store.go +++ b/vault/token_store.go @@ -310,7 +310,8 @@ func (ts *TokenStore) paths() []*framework.Path { Fields: map[string]*framework.FieldSchema{ "token": { Type: framework.TypeString, - Description: "Token to lookup (POST request body)", + Description: "Token to lookup", + Query: true, }, },