ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-48

AUTH_ID not handled correctly when no auth ids are present

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.0.0
    • Component/s: server
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      AUTH_ID is used (usually done using Ids.CREATOR_ALL_ACL ) to represent the id that was used to authenticate with ZooKeeper. Thus, an exception should be raised if there are no authenticated ids present. Currently, the exception is not being raised.

      1. acl_4.patch
        7 kB
        Benjamin Reed

        Issue Links

          Activity

          Benjamin Reed created issue -
          Benjamin Reed made changes -
          Field Original Value New Value
          Attachment acl.patch [ 12384344 ]
          Hide
          Benjamin Reed added a comment -

          The last patch did not throw an exception if AUTH_ID was used on a non authenticated connection and there were other Ids in the list. The patch fixes the issue adds a test to verify and also fixes some overly severe logging problems.

          Show
          Benjamin Reed added a comment - The last patch did not throw an exception if AUTH_ID was used on a non authenticated connection and there were other Ids in the list. The patch fixes the issue adds a test to verify and also fixes some overly severe logging problems.
          Benjamin Reed made changes -
          Attachment acl-2.patch [ 12384345 ]
          Benjamin Reed made changes -
          Attachment acl.patch [ 12384344 ]
          Hide
          Benjamin Reed added a comment -

          Since the fix for this issue overlaps with ZOOKEEPER-49 and since the fix and test for ZOOKEEPER-49 are so trival, I've included them in this patch.

          Show
          Benjamin Reed added a comment - Since the fix for this issue overlaps with ZOOKEEPER-49 and since the fix and test for ZOOKEEPER-49 are so trival, I've included them in this patch.
          Benjamin Reed made changes -
          Attachment acl_3.patch [ 12384350 ]
          Hide
          Patrick Hunt added a comment -

          I reviewed the patch, great to see that we are adding a test for this.

          Some comments:
          1) if attached patch3 supersedes patch 2 please remove 2.
          2) could you add documentation for "fixupACL" method? there's quite a bit going on in that method and it's not really clear what the contract is.
          3) line 409 (fixupacl method) logs error for "missing authenciation provider...", is this really an error? (no exception thrown as a result...) should we be notifying the client in this case (might help with client side debugging. Perhaps a new jira for this?
          4) line 65 preprequestprocessor.java, could we log an INFO message here that states that acl checking is being skipped (might help w/debugging down the road).

          Show
          Patrick Hunt added a comment - I reviewed the patch, great to see that we are adding a test for this. Some comments: 1) if attached patch3 supersedes patch 2 please remove 2. 2) could you add documentation for "fixupACL" method? there's quite a bit going on in that method and it's not really clear what the contract is. 3) line 409 (fixupacl method) logs error for "missing authenciation provider...", is this really an error? (no exception thrown as a result...) should we be notifying the client in this case (might help with client side debugging. Perhaps a new jira for this? 4) line 65 preprequestprocessor.java, could we log an INFO message here that states that acl checking is being skipped (might help w/debugging down the road).
          Benjamin Reed made changes -
          Attachment acl-2.patch [ 12384345 ]
          Hide
          Benjamin Reed added a comment -

          1) done
          2) will do once we have a repository to check into so that I can construct a proper patch
          3) we should probably open a new jira. I'm not really sure what the correct behavior is...
          4) this would create a large number of messages in the log. I would think we would only log it once at startup.

          Show
          Benjamin Reed added a comment - 1) done 2) will do once we have a repository to check into so that I can construct a proper patch 3) we should probably open a new jira. I'm not really sure what the correct behavior is... 4) this would create a large number of messages in the log. I would think we would only log it once at startup.
          Hide
          Patrick Hunt added a comment -

          4) this line is in the static initializer for the class - so it will typically be called once per run of the vm

          Show
          Patrick Hunt added a comment - 4) this line is in the static initializer for the class - so it will typically be called once per run of the vm
          Hide
          Benjamin Reed added a comment -

          Oh right. Sorry, I wasn't looking at the line number close enough. I'll fix 4) as well in the next patch.

          Show
          Benjamin Reed added a comment - Oh right. Sorry, I wasn't looking at the line number close enough. I'll fix 4) as well in the next patch.
          Hide
          Patrick Hunt added a comment -

          Ben what's the status on this one? Is it ready for review or further work needs to be done?

          Show
          Patrick Hunt added a comment - Ben what's the status on this one? Is it ready for review or further work needs to be done?
          Patrick Hunt made changes -
          Assignee Benjamin Reed [ breed ]
          Benjamin Reed made changes -
          Link This issue blocks ZOOKEEPER-49 [ ZOOKEEPER-49 ]
          Hide
          Benjamin Reed added a comment -

          Added documentation to fixupACL and added an INFO message if ACLs are skipped.

          Show
          Benjamin Reed added a comment - Added documentation to fixupACL and added an INFO message if ACLs are skipped.
          Benjamin Reed made changes -
          Attachment acl_4.patch [ 12386560 ]
          Benjamin Reed made changes -
          Attachment acl_3.patch [ 12384350 ]
          Benjamin Reed made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Patrick Hunt made changes -
          Hadoop Flags [Reviewed]
          Hide
          Patrick Hunt added a comment -

          Committed revision 681465.

          Show
          Patrick Hunt added a comment - Committed revision 681465.
          Patrick Hunt made changes -
          Fix Version/s 3.0.0 [ 12313216 ]
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Hide
          Hudson added a comment -

          Integrated in ZooKeeper-trunk #42 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/42/)

          Show
          Hudson added a comment - Integrated in ZooKeeper-trunk #42 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/42/ )
          Hide
          Patrick Hunt added a comment -

          3.0.0 has been released, closing issues.

          Show
          Patrick Hunt added a comment - 3.0.0 has been released, closing issues.
          Patrick Hunt made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Gavin made changes -
          Link This issue blocks ZOOKEEPER-49 [ ZOOKEEPER-49 ]
          Gavin made changes -
          Link This issue is depended upon by ZOOKEEPER-49 [ ZOOKEEPER-49 ]
          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Patch Available Patch Available
          31d 12h 58m 1 Benjamin Reed 21/Jul/08 20:01
          Patch Available Patch Available Resolved Resolved
          10d 1h 9m 1 Patrick Hunt 31/Jul/08 21:11
          Resolved Resolved Closed Closed
          86d 4h 59m 1 Patrick Hunt 26/Oct/08 01:10

            People

            • Assignee:
              Benjamin Reed
              Reporter:
              Benjamin Reed
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development