Skip to content
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

Update ActiveMQServerImpl.java #4102

Closed
wants to merge 91 commits into from
Closed

Update ActiveMQServerImpl.java #4102

wants to merge 91 commits into from

Conversation

Yesenkov
Copy link
Contributor

@Yesenkov Yesenkov commented Jun 2, 2022

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.
Test org.apache.activemq.artemis.tests.integration.server.ResourceLimitTestWithCerts.java

clebertsuconic and others added 30 commits January 14, 2022 08:54
(cherry picked from commit 95aa20b)
…ased and quite involved

(cherry picked from commit 6f4c609)
…on to core on AMQP Messages on console browsing

Done in collaboration with Erwin Dondorp through apache#3794

(cherry picked from commit a833d95)
Skip backup connector equivalent to cluster connector for cluster connections.

(cherry picked from commit dca3fac)
…onnection

add bridge connection verification after stop cluster connection
to wait for the bridge stop completely and avoid a failure
on stop/start cluster connection test

(cherry picked from commit 7791a26)
Commit 481b73c from ARTEMIS-3502
inadvertently broke this functionality. This commit restores the
original behavior.

autoDeleteAddress was renamed to forceAutoDeleteAddress which will ignore the address settings.

delete temporary queues will use forceAutoDeleteAddress=true.

this is done in collaboration with Justin Bertram

(cherry picked from commit 1d0c0a8)
Erwin Dondorp and others added 21 commits January 24, 2022 10:09
Move to the latest release of Hawtio 2.14.x.

Also, remove the Log4j archives from the web console application. It's
not necessary to include Log4j archives because Hawtio itself uses SLF4J
and the logging implementation will be provided by the broker runtime.
We already do this for SLF4J.

While not strictly necessary, removing Log4j will ease concerns about
security issues such as the recently announced CVE-2021-44228.

(cherry picked from commit 2e3c69c)
(cherry picked from commit b65c845)
(cherry picked from commit 97378a6)
(cherry picked from commit 0722b3c)
(cherry picked from commit 3af6d0d)
Bumps version to 3.8.5 to avoid errors when building
         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.
@jbertram
Copy link
Contributor

jbertram commented Jun 2, 2022

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.

@Yesenkov
Copy link
Contributor Author

Yesenkov commented Jun 6, 2022

ActiveMQServerImpl.getSessionCountForUser

done

@gemmellr
Copy link
Member

gemmellr commented Jun 7, 2022

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).

@Yesenkov
Copy link
Contributor Author

Yesenkov commented Jun 7, 2022

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.

@gemmellr
Copy link
Member

gemmellr commented Jun 7, 2022

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.

@Yesenkov Yesenkov changed the base branch from 2.19.x to main June 7, 2022 13:13
@Yesenkov
Copy link
Contributor Author

Yesenkov commented Jun 7, 2022

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.

@jbertram
Copy link
Contributor

I'm replacing this PR with #4146.

@jbertram jbertram closed this Jul 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.