-
Notifications
You must be signed in to change notification settings - Fork 108
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
PBENCH-865 First pass at implementing OIDC token decode framework #2942
PBENCH-865 First pass at implementing OIDC token decode framework #2942
Conversation
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 start; just a few minor comments.
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.
Great start. How are you thinking it would integrate along side our existing auth mechanism?
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
This shouldn't bring many changes in terms of how we authenticate the REST endpoints, REST endpoints would still have a decorator that checks the validity of the Bearer access_token and performs the required action. However, following subsequent changes will happen (that I can think of):
|
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 good
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.
Under the assumption that Nikhil is about to make a major revision to this PR, I'm going to stop reviewing now and just post what I've got.
lib/pbench/test/unit/server/user_identity_management/conftest.py
Outdated
Show resolved
Hide resolved
4b22fe7
to
3946008
Compare
Sure we don't want to be restricted to decoding only one type of token, but this framework can be extended to decode any identity tokens, not just the Keycloak broker. We should be able to decode any token that is generated by the OIDC-compliant identity providers. |
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.
Sequence diagram changes look okay to me.
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.
The diagram looks great!
However, I found one misspelling, and I think it would look better if you terminated the flow instead of letting it run off the bottom of the page. Also, there is one pair of somewhat mismatched labels which would be good to make match.
Beyond those, I found some capitalization and other typographical nits that you might want to consider amending.
Addressed the minor typos. |
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.
🚀
First pass at implementing the authentication framework for decoding third-party identity provider tokens. This PR is not replacing the current authentication mechanism we have right now, this PR is for adding a framework that we can adopt slowly in our authentication flow as we go.
Addresses issue PBENCH-865
Presently this PR provides the following things:
userinfo
oidc endpoint