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

Improve role config jwt bound constraint check #49

Conversation

shwuandwing
Copy link
Contributor

When configuring JWT roles, consider bound claims when checking if there is are bound constraints

…ering if there is at least one bound constraint"

This reverts commit af3aee5.
@hashicorp-cla
Copy link

hashicorp-cla commented May 9, 2019

CLA assistant check
All committers have signed the CLA.

@shwuandwing
Copy link
Contributor Author

#49

@shwuandwing
Copy link
Contributor Author

@kalafut , this is my first Github change. Is there any more steps I have to take to get this change approved?

@kalafut
Copy link
Contributor

kalafut commented May 13, 2019

@shwuandwing Hi. When I last looked I didn't see the CLA approval, but now that's in good shape so I'll be doing the review soon.

@kalafut kalafut self-assigned this May 13, 2019
Copy link
Contributor

@kalafut kalafut left a comment

Choose a reason for hiding this comment

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

Thanks! The code change looks good, but I'd like a small test update. Currently if I remove your code change, the tests still pass. This is because the config is being updated and not created from scratch (the CreateOperation doesn't necessarily create in this case). This fix is easy though. Instead of using role/test2 on all the cases, use different roles for each: role/test3, role/test4, etc. I tested this and it works better (i.e. the test fails when it should).

@kalafut kalafut merged commit 5bfe480 into hashicorp:master May 14, 2019
@kalafut
Copy link
Contributor

kalafut commented May 14, 2019

Good catch and thanks for the submission!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants