jbonofre commented on code in PR #1276:
URL: https://github.com/apache/activemq/pull/1276#discussion_r1694090546


##########
activemq-broker/README_OAUTH_OIDC.md.bak:
##########
@@ -0,0 +1,9 @@
+# OAuth and OIDC Implementation for ActiveMQ

Review Comment:
   bak file should not be part of the PR. Please remove this one.



##########
activemq-broker/src/main/java/org/apache/activemq/security/OIDCAuthenticationPlugin.java:
##########
@@ -0,0 +1,68 @@
+package org.apache.activemq.security;

Review Comment:
   ASF header is missing here.



##########
activemq-web-console/src/main/webapp/WEB-INF/activemq.xml:
##########
@@ -34,6 +34,9 @@
     <transportConnectors>
       <transportConnector name="openwire" uri="tcp://localhost:61616" />
       <transportConnector name="stomp" uri="stomp://localhost:61613" />
+      <!-- Add Jetty Transport Connector for Web Console -->

Review Comment:
   That's not correct: the jetty transport is not for the webconsole. Jetty 
transport is for http transport connect.
   The WebConsole is managed outside of the broker by inclusion of `jetty.xml`.



##########
activemq-broker/README_OAUTH_OIDC.md:
##########
@@ -0,0 +1,19 @@
+# OAuth and OIDC Implementation for ActiveMQ

Review Comment:
   I don't think it makes sense to add a specific README here. Better to add in 
the doc section.
   
   Also the ASF header is missing.



##########
activemq-broker/src/main/java/org/apache/activemq/security/OAuthValidator.java:
##########
@@ -0,0 +1,23 @@
+package org.apache.activemq.security;

Review Comment:
   ASF header is missing here.



##########
activemq-broker/pom.xml:
##########
@@ -67,6 +68,23 @@
       <optional>true</optional>
     </dependency>
 
+    <!-- =============================== -->
+    <!-- oAuth and OIDC Dependencies -->
+    <!-- Nimbus JOSE + JWT dependencies -->
+    <!-- =============================== -->
+
+    <dependency>
+      <groupId>com.nimbusds</groupId>
+      <artifactId>oauth2-oidc-sdk</artifactId>
+      <version>9.15</version>

Review Comment:
   1. The version should be managed by dependencyManagement in the root pom.
   2. These dependencies can be optional I guess.



##########
activemq-broker/src/main/java/org/apache/activemq/security/OIDCSecurityContext.java:
##########
@@ -0,0 +1,18 @@
+package org.apache.activemq.security;

Review Comment:
   ASF header is missing here.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact


Reply via email to