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

support "user_claim_json_pointer" in create_role() for JWT auth method #998

Closed
wants to merge 2 commits into from

Conversation

ferenc-hechler
Copy link
Contributor

@ferenc-hechler ferenc-hechler commented Jun 16, 2023

When creating a role for the JWT auth method, the optional parameter "user_claim_json_pointer" is missing.
See documentation here:

https://developer.hashicorp.com/vault/api-docs/auth/jwt#user_claim_json_pointer

The parameter is a bool value which defaults to false.
This pull request adds the missing parameter. I tested it for JWT auth and it works.
The parameter can be bool or a str containing "true" or "false".
If a wrong parameter is given, the server returns a corresponding error message.

The OIDC auth mehod inherits create_role() from JWT.
So, I also added this parameter to ODIC.create_role().
The OIDC use case was not tested.

Documentation was updated.

@ferenc-hechler ferenc-hechler requested a review from a team as a code owner June 16, 2023 19:35
@briantist briantist self-assigned this Jun 17, 2023
@briantist briantist added enhancement a new feature or addition auth methods generally related to a Vault auth method jwt/oidc JWT/OIDC auth method labels Jun 17, 2023
@briantist briantist added this to the 1.2.0 milestone Jun 17, 2023
@codecov
Copy link

codecov bot commented Jun 17, 2023

Codecov Report

Merging #998 (0886c96) into main (31aca14) will increase coverage by 0.09%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #998      +/-   ##
==========================================
+ Coverage   81.98%   82.08%   +0.09%     
==========================================
  Files          65       65              
  Lines        3019     3019              
==========================================
+ Hits         2475     2478       +3     
+ Misses        544      541       -3     
Impacted Files Coverage Δ
hvac/api/auth_methods/jwt.py 92.30% <ø> (ø)
hvac/api/auth_methods/oidc.py 100.00% <ø> (ø)

... and 1 file with indirect coverage changes

@briantist
Copy link
Contributor

Hi @ferenc-hechler ! Welcome and thanks for submitting this. I hope to look this over a more thoroughly as soon as I get a little time.

In the meantime, please ensure you set up your local environment and run the lint/format before pushing:

poetry run black .
poetry run flake8 .

Let me know if you need a hand with that.


I am curious, why make the parameter accept strings in addition to bool? The API documentation seems to expect a bool only, and since we are passing through the value directly, we aren't doing any conversion or checking of it.

My instinct is to make this bool only. If the API is documented as accepting a string, then I could see us accepting either, though I might still lean toward it being bool only in hvac.

I don't use this API currently so if I'm missing something please let me know.

Thanks again!

@ferenc-hechler
Copy link
Contributor Author

Hi @briantist ,
thanks for your feedback.

I formatted the code as you proposed.

About the additional "str" type:
I removed this. I saw, that there is "int | str" for other parameters and thought this is a pattern, to also allow "str". But looking at "token_no_default_policy" is also a "bool" without "str" alternative.

@briantist briantist changed the base branch from develop to main June 17, 2023 20:16
@briantist
Copy link
Contributor

Thank you @ferenc-hechler that makes sense.

The hvac project is simplifying its developer workflow and removing the develop branch. Please be aware for future PRs that the project's default branch is now main.

Since your PR was opened before this change, I have:

  • updated your PR to merge into the project's main branch
  • rebased your branch against main and force-pushed it (please pull before making any further changes)

@ferenc-hechler ferenc-hechler deleted the develop branch June 17, 2023 21:57
@ferenc-hechler
Copy link
Contributor Author

looks like my idea to switch the branch names from main and develop was not successful.
I will create a new pull request from the main branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auth methods generally related to a Vault auth method enhancement a new feature or addition jwt/oidc JWT/OIDC auth method
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants