Skip to content
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

make "role" input optional #291

Merged
merged 2 commits into from
Apr 7, 2022
Merged

Conversation

kdomanski
Copy link
Contributor

Per Vault documentation it doesn't have to be provided,
and the auth provider's "default_role" parameter is required
precisely for this case.
https://www.vaultproject.io/api/auth/jwt

@hashicorp-cla
Copy link

hashicorp-cla commented Jan 27, 2022

CLA assistant check
All committers have signed the CLA.

@tvoran tvoran added the enhancement New feature or request label Jan 28, 2022
Copy link
Contributor

@tomhjp tomhjp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable to me. It would be great to get a test case added to the JWT integration tests to exercise this: https://github.com/hashicorp/vault-action/blob/master/integrationTests/basic/jwt_auth.test.js

src/auth.js Show resolved Hide resolved
Per Vault documentation it doesn't have to be provided,
and the auth provider's "default_role" parameter is required
precisely for this case.
https://www.vaultproject.io/api/auth/jwt
@kdomanski kdomanski force-pushed the jwt-role-optional branch from c2d70ba to 4bd5334 Compare April 7, 2022 01:45
@kdomanski
Copy link
Contributor Author

Seems reasonable to me. It would be great to get a test case added to the JWT integration tests to exercise this: https://github.com/hashicorp/vault-action/blob/master/integrationTests/basic/jwt_auth.test.js

I've amended the commit with an extra testcase.

Copy link
Contributor

@tomhjp tomhjp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test didn't look quite right to me - it needed the default_role to be set on the mount config, but was passing. I took a look and realised the mocks were interfering across tests (the new test was actually sending "role": "default" instead of null/empty). As it was only test code and I was in there anyway, I just pushed the fix, hope you don't mind.

Thanks for the contribution!

@tomhjp tomhjp merged commit 2f64a97 into hashicorp:master Apr 7, 2022
@kdomanski
Copy link
Contributor Author

Thanks a lot for sorting out the tests. Pleasure to work with you.

@kdomanski
Copy link
Contributor Author

@tvoran How about releasing v2.5.0 now?

@kdomanski kdomanski deleted the jwt-role-optional branch April 24, 2022 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants