Revoke multiple tokens/accessors #2922
Conversation
vault/token_store.go
Outdated
| Type: framework.TypeString, | ||
| Description: "Accessor of the token (URL parameter)", | ||
| Type: framework.TypeCommaStringSlice, | ||
| Description: "Accessor(s) of the token (URL parameter)", |
There was a problem hiding this comment.
The URL parameter can't be used in this way, and it's also deprecated. Just the request body is sufficient.
vault/token_store.go
Outdated
| @@ -368,12 +368,12 @@ func NewTokenStore(c *Core, config *logical.BackendConfig) (*TokenStore, error) | |||
|
|
|||
| Fields: map[string]*framework.FieldSchema{ | |||
| "urltoken": &framework.FieldSchema{ | |||
| Type: framework.TypeString, | |||
vault/token_store.go
Outdated
| } | ||
|
|
||
| return nil, nil |
There was a problem hiding this comment.
If only a single accessor is given, the previous return values should be retained.
| } | ||
|
|
||
| if errs[idx] != nil { | ||
| failedRevokes = append(failedRevokes, map[string]string{ |
There was a problem hiding this comment.
While I'm not sure what this should be, I'm sure that it should not be a string map.
Possibly the return value should be a slice of the same size as the input with either nulls or error messages. There's no need to return the accessors if the ordering is the same.
There was a problem hiding this comment.
Possibly the return value should be a slice of the same size as the input with either nulls or error messages.
I am also not sure about this part, if number of revoked accessors/tokens are small, thats fine, if we are going to revoke millions of tokens at once, response may contain huge unnecessary data millions of nulls or empty strings
There's no need to return the accessors if the ordering is the same
How user will determine which accessors are failed, ordering is same, but not all accessors may fail?
There was a problem hiding this comment.
When something returns multiple errors, we normally use hashicorp/go-multierror and I think it would work in this case. I think it would remove a lot of the backwards compatibility logic since it would just contain one error in the case of a single accessor.
There was a problem hiding this comment.
I only added the comments to the accessor section but they also apply to the token methods since the logic is very similar.
I think this can be simplified quite a bit by just keeping a list of errors that happen during the processing and returning those errors at the end.
| @@ -81,6 +83,36 @@ func (r *Response) Error() error { | |||
| return nil | |||
| } | |||
|
|
|||
| func (r *Response) SetError(err error, errorData interface{}) { | |||
There was a problem hiding this comment.
I'm not sure we need this method on the response struct. I would much rather formatting of the errors being handled by the caller instead of trying to generalize it here.
| } | ||
|
|
||
| if errs[idx] != nil { | ||
| failedRevokes = append(failedRevokes, map[string]string{ |
There was a problem hiding this comment.
When something returns multiple errors, we normally use hashicorp/go-multierror and I think it would work in this case. I think it would remove a lot of the backwards compatibility logic since it would just contain one error in the case of a single accessor.
| for idx, accessor := range accessors { | ||
| aEntry, err := ts.lookupByAccessor(accessor, true) | ||
| if err != nil { | ||
| if len(accessors) == 1 { |
There was a problem hiding this comment.
It should be fine to let this flow through to the error handling blocks.
| return nil, err | ||
| } | ||
| errs[idx] = err | ||
| tokens[idx] = "" |
There was a problem hiding this comment.
You will need to continue after an error to move on to the next item.
| if err != nil { | ||
| return nil, err | ||
| errs := make([]error, len(accessors)) | ||
| tokens := make([]string, len(accessors)) |
There was a problem hiding this comment.
This list may not be the length of the accessor list in the case of errors. I would just set it to length zero and append to the slice.
| aEntry, err := ts.lookupByAccessor(accessor, true) | ||
| if err != nil { | ||
| return nil, err | ||
| errs := make([]error, len(accessors)) |
There was a problem hiding this comment.
See comments below about using hashicorp/go-multierror.
|
Closing due to inactivity. |
Fixes #2864
In this commit, if revoking token/accessor failed, errors field contains information about failed revokes in below format: