-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[AMQ-9244] Add JWT authentication plugin #1035
base: main
Are you sure you want to change the base?
Conversation
<dependency> | ||
<groupId>com.nimbusds</groupId> | ||
<artifactId>nimbus-jose-jwt</artifactId> | ||
<version>9.22</version> |
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 latest version is 9.31
. How come an older version is being used instead of the latest?
<dependency> | ||
<groupId>org.bitbucket.b_c</groupId> | ||
<artifactId>jose4j</artifactId> | ||
<version>0.7.9</version> |
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 latest version is 0.9.3
. How come an older version is being used instead of the latest?
|
||
import java.util.Set; | ||
|
||
public enum Claims { |
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.
What exactly are all the values, and why are so many unused values needed?
* some mapping functionalities. | ||
* The header name is also hard coded for simplicity. | ||
*/ | ||
public class JwtAuthenticationPlugin implements BrokerPlugin { |
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.
Perhaps I'm wrong, but this doesn't appear usable as is. How can users configure these values? Is the idea here that they will need implement and deploy their own version of this class in order to configure auth as needed? If so, are there plans to make this configurable in the future?
/** | ||
* Utilities for generating a JWT for testing | ||
*/ | ||
public class TokenUtils { |
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.
Nothing from this class is used anywhere. The JavaDoc says it's used for testing, but there are no tests. This class seems unnecessary at this point.
); | ||
|
||
final JwtConsumer jwtConsumer = builder.build(); | ||
final JwtContext jwtContext = jwtConsumer.process(username); |
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.
On the Jira it was indicated that the password field should contain the token. If username
is used instead will this fit the reported use-case?
Generally speaking this looks more like a proof-of-concept (i.e. based on this blog post) rather than a feature which is ready to use. I'm not sure it makes sense to merge it at this point especially with no tests to validate the functionality and to mitigate future regressions. |
That's the starting point. The intention is not to merge right now. I wanted to share here as few users want to try. I will work on this PR after the releases plan. |
@jbonofre, understood. That wasn't clear from your comment on the Jira where you said,
Thanks for the clarification. |
I went ahead and converted this to a draft PR. Generally speaking we should be using draft PRs if something is not ready to merge. This way it makes it clear it's still a work in progress. Marking things like this draft PRs in the future should hopefully help with the confusion about the state. |
No description provided.