Qpid
  1. Qpid
  2. QPID-3964

Incorrect ACL checks for passive declares

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.16
    • Fix Version/s: 0.17
    • Component/s: C++ Broker
    • Labels:
      None

      Description

      The broker checks for a 'create' permission when responding to a passive declare. This is not correct as a passive declare explicitly does not create the exchange/queue in question.

        Activity

        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4827/#review7067
        -----------------------------------------------------------

        Ship it!

        The reasoning behind this is clear enough. In the checkin comment please also describe the deletion of the 'passive' property.

        • Chug

        On 2012-04-20 10:38:09, Gordon Sim wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4827/

        -----------------------------------------------------------

        (Updated 2012-04-20 10:38:09)

        Review request for Ted Ross, Chug Rolke and rajith attapattu.

        Summary

        -------

        At present, the broker enforces the create permission for passive declares, albeit with the option of distinguishing that case through a value for the 'passive' property. This is unintuitive and causes confusion. The attached change removes the 'passive' property from the 'create' actions, and enforces the 'access' action instead for passive declares. As a passive declare is similar in nature to Queue- or Exchange- Query, this is more consistent.

        Note however that this change would not be backwards compatible for all possible ACLs. I can't see any case where the required change to the ACL would not be an improvement, but important to recignise that a change may in some cases be required.

        This addresses bug QPID-3964.

        https://issues.apache.org/jira/browse/QPID-3964

        Diffs

        -----

        /trunk/qpid/cpp/src/qpid/broker/AclModule.h 1328252

        /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1328252

        /trunk/qpid/cpp/src/qpid/broker/SessionAdapter.cpp 1328252

        /trunk/qpid/cpp/src/tests/acl.py 1328252

        Diff: https://reviews.apache.org/r/4827/diff

        Testing

        -------

        Fixed existing tests to cover the new approach; make check passes.

        Thanks,

        Gordon

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4827/#review7067 ----------------------------------------------------------- Ship it! The reasoning behind this is clear enough. In the checkin comment please also describe the deletion of the 'passive' property. Chug On 2012-04-20 10:38:09, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4827/ ----------------------------------------------------------- (Updated 2012-04-20 10:38:09) Review request for Ted Ross, Chug Rolke and rajith attapattu. Summary ------- At present, the broker enforces the create permission for passive declares, albeit with the option of distinguishing that case through a value for the 'passive' property. This is unintuitive and causes confusion. The attached change removes the 'passive' property from the 'create' actions, and enforces the 'access' action instead for passive declares. As a passive declare is similar in nature to Queue- or Exchange- Query, this is more consistent. Note however that this change would not be backwards compatible for all possible ACLs. I can't see any case where the required change to the ACL would not be an improvement, but important to recignise that a change may in some cases be required. This addresses bug QPID-3964 . https://issues.apache.org/jira/browse/QPID-3964 Diffs ----- /trunk/qpid/cpp/src/qpid/broker/AclModule.h 1328252 /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1328252 /trunk/qpid/cpp/src/qpid/broker/SessionAdapter.cpp 1328252 /trunk/qpid/cpp/src/tests/acl.py 1328252 Diff: https://reviews.apache.org/r/4827/diff Testing ------- Fixed existing tests to cover the new approach; make check passes. Thanks, Gordon
        Hide
        Gordon Sim added a comment -

        There is a 'passive' property that can be specified to give explicit permission for passive declares. That suggests that the current scheme was at some level intentional. It requires a 'create' permission with property 'passive' set to true however, which is odd. A passive declare is much like e.g. a query which requires the 'access' permission.

        Show
        Gordon Sim added a comment - There is a 'passive' property that can be specified to give explicit permission for passive declares. That suggests that the current scheme was at some level intentional. It requires a 'create' permission with property 'passive' set to true however, which is odd. A passive declare is much like e.g. a query which requires the 'access' permission.

          People

          • Assignee:
            Gordon Sim
            Reporter:
            Gordon Sim
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development