-
Notifications
You must be signed in to change notification settings - Fork 62
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
Improve role config jwt bound constraint check #49
Conversation
… there is at least one bound constraint
…ering if there is at least one bound constraint" This reverts commit af3aee5.
… there is at least one bound constraint
@kalafut , this is my first Github change. Is there any more steps I have to take to get this change approved? |
@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. |
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.
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).
Good catch and thanks for the submission! |
When configuring JWT roles, consider bound claims when checking if there is are bound constraints