-
Notifications
You must be signed in to change notification settings - Fork 49
[Fix #952] Refactoring auth code #975
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
Conversation
Signed-off-by: fjtirado <ftirados@redhat.com>
c46fe9b to
2356235
Compare
| import java.util.Map; | ||
| import java.util.ServiceLoader; | ||
|
|
||
| public class AccessTokenProvider { |
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.
Can we replace this provider in the extension? For this feature, we will be using the Quarkus OIDC Client, which we will likely be using.
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 Provider suffix here is kind of misleading, but I keep the original because lack of better alternative (Im terrible with naming). This class is just an aggregation of the info that is needed to perform the call, using JaxRS, to retrieve the token.
If in future we use Quarkus OIDC client, probably it will be better to write a new HTTP task (which can be done, base on the quarkus stack, in the quarkus extension)
| import java.util.Map; | ||
|
|
||
| public class OAuthRequestBuilder extends AbstractAuthRequestBuilder { | ||
| public class OAuthRequestBuilder |
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.
We may even consider this lightweight dep: https://www.pac4j.org/docs/clients/openid-connect.html (maybe as an optional library impl-http-oidc)
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.
There is not need to add the dependency right now, the call to retrieve the token is based on JaxRS and seems to be working fine.
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.
But the role OIDC API is complex to implement, have we already done it? cc @treblereel
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.
But its not in the spec schema, isnt it?
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.
To acquire the token, you need to come back and forth the token server.
The specification includes the scheme to control the endpoints where you'd get the token:
https://github.com/serverlessworkflow/specification/blob/main/dsl-reference.md#oauth2-authentication
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.
yes, this partially implemented, I will open PR for completion
Signed-off-by: fjtirado <ftirados@redhat.com>
Fix #952