-
Notifications
You must be signed in to change notification settings - Fork 4.4k
Add API client method for listing token accessors #10464
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
7131307
to
c495784
Compare
c495784
to
330d1a0
Compare
8d7f72c
to
1fe788d
Compare
1fe788d
to
6d519c2
Compare
6d519c2
to
2004b6f
Compare
api/secret.go
Outdated
@@ -113,69 +146,19 @@ func (s *Secret) TokenPolicies() ([]string, error) { | |||
return s.Auth.Policies, nil | |||
} | |||
|
|||
if s.Data == nil || s.Data["policies"] == nil { | |||
if s.Data == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you could get rid of the if
altogether, since the test is already present in extractListOfStrings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
} | ||
tokenPolicies, err := s.extractListOfStrings("policies") | ||
if tokenPolicies == nil || err != nil { | ||
return tokenPolicies, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are cases in the current code where we goto TOKEN_DONE despite having an empty tokenPolicies. I think this if
's condition should be just err != nil
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I didn't notice it. Now the latest version of the code should be equivalent in functionality
api/secret.go
Outdated
} | ||
|
||
if s.Auth != nil && len(s.Auth.Policies) > 0 { | ||
return s.Auth.Policies, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look correct. Policies aren't accessors, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. fixed now
@@ -189,6 +172,23 @@ DONE: | |||
return policies, nil | |||
} | |||
|
|||
// TokenAccessors returns the standardized list of accessors for the given secret | |||
func (s *Secret) TokenAccessors() ([]string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You name this TokenAccessors
, but it could just as well be called Keys
or something, right? It's the same code that would be used to extract the result of any list
operation from a Secret
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right on that. I removed the if statement that would return the secret's accessor value because that was wrong. Instead the function call basically just invokes the extractListOfStrings
with the correct parameters.
* Adds ListAccessors function to the API client * Adds docstrings to functions exported in the api/auth_token.go module * Rewrites the function that parses the return value of the Vault API to make it reusable * Adds TokenAccessors helper to extra token accessors from return value of API
2004b6f
to
35cebcd
Compare
@ncabatoff it looks like I need some sort of authorization from someone in order to get my PR deployed on Vercel? Other than that I fixed all your suggestions and added tests which are passing |
Fixes: #10463
Changes: