ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-1525

Plumb ZooKeeperServer object into auth plugins

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.5.0
    • Fix Version/s: 3.6.0
    • Component/s: None
    • Labels:
      None
    • Release Note:
      Plumb ZooKeeperServer object into auth plugins.

      Description

      I want to plumb the ZooKeeperServer object into the auth plugins so that I can store authentication data in zookeeper itself. With access to the ZooKeeperServer object, I also have access to the ZKDatabase and can look up entries in the local copy of the zookeeper data.

      In order to implement this, I make sure that a ZooKeeperServer instance is passed in to the ProviderRegistry.initialize() method. Then initialize() will try to find a constructor for the AuthenticationProvider that takes a ZooKeeperServer instance. If the constructor is found, it will be used. Otherwise, initialize() will look for a constructor that takes no arguments and use that instead.

      1. ZOOKEEPER-1525.patch
        37 kB
        Jordan Zimmerman
      2. ZOOKEEPER-1525.patch
        37 kB
        Jordan Zimmerman
      3. ZOOKEEPER-1525.patch
        36 kB
        Jordan Zimmerman
      4. ZOOKEEPER-1525.patch
        36 kB
        Jordan Zimmerman
      5. ZOOKEEPER-1525.patch
        36 kB
        Jordan Zimmerman
      6. ZOOKEEPER-1525.patch
        19 kB
        Tim Crowder
      7. ZOOKEEPER-1525.patch
        18 kB
        Tim Crowder
      8. ZOOKEEPER-1525.patch
        18 kB
        Tim Crowder
      9. ZOOKEEPER-1525.patch
        18 kB
        Tim Crowder
      10. ZOOKEEPER-1525.patch
        17 kB
        Tim Crowder
      11. ZOOKEEPER-1525.patch
        6 kB
        Tim Crowder
      12. ZOOKEEPER-1525.patch
        5 kB
        Warren Turkal

        Issue Links

          Activity

          Hide
          Warren Turkal added a comment -

          Convert tabs to spaces.

          Show
          Warren Turkal added a comment - Convert tabs to spaces.
          Hide
          Warren Turkal added a comment -

          Fixed some more whitespace issues. Improved type safety of reflection code.

          Show
          Warren Turkal added a comment - Fixed some more whitespace issues. Improved type safety of reflection code.
          Hide
          Eugene Koontz added a comment -

          Hi Warren,

          I'm not sure why the Jenkins build did not get triggered here on your patch.

          Although, even if Jenkins was triggered, I'm afraid this patch might not work with Jenkins because it has "a/" and "b/" paths in the patch. I think you need to add

          [diff]
               noprefix = true
          

          to your ~/.gitconfig.

          Show
          Eugene Koontz added a comment - Hi Warren, I'm not sure why the Jenkins build did not get triggered here on your patch. Although, even if Jenkins was triggered, I'm afraid this patch might not work with Jenkins because it has "a/" and "b/" paths in the patch. I think you need to add [diff] noprefix = true to your ~/.gitconfig.
          Hide
          Warren Turkal added a comment -

          I am working on making the change to the patch now.

          However, would it be possible to make the jenkins job accept git patches as is so that folks using the apache git won't have to remember to adopt a special setting to submit patches? For example, could the job try -p1 if -p0 fails when patching in the diff?

          Show
          Warren Turkal added a comment - I am working on making the change to the patch now. However, would it be possible to make the jenkins job accept git patches as is so that folks using the apache git won't have to remember to adopt a special setting to submit patches? For example, could the job try -p1 if -p0 fails when patching in the diff?
          Hide
          Warren Turkal added a comment -

          updated patch

          Show
          Warren Turkal added a comment - updated patch
          Hide
          Warren Turkal added a comment -

          This still doesn't appear to have triggered a jenkins build. What's wrong?

          Show
          Warren Turkal added a comment - This still doesn't appear to have triggered a jenkins build. What's wrong?
          Hide
          Warren Turkal added a comment -

          Patrick Hunt, Is assigning this to you the right way to move this issue forward?

          Show
          Warren Turkal added a comment - Patrick Hunt , Is assigning this to you the right way to move this issue forward?
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12539665/ZOOKEEPER-1525.patch
          against trunk revision 1373156.

          +1 @author. The patch does not contain any @author tags.

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1159//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1159//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1159//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12539665/ZOOKEEPER-1525.patch against trunk revision 1373156. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1159//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1159//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1159//console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12539665/ZOOKEEPER-1525.patch
          against trunk revision 1530166.

          +1 @author. The patch does not contain any @author tags.

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1658//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1658//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1658//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12539665/ZOOKEEPER-1525.patch against trunk revision 1530166. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1658//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1658//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1658//console This message is automatically generated.
          Hide
          Michi Mutsuzaki added a comment -

          Hi Warren,

          Sorry for super-slow response. I have 2 comments:

          1. Could you fix these warnings?

          compile:
              [javac] Compiling 3 source files to /home/michi/svndev/zookeeper/trunk/build/classes
              [javac] warning: [options] bootstrap class path not set in conjunction with -source 1.5
              [javac] /home/michi/svndev/zookeeper/trunk/src/java/main/org/apache/zookeeper/server/auth/ProviderRegistry.java:57: warning: [cast] redundant cast to AuthenticationProvider
              [javac]                             ap = (AuthenticationProvider)constructor.newInstance(zks);
              [javac]                                  ^
              [javac] /home/michi/svndev/zookeeper/trunk/src/java/main/org/apache/zookeeper/server/auth/ProviderRegistry.java:60: warning: [cast] redundant cast to AuthenticationProvider
              [javac]                             ap = (AuthenticationProvider)constructor.newInstance();
              [javac]                                  ^
              [javac] 3 warnings
          

          2. When does this throw NullPointerException? Isn't it supposed to throw IllegalArgumentException if the arguments don't match?

          +                        Constructor<AuthenticationProvider> constructor = c.getConstructor(ZooKeeperServer.class);
          +                        try {
          +                            ap = (AuthenticationProvider)constructor.newInstance(zks);
          +                        } catch (NullPointerException e) {
          +                            constructor = c.getConstructor();
          +                            ap = (AuthenticationProvider)constructor.newInstance();
          +                        }
          
          Show
          Michi Mutsuzaki added a comment - Hi Warren, Sorry for super-slow response. I have 2 comments: 1. Could you fix these warnings? compile: [javac] Compiling 3 source files to /home/michi/svndev/zookeeper/trunk/build/classes [javac] warning: [options] bootstrap class path not set in conjunction with -source 1.5 [javac] /home/michi/svndev/zookeeper/trunk/src/java/main/org/apache/zookeeper/server/auth/ProviderRegistry.java:57: warning: [cast] redundant cast to AuthenticationProvider [javac] ap = (AuthenticationProvider)constructor.newInstance(zks); [javac] ^ [javac] /home/michi/svndev/zookeeper/trunk/src/java/main/org/apache/zookeeper/server/auth/ProviderRegistry.java:60: warning: [cast] redundant cast to AuthenticationProvider [javac] ap = (AuthenticationProvider)constructor.newInstance(); [javac] ^ [javac] 3 warnings 2. When does this throw NullPointerException? Isn't it supposed to throw IllegalArgumentException if the arguments don't match? + Constructor<AuthenticationProvider> constructor = c.getConstructor(ZooKeeperServer.class); + try { + ap = (AuthenticationProvider)constructor.newInstance(zks); + } catch (NullPointerException e) { + constructor = c.getConstructor(); + ap = (AuthenticationProvider)constructor.newInstance(); + }
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12539665/ZOOKEEPER-1525.patch
          against trunk revision 1582572.

          +1 @author. The patch does not contain any @author tags.

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1995//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1995//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1995//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12539665/ZOOKEEPER-1525.patch against trunk revision 1582572. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1995//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1995//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1995//console This message is automatically generated.
          Hide
          Michi Mutsuzaki added a comment -

          It looks like this patch breaks SASL tests.

          Show
          Michi Mutsuzaki added a comment - It looks like this patch breaks SASL tests.
          Hide
          Tim Crowder added a comment -

          Hi,
          This feature is listed as part of release 3.5.0-alpha :
          http://zookeeper.apache.org/doc/r3.5.0-alpha/releasenotes.html
          However, downloading the release and looking at the sources,
          it doesn't seem like the patch is included.
          What is the status of this issue?

          Thanks!

          Show
          Tim Crowder added a comment - Hi, This feature is listed as part of release 3.5.0-alpha : http://zookeeper.apache.org/doc/r3.5.0-alpha/releasenotes.html However, downloading the release and looking at the sources, it doesn't seem like the patch is included. What is the status of this issue? Thanks!
          Hide
          Tim Crowder added a comment -

          Fix to Warren Turkal's original patch. He was expecting getConstructor() to return a NULL, but at least for some java versions it actually throws a NoSuchMethod exception.

          Show
          Tim Crowder added a comment - Fix to Warren Turkal's original patch. He was expecting getConstructor() to return a NULL, but at least for some java versions it actually throws a NoSuchMethod exception.
          Hide
          Tim Crowder added a comment -

          Updated patch

          Show
          Tim Crowder added a comment - Updated patch
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12692848/ZOOKEEPER-1525.patch
          against trunk revision 1646992.

          +1 @author. The patch does not contain any @author tags.

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          -1 javac. The applied patch generated 8 javac compiler warnings (more than the trunk's current 6 warnings).

          -1 findbugs. The patch appears to introduce 1 new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2481//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2481//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2481//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12692848/ZOOKEEPER-1525.patch against trunk revision 1646992. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javadoc. The javadoc tool did not generate any warning messages. -1 javac. The applied patch generated 8 javac compiler warnings (more than the trunk's current 6 warnings). -1 findbugs. The patch appears to introduce 1 new Findbugs (version 2.0.3) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2481//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2481//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2481//console This message is automatically generated.
          Hide
          Tim Crowder added a comment -

          It looks like the port-change test that failed here was already failing before this patch. i.e.
          https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2477/

          Show
          Tim Crowder added a comment - It looks like the port-change test that failed here was already failing before this patch. i.e. https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2477/
          Hide
          Tim Crowder added a comment -

          New patch. Warnings removed. Testcase and example added.

          Show
          Tim Crowder added a comment - New patch. Warnings removed. Testcase and example added.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12696698/ZOOKEEPER-1525.patch
          against trunk revision 1656167.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 findbugs. The patch appears to introduce 1 new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2511//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2511//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2511//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12696698/ZOOKEEPER-1525.patch against trunk revision 1656167. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 1 new Findbugs (version 2.0.3) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2511//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2511//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2511//console This message is automatically generated.
          Hide
          Michi Mutsuzaki added a comment -

          Tim Crowder could you post this to reviewboard? Thanks!

          Show
          Michi Mutsuzaki added a comment - Tim Crowder could you post this to reviewboard? Thanks!
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12696698/ZOOKEEPER-1525.patch
          against trunk revision 1666784.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 findbugs. The patch appears to introduce 1 new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2571//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2571//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2571//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12696698/ZOOKEEPER-1525.patch against trunk revision 1666784. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 1 new Findbugs (version 2.0.3) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2571//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2571//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2571//console This message is automatically generated.
          Hide
          Tim Crowder added a comment -

          Hi Michi-

          Is there a guide or directions somewhere?
          I created an apache review-board account, and tried to submit it.
          When I submit against zookeeper-git, the reviewboard page just spins forever.
          When I submit against zookeeper, it asks "What is the base directory for this diff?".
          None of the instructions I could find online were helpful.

          Thanks!
          .timrc

          Show
          Tim Crowder added a comment - Hi Michi- Is there a guide or directions somewhere? I created an apache review-board account, and tried to submit it. When I submit against zookeeper-git, the reviewboard page just spins forever. When I submit against zookeeper, it asks "What is the base directory for this diff?". None of the instructions I could find online were helpful. Thanks! .timrc
          Hide
          Michi Mutsuzaki added a comment -

          Hi Tim, I don't know if we have any instructions online. The project is "zookeeper" and the base directory in your case should be http://svn.apache.org/repos/asf/zookeeper/trunk/ . Let me know if it doesn't work. Thanks!

          Show
          Michi Mutsuzaki added a comment - Hi Tim, I don't know if we have any instructions online. The project is "zookeeper" and the base directory in your case should be http://svn.apache.org/repos/asf/zookeeper/trunk/ . Let me know if it doesn't work. Thanks!
          Hide
          Tim Crowder added a comment -

          Hi Michi-

          That got me further along, but now I'm getting:
          The specified diff file could not be parsed.
          Line 2: No valid separator after the filename was found in the diff header
          From the same patch-file as attached to this bug.
          Note: I tried editing the patch, but didn't find a way to make ReviewBoard happy.
          Jenkins clearly understood the patch as it built and tested it.

          Is there a special format or anything else I need for ReviewBoard?
          Is there a way to have ReviewBoard pick it up from this bug?

          Thanks!
          .timrc

          Show
          Tim Crowder added a comment - Hi Michi- That got me further along, but now I'm getting: The specified diff file could not be parsed. Line 2: No valid separator after the filename was found in the diff header From the same patch-file as attached to this bug. Note: I tried editing the patch, but didn't find a way to make ReviewBoard happy. Jenkins clearly understood the patch as it built and tested it. Is there a special format or anything else I need for ReviewBoard? Is there a way to have ReviewBoard pick it up from this bug? Thanks! .timrc
          Hide
          Tim Crowder added a comment -

          Hi Michi-

          I finally managed to create https://reviews.apache.org/r/33874/
          Please let me know if you have any questions.

          Thanks!
          .timrc

          Show
          Tim Crowder added a comment - Hi Michi- I finally managed to create https://reviews.apache.org/r/33874/ Please let me know if you have any questions. Thanks! .timrc
          Hide
          Raul Gutierrez Segales added a comment -

          Thanks for the patch Tim Crowder! I added a few comments in the RB. After updating that, mind re-attaching the patch here as well so CI can run too. Thanks!

          Show
          Raul Gutierrez Segales added a comment - Thanks for the patch Tim Crowder ! I added a few comments in the RB. After updating that, mind re-attaching the patch here as well so CI can run too. Thanks!
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12696698/ZOOKEEPER-1525.patch
          against trunk revision 1687876.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified tests.

          -1 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2781//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12696698/ZOOKEEPER-1525.patch against trunk revision 1687876. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2781//console This message is automatically generated.
          Hide
          Warren Turkal added a comment -

          It's great to see some progress on this. Is this actually going to make it into 3.5.2?

          Show
          Warren Turkal added a comment - It's great to see some progress on this. Is this actually going to make it into 3.5.2?
          Hide
          Tim Crowder added a comment -

          Patch with unit tests, updated for review feedback.

          Show
          Tim Crowder added a comment - Patch with unit tests, updated for review feedback.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12769431/ZOOKEEPER-1525.patch
          against trunk revision 1711151.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          -1 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2934//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12769431/ZOOKEEPER-1525.patch against trunk revision 1711151. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2934//console This message is automatically generated.
          Hide
          Tim Crowder added a comment -

          Patch for patch... Strip leading a/,b/ path elements. Annotate new files.

          Show
          Tim Crowder added a comment - Patch for patch... Strip leading a/,b/ path elements. Annotate new files.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12769449/ZOOKEEPER-1525.patch
          against trunk revision 1711151.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 findbugs. The patch appears to introduce 1 new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2935//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2935//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2935//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12769449/ZOOKEEPER-1525.patch against trunk revision 1711151. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 1 new Findbugs (version 2.0.3) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2935//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2935//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2935//console This message is automatically generated.
          Hide
          Michi Mutsuzaki added a comment -

          cancelling the patch. i had a couple of minor comments in https://reviews.apache.org/r/33874/ .

          Show
          Michi Mutsuzaki added a comment - cancelling the patch. i had a couple of minor comments in https://reviews.apache.org/r/33874/ .
          Hide
          Tim Crowder added a comment -

          Updated for review comments.

          Show
          Tim Crowder added a comment - Updated for review comments.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12771231/ZOOKEEPER-1525.patch
          against trunk revision 1713320.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          -1 javadoc. The javadoc tool appears to have generated 1 warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2951//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2951//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2951//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12771231/ZOOKEEPER-1525.patch against trunk revision 1713320. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. -1 javadoc. The javadoc tool appears to have generated 1 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2951//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2951//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2951//console This message is automatically generated.
          Hide
          Tim Crowder added a comment -

          Michi Mutsuzaki Flavio Junqueira
          Hopefully, I've addressed all the review comments.
          Please let me know if there's anything else I need to do to get this merged.

          Thanks!
          .timrc

          Show
          Tim Crowder added a comment - Michi Mutsuzaki Flavio Junqueira Hopefully, I've addressed all the review comments. Please let me know if there's anything else I need to do to get this merged. Thanks! .timrc
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12771231/ZOOKEEPER-1525.patch
          against trunk revision 1748630.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          -1 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3218//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12771231/ZOOKEEPER-1525.patch against trunk revision 1748630. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3218//console This message is automatically generated.
          Hide
          Jordan Zimmerman added a comment -

          This patch no longer applies for me. Is this still viable? I need this functionality. If this is patch is no longer maintained I'm happy to update it.

          Show
          Jordan Zimmerman added a comment - This patch no longer applies for me. Is this still viable? I need this functionality. If this is patch is no longer maintained I'm happy to update it.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12771231/ZOOKEEPER-1525.patch
          against trunk revision ec20c5434cc8a334b3fd25e27d26dccf4793c8f3.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          -1 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3463//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12771231/ZOOKEEPER-1525.patch against trunk revision ec20c5434cc8a334b3fd25e27d26dccf4793c8f3. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3463//console This message is automatically generated.
          Hide
          ASF GitHub Bot added a comment -

          GitHub user Randgalt opened a pull request:

          https://github.com/apache/zookeeper/pull/84

          ZOOKEEPER-1525 Plumb ZooKeeperServer object into auth plugins

          Based on patch work from https://issues.apache.org/jira/browse/ZOOKEEPER-1525

          Created ServerAuthenticationProvider which has a method to accept the ZooKeeper
          server so that auth can be done using values in the ZK database. As this is a new
          interface, existing implementations aren't affected helping backward compatibility

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/Randgalt/zookeeper ZOOKEEPER-1525

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/zookeeper/pull/84.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #84


          commit 4de95483cb624217e2f3cd63872e23d60f28749b
          Author: randgalt <jordan@jordanzimmerman.com>
          Date: 2016-10-04T15:23:30Z

          Based on patch work from https://issues.apache.org/jira/browse/ZOOKEEPER-1525

          Created ServerAuthenticationProvider which has a method to accept the ZooKeeper
          server so that auth can be done using values in the ZK database. As this is a new
          interface, existing implementations aren't affected helping backward compatibility


          Show
          ASF GitHub Bot added a comment - GitHub user Randgalt opened a pull request: https://github.com/apache/zookeeper/pull/84 ZOOKEEPER-1525 Plumb ZooKeeperServer object into auth plugins Based on patch work from https://issues.apache.org/jira/browse/ZOOKEEPER-1525 Created ServerAuthenticationProvider which has a method to accept the ZooKeeper server so that auth can be done using values in the ZK database. As this is a new interface, existing implementations aren't affected helping backward compatibility You can merge this pull request into a Git repository by running: $ git pull https://github.com/Randgalt/zookeeper ZOOKEEPER-1525 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/84.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #84 commit 4de95483cb624217e2f3cd63872e23d60f28749b Author: randgalt <jordan@jordanzimmerman.com> Date: 2016-10-04T15:23:30Z Based on patch work from https://issues.apache.org/jira/browse/ZOOKEEPER-1525 Created ServerAuthenticationProvider which has a method to accept the ZooKeeper server so that auth can be done using values in the ZK database. As this is a new interface, existing implementations aren't affected helping backward compatibility
          Hide
          Jordan Zimmerman added a comment -

          My PR is essentially the same as the OP's patch but a bit more backward compatible.

          Show
          Jordan Zimmerman added a comment - My PR is essentially the same as the OP's patch but a bit more backward compatible.
          Hide
          Jordan Zimmerman added a comment -

          Note: ZOOKEEPER-2143 merged into this PR as it is a natural fit

          Show
          Jordan Zimmerman added a comment - Note: ZOOKEEPER-2143 merged into this PR as it is a natural fit
          Hide
          ASF GitHub Bot added a comment -

          Github user Randgalt commented on the issue:

          https://github.com/apache/zookeeper/pull/84

          @fpj Any more thoughts on this Flavio?

          Show
          ASF GitHub Bot added a comment - Github user Randgalt commented on the issue: https://github.com/apache/zookeeper/pull/84 @fpj Any more thoughts on this Flavio?
          Hide
          ASF GitHub Bot added a comment -

          Github user hanm commented on the issue:

          https://github.com/apache/zookeeper/pull/84

          (Not particularly related to the logic of this PR) Might be better to do a git squash to consolidate 16 commits into a single commit to keep the commit history clean - I've seen most projects operate that way.

          Show
          ASF GitHub Bot added a comment - Github user hanm commented on the issue: https://github.com/apache/zookeeper/pull/84 (Not particularly related to the logic of this PR) Might be better to do a git squash to consolidate 16 commits into a single commit to keep the commit history clean - I've seen most projects operate that way.
          Hide
          ASF GitHub Bot added a comment -

          Github user fpj commented on a diff in the pull request:

          https://github.com/apache/zookeeper/pull/84#discussion_r85641975

          — Diff: src/java/main/org/apache/zookeeper/server/ServerCnxn.java —
          @@ -51,7 +47,7 @@
          final public static Object me = new Object();
          private static final Logger LOG = LoggerFactory.getLogger(ServerCnxn.class);

          • protected ArrayList<Id> authInfo = new ArrayList<Id>();
            + private Set<Id> authInfo = Collections.newSetFromMap(new ConcurrentHashMap<Id, Boolean>());
              • End diff –

          Ok.

          Show
          ASF GitHub Bot added a comment - Github user fpj commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/84#discussion_r85641975 — Diff: src/java/main/org/apache/zookeeper/server/ServerCnxn.java — @@ -51,7 +47,7 @@ final public static Object me = new Object(); private static final Logger LOG = LoggerFactory.getLogger(ServerCnxn.class); protected ArrayList<Id> authInfo = new ArrayList<Id>(); + private Set<Id> authInfo = Collections.newSetFromMap(new ConcurrentHashMap<Id, Boolean>()); End diff – Ok.
          Hide
          ASF GitHub Bot added a comment -

          Github user fpj commented on a diff in the pull request:

          https://github.com/apache/zookeeper/pull/84#discussion_r85642186

          — Diff: src/java/test/org/apache/zookeeper/test/KeyAuthClientTest.java —
          @@ -0,0 +1,131 @@
          +/**
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +package org.apache.zookeeper.test;
          +
          +import org.apache.zookeeper.CreateMode;
          +import org.apache.zookeeper.KeeperException;
          +import org.apache.zookeeper.ZooDefs.Ids;
          +import org.apache.zookeeper.ZooKeeper;
          +import org.apache.zookeeper.data.ACL;
          +import org.junit.Assert;
          +import org.junit.Test;
          +import org.slf4j.Logger;
          +import org.slf4j.LoggerFactory;
          +
          +import java.util.List;
          +
          +public class KeyAuthClientTest extends ClientBase {
          + private static final Logger LOG = LoggerFactory.getLogger(KeyAuthClientTest.class);
          + static

          { + System.setProperty("zookeeper.authProvider.1","org.apache.zookeeper.server.auth.KeyAuthenticationProvider"); + }

          +
          + public void createNodePrintAcl(ZooKeeper zk, String path, String testName) {
          + try {
          + LOG.debug("KeyAuthenticationProvider Creating Test Node:"path".\n");
          + zk.create(path, null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT);
          + List<ACL> acls = zk.getACL(path, null);
          + LOG.debug("Node: "path" Test:"testName" ACLs:");
          + for (ACL acl : acls)

          { + LOG.debug(" "+acl.toString()); + }

          + } catch (Exception e)

          { + LOG.debug(" EXCEPTION THROWN", e); + }

          + }
          +
          + public void testPreAuth() throws Exception {
          + ZooKeeper zk = createClient();
          + zk.addAuthInfo("key", "25".getBytes());
          + try

          { + createNodePrintAcl(zk, "/pre", "testPreAuth"); + zk.setACL("/", Ids.CREATOR_ALL_ACL, -1); + zk.getChildren("/", false); + zk.create("/abc", null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT); + zk.setData("/abc", "testData1".getBytes(), -1); + zk.create("/key", null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT); + zk.setData("/key", "5".getBytes(), -1); + Thread.sleep(1000); + }

          catch (KeeperException e)

          { + Assert.fail("test failed :" + e); + }

          + finally

          { + zk.close(); + }
          + }
          +
          + public void testMissingAuth() throws Exception {
          + ZooKeeper zk = createClient();
          + try { + zk.getData("/abc", false, null); + Assert.fail("Should not be able to get data"); + } catch (KeeperException correct) { + // correct + }
          + try { + zk.setData("/abc", "testData2".getBytes(), -1); + Assert.fail("Should not be able to set data"); + } catch (KeeperException correct) { + // correct + } finally { + zk.close(); + }

          + }
          +
          + public void testValidAuth() throws Exception {
          + ZooKeeper zk = createClient();
          + // any multiple of 5 will do...
          + zk.addAuthInfo("key", "25".getBytes());
          + try

          { + createNodePrintAcl(zk, "/valid", "testValidAuth"); + zk.getData("/abc", false, null); + zk.setData("/abc", "testData3".getBytes(), -1); + }

          catch (KeeperException.AuthFailedException e)

          { + Assert.fail("test failed :" + e); + }

          finally

          { + zk.close(); + }
          + }
          +
          + public void testValidAuth2() throws Exception {
          + ZooKeeper zk = createClient();
          + // any multiple of 5 will do...
          + zk.addAuthInfo("key", "125".getBytes());
          + try { + createNodePrintAcl(zk, "/valid2", "testValidAuth2"); + zk.getData("/abc", false, null); + zk.setData("/abc", "testData3".getBytes(), -1); + } catch (KeeperException.AuthFailedException e) { + Assert.fail("test failed :" + e); + } finally { + zk.close(); + }

          + }
          +
          + @Test
          + public void testAuth() throws Exception {
          + // NOTE: the tests need to run in-order, and older versions of
          — End diff –

          I think it is fine if you have a sequence of steps in your test case, but I really see this as a single test case with multiple steps. I suggest we remove the `test` prefix from the methods to make clear that they aren't independent test cases.

          Show
          ASF GitHub Bot added a comment - Github user fpj commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/84#discussion_r85642186 — Diff: src/java/test/org/apache/zookeeper/test/KeyAuthClientTest.java — @@ -0,0 +1,131 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.zookeeper.test; + +import org.apache.zookeeper.CreateMode; +import org.apache.zookeeper.KeeperException; +import org.apache.zookeeper.ZooDefs.Ids; +import org.apache.zookeeper.ZooKeeper; +import org.apache.zookeeper.data.ACL; +import org.junit.Assert; +import org.junit.Test; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.util.List; + +public class KeyAuthClientTest extends ClientBase { + private static final Logger LOG = LoggerFactory.getLogger(KeyAuthClientTest.class); + static { + System.setProperty("zookeeper.authProvider.1","org.apache.zookeeper.server.auth.KeyAuthenticationProvider"); + } + + public void createNodePrintAcl(ZooKeeper zk, String path, String testName) { + try { + LOG.debug("KeyAuthenticationProvider Creating Test Node:" path ".\n"); + zk.create(path, null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT); + List<ACL> acls = zk.getACL(path, null); + LOG.debug("Node: " path " Test:" testName " ACLs:"); + for (ACL acl : acls) { + LOG.debug(" "+acl.toString()); + } + } catch (Exception e) { + LOG.debug(" EXCEPTION THROWN", e); + } + } + + public void testPreAuth() throws Exception { + ZooKeeper zk = createClient(); + zk.addAuthInfo("key", "25".getBytes()); + try { + createNodePrintAcl(zk, "/pre", "testPreAuth"); + zk.setACL("/", Ids.CREATOR_ALL_ACL, -1); + zk.getChildren("/", false); + zk.create("/abc", null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT); + zk.setData("/abc", "testData1".getBytes(), -1); + zk.create("/key", null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT); + zk.setData("/key", "5".getBytes(), -1); + Thread.sleep(1000); + } catch (KeeperException e) { + Assert.fail("test failed :" + e); + } + finally { + zk.close(); + } + } + + public void testMissingAuth() throws Exception { + ZooKeeper zk = createClient(); + try { + zk.getData("/abc", false, null); + Assert.fail("Should not be able to get data"); + } catch (KeeperException correct) { + // correct + } + try { + zk.setData("/abc", "testData2".getBytes(), -1); + Assert.fail("Should not be able to set data"); + } catch (KeeperException correct) { + // correct + } finally { + zk.close(); + } + } + + public void testValidAuth() throws Exception { + ZooKeeper zk = createClient(); + // any multiple of 5 will do... + zk.addAuthInfo("key", "25".getBytes()); + try { + createNodePrintAcl(zk, "/valid", "testValidAuth"); + zk.getData("/abc", false, null); + zk.setData("/abc", "testData3".getBytes(), -1); + } catch (KeeperException.AuthFailedException e) { + Assert.fail("test failed :" + e); + } finally { + zk.close(); + } + } + + public void testValidAuth2() throws Exception { + ZooKeeper zk = createClient(); + // any multiple of 5 will do... + zk.addAuthInfo("key", "125".getBytes()); + try { + createNodePrintAcl(zk, "/valid2", "testValidAuth2"); + zk.getData("/abc", false, null); + zk.setData("/abc", "testData3".getBytes(), -1); + } catch (KeeperException.AuthFailedException e) { + Assert.fail("test failed :" + e); + } finally { + zk.close(); + } + } + + @Test + public void testAuth() throws Exception { + // NOTE: the tests need to run in-order, and older versions of — End diff – I think it is fine if you have a sequence of steps in your test case, but I really see this as a single test case with multiple steps. I suggest we remove the `test` prefix from the methods to make clear that they aren't independent test cases.
          Hide
          ASF GitHub Bot added a comment -

          Github user fpj commented on a diff in the pull request:

          https://github.com/apache/zookeeper/pull/84#discussion_r85642051

          — Diff: src/java/test/org/apache/zookeeper/test/KeyAuthClientTest.java —
          @@ -0,0 +1,131 @@
          +/**
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +package org.apache.zookeeper.test;
          +
          +import org.apache.zookeeper.CreateMode;
          +import org.apache.zookeeper.KeeperException;
          +import org.apache.zookeeper.ZooDefs.Ids;
          +import org.apache.zookeeper.ZooKeeper;
          +import org.apache.zookeeper.data.ACL;
          +import org.junit.Assert;
          +import org.junit.Test;
          +import org.slf4j.Logger;
          +import org.slf4j.LoggerFactory;
          +
          +import java.util.List;
          +
          +public class KeyAuthClientTest extends ClientBase {
          + private static final Logger LOG = LoggerFactory.getLogger(KeyAuthClientTest.class);
          + static

          { + System.setProperty("zookeeper.authProvider.1","org.apache.zookeeper.server.auth.KeyAuthenticationProvider"); + }

          +
          + public void createNodePrintAcl(ZooKeeper zk, String path, String testName) {
          + try {
          + LOG.debug("KeyAuthenticationProvider Creating Test Node:"path".\n");
          + zk.create(path, null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT);
          + List<ACL> acls = zk.getACL(path, null);
          + LOG.debug("Node: "path" Test:"testName" ACLs:");
          + for (ACL acl : acls) {
          + LOG.debug(" "+acl.toString());
          — End diff –

          Same here, space around symbols, please.

          Show
          ASF GitHub Bot added a comment - Github user fpj commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/84#discussion_r85642051 — Diff: src/java/test/org/apache/zookeeper/test/KeyAuthClientTest.java — @@ -0,0 +1,131 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.zookeeper.test; + +import org.apache.zookeeper.CreateMode; +import org.apache.zookeeper.KeeperException; +import org.apache.zookeeper.ZooDefs.Ids; +import org.apache.zookeeper.ZooKeeper; +import org.apache.zookeeper.data.ACL; +import org.junit.Assert; +import org.junit.Test; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.util.List; + +public class KeyAuthClientTest extends ClientBase { + private static final Logger LOG = LoggerFactory.getLogger(KeyAuthClientTest.class); + static { + System.setProperty("zookeeper.authProvider.1","org.apache.zookeeper.server.auth.KeyAuthenticationProvider"); + } + + public void createNodePrintAcl(ZooKeeper zk, String path, String testName) { + try { + LOG.debug("KeyAuthenticationProvider Creating Test Node:" path ".\n"); + zk.create(path, null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT); + List<ACL> acls = zk.getACL(path, null); + LOG.debug("Node: " path " Test:" testName " ACLs:"); + for (ACL acl : acls) { + LOG.debug(" "+acl.toString()); — End diff – Same here, space around symbols, please.
          Hide
          ASF GitHub Bot added a comment -

          Github user fpj commented on a diff in the pull request:

          https://github.com/apache/zookeeper/pull/84#discussion_r85642270

          — Diff: src/java/main/org/apache/zookeeper/server/auth/ProviderRegistry.java —
          @@ -31,7 +31,15 @@

          private static boolean initialized = false;
          private static HashMap<String, AuthenticationProvider> authenticationProviders =

          • new HashMap<String, AuthenticationProvider>();
            + new HashMap<>();
            +
            + //VisibleForTesting
            + public static void reset() {
              • End diff –

          Is this used anywhere?

          Show
          ASF GitHub Bot added a comment - Github user fpj commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/84#discussion_r85642270 — Diff: src/java/main/org/apache/zookeeper/server/auth/ProviderRegistry.java — @@ -31,7 +31,15 @@ private static boolean initialized = false; private static HashMap<String, AuthenticationProvider> authenticationProviders = new HashMap<String, AuthenticationProvider>(); + new HashMap<>(); + + //VisibleForTesting + public static void reset() { End diff – Is this used anywhere?
          Hide
          ASF GitHub Bot added a comment -

          Github user fpj commented on a diff in the pull request:

          https://github.com/apache/zookeeper/pull/84#discussion_r85641947

          — Diff: src/docs/src/documentation/content/xdocs/zookeeperProgrammers.xml —
          @@ -1193,6 +1193,55 @@ authProvider.2=com.f.MyAuth2
          only one will be used. Also all servers must have the same plugins defined, otherwise clients using
          the authentication schemes provided by the plugins will have problems connecting to some servers.
          </para>
          +
          + <para> <emphasis role="bold">Added in 3.6.0</emphasis>: An alternate abstraction is available for pluggable
          + authentication. It provides additional arguments.
          + </para>
          +
          + <programlisting>
          +public abstract class ServerAuthenticationProvider implements AuthenticationProvider {
          — End diff –

          Indentation? Not sure if it matters within the listing block.

          Show
          ASF GitHub Bot added a comment - Github user fpj commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/84#discussion_r85641947 — Diff: src/docs/src/documentation/content/xdocs/zookeeperProgrammers.xml — @@ -1193,6 +1193,55 @@ authProvider.2=com.f.MyAuth2 only one will be used. Also all servers must have the same plugins defined, otherwise clients using the authentication schemes provided by the plugins will have problems connecting to some servers. </para> + + <para> <emphasis role="bold">Added in 3.6.0</emphasis>: An alternate abstraction is available for pluggable + authentication. It provides additional arguments. + </para> + + <programlisting> +public abstract class ServerAuthenticationProvider implements AuthenticationProvider { — End diff – Indentation? Not sure if it matters within the listing block.
          Hide
          ASF GitHub Bot added a comment -

          Github user fpj commented on a diff in the pull request:

          https://github.com/apache/zookeeper/pull/84#discussion_r85641968

          — Diff: src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java —
          @@ -543,7 +546,7 @@ protected void pRequest2Txn(int type, long zxid, Request request,
          }

          nodeRecord = getRecordForPath(ZooDefs.CONFIG_NODE);

          • checkACL(zks, nodeRecord.acl, ZooDefs.Perms.WRITE, request.authInfo);
            + checkACL(zks, request.cnxn, nodeRecord.acl, ZooDefs.Perms.WRITE, request.authInfo, "/", null);
              • End diff –

          Why is the path `"/"` here?

          Show
          ASF GitHub Bot added a comment - Github user fpj commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/84#discussion_r85641968 — Diff: src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java — @@ -543,7 +546,7 @@ protected void pRequest2Txn(int type, long zxid, Request request, } nodeRecord = getRecordForPath(ZooDefs.CONFIG_NODE); checkACL(zks, nodeRecord.acl, ZooDefs.Perms.WRITE, request.authInfo); + checkACL(zks, request.cnxn, nodeRecord.acl, ZooDefs.Perms.WRITE, request.authInfo, "/", null); End diff – Why is the path `"/"` here?
          Hide
          ASF GitHub Bot added a comment -

          Github user fpj commented on a diff in the pull request:

          https://github.com/apache/zookeeper/pull/84#discussion_r85642389

          — Diff: src/java/main/org/apache/zookeeper/server/auth/WrappedAuthenticationProvider.java —
          @@ -0,0 +1,74 @@
          +/**
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +package org.apache.zookeeper.server.auth;
          +
          +import org.apache.zookeeper.KeeperException;
          +import org.apache.zookeeper.data.ACL;
          +import org.apache.zookeeper.server.ServerCnxn;
          +import org.apache.zookeeper.server.ZooKeeperServer;
          +
          +import java.util.List;
          +
          +/**
          + * Provides backwards compatibility between older

          {@link AuthenticationProvider}

          + * implementations and the new

          {@link ServerAuthenticationProvider}

          interface.
          + */
          +class WrappedAuthenticationProvider extends ServerAuthenticationProvider {
          + private final AuthenticationProvider implementation;
          +
          + static ServerAuthenticationProvider wrap(AuthenticationProvider provider)

          { + return (provider instanceof ServerAuthenticationProvider) ? (ServerAuthenticationProvider)provider + : new WrappedAuthenticationProvider(provider); + }

          +
          + private WrappedAuthenticationProvider(AuthenticationProvider implementation)

          { + this.implementation = implementation; + }

          +
          + @Override
          + public KeeperException.Code handleAuthentication(ZooKeeperServer zks, ServerCnxn cnxn, byte[] authData)

          { + return implementation.handleAuthentication(cnxn, authData); + }

          +
          + /**
          + *

          {@inheritDoc}

          — End diff –

          I've never used this before and from the name it implies that it will copy the doc blurb from the super class when running the javadoc tool. Is this correct? Do we have to do it for this method only or for others as well?

          Show
          ASF GitHub Bot added a comment - Github user fpj commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/84#discussion_r85642389 — Diff: src/java/main/org/apache/zookeeper/server/auth/WrappedAuthenticationProvider.java — @@ -0,0 +1,74 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.zookeeper.server.auth; + +import org.apache.zookeeper.KeeperException; +import org.apache.zookeeper.data.ACL; +import org.apache.zookeeper.server.ServerCnxn; +import org.apache.zookeeper.server.ZooKeeperServer; + +import java.util.List; + +/** + * Provides backwards compatibility between older {@link AuthenticationProvider} + * implementations and the new {@link ServerAuthenticationProvider} interface. + */ +class WrappedAuthenticationProvider extends ServerAuthenticationProvider { + private final AuthenticationProvider implementation; + + static ServerAuthenticationProvider wrap(AuthenticationProvider provider) { + return (provider instanceof ServerAuthenticationProvider) ? (ServerAuthenticationProvider)provider + : new WrappedAuthenticationProvider(provider); + } + + private WrappedAuthenticationProvider(AuthenticationProvider implementation) { + this.implementation = implementation; + } + + @Override + public KeeperException.Code handleAuthentication(ZooKeeperServer zks, ServerCnxn cnxn, byte[] authData) { + return implementation.handleAuthentication(cnxn, authData); + } + + /** + * {@inheritDoc} — End diff – I've never used this before and from the name it implies that it will copy the doc blurb from the super class when running the javadoc tool. Is this correct? Do we have to do it for this method only or for others as well?
          Hide
          ASF GitHub Bot added a comment -

          Github user fpj commented on a diff in the pull request:

          https://github.com/apache/zookeeper/pull/84#discussion_r85642202

          — Diff: src/java/test/org/apache/zookeeper/test/KeyAuthClientTest.java —
          @@ -0,0 +1,131 @@
          +/**
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +package org.apache.zookeeper.test;
          +
          +import org.apache.zookeeper.CreateMode;
          +import org.apache.zookeeper.KeeperException;
          +import org.apache.zookeeper.ZooDefs.Ids;
          +import org.apache.zookeeper.ZooKeeper;
          +import org.apache.zookeeper.data.ACL;
          +import org.junit.Assert;
          +import org.junit.Test;
          +import org.slf4j.Logger;
          +import org.slf4j.LoggerFactory;
          +
          +import java.util.List;
          +
          +public class KeyAuthClientTest extends ClientBase {
          + private static final Logger LOG = LoggerFactory.getLogger(KeyAuthClientTest.class);
          + static

          { + System.setProperty("zookeeper.authProvider.1","org.apache.zookeeper.server.auth.KeyAuthenticationProvider"); + }

          +
          + public void createNodePrintAcl(ZooKeeper zk, String path, String testName) {
          + try {
          + LOG.debug("KeyAuthenticationProvider Creating Test Node:"path".\n");
          + zk.create(path, null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT);
          + List<ACL> acls = zk.getACL(path, null);
          + LOG.debug("Node: "path" Test:"testName" ACLs:");
          + for (ACL acl : acls)

          { + LOG.debug(" "+acl.toString()); + }

          + } catch (Exception e)

          { + LOG.debug(" EXCEPTION THROWN", e); + }

          + }
          +
          + public void testPreAuth() throws Exception {
          + ZooKeeper zk = createClient();
          + zk.addAuthInfo("key", "25".getBytes());
          + try {
          + createNodePrintAcl(zk, "/pre", "testPreAuth");
          + zk.setACL("/", Ids.CREATOR_ALL_ACL, -1);
          + zk.getChildren("/", false);
          + zk.create("/abc", null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT);
          + zk.setData("/abc", "testData1".getBytes(), -1);
          + zk.create("/key", null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT);
          + zk.setData("/key", "5".getBytes(), -1);
          + Thread.sleep(1000);
          — End diff –

          I'm not sure why we need this `sleep` here.

          Show
          ASF GitHub Bot added a comment - Github user fpj commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/84#discussion_r85642202 — Diff: src/java/test/org/apache/zookeeper/test/KeyAuthClientTest.java — @@ -0,0 +1,131 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.zookeeper.test; + +import org.apache.zookeeper.CreateMode; +import org.apache.zookeeper.KeeperException; +import org.apache.zookeeper.ZooDefs.Ids; +import org.apache.zookeeper.ZooKeeper; +import org.apache.zookeeper.data.ACL; +import org.junit.Assert; +import org.junit.Test; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.util.List; + +public class KeyAuthClientTest extends ClientBase { + private static final Logger LOG = LoggerFactory.getLogger(KeyAuthClientTest.class); + static { + System.setProperty("zookeeper.authProvider.1","org.apache.zookeeper.server.auth.KeyAuthenticationProvider"); + } + + public void createNodePrintAcl(ZooKeeper zk, String path, String testName) { + try { + LOG.debug("KeyAuthenticationProvider Creating Test Node:" path ".\n"); + zk.create(path, null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT); + List<ACL> acls = zk.getACL(path, null); + LOG.debug("Node: " path " Test:" testName " ACLs:"); + for (ACL acl : acls) { + LOG.debug(" "+acl.toString()); + } + } catch (Exception e) { + LOG.debug(" EXCEPTION THROWN", e); + } + } + + public void testPreAuth() throws Exception { + ZooKeeper zk = createClient(); + zk.addAuthInfo("key", "25".getBytes()); + try { + createNodePrintAcl(zk, "/pre", "testPreAuth"); + zk.setACL("/", Ids.CREATOR_ALL_ACL, -1); + zk.getChildren("/", false); + zk.create("/abc", null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT); + zk.setData("/abc", "testData1".getBytes(), -1); + zk.create("/key", null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT); + zk.setData("/key", "5".getBytes(), -1); + Thread.sleep(1000); — End diff – I'm not sure why we need this `sleep` here.
          Hide
          ASF GitHub Bot added a comment -

          Github user fpj commented on a diff in the pull request:

          https://github.com/apache/zookeeper/pull/84#discussion_r85642282

          — Diff: src/java/main/org/apache/zookeeper/server/auth/ServerAuthenticationProvider.java —
          @@ -0,0 +1,79 @@
          +/**
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +package org.apache.zookeeper.server.auth;
          +
          +import org.apache.zookeeper.KeeperException;
          +import org.apache.zookeeper.data.ACL;
          +import org.apache.zookeeper.server.ServerCnxn;
          +import org.apache.zookeeper.server.ZooKeeperServer;
          +
          +import java.util.List;
          +
          +/**
          + * A variation on

          {@link AuthenticationProvider}

          that provides additional
          + * parameters for more detailed authentication
          + */
          +public abstract class ServerAuthenticationProvider implements AuthenticationProvider {
          + /**
          + * This method is called when a client passes authentication data for this
          + * scheme. The authData is directly from the authentication packet. The
          + * implementor may attach new ids to the authInfo field of cnxn or may use
          + * cnxn to send packets back to the client.
          + *
          + * @param cnxn
          + * the cnxn that received the authentication information.
          + * @param authData
          + * the authentication data received.
          + * @return indication of success or failure
          + */
          + public abstract KeeperException.Code handleAuthentication(ZooKeeperServer zks, ServerCnxn cnxn, byte authData[]);
          +
          + /**
          + * This method is called to see if the given id matches the given id
          + * expression in the ACL. This allows schemes to use application specific
          + * wild cards.
          + *
          + * @param zks
          + * the ZooKeeper server instance
          + * @param cnxn
          + * the active server connection being authenticated
          + * @param path
          + * the path of the operation being authenticated
          + * @param id
          + * the id to check.
          + * @param aclExpr
          + * the expression to match ids against.
          + * @param perm
          + * the permission value being authenticated
          + * @param setAcls
          + * for set ACL operations, the list of ACLs being set. Otherwise null.
          + * @return true if the arguments can be matched by the expression.
          + */
          + public abstract boolean matches(ZooKeeperServer zks, ServerCnxn cnxn, String path, String id, String aclExpr, int perm, List<ACL> setAcls);
          — End diff –

          Can we break this line, please?

          Show
          ASF GitHub Bot added a comment - Github user fpj commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/84#discussion_r85642282 — Diff: src/java/main/org/apache/zookeeper/server/auth/ServerAuthenticationProvider.java — @@ -0,0 +1,79 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.zookeeper.server.auth; + +import org.apache.zookeeper.KeeperException; +import org.apache.zookeeper.data.ACL; +import org.apache.zookeeper.server.ServerCnxn; +import org.apache.zookeeper.server.ZooKeeperServer; + +import java.util.List; + +/** + * A variation on {@link AuthenticationProvider} that provides additional + * parameters for more detailed authentication + */ +public abstract class ServerAuthenticationProvider implements AuthenticationProvider { + /** + * This method is called when a client passes authentication data for this + * scheme. The authData is directly from the authentication packet. The + * implementor may attach new ids to the authInfo field of cnxn or may use + * cnxn to send packets back to the client. + * + * @param cnxn + * the cnxn that received the authentication information. + * @param authData + * the authentication data received. + * @return indication of success or failure + */ + public abstract KeeperException.Code handleAuthentication(ZooKeeperServer zks, ServerCnxn cnxn, byte authData[]); + + /** + * This method is called to see if the given id matches the given id + * expression in the ACL. This allows schemes to use application specific + * wild cards. + * + * @param zks + * the ZooKeeper server instance + * @param cnxn + * the active server connection being authenticated + * @param path + * the path of the operation being authenticated + * @param id + * the id to check. + * @param aclExpr + * the expression to match ids against. + * @param perm + * the permission value being authenticated + * @param setAcls + * for set ACL operations, the list of ACLs being set. Otherwise null. + * @return true if the arguments can be matched by the expression. + */ + public abstract boolean matches(ZooKeeperServer zks, ServerCnxn cnxn, String path, String id, String aclExpr, int perm, List<ACL> setAcls); — End diff – Can we break this line, please?
          Hide
          ASF GitHub Bot added a comment -

          Github user fpj commented on a diff in the pull request:

          https://github.com/apache/zookeeper/pull/84#discussion_r85642046

          — Diff: src/java/test/org/apache/zookeeper/test/KeyAuthClientTest.java —
          @@ -0,0 +1,131 @@
          +/**
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +package org.apache.zookeeper.test;
          +
          +import org.apache.zookeeper.CreateMode;
          +import org.apache.zookeeper.KeeperException;
          +import org.apache.zookeeper.ZooDefs.Ids;
          +import org.apache.zookeeper.ZooKeeper;
          +import org.apache.zookeeper.data.ACL;
          +import org.junit.Assert;
          +import org.junit.Test;
          +import org.slf4j.Logger;
          +import org.slf4j.LoggerFactory;
          +
          +import java.util.List;
          +
          +public class KeyAuthClientTest extends ClientBase {
          + private static final Logger LOG = LoggerFactory.getLogger(KeyAuthClientTest.class);
          + static

          { + System.setProperty("zookeeper.authProvider.1","org.apache.zookeeper.server.auth.KeyAuthenticationProvider"); + }

          +
          + public void createNodePrintAcl(ZooKeeper zk, String path, String testName) {
          + try {
          + LOG.debug("KeyAuthenticationProvider Creating Test Node:"path".\n");
          + zk.create(path, null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT);
          + List<ACL> acls = zk.getACL(path, null);
          + LOG.debug("Node: "path" Test:"testName" ACLs:");
          — End diff –

          space around symbols, please?

          Show
          ASF GitHub Bot added a comment - Github user fpj commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/84#discussion_r85642046 — Diff: src/java/test/org/apache/zookeeper/test/KeyAuthClientTest.java — @@ -0,0 +1,131 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.zookeeper.test; + +import org.apache.zookeeper.CreateMode; +import org.apache.zookeeper.KeeperException; +import org.apache.zookeeper.ZooDefs.Ids; +import org.apache.zookeeper.ZooKeeper; +import org.apache.zookeeper.data.ACL; +import org.junit.Assert; +import org.junit.Test; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.util.List; + +public class KeyAuthClientTest extends ClientBase { + private static final Logger LOG = LoggerFactory.getLogger(KeyAuthClientTest.class); + static { + System.setProperty("zookeeper.authProvider.1","org.apache.zookeeper.server.auth.KeyAuthenticationProvider"); + } + + public void createNodePrintAcl(ZooKeeper zk, String path, String testName) { + try { + LOG.debug("KeyAuthenticationProvider Creating Test Node:" path ".\n"); + zk.create(path, null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT); + List<ACL> acls = zk.getACL(path, null); + LOG.debug("Node: " path " Test:" testName " ACLs:"); — End diff – space around symbols, please?
          Hide
          ASF GitHub Bot added a comment -

          Github user fpj commented on the issue:

          https://github.com/apache/zookeeper/pull/84

          @Randgalt My comments are pretty minor, but it will give it a chance to QA to test this PR.

          Show
          ASF GitHub Bot added a comment - Github user fpj commented on the issue: https://github.com/apache/zookeeper/pull/84 @Randgalt My comments are pretty minor, but it will give it a chance to QA to test this PR.
          Hide
          ASF GitHub Bot added a comment -

          Github user Randgalt commented on a diff in the pull request:

          https://github.com/apache/zookeeper/pull/84#discussion_r85648967

          — Diff: src/docs/src/documentation/content/xdocs/zookeeperProgrammers.xml —
          @@ -1193,6 +1193,55 @@ authProvider.2=com.f.MyAuth2
          only one will be used. Also all servers must have the same plugins defined, otherwise clients using
          the authentication schemes provided by the plugins will have problems connecting to some servers.
          </para>
          +
          + <para> <emphasis role="bold">Added in 3.6.0</emphasis>: An alternate abstraction is available for pluggable
          + authentication. It provides additional arguments.
          + </para>
          +
          + <programlisting>
          +public abstract class ServerAuthenticationProvider implements AuthenticationProvider {
          — End diff –

          I'm copying the other parts of the zookeeperProgrammers.xml here. I built the docs and it looks good.

          Show
          ASF GitHub Bot added a comment - Github user Randgalt commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/84#discussion_r85648967 — Diff: src/docs/src/documentation/content/xdocs/zookeeperProgrammers.xml — @@ -1193,6 +1193,55 @@ authProvider.2=com.f.MyAuth2 only one will be used. Also all servers must have the same plugins defined, otherwise clients using the authentication schemes provided by the plugins will have problems connecting to some servers. </para> + + <para> <emphasis role="bold">Added in 3.6.0</emphasis>: An alternate abstraction is available for pluggable + authentication. It provides additional arguments. + </para> + + <programlisting> +public abstract class ServerAuthenticationProvider implements AuthenticationProvider { — End diff – I'm copying the other parts of the zookeeperProgrammers.xml here. I built the docs and it looks good.
          Hide
          ASF GitHub Bot added a comment -

          Github user Randgalt commented on a diff in the pull request:

          https://github.com/apache/zookeeper/pull/84#discussion_r85649005

          — Diff: src/java/test/org/apache/zookeeper/test/KeyAuthClientTest.java —
          @@ -0,0 +1,131 @@
          +/**
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +package org.apache.zookeeper.test;
          +
          +import org.apache.zookeeper.CreateMode;
          +import org.apache.zookeeper.KeeperException;
          +import org.apache.zookeeper.ZooDefs.Ids;
          +import org.apache.zookeeper.ZooKeeper;
          +import org.apache.zookeeper.data.ACL;
          +import org.junit.Assert;
          +import org.junit.Test;
          +import org.slf4j.Logger;
          +import org.slf4j.LoggerFactory;
          +
          +import java.util.List;
          +
          +public class KeyAuthClientTest extends ClientBase {
          + private static final Logger LOG = LoggerFactory.getLogger(KeyAuthClientTest.class);
          + static

          { + System.setProperty("zookeeper.authProvider.1","org.apache.zookeeper.server.auth.KeyAuthenticationProvider"); + }

          +
          + public void createNodePrintAcl(ZooKeeper zk, String path, String testName) {
          + try {
          + LOG.debug("KeyAuthenticationProvider Creating Test Node:"path".\n");
          + zk.create(path, null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT);
          + List<ACL> acls = zk.getACL(path, null);
          + LOG.debug("Node: "path" Test:"testName" ACLs:");
          — End diff –

          I didn't write this code - only copied it. But, I'll reformat the file to style.

          Show
          ASF GitHub Bot added a comment - Github user Randgalt commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/84#discussion_r85649005 — Diff: src/java/test/org/apache/zookeeper/test/KeyAuthClientTest.java — @@ -0,0 +1,131 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.zookeeper.test; + +import org.apache.zookeeper.CreateMode; +import org.apache.zookeeper.KeeperException; +import org.apache.zookeeper.ZooDefs.Ids; +import org.apache.zookeeper.ZooKeeper; +import org.apache.zookeeper.data.ACL; +import org.junit.Assert; +import org.junit.Test; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.util.List; + +public class KeyAuthClientTest extends ClientBase { + private static final Logger LOG = LoggerFactory.getLogger(KeyAuthClientTest.class); + static { + System.setProperty("zookeeper.authProvider.1","org.apache.zookeeper.server.auth.KeyAuthenticationProvider"); + } + + public void createNodePrintAcl(ZooKeeper zk, String path, String testName) { + try { + LOG.debug("KeyAuthenticationProvider Creating Test Node:" path ".\n"); + zk.create(path, null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT); + List<ACL> acls = zk.getACL(path, null); + LOG.debug("Node: " path " Test:" testName " ACLs:"); — End diff – I didn't write this code - only copied it. But, I'll reformat the file to style.
          Hide
          ASF GitHub Bot added a comment -

          Github user Randgalt commented on a diff in the pull request:

          https://github.com/apache/zookeeper/pull/84#discussion_r85649028

          — Diff: src/java/main/org/apache/zookeeper/server/auth/ProviderRegistry.java —
          @@ -31,7 +31,15 @@

          private static boolean initialized = false;
          private static HashMap<String, AuthenticationProvider> authenticationProviders =

          • new HashMap<String, AuthenticationProvider>();
            + new HashMap<>();
            +
            + //VisibleForTesting
            + public static void reset() {
              • End diff –

          I use it in the external code that I'm writing that uses this feature. I can see it being useful to others as well. Without it it's very difficult to write unit tests using custom auth plugins.

          Show
          ASF GitHub Bot added a comment - Github user Randgalt commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/84#discussion_r85649028 — Diff: src/java/main/org/apache/zookeeper/server/auth/ProviderRegistry.java — @@ -31,7 +31,15 @@ private static boolean initialized = false; private static HashMap<String, AuthenticationProvider> authenticationProviders = new HashMap<String, AuthenticationProvider>(); + new HashMap<>(); + + //VisibleForTesting + public static void reset() { End diff – I use it in the external code that I'm writing that uses this feature. I can see it being useful to others as well. Without it it's very difficult to write unit tests using custom auth plugins.
          Hide
          ASF GitHub Bot added a comment -

          Github user Randgalt commented on a diff in the pull request:

          https://github.com/apache/zookeeper/pull/84#discussion_r85649050

          — Diff: src/java/main/org/apache/zookeeper/server/auth/WrappedAuthenticationProvider.java —
          @@ -0,0 +1,74 @@
          +/**
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +package org.apache.zookeeper.server.auth;
          +
          +import org.apache.zookeeper.KeeperException;
          +import org.apache.zookeeper.data.ACL;
          +import org.apache.zookeeper.server.ServerCnxn;
          +import org.apache.zookeeper.server.ZooKeeperServer;
          +
          +import java.util.List;
          +
          +/**
          + * Provides backwards compatibility between older

          {@link AuthenticationProvider}

          + * implementations and the new

          {@link ServerAuthenticationProvider}

          interface.
          + */
          +class WrappedAuthenticationProvider extends ServerAuthenticationProvider {
          + private final AuthenticationProvider implementation;
          +
          + static ServerAuthenticationProvider wrap(AuthenticationProvider provider)

          { + return (provider instanceof ServerAuthenticationProvider) ? (ServerAuthenticationProvider)provider + : new WrappedAuthenticationProvider(provider); + }

          +
          + private WrappedAuthenticationProvider(AuthenticationProvider implementation)

          { + this.implementation = implementation; + }

          +
          + @Override
          + public KeeperException.Code handleAuthentication(ZooKeeperServer zks, ServerCnxn cnxn, byte[] authData)

          { + return implementation.handleAuthentication(cnxn, authData); + }

          +
          + /**
          + *

          {@inheritDoc}

          — End diff –

          Yes, you are correct.

          >Do we have to do it for this method only or for others as well?
          Well, it can be done anywhere. I only need it here.

          Show
          ASF GitHub Bot added a comment - Github user Randgalt commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/84#discussion_r85649050 — Diff: src/java/main/org/apache/zookeeper/server/auth/WrappedAuthenticationProvider.java — @@ -0,0 +1,74 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.zookeeper.server.auth; + +import org.apache.zookeeper.KeeperException; +import org.apache.zookeeper.data.ACL; +import org.apache.zookeeper.server.ServerCnxn; +import org.apache.zookeeper.server.ZooKeeperServer; + +import java.util.List; + +/** + * Provides backwards compatibility between older {@link AuthenticationProvider} + * implementations and the new {@link ServerAuthenticationProvider} interface. + */ +class WrappedAuthenticationProvider extends ServerAuthenticationProvider { + private final AuthenticationProvider implementation; + + static ServerAuthenticationProvider wrap(AuthenticationProvider provider) { + return (provider instanceof ServerAuthenticationProvider) ? (ServerAuthenticationProvider)provider + : new WrappedAuthenticationProvider(provider); + } + + private WrappedAuthenticationProvider(AuthenticationProvider implementation) { + this.implementation = implementation; + } + + @Override + public KeeperException.Code handleAuthentication(ZooKeeperServer zks, ServerCnxn cnxn, byte[] authData) { + return implementation.handleAuthentication(cnxn, authData); + } + + /** + * {@inheritDoc} — End diff – Yes, you are correct. >Do we have to do it for this method only or for others as well? Well, it can be done anywhere. I only need it here.
          Hide
          ASF GitHub Bot added a comment -

          Github user Randgalt commented on the issue:

          https://github.com/apache/zookeeper/pull/84

          @fpj I've addressed and pushed changes for all your comments.

          Show
          ASF GitHub Bot added a comment - Github user Randgalt commented on the issue: https://github.com/apache/zookeeper/pull/84 @fpj I've addressed and pushed changes for all your comments.
          Hide
          ASF GitHub Bot added a comment -

          Github user Randgalt commented on the issue:

          https://github.com/apache/zookeeper/pull/84

          @fpj any updates on this?

          Show
          ASF GitHub Bot added a comment - Github user Randgalt commented on the issue: https://github.com/apache/zookeeper/pull/84 @fpj any updates on this?
          Hide
          ASF GitHub Bot added a comment -

          Github user fpj commented on the issue:

          https://github.com/apache/zookeeper/pull/84

          @Randgalt I'm sorry that QA isn't working for pull requests yet, could please create a diff patch and upload to the jira so that we can get QA to run on it?

          Show
          ASF GitHub Bot added a comment - Github user fpj commented on the issue: https://github.com/apache/zookeeper/pull/84 @Randgalt I'm sorry that QA isn't working for pull requests yet, could please create a diff patch and upload to the jira so that we can get QA to run on it?
          Hide
          Jordan Zimmerman added a comment -

          Latest patch

          Show
          Jordan Zimmerman added a comment - Latest patch
          Hide
          ASF GitHub Bot added a comment -

          Github user Randgalt commented on the issue:

          https://github.com/apache/zookeeper/pull/84

          latest patch added

          Show
          ASF GitHub Bot added a comment - Github user Randgalt commented on the issue: https://github.com/apache/zookeeper/pull/84 latest patch added
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12836663/ZOOKEEPER-1525.patch
          against trunk revision f6349d16fcd5f04b560095417fd2a1813ac3e855.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 findbugs. The patch appears to introduce 4 new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3508//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3508//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3508//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12836663/ZOOKEEPER-1525.patch against trunk revision f6349d16fcd5f04b560095417fd2a1813ac3e855. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 4 new Findbugs (version 2.0.3) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3508//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3508//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3508//console This message is automatically generated.
          Hide
          Jordan Zimmerman added a comment -

          I'll work on these issues.

          Show
          Jordan Zimmerman added a comment - I'll work on these issues.
          Hide
          Jordan Zimmerman added a comment -

          Fix bad tests and findbugs issues

          Show
          Jordan Zimmerman added a comment - Fix bad tests and findbugs issues
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12836937/ZOOKEEPER-1525.patch
          against trunk revision bcb07a09b06c91243ed244f04a71b8daf629e286.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 findbugs. The patch appears to introduce 19 new Findbugs (version 3.0.1) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3515//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3515//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3515//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12836937/ZOOKEEPER-1525.patch against trunk revision bcb07a09b06c91243ed244f04a71b8daf629e286. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 19 new Findbugs (version 3.0.1) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3515//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3515//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3515//console This message is automatically generated.
          Hide
          Jordan Zimmerman added a comment -

          getServerProvider and getProvider imps were reversed

          Show
          Jordan Zimmerman added a comment - getServerProvider and getProvider imps were reversed
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12837217/ZOOKEEPER-1525.patch
          against trunk revision bcb07a09b06c91243ed244f04a71b8daf629e286.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 findbugs. The patch appears to introduce 19 new Findbugs (version 3.0.1) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3518//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3518//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3518//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12837217/ZOOKEEPER-1525.patch against trunk revision bcb07a09b06c91243ed244f04a71b8daf629e286. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 19 new Findbugs (version 3.0.1) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3518//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3518//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3518//console This message is automatically generated.
          Hide
          Jordan Zimmerman added a comment -

          As has been discussed elsewhere, the FindBugs issues are not related to this PR. So, AFAICT, this passes.

          Show
          Jordan Zimmerman added a comment - As has been discussed elsewhere, the FindBugs issues are not related to this PR. So, AFAICT, this passes.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12837217/ZOOKEEPER-1525.patch
          against trunk revision bcb07a09b06c91243ed244f04a71b8daf629e286.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 findbugs. The patch appears to introduce 19 new Findbugs (version 3.0.1) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3519//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3519//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3519//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12837217/ZOOKEEPER-1525.patch against trunk revision bcb07a09b06c91243ed244f04a71b8daf629e286. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 19 new Findbugs (version 3.0.1) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3519//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3519//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3519//console This message is automatically generated.
          Hide
          ASF GitHub Bot added a comment -

          Github user lvfangmin commented on a diff in the pull request:

          https://github.com/apache/zookeeper/pull/84#discussion_r86824520

          — Diff: src/java/main/org/apache/zookeeper/server/auth/KeyAuthenticationProvider.java —
          @@ -0,0 +1,139 @@
          +/**
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +package org.apache.zookeeper.server.auth;
          +
          +import java.io.UnsupportedEncodingException;
          +import java.util.List;
          +
          +import org.apache.zookeeper.KeeperException;
          +import org.apache.zookeeper.KeeperException.NoNodeException;
          +import org.apache.zookeeper.data.ACL;
          +import org.apache.zookeeper.data.Id;
          +import org.apache.zookeeper.server.ServerCnxn;
          +import org.apache.zookeeper.server.ZooKeeperServer;
          +import org.apache.zookeeper.server.ZKDatabase;
          +import org.apache.zookeeper.data.Stat;
          +import org.slf4j.Logger;
          +import org.slf4j.LoggerFactory;
          +
          +/*
          + * This class is a sample implementation of being passed the ZooKeeperServer
          + * handle in the constructor, and reading data from zknodes to authenticate.
          + * At a minimum, a real Auth provider would need to override validate() to
          + * e.g. perform certificate validation of auth based a public key.
          + *
          + * See the "Pluggable ZooKeeper authentication" section of the
          + * "Zookeeper Programmer's Guide" for general details of implementing an
          + * authentication plugin. e.g.
          + * http://zookeeper.apache.org/doc/trunk/zookeeperProgrammers.html#sc_ZooKeeperPluggableAuthentication
          + *
          + * This class looks for a numeric "key" under the /key node.
          + * Authorizaton is granted if the user passes in as authorization a number
          + * which is a multiple of the key value, i.e.
          + * (auth % key) == 0
          + * In a real implementation, you might do something like storing a public
          + * key in /key, and using it to verify that auth tokens passed in were signed
          + * by the corresponding private key.
          + *
          + * When the node /key does not exist, any auth token is accepted, so that
          + * bootstrapping may occur.
          + *
          + */
          +public class KeyAuthenticationProvider extends ServerAuthenticationProvider {
          + private static final Logger LOG = LoggerFactory.getLogger(KeyAuthenticationProvider.class);
          +
          + public String getScheme()

          { + return "key"; + }

          +
          + public byte[] getKey(ZooKeeperServer zks) {
          + ZKDatabase db = zks.getZKDatabase();
          + if (db != null) {
          + try

          { + Stat stat = new Stat(); + return db.getData("/key", stat, null); + }

          catch (NoNodeException e)

          { + // ignore + }

          + }
          + return null;
          + }
          +
          + public boolean validate(byte[] key, byte[] auth) {
          + // perform arbitrary function (auth is a multiple of key)
          + try {
          + String keyStr = new String(key, "UTF-8");
          + String authStr = new String(auth, "UTF-8");
          + int keyVal = Integer.parseInt(keyStr);
          + int authVal = Integer.parseInt(authStr);
          + if (keyVal!=0 && ((authVal % keyVal) != 0))

          { + return false; + }

          + } catch (NumberFormatException | UnsupportedEncodingException nfe) {
          + return false;
          — End diff –

          Maybe it's nice to log error here for debugging purpose.

          Show
          ASF GitHub Bot added a comment - Github user lvfangmin commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/84#discussion_r86824520 — Diff: src/java/main/org/apache/zookeeper/server/auth/KeyAuthenticationProvider.java — @@ -0,0 +1,139 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.zookeeper.server.auth; + +import java.io.UnsupportedEncodingException; +import java.util.List; + +import org.apache.zookeeper.KeeperException; +import org.apache.zookeeper.KeeperException.NoNodeException; +import org.apache.zookeeper.data.ACL; +import org.apache.zookeeper.data.Id; +import org.apache.zookeeper.server.ServerCnxn; +import org.apache.zookeeper.server.ZooKeeperServer; +import org.apache.zookeeper.server.ZKDatabase; +import org.apache.zookeeper.data.Stat; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/* + * This class is a sample implementation of being passed the ZooKeeperServer + * handle in the constructor, and reading data from zknodes to authenticate. + * At a minimum, a real Auth provider would need to override validate() to + * e.g. perform certificate validation of auth based a public key. + * + * See the "Pluggable ZooKeeper authentication" section of the + * "Zookeeper Programmer's Guide" for general details of implementing an + * authentication plugin. e.g. + * http://zookeeper.apache.org/doc/trunk/zookeeperProgrammers.html#sc_ZooKeeperPluggableAuthentication + * + * This class looks for a numeric "key" under the /key node. + * Authorizaton is granted if the user passes in as authorization a number + * which is a multiple of the key value, i.e. + * (auth % key) == 0 + * In a real implementation, you might do something like storing a public + * key in /key, and using it to verify that auth tokens passed in were signed + * by the corresponding private key. + * + * When the node /key does not exist, any auth token is accepted, so that + * bootstrapping may occur. + * + */ +public class KeyAuthenticationProvider extends ServerAuthenticationProvider { + private static final Logger LOG = LoggerFactory.getLogger(KeyAuthenticationProvider.class); + + public String getScheme() { + return "key"; + } + + public byte[] getKey(ZooKeeperServer zks) { + ZKDatabase db = zks.getZKDatabase(); + if (db != null) { + try { + Stat stat = new Stat(); + return db.getData("/key", stat, null); + } catch (NoNodeException e) { + // ignore + } + } + return null; + } + + public boolean validate(byte[] key, byte[] auth) { + // perform arbitrary function (auth is a multiple of key) + try { + String keyStr = new String(key, "UTF-8"); + String authStr = new String(auth, "UTF-8"); + int keyVal = Integer.parseInt(keyStr); + int authVal = Integer.parseInt(authStr); + if (keyVal!=0 && ((authVal % keyVal) != 0)) { + return false; + } + } catch (NumberFormatException | UnsupportedEncodingException nfe) { + return false; — End diff – Maybe it's nice to log error here for debugging purpose.
          Hide
          ASF GitHub Bot added a comment -

          Github user lvfangmin commented on a diff in the pull request:

          https://github.com/apache/zookeeper/pull/84#discussion_r86857366

          — Diff: src/java/main/org/apache/zookeeper/server/auth/KeyAuthenticationProvider.java —
          @@ -0,0 +1,139 @@
          +/**
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +package org.apache.zookeeper.server.auth;
          +
          +import java.io.UnsupportedEncodingException;
          +import java.util.List;
          +
          +import org.apache.zookeeper.KeeperException;
          +import org.apache.zookeeper.KeeperException.NoNodeException;
          +import org.apache.zookeeper.data.ACL;
          +import org.apache.zookeeper.data.Id;
          +import org.apache.zookeeper.server.ServerCnxn;
          +import org.apache.zookeeper.server.ZooKeeperServer;
          +import org.apache.zookeeper.server.ZKDatabase;
          +import org.apache.zookeeper.data.Stat;
          +import org.slf4j.Logger;
          +import org.slf4j.LoggerFactory;
          +
          +/*
          + * This class is a sample implementation of being passed the ZooKeeperServer
          + * handle in the constructor, and reading data from zknodes to authenticate.
          + * At a minimum, a real Auth provider would need to override validate() to
          + * e.g. perform certificate validation of auth based a public key.
          + *
          + * See the "Pluggable ZooKeeper authentication" section of the
          + * "Zookeeper Programmer's Guide" for general details of implementing an
          + * authentication plugin. e.g.
          + * http://zookeeper.apache.org/doc/trunk/zookeeperProgrammers.html#sc_ZooKeeperPluggableAuthentication
          + *
          + * This class looks for a numeric "key" under the /key node.
          + * Authorizaton is granted if the user passes in as authorization a number
          + * which is a multiple of the key value, i.e.
          + * (auth % key) == 0
          + * In a real implementation, you might do something like storing a public
          + * key in /key, and using it to verify that auth tokens passed in were signed
          + * by the corresponding private key.
          + *
          + * When the node /key does not exist, any auth token is accepted, so that
          + * bootstrapping may occur.
          + *
          + */
          +public class KeyAuthenticationProvider extends ServerAuthenticationProvider {
          + private static final Logger LOG = LoggerFactory.getLogger(KeyAuthenticationProvider.class);
          +
          + public String getScheme()

          { + return "key"; + }

          +
          + public byte[] getKey(ZooKeeperServer zks) {
          — End diff –

          Do we need this to be public? I only saw it's being called in handleAuthentication.

          Show
          ASF GitHub Bot added a comment - Github user lvfangmin commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/84#discussion_r86857366 — Diff: src/java/main/org/apache/zookeeper/server/auth/KeyAuthenticationProvider.java — @@ -0,0 +1,139 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.zookeeper.server.auth; + +import java.io.UnsupportedEncodingException; +import java.util.List; + +import org.apache.zookeeper.KeeperException; +import org.apache.zookeeper.KeeperException.NoNodeException; +import org.apache.zookeeper.data.ACL; +import org.apache.zookeeper.data.Id; +import org.apache.zookeeper.server.ServerCnxn; +import org.apache.zookeeper.server.ZooKeeperServer; +import org.apache.zookeeper.server.ZKDatabase; +import org.apache.zookeeper.data.Stat; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/* + * This class is a sample implementation of being passed the ZooKeeperServer + * handle in the constructor, and reading data from zknodes to authenticate. + * At a minimum, a real Auth provider would need to override validate() to + * e.g. perform certificate validation of auth based a public key. + * + * See the "Pluggable ZooKeeper authentication" section of the + * "Zookeeper Programmer's Guide" for general details of implementing an + * authentication plugin. e.g. + * http://zookeeper.apache.org/doc/trunk/zookeeperProgrammers.html#sc_ZooKeeperPluggableAuthentication + * + * This class looks for a numeric "key" under the /key node. + * Authorizaton is granted if the user passes in as authorization a number + * which is a multiple of the key value, i.e. + * (auth % key) == 0 + * In a real implementation, you might do something like storing a public + * key in /key, and using it to verify that auth tokens passed in were signed + * by the corresponding private key. + * + * When the node /key does not exist, any auth token is accepted, so that + * bootstrapping may occur. + * + */ +public class KeyAuthenticationProvider extends ServerAuthenticationProvider { + private static final Logger LOG = LoggerFactory.getLogger(KeyAuthenticationProvider.class); + + public String getScheme() { + return "key"; + } + + public byte[] getKey(ZooKeeperServer zks) { — End diff – Do we need this to be public? I only saw it's being called in handleAuthentication.
          Hide
          ASF GitHub Bot added a comment -

          Github user lvfangmin commented on a diff in the pull request:

          https://github.com/apache/zookeeper/pull/84#discussion_r86824750

          — Diff: src/java/main/org/apache/zookeeper/server/auth/KeyAuthenticationProvider.java —
          @@ -0,0 +1,139 @@
          +/**
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +package org.apache.zookeeper.server.auth;
          +
          +import java.io.UnsupportedEncodingException;
          +import java.util.List;
          +
          +import org.apache.zookeeper.KeeperException;
          +import org.apache.zookeeper.KeeperException.NoNodeException;
          +import org.apache.zookeeper.data.ACL;
          +import org.apache.zookeeper.data.Id;
          +import org.apache.zookeeper.server.ServerCnxn;
          +import org.apache.zookeeper.server.ZooKeeperServer;
          +import org.apache.zookeeper.server.ZKDatabase;
          +import org.apache.zookeeper.data.Stat;
          +import org.slf4j.Logger;
          +import org.slf4j.LoggerFactory;
          +
          +/*
          + * This class is a sample implementation of being passed the ZooKeeperServer
          + * handle in the constructor, and reading data from zknodes to authenticate.
          + * At a minimum, a real Auth provider would need to override validate() to
          + * e.g. perform certificate validation of auth based a public key.
          + *
          + * See the "Pluggable ZooKeeper authentication" section of the
          + * "Zookeeper Programmer's Guide" for general details of implementing an
          + * authentication plugin. e.g.
          + * http://zookeeper.apache.org/doc/trunk/zookeeperProgrammers.html#sc_ZooKeeperPluggableAuthentication
          + *
          + * This class looks for a numeric "key" under the /key node.
          + * Authorizaton is granted if the user passes in as authorization a number
          + * which is a multiple of the key value, i.e.
          + * (auth % key) == 0
          + * In a real implementation, you might do something like storing a public
          + * key in /key, and using it to verify that auth tokens passed in were signed
          + * by the corresponding private key.
          + *
          + * When the node /key does not exist, any auth token is accepted, so that
          + * bootstrapping may occur.
          + *
          + */
          +public class KeyAuthenticationProvider extends ServerAuthenticationProvider {
          + private static final Logger LOG = LoggerFactory.getLogger(KeyAuthenticationProvider.class);
          +
          + public String getScheme()

          { + return "key"; + }

          +
          + public byte[] getKey(ZooKeeperServer zks) {
          + ZKDatabase db = zks.getZKDatabase();
          + if (db != null) {
          + try

          { + Stat stat = new Stat(); + return db.getData("/key", stat, null); + }

          catch (NoNodeException e)

          { + // ignore + }

          + }
          + return null;
          + }
          +
          + public boolean validate(byte[] key, byte[] auth) {
          + // perform arbitrary function (auth is a multiple of key)
          + try {
          + String keyStr = new String(key, "UTF-8");
          + String authStr = new String(auth, "UTF-8");
          + int keyVal = Integer.parseInt(keyStr);
          + int authVal = Integer.parseInt(authStr);
          + if (keyVal!=0 && ((authVal % keyVal) != 0))

          { + return false; + }

          + } catch (NumberFormatException | UnsupportedEncodingException nfe)

          { + return false; + }

          + return true;
          + }
          +
          + @Override
          + public KeeperException.Code handleAuthentication(ZooKeeperServer zks, ServerCnxn cnxn, byte[] authData) {
          + byte[] key = getKey(zks);
          + String authStr = "";
          + String keyStr = "";
          + try

          { + authStr = new String(authData, "UTF-8"); + }

          catch (Exception e) {
          + // empty authData
          — End diff –

          Same here, better to log the exception.

          Show
          ASF GitHub Bot added a comment - Github user lvfangmin commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/84#discussion_r86824750 — Diff: src/java/main/org/apache/zookeeper/server/auth/KeyAuthenticationProvider.java — @@ -0,0 +1,139 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.zookeeper.server.auth; + +import java.io.UnsupportedEncodingException; +import java.util.List; + +import org.apache.zookeeper.KeeperException; +import org.apache.zookeeper.KeeperException.NoNodeException; +import org.apache.zookeeper.data.ACL; +import org.apache.zookeeper.data.Id; +import org.apache.zookeeper.server.ServerCnxn; +import org.apache.zookeeper.server.ZooKeeperServer; +import org.apache.zookeeper.server.ZKDatabase; +import org.apache.zookeeper.data.Stat; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/* + * This class is a sample implementation of being passed the ZooKeeperServer + * handle in the constructor, and reading data from zknodes to authenticate. + * At a minimum, a real Auth provider would need to override validate() to + * e.g. perform certificate validation of auth based a public key. + * + * See the "Pluggable ZooKeeper authentication" section of the + * "Zookeeper Programmer's Guide" for general details of implementing an + * authentication plugin. e.g. + * http://zookeeper.apache.org/doc/trunk/zookeeperProgrammers.html#sc_ZooKeeperPluggableAuthentication + * + * This class looks for a numeric "key" under the /key node. + * Authorizaton is granted if the user passes in as authorization a number + * which is a multiple of the key value, i.e. + * (auth % key) == 0 + * In a real implementation, you might do something like storing a public + * key in /key, and using it to verify that auth tokens passed in were signed + * by the corresponding private key. + * + * When the node /key does not exist, any auth token is accepted, so that + * bootstrapping may occur. + * + */ +public class KeyAuthenticationProvider extends ServerAuthenticationProvider { + private static final Logger LOG = LoggerFactory.getLogger(KeyAuthenticationProvider.class); + + public String getScheme() { + return "key"; + } + + public byte[] getKey(ZooKeeperServer zks) { + ZKDatabase db = zks.getZKDatabase(); + if (db != null) { + try { + Stat stat = new Stat(); + return db.getData("/key", stat, null); + } catch (NoNodeException e) { + // ignore + } + } + return null; + } + + public boolean validate(byte[] key, byte[] auth) { + // perform arbitrary function (auth is a multiple of key) + try { + String keyStr = new String(key, "UTF-8"); + String authStr = new String(auth, "UTF-8"); + int keyVal = Integer.parseInt(keyStr); + int authVal = Integer.parseInt(authStr); + if (keyVal!=0 && ((authVal % keyVal) != 0)) { + return false; + } + } catch (NumberFormatException | UnsupportedEncodingException nfe) { + return false; + } + return true; + } + + @Override + public KeeperException.Code handleAuthentication(ZooKeeperServer zks, ServerCnxn cnxn, byte[] authData) { + byte[] key = getKey(zks); + String authStr = ""; + String keyStr = ""; + try { + authStr = new String(authData, "UTF-8"); + } catch (Exception e) { + // empty authData — End diff – Same here, better to log the exception.
          Hide
          ASF GitHub Bot added a comment -

          Github user lvfangmin commented on a diff in the pull request:

          https://github.com/apache/zookeeper/pull/84#discussion_r86823972

          — Diff: src/java/main/org/apache/zookeeper/server/auth/AuthenticationProvider.java —
          @@ -20,6 +20,9 @@

          import org.apache.zookeeper.KeeperException;
          import org.apache.zookeeper.server.ServerCnxn;
          +import org.apache.zookeeper.server.ZooKeeperServer;
          +
          +import java.util.List;
          — End diff –

          These imports are not being used, get rid of it?

          Show
          ASF GitHub Bot added a comment - Github user lvfangmin commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/84#discussion_r86823972 — Diff: src/java/main/org/apache/zookeeper/server/auth/AuthenticationProvider.java — @@ -20,6 +20,9 @@ import org.apache.zookeeper.KeeperException; import org.apache.zookeeper.server.ServerCnxn; +import org.apache.zookeeper.server.ZooKeeperServer; + +import java.util.List; — End diff – These imports are not being used, get rid of it?
          Hide
          ASF GitHub Bot added a comment -

          Github user Randgalt commented on a diff in the pull request:

          https://github.com/apache/zookeeper/pull/84#discussion_r86868035

          — Diff: src/java/test/org/apache/zookeeper/test/KeyAuthClientTest.java —
          @@ -0,0 +1,131 @@
          +/**
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +package org.apache.zookeeper.test;
          +
          +import org.apache.zookeeper.CreateMode;
          +import org.apache.zookeeper.KeeperException;
          +import org.apache.zookeeper.ZooDefs.Ids;
          +import org.apache.zookeeper.ZooKeeper;
          +import org.apache.zookeeper.data.ACL;
          +import org.junit.Assert;
          +import org.junit.Test;
          +import org.slf4j.Logger;
          +import org.slf4j.LoggerFactory;
          +
          +import java.util.List;
          +
          +public class KeyAuthClientTest extends ClientBase {
          + private static final Logger LOG = LoggerFactory.getLogger(KeyAuthClientTest.class);
          + static

          { + System.setProperty("zookeeper.authProvider.1","org.apache.zookeeper.server.auth.KeyAuthenticationProvider"); + }

          +
          + public void createNodePrintAcl(ZooKeeper zk, String path, String testName) {
          + try {
          + LOG.debug("KeyAuthenticationProvider Creating Test Node:"path".\n");
          + zk.create(path, null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT);
          + List<ACL> acls = zk.getACL(path, null);
          + LOG.debug("Node: "path" Test:"testName" ACLs:");
          + for (ACL acl : acls)

          { + LOG.debug(" "+acl.toString()); + }

          + } catch (Exception e)

          { + LOG.debug(" EXCEPTION THROWN", e); + }

          + }
          +
          + public void testPreAuth() throws Exception {
          + ZooKeeper zk = createClient();
          + zk.addAuthInfo("key", "25".getBytes());
          + try {
          + createNodePrintAcl(zk, "/pre", "testPreAuth");
          + zk.setACL("/", Ids.CREATOR_ALL_ACL, -1);
          + zk.getChildren("/", false);
          + zk.create("/abc", null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT);
          + zk.setData("/abc", "testData1".getBytes(), -1);
          + zk.create("/key", null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT);
          + zk.setData("/key", "5".getBytes(), -1);
          + Thread.sleep(1000);
          — End diff –

          Me neither - I didn't write this code.

          Show
          ASF GitHub Bot added a comment - Github user Randgalt commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/84#discussion_r86868035 — Diff: src/java/test/org/apache/zookeeper/test/KeyAuthClientTest.java — @@ -0,0 +1,131 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.zookeeper.test; + +import org.apache.zookeeper.CreateMode; +import org.apache.zookeeper.KeeperException; +import org.apache.zookeeper.ZooDefs.Ids; +import org.apache.zookeeper.ZooKeeper; +import org.apache.zookeeper.data.ACL; +import org.junit.Assert; +import org.junit.Test; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.util.List; + +public class KeyAuthClientTest extends ClientBase { + private static final Logger LOG = LoggerFactory.getLogger(KeyAuthClientTest.class); + static { + System.setProperty("zookeeper.authProvider.1","org.apache.zookeeper.server.auth.KeyAuthenticationProvider"); + } + + public void createNodePrintAcl(ZooKeeper zk, String path, String testName) { + try { + LOG.debug("KeyAuthenticationProvider Creating Test Node:" path ".\n"); + zk.create(path, null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT); + List<ACL> acls = zk.getACL(path, null); + LOG.debug("Node: " path " Test:" testName " ACLs:"); + for (ACL acl : acls) { + LOG.debug(" "+acl.toString()); + } + } catch (Exception e) { + LOG.debug(" EXCEPTION THROWN", e); + } + } + + public void testPreAuth() throws Exception { + ZooKeeper zk = createClient(); + zk.addAuthInfo("key", "25".getBytes()); + try { + createNodePrintAcl(zk, "/pre", "testPreAuth"); + zk.setACL("/", Ids.CREATOR_ALL_ACL, -1); + zk.getChildren("/", false); + zk.create("/abc", null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT); + zk.setData("/abc", "testData1".getBytes(), -1); + zk.create("/key", null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT); + zk.setData("/key", "5".getBytes(), -1); + Thread.sleep(1000); — End diff – Me neither - I didn't write this code.
          Hide
          ASF GitHub Bot added a comment -

          Github user eribeiro commented on a diff in the pull request:

          https://github.com/apache/zookeeper/pull/84#discussion_r87052291

          — Diff: src/java/test/org/apache/zookeeper/test/KeyAuthClientTest.java —
          @@ -0,0 +1,131 @@
          +/**
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +package org.apache.zookeeper.test;
          +
          +import org.apache.zookeeper.CreateMode;
          +import org.apache.zookeeper.KeeperException;
          +import org.apache.zookeeper.ZooDefs.Ids;
          +import org.apache.zookeeper.ZooKeeper;
          +import org.apache.zookeeper.data.ACL;
          +import org.junit.Assert;
          +import org.junit.Test;
          +import org.slf4j.Logger;
          +import org.slf4j.LoggerFactory;
          +
          +import java.util.List;
          +
          +public class KeyAuthClientTest extends ClientBase {
          + private static final Logger LOG = LoggerFactory.getLogger(KeyAuthClientTest.class);
          + static

          { + System.setProperty("zookeeper.authProvider.1","org.apache.zookeeper.server.auth.KeyAuthenticationProvider"); + }

          +
          + public void createNodePrintAcl(ZooKeeper zk, String path, String testName) {
          + try {
          + LOG.debug("KeyAuthenticationProvider Creating Test Node:"path".\n");
          + zk.create(path, null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT);
          + List<ACL> acls = zk.getACL(path, null);
          + LOG.debug("Node: "path" Test:"testName" ACLs:");
          + for (ACL acl : acls)

          { + LOG.debug(" "+acl.toString()); + }

          + } catch (Exception e)

          { + LOG.debug(" EXCEPTION THROWN", e); + }

          + }
          +
          + public void testPreAuth() throws Exception {
          + ZooKeeper zk = createClient();
          + zk.addAuthInfo("key", "25".getBytes());
          + try {
          + createNodePrintAcl(zk, "/pre", "testPreAuth");
          + zk.setACL("/", Ids.CREATOR_ALL_ACL, -1);
          + zk.getChildren("/", false);
          + zk.create("/abc", null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT);
          + zk.setData("/abc", "testData1".getBytes(), -1);
          + zk.create("/key", null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT);
          + zk.setData("/key", "5".getBytes(), -1);
          + Thread.sleep(1000);
          — End diff –

          Only thing I can suppose (and *please* correct me if I am saying something *utterly stupid*) is that this ``sleep`` was inserted in the hopes of giving some time to ZK commit the changes. Naive but yet (shrug)...

          Well, I guess we can either remove it or replace it with a ``zk.getData("/key");``, right?

          Show
          ASF GitHub Bot added a comment - Github user eribeiro commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/84#discussion_r87052291 — Diff: src/java/test/org/apache/zookeeper/test/KeyAuthClientTest.java — @@ -0,0 +1,131 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.zookeeper.test; + +import org.apache.zookeeper.CreateMode; +import org.apache.zookeeper.KeeperException; +import org.apache.zookeeper.ZooDefs.Ids; +import org.apache.zookeeper.ZooKeeper; +import org.apache.zookeeper.data.ACL; +import org.junit.Assert; +import org.junit.Test; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.util.List; + +public class KeyAuthClientTest extends ClientBase { + private static final Logger LOG = LoggerFactory.getLogger(KeyAuthClientTest.class); + static { + System.setProperty("zookeeper.authProvider.1","org.apache.zookeeper.server.auth.KeyAuthenticationProvider"); + } + + public void createNodePrintAcl(ZooKeeper zk, String path, String testName) { + try { + LOG.debug("KeyAuthenticationProvider Creating Test Node:" path ".\n"); + zk.create(path, null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT); + List<ACL> acls = zk.getACL(path, null); + LOG.debug("Node: " path " Test:" testName " ACLs:"); + for (ACL acl : acls) { + LOG.debug(" "+acl.toString()); + } + } catch (Exception e) { + LOG.debug(" EXCEPTION THROWN", e); + } + } + + public void testPreAuth() throws Exception { + ZooKeeper zk = createClient(); + zk.addAuthInfo("key", "25".getBytes()); + try { + createNodePrintAcl(zk, "/pre", "testPreAuth"); + zk.setACL("/", Ids.CREATOR_ALL_ACL, -1); + zk.getChildren("/", false); + zk.create("/abc", null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT); + zk.setData("/abc", "testData1".getBytes(), -1); + zk.create("/key", null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT); + zk.setData("/key", "5".getBytes(), -1); + Thread.sleep(1000); — End diff – Only thing I can suppose (and * please * correct me if I am saying something * utterly stupid *) is that this ``sleep`` was inserted in the hopes of giving some time to ZK commit the changes. Naive but yet (shrug)... Well, I guess we can either remove it or replace it with a ``zk.getData("/key");``, right?
          Hide
          ASF GitHub Bot added a comment -

          Github user Randgalt commented on a diff in the pull request:

          https://github.com/apache/zookeeper/pull/84#discussion_r87054533

          — Diff: src/java/test/org/apache/zookeeper/test/KeyAuthClientTest.java —
          @@ -0,0 +1,131 @@
          +/**
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +package org.apache.zookeeper.test;
          +
          +import org.apache.zookeeper.CreateMode;
          +import org.apache.zookeeper.KeeperException;
          +import org.apache.zookeeper.ZooDefs.Ids;
          +import org.apache.zookeeper.ZooKeeper;
          +import org.apache.zookeeper.data.ACL;
          +import org.junit.Assert;
          +import org.junit.Test;
          +import org.slf4j.Logger;
          +import org.slf4j.LoggerFactory;
          +
          +import java.util.List;
          +
          +public class KeyAuthClientTest extends ClientBase {
          + private static final Logger LOG = LoggerFactory.getLogger(KeyAuthClientTest.class);
          + static

          { + System.setProperty("zookeeper.authProvider.1","org.apache.zookeeper.server.auth.KeyAuthenticationProvider"); + }

          +
          + public void createNodePrintAcl(ZooKeeper zk, String path, String testName) {
          + try {
          + LOG.debug("KeyAuthenticationProvider Creating Test Node:"path".\n");
          + zk.create(path, null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT);
          + List<ACL> acls = zk.getACL(path, null);
          + LOG.debug("Node: "path" Test:"testName" ACLs:");
          + for (ACL acl : acls)

          { + LOG.debug(" "+acl.toString()); + }

          + } catch (Exception e)

          { + LOG.debug(" EXCEPTION THROWN", e); + }

          + }
          +
          + public void testPreAuth() throws Exception {
          + ZooKeeper zk = createClient();
          + zk.addAuthInfo("key", "25".getBytes());
          + try {
          + createNodePrintAcl(zk, "/pre", "testPreAuth");
          + zk.setACL("/", Ids.CREATOR_ALL_ACL, -1);
          + zk.getChildren("/", false);
          + zk.create("/abc", null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT);
          + zk.setData("/abc", "testData1".getBytes(), -1);
          + zk.create("/key", null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT);
          + zk.setData("/key", "5".getBytes(), -1);
          + Thread.sleep(1000);
          — End diff –

          These types of sleeps are all over the ZK tests (and Curator for that matter).

          Show
          ASF GitHub Bot added a comment - Github user Randgalt commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/84#discussion_r87054533 — Diff: src/java/test/org/apache/zookeeper/test/KeyAuthClientTest.java — @@ -0,0 +1,131 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.zookeeper.test; + +import org.apache.zookeeper.CreateMode; +import org.apache.zookeeper.KeeperException; +import org.apache.zookeeper.ZooDefs.Ids; +import org.apache.zookeeper.ZooKeeper; +import org.apache.zookeeper.data.ACL; +import org.junit.Assert; +import org.junit.Test; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.util.List; + +public class KeyAuthClientTest extends ClientBase { + private static final Logger LOG = LoggerFactory.getLogger(KeyAuthClientTest.class); + static { + System.setProperty("zookeeper.authProvider.1","org.apache.zookeeper.server.auth.KeyAuthenticationProvider"); + } + + public void createNodePrintAcl(ZooKeeper zk, String path, String testName) { + try { + LOG.debug("KeyAuthenticationProvider Creating Test Node:" path ".\n"); + zk.create(path, null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT); + List<ACL> acls = zk.getACL(path, null); + LOG.debug("Node: " path " Test:" testName " ACLs:"); + for (ACL acl : acls) { + LOG.debug(" "+acl.toString()); + } + } catch (Exception e) { + LOG.debug(" EXCEPTION THROWN", e); + } + } + + public void testPreAuth() throws Exception { + ZooKeeper zk = createClient(); + zk.addAuthInfo("key", "25".getBytes()); + try { + createNodePrintAcl(zk, "/pre", "testPreAuth"); + zk.setACL("/", Ids.CREATOR_ALL_ACL, -1); + zk.getChildren("/", false); + zk.create("/abc", null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT); + zk.setData("/abc", "testData1".getBytes(), -1); + zk.create("/key", null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT); + zk.setData("/key", "5".getBytes(), -1); + Thread.sleep(1000); — End diff – These types of sleeps are all over the ZK tests (and Curator for that matter).
          Hide
          ASF GitHub Bot added a comment -

          Github user eribeiro commented on a diff in the pull request:

          https://github.com/apache/zookeeper/pull/84#discussion_r87057041

          — Diff: src/java/test/org/apache/zookeeper/test/KeyAuthClientTest.java —
          @@ -0,0 +1,131 @@
          +/**
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +package org.apache.zookeeper.test;
          +
          +import org.apache.zookeeper.CreateMode;
          +import org.apache.zookeeper.KeeperException;
          +import org.apache.zookeeper.ZooDefs.Ids;
          +import org.apache.zookeeper.ZooKeeper;
          +import org.apache.zookeeper.data.ACL;
          +import org.junit.Assert;
          +import org.junit.Test;
          +import org.slf4j.Logger;
          +import org.slf4j.LoggerFactory;
          +
          +import java.util.List;
          +
          +public class KeyAuthClientTest extends ClientBase {
          + private static final Logger LOG = LoggerFactory.getLogger(KeyAuthClientTest.class);
          + static

          { + System.setProperty("zookeeper.authProvider.1","org.apache.zookeeper.server.auth.KeyAuthenticationProvider"); + }

          +
          + public void createNodePrintAcl(ZooKeeper zk, String path, String testName) {
          + try {
          + LOG.debug("KeyAuthenticationProvider Creating Test Node:"path".\n");
          + zk.create(path, null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT);
          + List<ACL> acls = zk.getACL(path, null);
          + LOG.debug("Node: "path" Test:"testName" ACLs:");
          + for (ACL acl : acls)

          { + LOG.debug(" "+acl.toString()); + }

          + } catch (Exception e)

          { + LOG.debug(" EXCEPTION THROWN", e); + }

          + }
          +
          + public void testPreAuth() throws Exception {
          + ZooKeeper zk = createClient();
          + zk.addAuthInfo("key", "25".getBytes());
          + try {
          + createNodePrintAcl(zk, "/pre", "testPreAuth");
          + zk.setACL("/", Ids.CREATOR_ALL_ACL, -1);
          + zk.getChildren("/", false);
          + zk.create("/abc", null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT);
          + zk.setData("/abc", "testData1".getBytes(), -1);
          + zk.create("/key", null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT);
          + zk.setData("/key", "5".getBytes(), -1);
          + Thread.sleep(1000);
          — End diff –

          So, a question for a future ticket (IMHO). :cactus:

          Show
          ASF GitHub Bot added a comment - Github user eribeiro commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/84#discussion_r87057041 — Diff: src/java/test/org/apache/zookeeper/test/KeyAuthClientTest.java — @@ -0,0 +1,131 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.zookeeper.test; + +import org.apache.zookeeper.CreateMode; +import org.apache.zookeeper.KeeperException; +import org.apache.zookeeper.ZooDefs.Ids; +import org.apache.zookeeper.ZooKeeper; +import org.apache.zookeeper.data.ACL; +import org.junit.Assert; +import org.junit.Test; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.util.List; + +public class KeyAuthClientTest extends ClientBase { + private static final Logger LOG = LoggerFactory.getLogger(KeyAuthClientTest.class); + static { + System.setProperty("zookeeper.authProvider.1","org.apache.zookeeper.server.auth.KeyAuthenticationProvider"); + } + + public void createNodePrintAcl(ZooKeeper zk, String path, String testName) { + try { + LOG.debug("KeyAuthenticationProvider Creating Test Node:" path ".\n"); + zk.create(path, null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT); + List<ACL> acls = zk.getACL(path, null); + LOG.debug("Node: " path " Test:" testName " ACLs:"); + for (ACL acl : acls) { + LOG.debug(" "+acl.toString()); + } + } catch (Exception e) { + LOG.debug(" EXCEPTION THROWN", e); + } + } + + public void testPreAuth() throws Exception { + ZooKeeper zk = createClient(); + zk.addAuthInfo("key", "25".getBytes()); + try { + createNodePrintAcl(zk, "/pre", "testPreAuth"); + zk.setACL("/", Ids.CREATOR_ALL_ACL, -1); + zk.getChildren("/", false); + zk.create("/abc", null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT); + zk.setData("/abc", "testData1".getBytes(), -1); + zk.create("/key", null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT); + zk.setData("/key", "5".getBytes(), -1); + Thread.sleep(1000); — End diff – So, a question for a future ticket (IMHO). :cactus:
          Hide
          ASF GitHub Bot added a comment -

          Github user lvfangmin commented on the issue:

          https://github.com/apache/zookeeper/pull/84

          Generally looks good to me, but I have the same concern as @fpj, maybe in the future the newly added auth needs additional parameters, then we have to add another wrapper with new interface to keep the old ones backward compatible. I also agree a general metadata map is error-prone, but I think we can have a compromise solution between explicit parameters and general metadata map:

          Create a class (e.g. AuthData) with the explicit parameters(zks, cnxn, path, perm, setAcls) in it, which has the get and set method, and use this class in the newly added ServerAuthenticationProvider interface. In the future if we need additional parameter, we only need to change this class, no interface and backward compatible issues will be involved. What do you think @Randgalt @fpj?

          Show
          ASF GitHub Bot added a comment - Github user lvfangmin commented on the issue: https://github.com/apache/zookeeper/pull/84 Generally looks good to me, but I have the same concern as @fpj, maybe in the future the newly added auth needs additional parameters, then we have to add another wrapper with new interface to keep the old ones backward compatible. I also agree a general metadata map is error-prone, but I think we can have a compromise solution between explicit parameters and general metadata map: Create a class (e.g. AuthData) with the explicit parameters(zks, cnxn, path, perm, setAcls) in it, which has the get and set method, and use this class in the newly added ServerAuthenticationProvider interface. In the future if we need additional parameter, we only need to change this class, no interface and backward compatible issues will be involved. What do you think @Randgalt @fpj?
          Hide
          ASF GitHub Bot added a comment -

          Github user Randgalt commented on the issue:

          https://github.com/apache/zookeeper/pull/84

          OK - I've put the args into container classes. Let me know what you think.

          Show
          ASF GitHub Bot added a comment - Github user Randgalt commented on the issue: https://github.com/apache/zookeeper/pull/84 OK - I've put the args into container classes. Let me know what you think.
          Hide
          Jordan Zimmerman added a comment -

          Move ServerAuthenticationProvider args into container classes so that this can be upgraded more easily in the future without resorting more wrappers.

          Show
          Jordan Zimmerman added a comment - Move ServerAuthenticationProvider args into container classes so that this can be upgraded more easily in the future without resorting more wrappers.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12838425/ZOOKEEPER-1525.patch
          against trunk revision 440e0923dd9e3be533a196fdd6ada960860ca7f6.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 findbugs. The patch appears to introduce 19 new Findbugs (version 3.0.1) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3530//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3530//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3530//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12838425/ZOOKEEPER-1525.patch against trunk revision 440e0923dd9e3be533a196fdd6ada960860ca7f6. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 19 new Findbugs (version 3.0.1) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3530//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3530//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3530//console This message is automatically generated.
          Hide
          ASF GitHub Bot added a comment -

          Github user fpj commented on a diff in the pull request:

          https://github.com/apache/zookeeper/pull/84#discussion_r87957002

          — Diff: src/docs/src/documentation/content/xdocs/zookeeperProgrammers.xml —
          @@ -1193,6 +1193,55 @@ authProvider.2=com.f.MyAuth2
          only one will be used. Also all servers must have the same plugins defined, otherwise clients using
          the authentication schemes provided by the plugins will have problems connecting to some servers.
          </para>
          +
          + <para> <emphasis role="bold">Added in 3.6.0</emphasis>: An alternate abstraction is available for pluggable
          + authentication. It provides additional arguments.
          + </para>
          +
          + <programlisting>
          +public abstract class ServerAuthenticationProvider implements AuthenticationProvider {
          — End diff –

          Ok.

          Show
          ASF GitHub Bot added a comment - Github user fpj commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/84#discussion_r87957002 — Diff: src/docs/src/documentation/content/xdocs/zookeeperProgrammers.xml — @@ -1193,6 +1193,55 @@ authProvider.2=com.f.MyAuth2 only one will be used. Also all servers must have the same plugins defined, otherwise clients using the authentication schemes provided by the plugins will have problems connecting to some servers. </para> + + <para> <emphasis role="bold">Added in 3.6.0</emphasis>: An alternate abstraction is available for pluggable + authentication. It provides additional arguments. + </para> + + <programlisting> +public abstract class ServerAuthenticationProvider implements AuthenticationProvider { — End diff – Ok.
          Hide
          ASF GitHub Bot added a comment -

          Github user fpj commented on a diff in the pull request:

          https://github.com/apache/zookeeper/pull/84#discussion_r87957495

          — Diff: src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java —
          @@ -543,7 +546,7 @@ protected void pRequest2Txn(int type, long zxid, Request request,
          }

          nodeRecord = getRecordForPath(ZooDefs.CONFIG_NODE);

          • checkACL(zks, nodeRecord.acl, ZooDefs.Perms.WRITE, request.authInfo);
            + checkACL(zks, request.cnxn, nodeRecord.acl, ZooDefs.Perms.WRITE, request.authInfo, "/", null);
              • End diff –

          `null` tells me that the method is not expecting a value from the caller in that position, which is better than an arbitrary value. I think it is fine to change it to `null`.

          Show
          ASF GitHub Bot added a comment - Github user fpj commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/84#discussion_r87957495 — Diff: src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java — @@ -543,7 +546,7 @@ protected void pRequest2Txn(int type, long zxid, Request request, } nodeRecord = getRecordForPath(ZooDefs.CONFIG_NODE); checkACL(zks, nodeRecord.acl, ZooDefs.Perms.WRITE, request.authInfo); + checkACL(zks, request.cnxn, nodeRecord.acl, ZooDefs.Perms.WRITE, request.authInfo, "/", null); End diff – `null` tells me that the method is not expecting a value from the caller in that position, which is better than an arbitrary value. I think it is fine to change it to `null`.
          Hide
          ASF GitHub Bot added a comment -

          Github user fpj commented on the issue:

          https://github.com/apache/zookeeper/pull/84

          @Randgalt thanks for the recent changes, they look fine for me, except that we didn't get QA to run on it.

          @lvfangmin there are a few levels of indirection here, which doesn't make it look very pretty, but I don't see a much better way of doing it. how do you see the recent changes, do they look goo to you?

          Show
          ASF GitHub Bot added a comment - Github user fpj commented on the issue: https://github.com/apache/zookeeper/pull/84 @Randgalt thanks for the recent changes, they look fine for me, except that we didn't get QA to run on it. @lvfangmin there are a few levels of indirection here, which doesn't make it look very pretty, but I don't see a much better way of doing it. how do you see the recent changes, do they look goo to you?
          Hide
          ASF GitHub Bot added a comment -

          Github user Randgalt commented on a diff in the pull request:

          https://github.com/apache/zookeeper/pull/84#discussion_r88091830

          — Diff: src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java —
          @@ -543,7 +546,7 @@ protected void pRequest2Txn(int type, long zxid, Request request,
          }

          nodeRecord = getRecordForPath(ZooDefs.CONFIG_NODE);

          • checkACL(zks, nodeRecord.acl, ZooDefs.Perms.WRITE, request.authInfo);
            + checkACL(zks, request.cnxn, nodeRecord.acl, ZooDefs.Perms.WRITE, request.authInfo, "/", null);
              • End diff –

          done

          Show
          ASF GitHub Bot added a comment - Github user Randgalt commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/84#discussion_r88091830 — Diff: src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java — @@ -543,7 +546,7 @@ protected void pRequest2Txn(int type, long zxid, Request request, } nodeRecord = getRecordForPath(ZooDefs.CONFIG_NODE); checkACL(zks, nodeRecord.acl, ZooDefs.Perms.WRITE, request.authInfo); + checkACL(zks, request.cnxn, nodeRecord.acl, ZooDefs.Perms.WRITE, request.authInfo, "/", null); End diff – done
          Hide
          Jordan Zimmerman added a comment -

          pass null for path for reconfig as there is no path

          Show
          Jordan Zimmerman added a comment - pass null for path for reconfig as there is no path
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12839022/ZOOKEEPER-1525.patch
          against trunk revision 73e102a58d01b27bc6208bbfbde2d12f0deba1f4.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 findbugs. The patch appears to introduce 20 new Findbugs (version 3.0.1) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3533//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3533//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3533//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12839022/ZOOKEEPER-1525.patch against trunk revision 73e102a58d01b27bc6208bbfbde2d12f0deba1f4. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 20 new Findbugs (version 3.0.1) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3533//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3533//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3533//console This message is automatically generated.
          Hide
          Jordan Zimmerman added a comment -

          Looks to be flakey tests - they pass for me.

          Show
          Jordan Zimmerman added a comment - Looks to be flakey tests - they pass for me.
          Hide
          ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/zookeeper/pull/84

          Show
          ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/zookeeper/pull/84
          Hide
          Flavio Junqueira added a comment -

          +1, thanks for that patch, Jordan Zimmerman. I have merged to master, but the merge to 3.5 failed. The fix version in the jira says 3.5.3, so if this needs to go to 3.5.3, I'll need a patch for that branch. If not, then let me know and I'll close this issue.

          Show
          Flavio Junqueira added a comment - +1, thanks for that patch, Jordan Zimmerman . I have merged to master, but the merge to 3.5 failed. The fix version in the jira says 3.5.3, so if this needs to go to 3.5.3, I'll need a patch for that branch. If not, then let me know and I'll close this issue.
          Hide
          Hudson added a comment -

          FAILURE: Integrated in Jenkins build ZooKeeper-trunk #3159 (See https://builds.apache.org/job/ZooKeeper-trunk/3159/)
          ZOOKEEPER-1525: Plumb ZooKeeperServer object into auth plugins (fpj: rev 179c8db6df20beccd64fac2c99ca77dbe8a3242c)

          • (edit) src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java
          • (edit) src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java
          • (add) src/java/main/org/apache/zookeeper/server/auth/ServerAuthenticationProvider.java
          • (edit) src/java/main/org/apache/zookeeper/server/ServerCnxn.java
          • (add) src/java/main/org/apache/zookeeper/server/auth/KeyAuthenticationProvider.java
          • (add) src/java/test/org/apache/zookeeper/test/KeyAuthClientTest.java
          • (edit) src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java
          • (add) src/java/main/org/apache/zookeeper/server/auth/WrappedAuthenticationProvider.java
          • (edit) src/docs/src/documentation/content/xdocs/zookeeperProgrammers.xml
          • (edit) src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java
          • (edit) src/java/main/org/apache/zookeeper/server/auth/ProviderRegistry.java
          Show
          Hudson added a comment - FAILURE: Integrated in Jenkins build ZooKeeper-trunk #3159 (See https://builds.apache.org/job/ZooKeeper-trunk/3159/ ) ZOOKEEPER-1525 : Plumb ZooKeeperServer object into auth plugins (fpj: rev 179c8db6df20beccd64fac2c99ca77dbe8a3242c) (edit) src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java (edit) src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java (add) src/java/main/org/apache/zookeeper/server/auth/ServerAuthenticationProvider.java (edit) src/java/main/org/apache/zookeeper/server/ServerCnxn.java (add) src/java/main/org/apache/zookeeper/server/auth/KeyAuthenticationProvider.java (add) src/java/test/org/apache/zookeeper/test/KeyAuthClientTest.java (edit) src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java (add) src/java/main/org/apache/zookeeper/server/auth/WrappedAuthenticationProvider.java (edit) src/docs/src/documentation/content/xdocs/zookeeperProgrammers.xml (edit) src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java (edit) src/java/main/org/apache/zookeeper/server/auth/ProviderRegistry.java
          Hide
          Jordan Zimmerman added a comment -

          Flavio Junqueira What branch is 3.5? How can I duplicate this?

          Show
          Jordan Zimmerman added a comment - Flavio Junqueira What branch is 3.5? How can I duplicate this?
          Hide
          Flavio Junqueira added a comment -

          Branch 3.5:

          https://github.com/apache/zookeeper/tree/branch-3.5

          I'm basically asking if this needs to go to 3.5.3 or not. This is currently in master (future 3.6.0). If we don't need it in 3.5.3, then we can simply close this issue.

          Show
          Flavio Junqueira added a comment - Branch 3.5: https://github.com/apache/zookeeper/tree/branch-3.5 I'm basically asking if this needs to go to 3.5.3 or not. This is currently in master (future 3.6.0). If we don't need it in 3.5.3, then we can simply close this issue.
          Hide
          Jordan Zimmerman added a comment -

          Wow - I just checked. It may not be worth it to merge into 3.5.3. So, I'll change it to 3.6.0. Hopefully we don't have to wait a long time for 3.6.0?

          Show
          Jordan Zimmerman added a comment - Wow - I just checked. It may not be worth it to merge into 3.5.3. So, I'll change it to 3.6.0. Hopefully we don't have to wait a long time for 3.6.0?
          Hide
          Tim Crowder added a comment -

          I just wanted to thank everyone for finally getting this merged.
          I've been waiting a long time to see it go thru.

          Show
          Tim Crowder added a comment - I just wanted to thank everyone for finally getting this merged. I've been waiting a long time to see it go thru.

            People

            • Assignee:
              Jordan Zimmerman
              Reporter:
              Warren Turkal
            • Votes:
              7 Vote for this issue
              Watchers:
              12 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development