Hadoop Common
  1. Hadoop Common
  2. HADOOP-6715

AccessControlList.toString() returns empty string when we set acl to "*"

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.22.0
    • Component/s: security, util
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      AccessControlList.toString() returns empty string when we set the acl to "*" and also when we set it to empty(i.e. " "). This is causing wrong values for ACLs shown on jobdetails.jsp and jobdetailshistory.jsp web pages when acls are set to "*".

      I think AccessControlList.toString() needs to be changed to return "*" when we set the acl to "*".

      1. 6715.v1.patch
        10 kB
        Ravi Gummadi
      2. 6715.patch
        8 kB
        Ravi Gummadi
      3. 6715.20S.6.patch
        11 kB
        Ravi Gummadi

        Issue Links

          Activity

          Hide
          Hemanth Yamijala added a comment -

          Maybe something a little more user-friendly ? Like ALL USERS or something like that ? There is a similar, but more pertinent, display issue with empty string. There, we display an empty string, which in effect shows up as nothing. It would be nice to show something like NONE.

          Show
          Hemanth Yamijala added a comment - Maybe something a little more user-friendly ? Like ALL USERS or something like that ? There is a similar, but more pertinent, display issue with empty string. There, we display an empty string, which in effect shows up as nothing. It would be nice to show something like NONE.
          Hide
          Ravi Gummadi added a comment -

          Right. Would look good, if .toString() is used for displaying only.
          But if .toString() is used to save it as a String and later set it to a key in some Configuration object, then "ALL USERS" will not be considered as "*" and will cause an issue ?

          Show
          Ravi Gummadi added a comment - Right. Would look good, if .toString() is used for displaying only. But if .toString() is used to save it as a String and later set it to a key in some Configuration object, then "ALL USERS" will not be considered as "*" and will cause an issue ?
          Hide
          Vinod Kumar Vavilapalli added a comment -

          I propose we separate the display functionality from toString() as the latter can potentially be used for other purposes as Ravi mentioned above. So we will

          • fix toString() addressing the bug related to the wild-card '*'
          • add a new API say AccessControlList.getDisplayString(). This should return "ALL USERS" for '*' and "NONE" for ' ' (empty space character) and the output of toString for every other value of ACL.

          Thoughts?

          Show
          Vinod Kumar Vavilapalli added a comment - I propose we separate the display functionality from toString() as the latter can potentially be used for other purposes as Ravi mentioned above. So we will fix toString() addressing the bug related to the wild-card '*' add a new API say AccessControlList.getDisplayString() . This should return "ALL USERS" for '*' and "NONE" for ' ' (empty space character) and the output of toString for every other value of ACL. Thoughts?
          Hide
          Hemanth Yamijala added a comment -

          Can one of you help me understand what Ravi means by "save it as a String and later set it to a key in some Configuration object" ? It seems like we need to store the ACL objects in some map, and possibly these need to be reconstructed from a serialized representation (like for task log access, maybe ?) and we are using the key of the map as the String that is thus serialized.

          If that's the case, can we serialize the ACL using some representation that stores the actual name and value as separate fields rather than a toString representation, use a hashCode / equals on the ACL object to build a key based on these fields, and use toString for display purposes. This seems more canonical to me (inline with what toString is typically used for).

          Show
          Hemanth Yamijala added a comment - Can one of you help me understand what Ravi means by "save it as a String and later set it to a key in some Configuration object" ? It seems like we need to store the ACL objects in some map, and possibly these need to be reconstructed from a serialized representation (like for task log access, maybe ?) and we are using the key of the map as the String that is thus serialized. If that's the case, can we serialize the ACL using some representation that stores the actual name and value as separate fields rather than a toString representation, use a hashCode / equals on the ACL object to build a key based on these fields, and use toString for display purposes. This seems more canonical to me (inline with what toString is typically used for).
          Hide
          Ravi Gummadi added a comment -

          Right Hemanth. People wouldn't expect toString() to give serialized string of object. We can have our own messages in it.
          Will modify AccessControlList.toString() itself to give "ALL USERS" and "NONE" for "*" and empty acl cases.

          Show
          Ravi Gummadi added a comment - Right Hemanth. People wouldn't expect toString() to give serialized string of object. We can have our own messages in it. Will modify AccessControlList.toString() itself to give "ALL USERS" and "NONE" for "*" and empty acl cases.
          Hide
          Vinod Kumar Vavilapalli added a comment -

          Um.. 'ALL', 'USERS', 'NONE' can themselves be users/groups and will lead to confusions, however corner-cased they might be.

          How about something simpler like the following? Essentially, I'm trying to make the space character stand out by using special chars '[' and ']' which are unlikely to end up in user/group names.

          Job-ACLs: [*]
          Job-ACLs: [ ]
          Job-ACLs: [ group1]
          Job-ACLs: [user1,user2 group1,group2]
          
          Show
          Vinod Kumar Vavilapalli added a comment - Um.. 'ALL', 'USERS', 'NONE' can themselves be users/groups and will lead to confusions, however corner-cased they might be. How about something simpler like the following? Essentially, I'm trying to make the space character stand out by using special chars ' [' and '] ' which are unlikely to end up in user/group names. Job-ACLs: [*] Job-ACLs: [ ] Job-ACLs: [ group1] Job-ACLs: [user1,user2 group1,group2]
          Hide
          Vinod Kumar Vavilapalli added a comment -

          In fact more spaces between user-list and group-list makes things clearer, I think

          Job-ACLs: [*]
          Job-ACLs: [             ]
          Job-ACLs: [             group1]
          Job-ACLs: [user1,user2             group1,group2]
          
          Show
          Vinod Kumar Vavilapalli added a comment - In fact more spaces between user-list and group-list makes things clearer, I think Job-ACLs: [*] Job-ACLs: [ ] Job-ACLs: [ group1] Job-ACLs: [user1,user2 group1,group2]
          Hide
          Hemanth Yamijala added a comment -

          We could solve the problem of All, Users, None etc being valid user names by modifying the display string. Note that this is all primarily presentation layer changes.

          Firstly, I think displaying allowed users and groups separately in the UI would make it much more user friendly - rather than sticking to our internal representation.
          So, we could say:

          Allowed Users: a,b,c
          Allowed Groups: d,e,f

          When All or No users / groups have access, instead of saying:
          "Users: All" or "Groups: None"
          we could say
          "All users can access job" or "No groups can access job"

          Would this work ?

          Show
          Hemanth Yamijala added a comment - We could solve the problem of All, Users, None etc being valid user names by modifying the display string. Note that this is all primarily presentation layer changes. Firstly, I think displaying allowed users and groups separately in the UI would make it much more user friendly - rather than sticking to our internal representation. So, we could say: Allowed Users: a,b,c Allowed Groups: d,e,f When All or No users / groups have access, instead of saying: "Users: All" or "Groups: None" we could say "All users can access job" or "No groups can access job" Would this work ?
          Hide
          Vinod Kumar Vavilapalli added a comment -

          +1. That's looks far better than anything else we considered before.

          So, in summary, what we have currently

          Job-ACLs:
                mapreduce.job.acl-view-job:  group1,group2
                mapreduce.job.acl-modify-job: *
          

          will become

          Job-ACLs:
                mapreduce.job.acl-view-job:
                      Users: No users are allowed
                     Groups: group1,group2
                mapreduce.job.acl-modify-job:
                       Users: All users are allowed
                      Groups: All groups are allowed
          

          Right?

          Show
          Vinod Kumar Vavilapalli added a comment - +1. That's looks far better than anything else we considered before. So, in summary, what we have currently Job-ACLs: mapreduce.job.acl-view-job: group1,group2 mapreduce.job.acl-modify-job: * will become Job-ACLs: mapreduce.job.acl-view-job: Users: No users are allowed Groups: group1,group2 mapreduce.job.acl-modify-job: Users: All users are allowed Groups: All groups are allowed Right?
          Hide
          Hemanth Yamijala added a comment -

          Yes, that's the general idea. +1 from me. You may just want to get some opinion (from users or operations folks about whether users will be confused if it says "No users are allowed", but some groups are allowed.

          Show
          Hemanth Yamijala added a comment - Yes, that's the general idea. +1 from me. You may just want to get some opinion (from users or operations folks about whether users will be confused if it says "No users are allowed", but some groups are allowed.
          Hide
          Vinod Kumar Vavilapalli added a comment -

          Argh.. that is confusing too.. one final jab at it:

          ACL Message
          user1,user2 group1,group2 Users user1,user2 and members of the groups group1,group2 are allowed
          user1,user2 Users user1,user2 are allowed
          group1,group2 Members of the groups group1,group2 are allowed
          (blank space) No users are allowed
          *(asterisk) All users are allowed

          How about that? (Nothing is easy around here.. )

          Show
          Vinod Kumar Vavilapalli added a comment - Argh.. that is confusing too.. one final jab at it: ACL Message user1,user2 group1,group2 Users user1,user2 and members of the groups group1,group2 are allowed user1,user2 Users user1,user2 are allowed group1,group2 Members of the groups group1,group2 are allowed (blank space) No users are allowed *(asterisk) All users are allowed How about that? (Nothing is easy around here.. )
          Hide
          Ravi Gummadi added a comment -

          Attaching patch for earlier version of hadoop. Not for commit here.

          Show
          Ravi Gummadi added a comment - Attaching patch for earlier version of hadoop. Not for commit here.
          Hide
          Ravi Gummadi added a comment -

          Attaching patch for trunk.

          AccessControlList.toString() returns a descriptive String of users and groups that are part of this ACL. This is same as that is there in 6715.20S.6.patch.

          Also added a new public method AccessControlList.getAclString() that returns the exact String that can be used for creating a new instance of AccessControlList by passing it to the constructor.

          Show
          Ravi Gummadi added a comment - Attaching patch for trunk. AccessControlList.toString() returns a descriptive String of users and groups that are part of this ACL. This is same as that is there in 6715.20S.6.patch. Also added a new public method AccessControlList.getAclString() that returns the exact String that can be used for creating a new instance of AccessControlList by passing it to the constructor.
          Hide
          Ravi Gummadi added a comment -

          Attaching new patch for current trunk because HADOOP-6862 just got merged to trunk.

          Show
          Ravi Gummadi added a comment - Attaching new patch for current trunk because HADOOP-6862 just got merged to trunk.
          Hide
          Vinod Kumar Vavilapalli added a comment -

          Patch looks good. +1.

          We should commit this together with MAPREDUCE-1780 so that mapreduce doesn't get broken.

          Show
          Vinod Kumar Vavilapalli added a comment - Patch looks good. +1. We should commit this together with MAPREDUCE-1780 so that mapreduce doesn't get broken.
          Hide
          Ravi Gummadi added a comment -

          Hudson didn't come back for 1 day.
          I manually ran unit tests and test-patch.
          All unit tests passed.
          test-patch gave:

          [exec] -1 overall.
          [exec]
          [exec] +1 @author. The patch does not contain any @author tags.
          [exec]
          [exec] +1 tests included. The patch appears to include 3 new or modified tests.
          [exec]
          [exec] -1 javadoc. The javadoc tool appears to have generated 1 warning messages.
          [exec]
          [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
          [exec]
          [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
          [exec]
          [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.

          I verified that the existing javadoc warnings are not related to this patch.

          Show
          Ravi Gummadi added a comment - Hudson didn't come back for 1 day. I manually ran unit tests and test-patch. All unit tests passed. test-patch gave: [exec] -1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] +1 tests included. The patch appears to include 3 new or modified tests. [exec] [exec] -1 javadoc. The javadoc tool appears to have generated 1 warning messages. [exec] [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings. [exec] [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings. I verified that the existing javadoc warnings are not related to this patch.
          Hide
          Amareshwari Sriramadasu added a comment -

          I just committed this. Thanks Ravi !

          Show
          Amareshwari Sriramadasu added a comment - I just committed this. Thanks Ravi !
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #357 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk-Commit/357/)
          HADOOP-6715. Fixes AccessControlList.toString() to return a descriptive String representation of the ACL. Contributed by Ravi Gummadi

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #357 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk-Commit/357/ ) HADOOP-6715 . Fixes AccessControlList.toString() to return a descriptive String representation of the ACL. Contributed by Ravi Gummadi
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk #422 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk/422/)
          HADOOP-6715. Fixes AccessControlList.toString() to return a descriptive String representation of the ACL. Contributed by Ravi Gummadi

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk #422 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk/422/ ) HADOOP-6715 . Fixes AccessControlList.toString() to return a descriptive String representation of the ACL. Contributed by Ravi Gummadi

            People

            • Assignee:
              Ravi Gummadi
              Reporter:
              Ravi Gummadi
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development