Hive
  1. Hive
  2. HIVE-5837 SQL standard based secure authorization for hive
  3. HIVE-5931

SQL std auth - add metastore get_principals_in_role api, support SHOW PRINCIPALS role_name

    Details

    • Type: Sub-task Sub-task
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.13.0
    • Component/s: Authorization
    • Labels:
      None

      Description

      Support command for listing all members of a role.
      A new metastore api call also needs to be added for this.

      1. HIVE-5931.thriftapi.patch
        1 kB
        Thejas M Nair
      2. HIVE-5931.thriftapi.followup.patch
        2 kB
        Thejas M Nair
      3. HIVE-5931.thriftapi.3.patch
        2 kB
        Thejas M Nair
      4. HIVE-5931.thriftapi.2.patch
        2 kB
        Thejas M Nair
      5. HIVE-5931.nothrifgen.2.patch
        83 kB
        Thejas M Nair
      6. HIVE-5931.nothrifgen.1.patch
        83 kB
        Thejas M Nair
      7. HIVE-5931.2.patch
        1.06 MB
        Thejas M Nair
      8. HIVE-5931.1.patch
        1.06 MB
        Thejas M Nair

        Issue Links

          Activity

          Hide
          Thejas M Nair added a comment -

          Patch committed to trunk and 0.13 branch.
          I didn't realize that I had not checked with Harish on including this in 0.13. Sorry, about that. I have now included it the wiki page. cc Harish Butani

          Show
          Thejas M Nair added a comment - Patch committed to trunk and 0.13 branch. I didn't realize that I had not checked with Harish on including this in 0.13. Sorry, about that. I have now included it the wiki page. cc Harish Butani
          Hide
          Ashutosh Chauhan added a comment -

          +1

          Show
          Ashutosh Chauhan added a comment - +1
          Hide
          Hive QA added a comment -

          Overall: +1 all checks pass

          Here are the results of testing the latest attachment:
          https://issues.apache.org/jira/secure/attachment/12633684/HIVE-5931.1.patch

          SUCCESS: +1 5379 tests passed

          Test results: http://bigtop01.cloudera.org:8080/job/PreCommit-HIVE-Build/1701/testReport
          Console output: http://bigtop01.cloudera.org:8080/job/PreCommit-HIVE-Build/1701/console

          Messages:

          Executing org.apache.hive.ptest.execution.PrepPhase
          Executing org.apache.hive.ptest.execution.ExecutionPhase
          Executing org.apache.hive.ptest.execution.ReportingPhase
          

          This message is automatically generated.

          ATTACHMENT ID: 12633684

          Show
          Hive QA added a comment - Overall : +1 all checks pass Here are the results of testing the latest attachment: https://issues.apache.org/jira/secure/attachment/12633684/HIVE-5931.1.patch SUCCESS: +1 5379 tests passed Test results: http://bigtop01.cloudera.org:8080/job/PreCommit-HIVE-Build/1701/testReport Console output: http://bigtop01.cloudera.org:8080/job/PreCommit-HIVE-Build/1701/console Messages: Executing org.apache.hive.ptest.execution.PrepPhase Executing org.apache.hive.ptest.execution.ExecutionPhase Executing org.apache.hive.ptest.execution.ReportingPhase This message is automatically generated. ATTACHMENT ID: 12633684
          Hide
          Thejas M Nair added a comment -

          HIVE-5931.*.2.patch - addressing review comments. Update syntax to 'show principals role_name'

          Show
          Thejas M Nair added a comment - HIVE-5931 .*.2.patch - addressing review comments. Update syntax to 'show principals role_name'
          Hide
          Ashutosh Chauhan added a comment -

          Patch looks good. I have a minor comment on RB.
          Although, I think following syntax is better :

          SHOW PRINCIPALS role_name;
          

          having role there is redundant.

          Show
          Ashutosh Chauhan added a comment - Patch looks good. I have a minor comment on RB. Although, I think following syntax is better : SHOW PRINCIPALS role_name; having role there is redundant.
          Hide
          Thejas M Nair added a comment -

          Adding review board link

          Show
          Thejas M Nair added a comment - Adding review board link
          Hide
          Thejas M Nair added a comment -

          Instead of describe-role, I have added support for the command: SHOW ROLE PRINCIPALS role_name;

          Attaching a patch file without the thrift generated files as well, for ease of review ( HIVE-5931.nothrifgen.1.patch is no thrift-gen version of HIVE-5931.1.patch )

          Show
          Thejas M Nair added a comment - Instead of describe-role, I have added support for the command: SHOW ROLE PRINCIPALS role_name; Attaching a patch file without the thrift generated files as well, for ease of review ( HIVE-5931 .nothrifgen.1.patch is no thrift-gen version of HIVE-5931 .1.patch )
          Hide
          Thejas M Nair added a comment -

          The reason why 'describe function <funcname>' works is that for some historic reason the keyword function was not marked as a non reserved keyword in IdentifiersParser.g. This is not the case with ROLE keyword.

          Show
          Thejas M Nair added a comment - The reason why 'describe function <funcname>' works is that for some historic reason the keyword function was not marked as a non reserved keyword in IdentifiersParser.g. This is not the case with ROLE keyword.
          Hide
          Thejas M Nair added a comment -

          I am working on adding support for the describe role syntax as well, as part of this jira.

          But there is a problem, the current describe table syntax allows the following -

          DESCRIBE t1 key1;
          DESCRIBE EXTENDED t1 key1;
          

          In hive, almost all keywords are also identifiers . So "describe role <rolename>" also gets translated to a describe table command with "role" as table name and "<rolename>" as a column.

          This is not a documented syntax AFAIK, but we do have .q tests for it and it would break backward compatibility.The documented syntax requires a dot https://cwiki.apache.org/confluence/display/Hive/LanguageManual+DDL#LanguageManualDDL-Describe . Dot became optional with HIVE-1977 . Since there are tests added as part of the patch that use this format, it looks like the change was intentional.

          I will look at syntax alternatives to 'describe role'.

          Show
          Thejas M Nair added a comment - I am working on adding support for the describe role syntax as well, as part of this jira. But there is a problem, the current describe table syntax allows the following - DESCRIBE t1 key1; DESCRIBE EXTENDED t1 key1; In hive, almost all keywords are also identifiers . So "describe role <rolename>" also gets translated to a describe table command with "role" as table name and "<rolename>" as a column. This is not a documented syntax AFAIK, but we do have .q tests for it and it would break backward compatibility.The documented syntax requires a dot https://cwiki.apache.org/confluence/display/Hive/LanguageManual+DDL#LanguageManualDDL-Describe . Dot became optional with HIVE-1977 . Since there are tests added as part of the patch that use this format, it looks like the change was intentional. I will look at syntax alternatives to 'describe role'.
          Hide
          Ashutosh Chauhan added a comment -

          OK. Current proposal looks good.

          Show
          Ashutosh Chauhan added a comment - OK. Current proposal looks good.
          Hide
          Thejas M Nair added a comment -

          Yes, I was planning to do that as part of HIVE-6547 .

          Show
          Thejas M Nair added a comment - Yes, I was planning to do that as part of HIVE-6547 .
          Hide
          Ashutosh Chauhan added a comment -

          Don't see {{ list<RoleGrant> get_role_grants_for_principal}} in latest patch. You want to do that in separate patch?

          Show
          Ashutosh Chauhan added a comment - Don't see {{ list<RoleGrant> get_role_grants_for_principal}} in latest patch. You want to do that in separate patch?
          Hide
          Ashutosh Chauhan added a comment -

          Few comments on proposed api:

          • Better name for 1st method : get_principals_in_role() ?
          • Better name for 2nd method : get_roles_granted_to_principal() ?
          • Also struct needs better name. Also, put explanation for struct, since it carries redundant info, depending on method it is used in.
          • principalType in struct should be enum
          Show
          Ashutosh Chauhan added a comment - Few comments on proposed api: Better name for 1st method : get_principals_in_role() ? Better name for 2nd method : get_roles_granted_to_principal() ? Also struct needs better name. Also, put explanation for struct, since it carries redundant info, depending on method it is used in. principalType in struct should be enum
          Hide
          Thejas M Nair added a comment -

          fyi, the addition of user-to-role mapping into Role 'class' was added in HIVE-6204 (hive 0.13, so this will not be a backward incompatible change). But while working on this I feel, mixing the role-to-user mapping and Role object is not very intuitive/clean, and we might be taking on some technical debt there.

          Show
          Thejas M Nair added a comment - fyi, the addition of user-to-role mapping into Role 'class' was added in HIVE-6204 (hive 0.13, so this will not be a backward incompatible change). But while working on this I feel, mixing the role-to-user mapping and Role object is not very intuitive/clean, and we might be taking on some technical debt there.
          Hide
          Thejas M Nair added a comment -

          HIVE-5931.thriftapi.followup.patch - The follow up change that I am planning to introduce. Along with this change, the show-role-grants would use get_role_grants_for_principal instead of list_roles. That way we won't be storing role-to-principal mapping in the Role thrift object. This is a better normalized representation IMO.

          Show
          Thejas M Nair added a comment - HIVE-5931 .thriftapi.followup.patch - The follow up change that I am planning to introduce. Along with this change, the show-role-grants would use get_role_grants_for_principal instead of list_roles. That way we won't be storing role-to-principal mapping in the Role thrift object. This is a better normalized representation IMO.
          Hide
          Thejas M Nair added a comment - - edited

          HIVE-5931.thriftapi.patch - thrift api that I plan to introduce.
          I also think it will be cleaner to use an api that returns RoleGrant for the show-role-grant instead of adding the RoleGrant information to Role (which becomes confusing in the context of create-role). I am thinking of making that change in a separate follow up patch for 0.13 . I am hoping we can include that during the stabilization phase of 0.13 (ie, post branching).

          cc Navis Ashutosh Chauhan

          Show
          Thejas M Nair added a comment - - edited HIVE-5931 .thriftapi.patch - thrift api that I plan to introduce. I also think it will be cleaner to use an api that returns RoleGrant for the show-role-grant instead of adding the RoleGrant information to Role (which becomes confusing in the context of create-role). I am thinking of making that change in a separate follow up patch for 0.13 . I am hoping we can include that during the stabilization phase of 0.13 (ie, post branching). cc Navis Ashutosh Chauhan
          Hide
          Thejas M Nair added a comment -

          Navis Can you please see if you can contribute the patch for this jira ?

          Show
          Thejas M Nair added a comment - Navis Can you please see if you can contribute the patch for this jira ?
          Hide
          Navis added a comment -

          Thejas M Nair I've tried rebase NHIVE-31 on trunk but it's heavily dependent to HIVE-6204 and it is again dependent to HIVE-6122. I don't like the way HIVE-6122 is implemented but should hurry on it.

          Show
          Navis added a comment - Thejas M Nair I've tried rebase NHIVE-31 on trunk but it's heavily dependent to HIVE-6204 and it is again dependent to HIVE-6122 . I don't like the way HIVE-6122 is implemented but should hurry on it.
          Hide
          Thejas M Nair added a comment -

          Navis, can you please contribute NHIVE-31 patch ? Do let me know if that patch is blocked on some other patch already submitted.

          Show
          Thejas M Nair added a comment - Navis, can you please contribute NHIVE-31 patch ? Do let me know if that patch is blocked on some other patch already submitted.
          Hide
          Thejas M Nair added a comment -

          Navis I believe your internal patch "NHIVE-31 Add API for retrieving principals endowed with the specific role" must be doing the same as what is described in this jira. Will you able able to contribute that ?

          Show
          Thejas M Nair added a comment - Navis I believe your internal patch "NHIVE-31 Add API for retrieving principals endowed with the specific role" must be doing the same as what is described in this jira. Will you able able to contribute that ?

            People

            • Assignee:
              Thejas M Nair
              Reporter:
              Thejas M Nair
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Time Tracking

                Estimated:
                Original Estimate - 24h
                24h
                Remaining:
                Remaining Estimate - 24h
                24h
                Logged:
                Time Spent - Not Specified
                Not Specified

                  Development