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/06/02 18:38:50 UTC
[GitHub] [activemq-artemis] Yesenkov opened a new pull request, #4102: Update ActiveMQServerImpl.java
Yesenkov opened a new pull request, #4102:
URL: https://github.com/apache/activemq-artemis/pull/4102
This fixes the situation in ActiveMQServerImpl.createSession () ActiveMQServerImpl.checkSessionLimit () is called with the ServerSessionImpl.validatedUser parameter, but to count the list of sessions, the ActiveMQServerImpl.getSessionCountForUser () method is called, which already iterates over the sessions, ServerSessionImpl.getUsername () is called. In the case of certificate authentication via the TextFileCertificateLoginModule, getUsername() is always null for client connections, while ServerSessionImpl.validatedUser is set to the normal user ID via the call to securityStore.authenticate() at the very beginning of the ActiveMQServerImpl.createSession() method.
--
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-artemis] jbertram commented on pull request #4102: Update ActiveMQServerImpl.java
Posted by GitBox <gi...@apache.org>.
jbertram commented on PR #4102:
URL: https://github.com/apache/activemq-artemis/pull/4102#issuecomment-1182489452
I'm replacing this PR with #4146.
--
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-artemis] jbertram closed pull request #4102: Update ActiveMQServerImpl.java
Posted by GitBox <gi...@apache.org>.
jbertram closed pull request #4102: Update ActiveMQServerImpl.java
URL: https://github.com/apache/activemq-artemis/pull/4102
--
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-artemis] jbertram commented on pull request #4102: Update ActiveMQServerImpl.java
Posted by GitBox <gi...@apache.org>.
jbertram commented on PR #4102:
URL: https://github.com/apache/activemq-artemis/pull/4102#issuecomment-1145319072
This really needs a test to validate the fix and also to ensure there are no regressions in the future.
Basic tests for resource limits are at `org.apache.activemq.artemis.tests.integration.server.ResourceLimitTest`. A test using certificates is at `org.apache.activemq.artemis.tests.integration.security.SecurityTest#testJAASSecurityManagerAuthenticationWithCerts()`. Take bits and pieces from both and create a new class `org.apache.activemq.artemis.tests.integration.server.ResourceLimitTestWithCerts`.
--
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-artemis] Yesenkov commented on pull request #4102: Update ActiveMQServerImpl.java
Posted by GitBox <gi...@apache.org>.
Yesenkov commented on PR #4102:
URL: https://github.com/apache/activemq-artemis/pull/4102#issuecomment-1147542823
> ActiveMQServerImpl.getSessionCountForUser
done
--
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-artemis] gemmellr commented on pull request #4102: Update ActiveMQServerImpl.java
Posted by GitBox <gi...@apache.org>.
gemmellr commented on PR #4102:
URL: https://github.com/apache/activemq-artemis/pull/4102#issuecomment-1148376313
This PR is targetting 2.19.x, it should instead target main (if it were to be backported the change would then be picked from main; however there are no plans to do further 2.19.x releases).
The test should be added in the same commit as the change, i.e in this PR, rather than in a seperate PR as you have created (where you actually did target main). You can update a PR by force pushing to the same PR branch after you have made the needed changes, no need to close and reopen.
Aside, at the very least your test is missing the licence header, which failed the Travis CI build, although it looks like there may be other style issues as well. You can enable the GitHub Actions based CI jobs in your own fork repo (see the Actions tab) which has more granular checks, and use it to run the jobs on your own fork before opening/updating a PR (since you already opened this one, you would need to use a seperate testing branch to pre-test your updates before updating the original branch).
--
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-artemis] gemmellr commented on pull request #4102: Update ActiveMQServerImpl.java
Posted by GitBox <gi...@apache.org>.
gemmellr commented on PR #4102:
URL: https://github.com/apache/activemq-artemis/pull/4102#issuecomment-1148613043
Im not sure what part of my comment was unclear, but again more succinctly:
- Update *this* PR as requested when feedback is given, dont create new ones.
- Initial PRs should always target *main*, not branches.
- There are no plans to release another 2.19.x so nothing should be targetting it regardless.
--
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-artemis] Yesenkov commented on pull request #4102: Update ActiveMQServerImpl.java
Posted by GitBox <gi...@apache.org>.
Yesenkov commented on PR #4102:
URL: https://github.com/apache/activemq-artemis/pull/4102#issuecomment-1148673393
> Im not sure what part of my comment was unclear, but again more succinctly:
>
> * Update _this_ PR as requested when feedback is given, dont create new ones.
> * Initial PRs should always target _main_, not branches.
> * There are no plans to release another 2.19.x so nothing should be targetting it regardless.
Ok, it's clear now.
I changed the patch and test branch to main.
--
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-artemis] Yesenkov commented on pull request #4102: Update ActiveMQServerImpl.java
Posted by GitBox <gi...@apache.org>.
Yesenkov commented on PR #4102:
URL: https://github.com/apache/activemq-artemis/pull/4102#issuecomment-1148592935
> This PR is targetting 2.19.x, it should instead target main (if it were to be backported the change would then be picked from main; however there are no plans to do further 2.19.x releases).
>
> The test should be added in the same commit as the change, i.e in this PR, rather than in a seperate PR as you have created (where you actually did target main). You can update a PR by force pushing to the same PR branch after you have made the needed changes, no need to close and reopen.
>
> Aside, at the very least your test is missing the licence header, which failed the Travis CI build, although it looks like there may be other style issues as well. You can enable the GitHub Actions based CI jobs in your own fork repo (see the Actions tab) which has more granular checks, and use it to run the jobs on your own fork before opening/updating a PR (since you already opened this one, you would need to use a seperate testing branch to pre-test your updates before updating the original branch).
I made new PR.
But, main branch contain same problem, possible need same test.
--
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