You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by GitBox <gi...@apache.org> on 2022/10/14 18:01:11 UTC
[GitHub] [activemq] Dm-Chebotarskyi opened a new pull request, #699: AMQ-6148 Reusing LDAP context to avoid creating new connections
Dm-Chebotarskyi opened a new pull request, #699:
URL: https://github.com/apache/activemq/pull/699
### Description
Re-using LDAP context for authentication. The context is created only once and re-used each time ActiveMQ server established connection with ActiveMQ clients.
Refactoring some code in LDAPLoginModule.java
This PR is based on fix introduced to Artemis: https://github.com/apache/activemq-artemis/commit/f3a8619d7eeabded75f3725f2e77af267e8cb450#diff-706fd9b54d2aed5a0ea5d28fa7c70f7ee733672f7e91d847137517e3b147d716
### Testing
Manual test authentication using LDAP
### Issue
https://issues.apache.org/jira/browse/AMQ-6148
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] jbonofre merged pull request #699: AMQ-6148 Reusing LDAP context to avoid creating new connections
Posted by GitBox <gi...@apache.org>.
jbonofre merged PR #699:
URL: https://github.com/apache/activemq/pull/699
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] Dm-Chebotarskyi closed pull request #699: AMQ-6148 Reusing LDAP context to avoid creating new connections
Posted by GitBox <gi...@apache.org>.
Dm-Chebotarskyi closed pull request #699: AMQ-6148 Reusing LDAP context to avoid creating new connections
URL: https://github.com/apache/activemq/pull/699
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] jbonofre commented on pull request #699: AMQ-6148 Reusing LDAP context to avoid creating new connections
Posted by GitBox <gi...@apache.org>.
jbonofre commented on PR #699:
URL: https://github.com/apache/activemq/pull/699#issuecomment-1279229450
@Dm-Chebotarskyi do you mind to squash the 9 commits in one and rebase ? If you prefer, I can do it myself and manually merge. Thoughts ?
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] mattrpav commented on a diff in pull request #699: AMQ-6148 Reusing LDAP context to avoid creating new connections
Posted by GitBox <gi...@apache.org>.
mattrpav commented on code in PR #699:
URL: https://github.com/apache/activemq/pull/699#discussion_r967355327
##########
activemq-jaas/src/main/java/org/apache/activemq/jaas/LDAPLoginModule.java:
##########
@@ -175,7 +171,7 @@ public boolean commit() throws LoginException {
public boolean abort() throws LoginException {
if (!succeeded) {
return false;
- } else if (succeeded && commitSucceeded) {
Review Comment:
Why is the logic changing here?
##########
activemq-jaas/src/main/java/org/apache/activemq/jaas/LDAPLoginModule.java:
##########
@@ -450,7 +440,6 @@ protected boolean bindUser(DirContext context, String dn, String password) throw
log.debug("User " + dn + " successfully bound.");
}
} catch (AuthenticationException e) {
- isValid = false;
Review Comment:
Why is this being removed?
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] Dm-Chebotarskyi commented on a diff in pull request #699: AMQ-6148 Reusing LDAP context to avoid creating new connections
Posted by GitBox <gi...@apache.org>.
Dm-Chebotarskyi commented on code in PR #699:
URL: https://github.com/apache/activemq/pull/699#discussion_r967360453
##########
activemq-jaas/src/main/java/org/apache/activemq/jaas/LDAPLoginModule.java:
##########
@@ -175,7 +171,7 @@ public boolean commit() throws LoginException {
public boolean abort() throws LoginException {
if (!succeeded) {
return false;
- } else if (succeeded && commitSucceeded) {
Review Comment:
Variable `succeeded` is alway `true` in line 174. Logic hasn't been changed
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] jbonofre commented on pull request #699: AMQ-6148 Reusing LDAP context to avoid creating new connections
Posted by GitBox <gi...@apache.org>.
jbonofre commented on PR #699:
URL: https://github.com/apache/activemq/pull/699#issuecomment-1279238900
@Dm-Chebotarskyi awesome ! Thank you so much ! Let me know if I can help on anything.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] mattrpav commented on a diff in pull request #699: AMQ-6148 Reusing LDAP context to avoid creating new connections
Posted by GitBox <gi...@apache.org>.
mattrpav commented on code in PR #699:
URL: https://github.com/apache/activemq/pull/699#discussion_r951988296
##########
activemq-jaas/src/main/java/org/apache/activemq/jaas/LDAPLoginModule.java:
##########
@@ -320,22 +316,17 @@ protected boolean authenticate(String username, String password) throws LoginExc
throw new FailedLoginException("Password does not match for user: " + username);
}
} catch (CommunicationException e) {
+ closeContext();
Review Comment:
Can all the closeContext() calls be moved to a finally {}
##########
activemq-jaas/src/main/java/org/apache/activemq/jaas/LDAPLoginModule.java:
##########
@@ -187,9 +183,13 @@ public boolean abort() throws LoginException {
return true;
}
- protected void close(DirContext context) {
+ protected void closeContext() {
+ if (context == null) {
+ return;
+ }
try {
context.close();
+ context = null;
Review Comment:
The context = null assignment should probably be in a finally {}
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] jbonofre commented on a diff in pull request #699: AMQ-6148 Reusing LDAP context to avoid creating new connections
Posted by GitBox <gi...@apache.org>.
jbonofre commented on code in PR #699:
URL: https://github.com/apache/activemq/pull/699#discussion_r995941852
##########
activemq-jaas/src/main/java/org/apache/activemq/jaas/LDAPLoginModule.java:
##########
@@ -175,7 +171,7 @@ public boolean commit() throws LoginException {
public boolean abort() throws LoginException {
if (!succeeded) {
return false;
- } else if (succeeded && commitSucceeded) {
Review Comment:
I agree with @Dm-Chebotarskyi : as we already test `succeeded` at line 172, no need to test again.
##########
activemq-jaas/src/main/java/org/apache/activemq/jaas/LDAPLoginModule.java:
##########
@@ -450,7 +440,6 @@ protected boolean bindUser(DirContext context, String dn, String password) throw
log.debug("User " + dn + " successfully bound.");
}
} catch (AuthenticationException e) {
- isValid = false;
Review Comment:
I agree with @Dm-Chebotarskyi : `isValid` is already set to `false` at line 429.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] Dm-Chebotarskyi commented on a diff in pull request #699: AMQ-6148 Reusing LDAP context to avoid creating new connections
Posted by GitBox <gi...@apache.org>.
Dm-Chebotarskyi commented on code in PR #699:
URL: https://github.com/apache/activemq/pull/699#discussion_r967361744
##########
activemq-jaas/src/main/java/org/apache/activemq/jaas/LDAPLoginModule.java:
##########
@@ -450,7 +440,6 @@ protected boolean bindUser(DirContext context, String dn, String password) throw
log.debug("User " + dn + " successfully bound.");
}
} catch (AuthenticationException e) {
- isValid = false;
Review Comment:
Because `isValid` variable is `false` here and this assignment is not needed.
Logic hasn't been changed
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] Dm-Chebotarskyi commented on a diff in pull request #699: AMQ-6148 Reusing LDAP context to avoid creating new connections
Posted by GitBox <gi...@apache.org>.
Dm-Chebotarskyi commented on code in PR #699:
URL: https://github.com/apache/activemq/pull/699#discussion_r967361744
##########
activemq-jaas/src/main/java/org/apache/activemq/jaas/LDAPLoginModule.java:
##########
@@ -450,7 +440,6 @@ protected boolean bindUser(DirContext context, String dn, String password) throw
log.debug("User " + dn + " successfully bound.");
}
} catch (AuthenticationException e) {
- isValid = false;
Review Comment:
Because it is `false` here and this assignment is not needed.
Logic hasn't been changed
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] mattrpav commented on pull request #699: AMQ-6148 Reusing LDAP context to avoid creating new connections
Posted by GitBox <gi...@apache.org>.
mattrpav commented on PR #699:
URL: https://github.com/apache/activemq/pull/699#issuecomment-1223298292
FWIW-- The open-close problem can also be solved with simply configuring LDAP to use the JDK built-in LDAP pooling.
Please rebase, and take a look at the comments about moving some clean-up tasks to finally {} blocks instead to prevent leaks or deadlock when pooling is used.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] Dm-Chebotarskyi commented on pull request #699: AMQ-6148 Reusing LDAP context to avoid creating new connections
Posted by GitBox <gi...@apache.org>.
Dm-Chebotarskyi commented on PR #699:
URL: https://github.com/apache/activemq/pull/699#issuecomment-1279236338
Thanks for the review @jbonofre .
I will squash and update the PR shortly
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] Dm-Chebotarskyi commented on pull request #699: AMQ-6148 Reusing LDAP context to avoid creating new connections
Posted by GitBox <gi...@apache.org>.
Dm-Chebotarskyi commented on PR #699:
URL: https://github.com/apache/activemq/pull/699#issuecomment-1223411074
> Please rebase, and take a look at the comments about moving some clean-up tasks to finally {} blocks instead to prevent leaks or deadlock.
Hey Matt, thanks for the quick review.
Could you please clarify what comments do you mean? I can't see any comments in the CR nor in the rebased code itself.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] Dm-Chebotarskyi commented on a diff in pull request #699: AMQ-6148 Reusing LDAP context to avoid creating new connections
Posted by GitBox <gi...@apache.org>.
Dm-Chebotarskyi commented on code in PR #699:
URL: https://github.com/apache/activemq/pull/699#discussion_r954141703
##########
activemq-jaas/src/main/java/org/apache/activemq/jaas/LDAPLoginModule.java:
##########
@@ -187,9 +183,13 @@ public boolean abort() throws LoginException {
return true;
}
- protected void close(DirContext context) {
+ protected void closeContext() {
+ if (context == null) {
+ return;
+ }
try {
context.close();
+ context = null;
Review Comment:
fixed
##########
activemq-jaas/src/main/java/org/apache/activemq/jaas/LDAPLoginModule.java:
##########
@@ -320,22 +316,17 @@ protected boolean authenticate(String username, String password) throws LoginExc
throw new FailedLoginException("Password does not match for user: " + username);
}
} catch (CommunicationException e) {
+ closeContext();
Review Comment:
fixed
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] Dm-Chebotarskyi commented on a diff in pull request #699: AMQ-6148 Reusing LDAP context to avoid creating new connections
Posted by GitBox <gi...@apache.org>.
Dm-Chebotarskyi commented on code in PR #699:
URL: https://github.com/apache/activemq/pull/699#discussion_r967360453
##########
activemq-jaas/src/main/java/org/apache/activemq/jaas/LDAPLoginModule.java:
##########
@@ -175,7 +171,7 @@ public boolean commit() throws LoginException {
public boolean abort() throws LoginException {
if (!succeeded) {
return false;
- } else if (succeeded && commitSucceeded) {
Review Comment:
succeeded is alway `true` in line 174. Logic hasn't been changed
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org