Alex/add max num uses to tokenhelper v2 #6701
Conversation
ghost
requested a review
from 
| @@ -38,6 +38,9 @@ type TokenParams struct { | |||
|
|
|||
| // The TTL to user for the token | |||
| TokenTTL time.Duration `json:"token_ttl" mapstructure:"token_ttl"` | |||
|
|
|||
| // The maximum number of times a token issued from this role may be used. | |||
| TokenMaxNumUses int `json:"token_max_num_uses" mapstructure:"token_max_num_uses"` | |||
There was a problem hiding this comment.
Should we use omitempty here? Assuming this value gets output to the user at some point, I wonder if 0 would be confusing. Internally zero means "unlimited" but that's mostly an implementation detail, yeah?
vault/token_store.go
Outdated
| @@ -2969,6 +2985,7 @@ func (ts *TokenStore) tokenStoreRoleRead(ctx context.Context, req *logical.Reque | |||
| "path_suffix": role.PathSuffix, | |||
| "renewable": role.Renewable, | |||
| "token_type": role.TokenType.String(), | |||
| "token_max_num_uses": role.TokenMaxNumUses, | |||
There was a problem hiding this comment.
Should we omit this if the value is zero? It may be confusing to users
|
|
||
| "token_max_num_uses": &framework.FieldSchema{ | ||
| Type: framework.TypeInt, | ||
| Description: "The maximum number of times a token issued from this role may be used.", |
There was a problem hiding this comment.
I think we should add to the description that in order to remove the limit/max, to set this value to 0, assuming that's valid
| @@ -38,6 +38,9 @@ type TokenParams struct { | |||
|
|
|||
| // The TTL to user for the token | |||
| TokenTTL time.Duration `json:"token_ttl" mapstructure:"token_ttl"` | |||
|
|
|||
| // The maximum number of times a token issued from this role may be used. | |||
| TokenNumUses int `json:"token_num_uses,omitempty" mapstructure:"token_num_uses,omitempty"` | |||
There was a problem hiding this comment.
I'm not sure if this should be omitempty or not, but I'm pretty sure we should be consistent with the other parameters, whether we make them all omitempty or none.
There was a problem hiding this comment.
Also note that because PopulateTokenData adds to the map regardless, this actually has no effect currently.
| switch { | ||
| case role.TokenNumUses > 0 && te.NumUses == 0: | ||
| te.NumUses = role.TokenNumUses | ||
| case role.TokenNumUses < te.NumUses: |
There was a problem hiding this comment.
If TokenNumUses is 0 and te.NumUses is 2, this will actually end up setting te.NumUses to 0. As a base case, if role.TokenNumUses is zero, it should probably just use whatever te.NumUses is.
There was a problem hiding this comment.
Yes. Oops.
This test case catches that and passes with the new base case.
…n created from a token role. Fixes faulty logic. Removes some useless omitempty tags.
ghost
mentioned this pull request
No description provided.