Details

    • Type: Task Task
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.10
    • Fix Version/s: 0.13
    • Component/s: Java Broker
    • Labels:
      None

      Description

      This task is to refactor the broker to pass through a Subject from the authentication layer downwards, rather than a Principal. The motivation for this change is to allow the security modules to make decisions based on all principals (including Group principals) rather than merely the UsernamePrincipal.

      This task will support QPID-3283.

        Issue Links

          Activity

          Hide
          Keith Wall added a comment -

          Hi Alex

          Could you review the attached patch?

          cheers Keith

          Show
          Keith Wall added a comment - Hi Alex Could you review the attached patch? cheers Keith
          Hide
          Alex Rudyy added a comment -

          Keith,

          I reviewed your patch. It seems Ok for me.

          The only question I have regarding your work is

          Would not it be better to get Subject object in ServerSession from a Connection object like in example below

          public Subject getAuthorizedSubject()

          { return ((ServerConnection)getConnection()).getSubject(); }

          rather then keeping a reference to Subject in ServerSession field?

          Show
          Alex Rudyy added a comment - Keith, I reviewed your patch. It seems Ok for me. The only question I have regarding your work is Would not it be better to get Subject object in ServerSession from a Connection object like in example below public Subject getAuthorizedSubject() { return ((ServerConnection)getConnection()).getSubject(); } rather then keeping a reference to Subject in ServerSession field?
          Hide
          Robbie Gemmell added a comment -

          This issue now also incorporates QPID-3267, see the linked JIRA for details.

          Show
          Robbie Gemmell added a comment - This issue now also incorporates QPID-3267 , see the linked JIRA for details.
          Hide
          Keith Wall added a comment -

          This patch replaces my previous. It addresses the comments raised by Alex, and I've also renamed PrincipalHolder to AuthorisationHolder, as I believe it more accurately represents its purpose.

          Hi Alex, could you take another look please?

          Show
          Keith Wall added a comment - This patch replaces my previous. It addresses the comments raised by Alex, and I've also renamed PrincipalHolder to AuthorisationHolder, as I believe it more accurately represents its purpose. Hi Alex, could you take another look please?
          Hide
          Keith Wall added a comment -

          Hi Alex

          As discussed, I've incorporated your latest comments and recreated the patch.

          Thanks Keith

          Show
          Keith Wall added a comment - Hi Alex As discussed, I've incorporated your latest comments and recreated the patch. Thanks Keith
          Hide
          Alex Rudyy added a comment -

          Keith,

          Thanks for changes. Patch is fine for me.

          Show
          Alex Rudyy added a comment - Keith, Thanks for changes. Patch is fine for me.
          Hide
          Keith Wall added a comment -

          Hi Robbie,

          I've addressed your review comments and re-cut the patch.

          cheers Keith

          Show
          Keith Wall added a comment - Hi Robbie, I've addressed your review comments and re-cut the patch. cheers Keith
          Hide
          Robbie Gemmell added a comment -

          Woops, I didn't post the comments on the JIRA when I sent you them. For anyone later wondering, they were:

          The newly introduced for loop in AccessControl to validate rights depends on the ordering of rules checked to ensure the correct result, and so may return the wrong result if the iterator is not returning them in the appropriate order.

          There are a couple of code style issues with braces not on new lines.

          In the new control flow added in ServerConnection#setAuthorizedSubject(), whilst actually functional, looks uninentially odd due to checking things are null and then assigning them to be null once they are known to be.

          Show
          Robbie Gemmell added a comment - Woops, I didn't post the comments on the JIRA when I sent you them. For anyone later wondering, they were: The newly introduced for loop in AccessControl to validate rights depends on the ordering of rules checked to ensure the correct result, and so may return the wrong result if the iterator is not returning them in the appropriate order. There are a couple of code style issues with braces not on new lines. In the new control flow added in ServerConnection#setAuthorizedSubject(), whilst actually functional, looks uninentially odd due to checking things are null and then assigning them to be null once they are known to be.
          Hide
          Robbie Gemmell added a comment -

          Patch applied.

          Show
          Robbie Gemmell added a comment - Patch applied.

            People

            • Assignee:
              Robbie Gemmell
              Reporter:
              Keith Wall
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development