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 2021/07/08 17:05:33 UTC
[GitHub] [activemq-artemis] jbertram opened a new pull request #3649: ARTEMIS-3374 fix tests and shared subscriber case
jbertram opened a new pull request #3649:
URL: https://github.com/apache/activemq-artemis/pull/3649
A couple of tests were explicitly written to delete the
configuration-managed queue. Since this is no longer allowed these
tests had to change.
--
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 a change in pull request #3649: ARTEMIS-3374 fix tests and shared subscriber case
Posted by GitBox <gi...@apache.org>.
jbertram commented on a change in pull request #3649:
URL: https://github.com/apache/activemq-artemis/pull/3649#discussion_r666499250
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/server/SecureConfigurationTest.java
##########
@@ -148,20 +134,6 @@ public void testSecureDurableSubscriber() throws Exception {
} catch (JMSSecurityException j) {
//Expected exception
}
-
- try {
Review comment:
Actually, don't merge it yet. I'm investigating some other failures in the test-suite.
--
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 a change in pull request #3649: ARTEMIS-3374 fix tests and shared subscriber case
Posted by GitBox <gi...@apache.org>.
jbertram commented on a change in pull request #3649:
URL: https://github.com/apache/activemq-artemis/pull/3649#discussion_r666498394
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/server/SecureConfigurationTest.java
##########
@@ -148,20 +134,6 @@ public void testSecureDurableSubscriber() throws Exception {
} catch (JMSSecurityException j) {
//Expected exception
}
-
- try {
Review comment:
I created a new test to verify an unauthorized user cannot, in fact, delete a durable subscription queue by re-subscribing with a different selector and it failed which means that the original test was *not* testing this. I believe this PR can be merged as-is. I will send another PR to fix the insecure deletion problem.
--
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 a change in pull request #3649: ARTEMIS-3374 fix tests and shared subscriber case
Posted by GitBox <gi...@apache.org>.
jbertram commented on a change in pull request #3649:
URL: https://github.com/apache/activemq-artemis/pull/3649#discussion_r666498394
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/server/SecureConfigurationTest.java
##########
@@ -148,20 +134,6 @@ public void testSecureDurableSubscriber() throws Exception {
} catch (JMSSecurityException j) {
//Expected exception
}
-
- try {
Review comment:
I created a new test to verify an unauthorized user cannot, in fact, delete a durable subscription queue by re-subscribing with a different selector and it failed for AMQP which means that the original test was *not* actually testing this condition. I believe this PR can be merged as-is. I will send another PR to fix the insecure deletion problem.
--
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] clebertsuconic commented on a change in pull request #3649: ARTEMIS-3374 fix tests and shared subscriber case
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3649:
URL: https://github.com/apache/activemq-artemis/pull/3649#discussion_r666459529
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/server/SecureConfigurationTest.java
##########
@@ -148,20 +134,6 @@ public void testSecureDurableSubscriber() throws Exception {
} catch (JMSSecurityException j) {
//Expected exception
}
-
- try {
Review comment:
as long as we are still testing removing topic subscriptions and security elsewhere?
--
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] clebertsuconic commented on a change in pull request #3649: ARTEMIS-3374 fix tests and shared subscriber case
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3649:
URL: https://github.com/apache/activemq-artemis/pull/3649#discussion_r666382153
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/server/SecureConfigurationTest.java
##########
@@ -148,20 +134,6 @@ public void testSecureDurableSubscriber() throws Exception {
} catch (JMSSecurityException j) {
//Expected exception
}
-
- try {
Review comment:
why is this being removed? why the test is no longer valid?
--
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 a change in pull request #3649: ARTEMIS-3374 fix tests and shared subscriber case
Posted by GitBox <gi...@apache.org>.
jbertram commented on a change in pull request #3649:
URL: https://github.com/apache/activemq-artemis/pull/3649#discussion_r666457616
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/server/SecureConfigurationTest.java
##########
@@ -148,20 +134,6 @@ public void testSecureDurableSubscriber() throws Exception {
} catch (JMSSecurityException j) {
//Expected exception
}
-
- try {
Review comment:
FWIW, we already have tests for authorization on removing a queue.
--
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] clebertsuconic merged pull request #3649: ARTEMIS-3374 & ARTEMIS-3381
Posted by GitBox <gi...@apache.org>.
clebertsuconic merged pull request #3649:
URL: https://github.com/apache/activemq-artemis/pull/3649
--
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] clebertsuconic commented on a change in pull request #3649: ARTEMIS-3374 fix tests and shared subscriber case
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3649:
URL: https://github.com/apache/activemq-artemis/pull/3649#discussion_r666396721
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/server/SecureConfigurationTest.java
##########
@@ -148,20 +134,6 @@ public void testSecureDurableSubscriber() throws Exception {
} catch (JMSSecurityException j) {
//Expected exception
}
-
- try {
Review comment:
the test is about validating permissions on removing queues.
removeQueue is still a valid operation, isn't? on that case I think you have to change the test in a way it still validating removeQueue over security.
--
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 a change in pull request #3649: ARTEMIS-3374 fix tests and shared subscriber case
Posted by GitBox <gi...@apache.org>.
jbertram commented on a change in pull request #3649:
URL: https://github.com/apache/activemq-artemis/pull/3649#discussion_r666384773
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/server/SecureConfigurationTest.java
##########
@@ -148,20 +134,6 @@ public void testSecureDurableSubscriber() throws Exception {
} catch (JMSSecurityException j) {
//Expected exception
}
-
- try {
Review comment:
The explanation is in the commit message.
--
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] clebertsuconic commented on a change in pull request #3649: ARTEMIS-3374 fix tests and shared subscriber case
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3649:
URL: https://github.com/apache/activemq-artemis/pull/3649#discussion_r666395658
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/server/SecureConfigurationTest.java
##########
@@ -148,20 +134,6 @@ public void testSecureDurableSubscriber() throws Exception {
} catch (JMSSecurityException j) {
//Expected exception
}
-
- try {
Review comment:
shouldn't you change the test to use a destination that is not managed by the configuration?
perhaps play with both options through a parameterized test and validate both options?
--
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 a change in pull request #3649: ARTEMIS-3374 fix tests and shared subscriber case
Posted by GitBox <gi...@apache.org>.
jbertram commented on a change in pull request #3649:
URL: https://github.com/apache/activemq-artemis/pull/3649#discussion_r666456206
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/server/SecureConfigurationTest.java
##########
@@ -148,20 +134,6 @@ public void testSecureDurableSubscriber() throws Exception {
} catch (JMSSecurityException j) {
//Expected exception
}
-
- try {
Review comment:
The address and subscription queue are both managed in the configuration (i.e. they are configured in the XML). If you remove either of these from the XML then the test will fail because the user doesn't have permission to create the subscription queue. This appears to be the main point of the test, although it tests the security on removal as well (which is no longer valid).
--
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 a change in pull request #3649: ARTEMIS-3374 fix tests and shared subscriber case
Posted by GitBox <gi...@apache.org>.
jbertram commented on a change in pull request #3649:
URL: https://github.com/apache/activemq-artemis/pull/3649#discussion_r666599758
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/server/SecureConfigurationTest.java
##########
@@ -148,20 +134,6 @@ public void testSecureDurableSubscriber() throws Exception {
} catch (JMSSecurityException j) {
//Expected exception
}
-
- try {
Review comment:
I re-ran the test-suite and the previous failures are gone. I ran the failing tests locally and none failed (even after many repeated runs) except `org.apache.activemq.artemis.tests.integration.openwire.management.OpenWireDeleteQueueTest#testDestroyQueueClosingConsumers` which was added recently and only after after > 300 runs. It looks like it has a small race-condition, but it's not related to this commit from what I can tell.
--
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 a change in pull request #3649: ARTEMIS-3374 fix tests and shared subscriber case
Posted by GitBox <gi...@apache.org>.
jbertram commented on a change in pull request #3649:
URL: https://github.com/apache/activemq-artemis/pull/3649#discussion_r666599758
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/server/SecureConfigurationTest.java
##########
@@ -148,20 +134,6 @@ public void testSecureDurableSubscriber() throws Exception {
} catch (JMSSecurityException j) {
//Expected exception
}
-
- try {
Review comment:
I re-ran the test-suite and the previous failures are gone. I ran the failing tests locally and none failed (even after many repeated runs) except `org.apache.activemq.artemis.tests.integration.openwire.management.OpenWireDeleteQueueTest#testDestroyQueueClosingConsumers` which was added recently and only after after > 300 runs. It looks like it has a small race-condition, but it's not related to this commit from what I can tell. I believe this is safe to merge.
--
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] clebertsuconic commented on a change in pull request #3649: ARTEMIS-3374 fix tests and shared subscriber case
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3649:
URL: https://github.com/apache/activemq-artemis/pull/3649#discussion_r666403304
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/server/SecureConfigurationTest.java
##########
@@ -148,20 +134,6 @@ public void testSecureDurableSubscriber() throws Exception {
} catch (JMSSecurityException j) {
//Expected exception
}
-
- try {
Review comment:
@jbertram isn't the test valid anyways?
This is about creating a queue for the subscription. Meaning the topic is managed, but not the subscription. it should still throw an error or manage the creation of the queue correct.
--
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