[AMQ-9244] Add JWT authentication plugin #1035
Conversation
| <dependency> | ||
| <groupId>com.nimbusds</groupId> | ||
| <artifactId>nimbus-jose-jwt</artifactId> | ||
| <version>9.22</version> |
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.