Qpid
  1. Qpid
  2. QPID-3428

Verification of unique client ID does not work with Java Broker

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.10
    • Fix Version/s: 0.13
    • Component/s: Java Broker, Java Client
    • Labels:
      None

      Description

      On settings client JVM property "qpid.verify_client_id" to "true" Qpid broker should not allow creation of connections with the same client id. However, this functionality does not work at the moment.

      JUnit test org.apache.qpid.test.unit.client.connection.ConnectionTest#testClientIDVerification fails with the following message:
      "The client should throw a ConnectionException stating the client ID is not unique"

      junit.framework.AssertionFailedError: The client should throw a ConnectionException stating the client ID is not unique
      at org.apache.qpid.test.unit.client.connection.ConnectionTest.testClientIDVerification(ConnectionTest.java:307)
      at org.apache.qpid.test.utils.QpidBrokerTestCase.runBare(QpidBrokerTestCase.java:243)
      at org.apache.qpid.test.utils.QpidTestCase.run(QpidTestCase.java:125)

        Activity

        Alex Rudyy created issue -
        Hide
        Andrew MacBean added a comment -

        Based on test case and implementation in Java client and C++ broker for (QPID-3269) the broker has been changed to validate that the client id used is unique when creating a new session and attaching.

        If the client id is null or is unique then the session is attached as before, otherwise it invokes session detached and sets the session detach code to SESSION_BUSY and calls session.closed().

        A patch has been attached for review. The patch also includes some minor client changes to fix a small bug found during development in the Session.awaitOpen() method which was throwing a SessionException even if the state was OPEN as expected. This was "swallowed" later so had no real knock on effect anyway.

        Show
        Andrew MacBean added a comment - Based on test case and implementation in Java client and C++ broker for ( QPID-3269 ) the broker has been changed to validate that the client id used is unique when creating a new session and attaching. If the client id is null or is unique then the session is attached as before, otherwise it invokes session detached and sets the session detach code to SESSION_BUSY and calls session.closed(). A patch has been attached for review. The patch also includes some minor client changes to fix a small bug found during development in the Session.awaitOpen() method which was throwing a SessionException even if the state was OPEN as expected. This was "swallowed" later so had no real knock on effect anyway.
        Andrew MacBean made changes -
        Field Original Value New Value
        Attachment [QPID-3428]-verifification-of-unique-client-ID-does-not-work-in-Java-Broker.patch [ 12492590 ]
        Hide
        Andrew MacBean added a comment -

        Robbie, could you please review the attached patch and apply if suitable.

        Thanks
        Andrew

        Show
        Andrew MacBean added a comment - Robbie, could you please review the attached patch and apply if suitable. Thanks Andrew
        Andrew MacBean made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Andrew MacBean made changes -
        Resolution Fixed [ 1 ]
        Status Resolved [ 5 ] Reopened [ 4 ]
        Andrew MacBean made changes -
        Assignee Robbie Gemmell [ gemmellr ] Andrew MacBean [ macbean ]
        Andrew MacBean made changes -
        Status Reopened [ 4 ] In Progress [ 3 ]
        Andrew MacBean made changes -
        Status In Progress [ 3 ] Ready To Review [ 10006 ]
        Andrew MacBean made changes -
        Assignee Andrew MacBean [ macbean ] Robbie Gemmell [ gemmellr ]
        Hide
        Robbie Gemmell added a comment -

        Changes look good overall, but theres a some bits I think could be improved:

        In ServerConnectionDelegate.sessionAttach, the clientId (sessionName, rather?) variable can never be null so the check is redundant.Also, in the new 'isSessionNameUnique' method, we can probably be mroe efficient than iterating over every Session in the broker and comparing ClientIDs. Each Connection object has a Map of session names to their Session, so it should be possible to just ask query connection whether it has a Session with a given name, with the 0-8/9/9-1 connections simply returning false since they dont support that.

        In AMQConnectionDelegate / AMQConnection, it seems like it would be better to use a boolean for the result normally and rethrow any unexpected exceptions, rather than detecting whether or not there is a linked exception set to distinguish between two JMSExceptions.

        Finally, in ConnectionTest you could use the setTestSystemProperty() method from QpidTestCase method to update and revert the 'qpid.verify_client_id' property (which should probably be a constant; there should be one in ClientProperties, but if there isnt it needs added).

        Show
        Robbie Gemmell added a comment - Changes look good overall, but theres a some bits I think could be improved: In ServerConnectionDelegate.sessionAttach, the clientId (sessionName, rather?) variable can never be null so the check is redundant.Also, in the new 'isSessionNameUnique' method, we can probably be mroe efficient than iterating over every Session in the broker and comparing ClientIDs. Each Connection object has a Map of session names to their Session, so it should be possible to just ask query connection whether it has a Session with a given name, with the 0-8/9/9-1 connections simply returning false since they dont support that. In AMQConnectionDelegate / AMQConnection, it seems like it would be better to use a boolean for the result normally and rethrow any unexpected exceptions, rather than detecting whether or not there is a linked exception set to distinguish between two JMSExceptions. Finally, in ConnectionTest you could use the setTestSystemProperty() method from QpidTestCase method to update and revert the 'qpid.verify_client_id' property (which should probably be a constant; there should be one in ClientProperties, but if there isnt it needs added).
        Robbie Gemmell made changes -
        Status Ready To Review [ 10006 ] Open [ 1 ]
        Assignee Robbie Gemmell [ gemmellr ] Andrew MacBean [ macbean ]
        Robbie Gemmell made changes -
        Summary Verifification of unique client ID does not work in Java Broker Verification of unique client ID does not work with Java Broker
        Robbie Gemmell made changes -
        Fix Version/s 0.13 [ 12316854 ]
        Hide
        Andrew MacBean added a comment -

        Review comments actioned, please review.

        Show
        Andrew MacBean added a comment - Review comments actioned, please review.
        Andrew MacBean made changes -
        Assignee Andrew MacBean [ macbean ] Robbie Gemmell [ gemmellr ]
        Hide
        Robbie Gemmell added a comment -

        Looks good, but I missed something in my original review that still needs addressed. The 0-10 session naming should be per-principal, so currently the verification being done is too strict in that it checks only if the name exists, not whether it is for the same principal, so we need to relax that accordingly.

        Show
        Robbie Gemmell added a comment - Looks good, but I missed something in my original review that still needs addressed. The 0-10 session naming should be per-principal, so currently the verification being done is too strict in that it checks only if the name exists, not whether it is for the same principal, so we need to relax that accordingly.
        Robbie Gemmell made changes -
        Assignee Robbie Gemmell [ gemmellr ] Andrew MacBean [ macbean ]
        Hide
        Andrew MacBean added a comment -

        Checked this vs the C++ broker and you were right, modified the patch and added a new test to highlight the difference.

        Show
        Andrew MacBean added a comment - Checked this vs the C++ broker and you were right, modified the patch and added a new test to highlight the difference.
        Andrew MacBean made changes -
        Status Open [ 1 ] In Progress [ 3 ]
        Andrew MacBean made changes -
        Status In Progress [ 3 ] Ready To Review [ 10006 ]
        Andrew MacBean made changes -
        Assignee Andrew MacBean [ macbean ] Robbie Gemmell [ gemmellr ]
        Hide
        Robbie Gemmell added a comment -

        Patch applied, along with a fix for the excludes file and some small test tweaks.

        Show
        Robbie Gemmell added a comment - Patch applied, along with a fix for the excludes file and some small test tweaks.
        Robbie Gemmell made changes -
        Status Ready To Review [ 10006 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        Robbie Gemmell added a comment -

        Reopening, investigation of an apparent memory leak has shown the previous changes result in leaking the sessions+connections in the management layer by causing duplicate registrations to occur.

        Show
        Robbie Gemmell added a comment - Reopening, investigation of an apparent memory leak has shown the previous changes result in leaking the sessions+connections in the management layer by causing duplicate registrations to occur.
        Robbie Gemmell made changes -
        Resolution Fixed [ 1 ]
        Status Resolved [ 5 ] Reopened [ 4 ]
        Hide
        Robbie Gemmell added a comment -

        Updated to prevent the duplicate 0-10 ServerSession creations + registrations.

        Show
        Robbie Gemmell added a comment - Updated to prevent the duplicate 0-10 ServerSession creations + registrations.
        Robbie Gemmell made changes -
        Status Reopened [ 4 ] Closed [ 6 ]
        Resolution Fixed [ 1 ]

          People

          • Assignee:
            Robbie Gemmell
            Reporter:
            Alex Rudyy
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development