You are viewing a plain text version of this content. The canonical link for it is here.
Posted to codereview@trafodion.apache.org by robertamarton <gi...@git.apache.org> on 2018/10/03 17:56:51 UTC
[GitHub] trafodion pull request #1723: TRAFODION - 3218 User still has privilege afte...
GitHub user robertamarton opened a pull request:
https://github.com/apache/trafodion/pull/1723
TRAFODION - 3218 User still has privilege after user's role has been …
…revoked ...
Partial support for column level privileges with QI support for:
column select
column insert
column references
column update
Also, as part of this, updated privilege code in a couple of areas:
Changed object caching code in NATable and NARoutine to store all privileges
assigned to the object when the object is cached (privDescs_). During the load
operation, the code creates bitmaps (privInfo_) for the current user. Privilege
checks are performed against the user bitmaps (privInfo_). This is in
anticipation for some performance updates when connecting to Trafodion (mxosrvr)
with different users.
Change getRoleList to include the roleID and the granteeID that granted the
privilege. The grantee can be a user or a role.
When a privilege is revoked from a role, send QI keys for every user that has
been granted to role.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/robertamarton/incubator-trafodion qi-issue
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafodion/pull/1723.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 #1723
----
commit adf2b8f23d87bd3bdcccf64523b730a4c9b57843
Author: Roberta Marton <ro...@...>
Date: 2018-10-03T17:54:39Z
TRAFODION - 3218 User still has privilege after user's role has been revoked ...
Partial support for column level privileges with QI support for:
column select
column insert
column references
column update
Also, as part of this, updated privilege code in a couple of areas:
Changed object caching code in NATable and NARoutine to store all privileges
assigned to the object when the object is cached (privDescs_). During the load
operation, the code creates bitmaps (privInfo_) for the current user. Privilege
checks are performed against the user bitmaps (privInfo_). This is in
anticipation for some performance updates when connecting to Trafodion (mxosrvr)
with different users.
Change getRoleList to include the roleID and the granteeID that granted the
privilege. The grantee can be a user or a role.
When a privilege is revoked from a role, send QI keys for every user that has
been granted to role.
----
---
[GitHub] trafodion pull request #1723: TRAFODION - 3218 User still has privilege afte...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/trafodion/pull/1723
---
[GitHub] trafodion pull request #1723: TRAFODION - 3218 User still has privilege afte...
Posted by robertamarton <gi...@git.apache.org>.
Github user robertamarton commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1723#discussion_r227473715
--- Diff: core/sql/common/ComSecurityKey.cpp ---
@@ -191,33 +216,41 @@ bool buildSecurityKeys( const int32_t userID,
return false;
}
- // If the grantee is a role, generate a special security key
- // If the role is revoked from the user, this key takes affect
+ // If the grantee is a role, generate special security keys, one for
+ // the user and one for each of the user's roles.
+ // If the role is revoked from the user these key takes affect
if (PrivMgr::isRoleID(granteeID))
{
- ComSecurityKey key (userID, granteeID, ComSecurityKey::SUBJECT_IS_USER);
- if (doDebug)
+ char buf [200];
--- End diff --
Will remove it in a subsequent release
---
[GitHub] trafodion pull request #1723: TRAFODION - 3218 User still has privilege afte...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1723#discussion_r225695077
--- Diff: core/sql/optimizer/NATable.cpp ---
@@ -6805,46 +6809,94 @@ void NATable::getPrivileges(TrafDesc * priv_desc)
ComSecurityKeySet secKeyVec(heap_);
if (priv_desc == NULL)
{
- if (isHiveTable() || isHbaseCellTable() ||
- isHbaseRowTable() || isHbaseMapTable())
+ if (!isSeabaseTable())
--- End diff --
The old code would return if this "if" were true, but the new code does not (because the return statement after the "else" was placed inside braces. Was this intentional?
---
[GitHub] trafodion pull request #1723: TRAFODION - 3218 User still has privilege afte...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1723#discussion_r225659935
--- Diff: core/sql/common/ComSecurityKey.cpp ---
@@ -191,33 +216,41 @@ bool buildSecurityKeys( const int32_t userID,
return false;
}
- // If the grantee is a role, generate a special security key
- // If the role is revoked from the user, this key takes affect
+ // If the grantee is a role, generate special security keys, one for
+ // the user and one for each of the user's roles.
+ // If the role is revoked from the user these key takes affect
if (PrivMgr::isRoleID(granteeID))
{
- ComSecurityKey key (userID, granteeID, ComSecurityKey::SUBJECT_IS_USER);
- if (doDebug)
+ char buf [200];
--- End diff --
This variable isn't used anywhere. Maybe we should delete it?
---
[GitHub] trafodion pull request #1723: TRAFODION - 3218 User still has privilege afte...
Posted by robertamarton <gi...@git.apache.org>.
Github user robertamarton commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1723#discussion_r227473870
--- Diff: core/sql/optimizer/NATable.cpp ---
@@ -6805,46 +6809,94 @@ void NATable::getPrivileges(TrafDesc * priv_desc)
ComSecurityKeySet secKeyVec(heap_);
if (priv_desc == NULL)
{
- if (isHiveTable() || isHbaseCellTable() ||
- isHbaseRowTable() || isHbaseMapTable())
+ if (!isSeabaseTable())
--- End diff --
The new code is cleaner. If it isn't a seabase table, then we need to get privileges, otherwise they are passed in with the TrafDesc.
---
[GitHub] trafodion pull request #1723: TRAFODION - 3218 User still has privilege afte...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1723#discussion_r225662018
--- Diff: core/sql/executor/ExExeUtilGet.cpp ---
@@ -1665,10 +1665,10 @@ NABoolean ExExeUtilGetMetadataInfoTcb::checkUserPrivs(
if (ComUser::isRootUserID())
return FALSE;
- // any user granted the DB__ROOTROLE sees everything
Int32 numRoles;
Int32 *roleList;
- if (currContext->getRoleList(numRoles, roleList) == SUCCESS)
+ Int32 *granteeList;
--- End diff --
Who deletes the memory for the granteeList? (or the roleList for that matter)
---
[GitHub] trafodion pull request #1723: TRAFODION - 3218 User still has privilege afte...
Posted by robertamarton <gi...@git.apache.org>.
Github user robertamarton commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1723#discussion_r227473824
--- Diff: core/sql/executor/ExExeUtilGet.cpp ---
@@ -1665,10 +1665,10 @@ NABoolean ExExeUtilGetMetadataInfoTcb::checkUserPrivs(
if (ComUser::isRootUserID())
return FALSE;
- // any user granted the DB__ROOTROLE sees everything
Int32 numRoles;
Int32 *roleList;
- if (currContext->getRoleList(numRoles, roleList) == SUCCESS)
+ Int32 *granteeList;
--- End diff --
The roleList and granteeList members are allocated on the exHeap and stored in context cache. When context is initialized, these list values are set to NULL. The first time they are referenced, metadata is read to get details for the current user. They are updated when privileges change through QI. They are also updated when a new user connects to Trafodion. The heap is destroyed when the context class is destructed.
---
[GitHub] trafodion pull request #1723: TRAFODION - 3218 User still has privilege afte...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1723#discussion_r225658074
--- Diff: core/sql/cli/Context.cpp ---
@@ -4199,19 +4219,35 @@ RETCODE ContextCli::setDatabaseUserByName(const char *userName)
// *
// * Function: ContextCli::getRoleList
// *
-// * Return the role IDs granted to the current user
+// * Return the role IDs and their grantees for the current user.
// * If the list of roles is already stored, just return this list.
// * If the list of roles does not exist extract the roles granted to the
// * current user from the Metadata and store in roleIDs_.
// *
// ****************************************************************************
RETCODE ContextCli::getRoleList(
- Int32 &numRoles,
- Int32 *&roleIDs)
+ Int32 &numEntries,
+ Int32 *& roleIDs,
+ Int32 *& granteeIDs)
{
// If role list has not been created, go read metadata
if (roleIDs_ == NULL)
{
+ // If authorization is not enabled, just setup the PUBLIC role
+ CmpContext *cmpCntxt = CmpCommon::context();
+ ex_assert(cmpCntxt, "No compiler context exists");
--- End diff --
The assert is fine, but unnecessary since we'd just core anyway on the very next statement when we dereference a null pointer.
---