HBase
  1. HBase
  2. HBASE-10883

Restrict the universe of labels and authorizations

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.98.1
    • Fix Version/s: 0.99.0, 0.98.2
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Currently we allow any string as visibility label or request authorization. However as seen on HBASE-10878, we accept for authorizations strings that would not work if provided as labels in visibility expressions. We should throw an exception at least in cases where someone tries to define or use a label or authorization including visibility expression operators '&', '|', '!', '(', ')'.

      1. HBASE-10883_7.patch
        9 kB
        ramkrishna.s.vasudevan
      2. HBASE-10883_6.patch
        10 kB
        ramkrishna.s.vasudevan
      3. HBASE-10883_5.patch
        9 kB
        ramkrishna.s.vasudevan
      4. HBASE-10883_4.patch
        10 kB
        ramkrishna.s.vasudevan
      5. HBASE-10883_3.patch
        10 kB
        ramkrishna.s.vasudevan
      6. HBASE-10883_2.patch
        9 kB
        ramkrishna.s.vasudevan
      7. HBASE-10883_1.patch
        5 kB
        ramkrishna.s.vasudevan
      8. HBASE-10883.patch
        3 kB
        ramkrishna.s.vasudevan

        Activity

        Hide
        Enis Soztutar added a comment -

        Closing this issue after 0.99.0 release.

        Show
        Enis Soztutar added a comment - Closing this issue after 0.99.0 release.
        Hide
        Andrew Purtell added a comment -

        This was committed.

        Show
        Andrew Purtell added a comment - This was committed.
        Hide
        Hudson added a comment -

        FAILURE: Integrated in HBase-0.98-on-Hadoop-1.1 #254 (See https://builds.apache.org/job/HBase-0.98-on-Hadoop-1.1/254/)
        HBASE-10883-Restrict the universe of labels and authorizations(Ram) (ramkrishna: rev 1585947)

        • /hbase/branches/0.98/hbase-client/src/main/java/org/apache/hadoop/hbase/security/visibility/Authorizations.java
        • /hbase/branches/0.98/hbase-client/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityLabelsValidator.java
        • /hbase/branches/0.98/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestScan.java
        • /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/rest/model/ScannerModel.java
        • /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java
        Show
        Hudson added a comment - FAILURE: Integrated in HBase-0.98-on-Hadoop-1.1 #254 (See https://builds.apache.org/job/HBase-0.98-on-Hadoop-1.1/254/ ) HBASE-10883 -Restrict the universe of labels and authorizations(Ram) (ramkrishna: rev 1585947) /hbase/branches/0.98/hbase-client/src/main/java/org/apache/hadoop/hbase/security/visibility/Authorizations.java /hbase/branches/0.98/hbase-client/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityLabelsValidator.java /hbase/branches/0.98/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestScan.java /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/rest/model/ScannerModel.java /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java
        Hide
        Hudson added a comment -

        SUCCESS: Integrated in HBase-TRUNK #5073 (See https://builds.apache.org/job/HBase-TRUNK/5073/)
        HBASE-10883-Restrict the universe of labels and authorizations(Ram) (ramkrishna: rev 1585946)

        • /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/security/visibility/Authorizations.java
        • /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityLabelsValidator.java
        • /hbase/trunk/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestScan.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/rest/model/ScannerModel.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java
        Show
        Hudson added a comment - SUCCESS: Integrated in HBase-TRUNK #5073 (See https://builds.apache.org/job/HBase-TRUNK/5073/ ) HBASE-10883 -Restrict the universe of labels and authorizations(Ram) (ramkrishna: rev 1585946) /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/security/visibility/Authorizations.java /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityLabelsValidator.java /hbase/trunk/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestScan.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/rest/model/ScannerModel.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java
        Hide
        Hudson added a comment -

        SUCCESS: Integrated in HBase-0.98 #270 (See https://builds.apache.org/job/HBase-0.98/270/)
        HBASE-10883-Restrict the universe of labels and authorizations(Ram) (ramkrishna: rev 1585947)

        • /hbase/branches/0.98/hbase-client/src/main/java/org/apache/hadoop/hbase/security/visibility/Authorizations.java
        • /hbase/branches/0.98/hbase-client/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityLabelsValidator.java
        • /hbase/branches/0.98/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestScan.java
        • /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/rest/model/ScannerModel.java
        • /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java
        Show
        Hudson added a comment - SUCCESS: Integrated in HBase-0.98 #270 (See https://builds.apache.org/job/HBase-0.98/270/ ) HBASE-10883 -Restrict the universe of labels and authorizations(Ram) (ramkrishna: rev 1585947) /hbase/branches/0.98/hbase-client/src/main/java/org/apache/hadoop/hbase/security/visibility/Authorizations.java /hbase/branches/0.98/hbase-client/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityLabelsValidator.java /hbase/branches/0.98/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestScan.java /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/rest/model/ScannerModel.java /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java
        Hide
        ramkrishna.s.vasudevan added a comment -

        This is what was committed to trunk and 0.98.
        Thanks for the reviews everyone.

        Show
        ramkrishna.s.vasudevan added a comment - This is what was committed to trunk and 0.98. Thanks for the reviews everyone.
        Hide
        Anoop Sam John added a comment -

        VisibilityLabelsValidator#isValidLabel(List<String> labels) - This is not used anywhere now I guess. Pls remove on commit.
        +1

        Show
        Anoop Sam John added a comment - VisibilityLabelsValidator#isValidLabel(List<String> labels) - This is not used anywhere now I guess. Pls remove on commit. +1
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12638965/HBASE-10883_6.patch
        against trunk revision .
        ATTACHMENT ID: 12638965

        +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 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 lineLengths. The patch does not introduce lines longer than 100

        +1 site. The mvn site goal succeeds with this patch.

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

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//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/12638965/HBASE-10883_6.patch against trunk revision . ATTACHMENT ID: 12638965 +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 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 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9213//console This message is automatically generated.
        Hide
        Anoop Sam John added a comment -

        Authorizations(List<String> labels)
        Validation from here can be for label one after other so that in the Exception msg, u can clearly say which Auth label is invalid. Same applicable to VisibilityController#createVisibilityLabelFilter
        We can just use VisibilityLabelsValidator#isValidLabel(byte[] label) which is already there and used by put ?

        throw new IllegalArgumentException("Authorizations cannot contain '(', ')' ,'&' ,'|', '!'"
        +          + " and cannot be empty :"+label);
        

        Error message can be bettter I think. This is invalid Auth label

        Show
        Anoop Sam John added a comment - Authorizations(List<String> labels) Validation from here can be for label one after other so that in the Exception msg, u can clearly say which Auth label is invalid. Same applicable to VisibilityController#createVisibilityLabelFilter We can just use VisibilityLabelsValidator#isValidLabel(byte[] label) which is already there and used by put ? throw new IllegalArgumentException( "Authorizations cannot contain '(', ')' ,'&' ,'|', '!'" + + " and cannot be empty :" +label); Error message can be bettter I think. This is invalid Auth label
        Hide
        ramkrishna.s.vasudevan added a comment -

        Ping to commit?

        Show
        ramkrishna.s.vasudevan added a comment - Ping to commit?
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12638532/HBASE-10883_5.patch
        against trunk revision .
        ATTACHMENT ID: 12638532

        +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 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 lineLengths. The patch does not introduce lines longer than 100

        +1 site. The mvn site goal succeeds with this patch.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.procedure.TestZKProcedure

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//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/12638532/HBASE-10883_5.patch against trunk revision . ATTACHMENT ID: 12638532 +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 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 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn site goal succeeds with this patch. -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.procedure.TestZKProcedure Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9194//console This message is automatically generated.
        Hide
        ramkrishna.s.vasudevan added a comment -

        Patch got stale with latest code.

        Show
        ramkrishna.s.vasudevan added a comment - Patch got stale with latest code.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12638525/HBASE-10883_4.patch
        against trunk revision .
        ATTACHMENT ID: 12638525

        +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-HBASE-Build/9193//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/12638525/HBASE-10883_4.patch against trunk revision . ATTACHMENT ID: 12638525 +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-HBASE-Build/9193//console This message is automatically generated.
        Hide
        ramkrishna.s.vasudevan added a comment -

        Sorry for the mistake. This patch works fine.

        Show
        ramkrishna.s.vasudevan added a comment - Sorry for the mistake. This patch works fine.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12638458/HBASE-10883_3.patch
        against trunk revision .
        ATTACHMENT ID: 12638458

        +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 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 lineLengths. The patch does not introduce lines longer than 100

        +1 site. The mvn site goal succeeds with this patch.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.security.visibility.TestVisibilityLabels
        org.apache.hadoop.hbase.rest.TestScannersWithLabels
        org.apache.hadoop.hbase.mapreduce.TestImportTSVWithVisibilityLabels
        org.apache.hadoop.hbase.security.visibility.TestVisibilityLabelsWithACL
        org.apache.hadoop.hbase.security.visibility.TestVisibilityLabelsOpWithDifferentUsersNoACL
        org.apache.hadoop.hbase.security.visibility.TestVisibilityLabelsWithDistributedLogReplay

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//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/12638458/HBASE-10883_3.patch against trunk revision . ATTACHMENT ID: 12638458 +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 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 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn site goal succeeds with this patch. -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.security.visibility.TestVisibilityLabels org.apache.hadoop.hbase.rest.TestScannersWithLabels org.apache.hadoop.hbase.mapreduce.TestImportTSVWithVisibilityLabels org.apache.hadoop.hbase.security.visibility.TestVisibilityLabelsWithACL org.apache.hadoop.hbase.security.visibility.TestVisibilityLabelsOpWithDifferentUsersNoACL org.apache.hadoop.hbase.security.visibility.TestVisibilityLabelsWithDistributedLogReplay Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9191//console This message is automatically generated.
        Hide
        ramkrishna.s.vasudevan added a comment -

        Updated patch.

        Show
        ramkrishna.s.vasudevan added a comment - Updated patch.
        Hide
        Ted Yu added a comment -
        +  public static final boolean isValidLabel(List<String> labels) {
        

        The method can be package-private, right ?

           public Authorizations(List<String> labels) {
        +    if (!VisibilityLabelsValidator.isValidLabel(labels)) {
        

        Better include the label which is invalid in exception message.

        Show
        Ted Yu added a comment - + public static final boolean isValidLabel(List< String > labels) { The method can be package-private, right ? public Authorizations(List< String > labels) { + if (!VisibilityLabelsValidator.isValidLabel(labels)) { Better include the label which is invalid in exception message.
        Hide
        ramkrishna.s.vasudevan added a comment -

        Checks both on client and server side. The matcher code is moved to VisbililtyLabelsValidator.

        Show
        ramkrishna.s.vasudevan added a comment - Checks both on client and server side. The matcher code is moved to VisbililtyLabelsValidator.
        Hide
        ramkrishna.s.vasudevan added a comment -

        We better keep the validation at server side?

        I thought HConstants would help in client side validation. I thought of unifying but decided not to as I thought better to be client side, just on creation of Authorization object.

        Show
        ramkrishna.s.vasudevan added a comment - We better keep the validation at server side? I thought HConstants would help in client side validation. I thought of unifying but decided not to as I thought better to be client side, just on creation of Authorization object.
        Hide
        Andrew Purtell added a comment -

        Why not re-use the code for validating labels? Checking on the client is fine but the check has to happen on the server side as well since we can't guarantee a validating client (REST? Thrift?)

        Show
        Andrew Purtell added a comment - Why not re-use the code for validating labels? Checking on the client is fine but the check has to happen on the server side as well since we can't guarantee a validating client (REST? Thrift?)
        Hide
        Anoop Sam John added a comment -

        Why to add pattern and regex to HConstants?
        We better keep the validation at server side?
        And move it to VisibilityLabelsValidator? VisibilityLabelsValidator#isValidLabel(String)

        Show
        Anoop Sam John added a comment - Why to add pattern and regex to HConstants? We better keep the validation at server side? And move it to VisibilityLabelsValidator? VisibilityLabelsValidator#isValidLabel(String)
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12638051/HBASE-10883_1.patch
        against trunk revision .
        ATTACHMENT ID: 12638051

        +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 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 lineLengths. The patch does not introduce lines longer than 100

        +1 site. The mvn site goal succeeds with this patch.

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

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//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/12638051/HBASE-10883_1.patch against trunk revision . ATTACHMENT ID: 12638051 +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 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 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9157//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/12638028/HBASE-10883.patch
        against trunk revision .
        ATTACHMENT ID: 12638028

        +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 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 1.3.9) warnings.

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

        +1 lineLengths. The patch does not introduce lines longer than 100

        +1 site. The mvn site goal succeeds with this patch.

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

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//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/12638028/HBASE-10883.patch against trunk revision . ATTACHMENT ID: 12638028 +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 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 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9155//console This message is automatically generated.
        Hide
        Ashish Singhi added a comment -

        Minor nit.

         } catch (IllegalArgumentException e) {
        +      fail("Should have failed for -B");
        +    }
        

        Message should be something like "should not fail".

        Show
        Ashish Singhi added a comment - Minor nit. } catch (IllegalArgumentException e) { + fail( "Should have failed for -B" ); + } Message should be something like "should not fail".
        Hide
        ramkrishna.s.vasudevan added a comment -

        Now validation is sync with VisibilityLabelsValidator. Also checks for empty string or white space String.

        Show
        ramkrishna.s.vasudevan added a comment - Now validation is sync with VisibilityLabelsValidator. Also checks for empty string or white space String.
        Hide
        Andrew Purtell added a comment -

        Well, you do have to change the current regexp somehow because it will match the empty string and that's not valid.

        Show
        Andrew Purtell added a comment - Well, you do have to change the current regexp somehow because it will match the empty string and that's not valid.
        Hide
        Andrew Purtell added a comment -
        +  private final String regex = "[A-Za-z_0-9]*";
        

        I think this should be

        +  private final String regex = "[A-Za-z\\_][A-Za-z0-9\\_\\-]*";
        

        but this is a minor thing, if you want the other then no problem.

        The regex should be precompiled since it may be applied often.

        Like Anoop says, we need to validate authorizations and labels the same way now.

        Show
        Andrew Purtell added a comment - + private final String regex = "[A-Za-z_0-9]*" ; I think this should be + private final String regex = "[A-Za-z\\_][A-Za-z0-9\\_\\-]*" ; but this is a minor thing, if you want the other then no problem. The regex should be precompiled since it may be applied often. Like Anoop says, we need to validate authorizations and labels the same way now.
        Hide
        Anoop Sam John added a comment -

        Referring VisibilityLabelsValidator we allow some special characters to be part of labels. So this check should contain those also.

        Show
        Anoop Sam John added a comment - Referring VisibilityLabelsValidator we allow some special characters to be part of labels. So this check should contain those also.
        Hide
        ramkrishna.s.vasudevan added a comment -

        Patch for trunk. Should apply on 0.98 also.
        Tested with shell also. Throws exception if any special characters are used.

        Show
        ramkrishna.s.vasudevan added a comment - Patch for trunk. Should apply on 0.98 also. Tested with shell also. Throws exception if any special characters are used.
        Hide
        Andrew Purtell added a comment -

        I think we should use the common pattern for identifiers in various programming languages, [A-Za-z\_][A-Za-z0-9\_\-]*

        Show
        Andrew Purtell added a comment - I think we should use the common pattern for identifiers in various programming languages, [A-Za-z\_] [A-Za-z0-9\_\-] *

          People

          • Assignee:
            ramkrishna.s.vasudevan
            Reporter:
            Andrew Purtell
          • Votes:
            0 Vote for this issue
            Watchers:
            9 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development