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 -

          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 ?
          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
          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 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
          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 -

          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 -

          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
          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
          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
          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 -

          OK. Current proposal looks good.

          Show
          Ashutosh Chauhan added a comment - OK. Current proposal looks good.
          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
          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 -

          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 -

          Adding review board link

          Show
          Thejas M Nair added a comment - Adding review board link
          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 -

          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
          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
          Ashutosh Chauhan added a comment -

          +1

          Show
          Ashutosh Chauhan added a comment - +1
          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

            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