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 2015/11/19 02:47:52 UTC
[GitHub] incubator-trafodion pull request: [TRAFODION-1100] Creator of view...
GitHub user robertamarton opened a pull request:
https://github.com/apache/incubator-trafodion/pull/182
[TRAFODION-1100] Creator of view in private schema unable to select from view
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/robertamarton/incubator-trafodion privs
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-trafodion/pull/182.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 #182
----
commit 1c457170778b346291a330b6b6c86a275d3f6025
Author: Roberta Marton <ro...@esgyn.com>
Date: 2015-11-11T22:46:28Z
Merge [TRAFODION-1612] and [TRAFODION-1613] build changes
commit 013bb77aaae4b4e972fbdde9cfe2525924baf4a0
Author: Roberta Marton <ro...@esgyn.com>
Date: 2015-11-12T16:52:08Z
Merge branch 'master' into privs
commit a682d8531cc8a08b3e2636c7f9eaf1d79c134005
Author: Roberta Marton <ro...@esgyn.com>
Date: 2015-11-18T20:07:16Z
Merge branch 'master' into privs
commit 8f658efa21fea35336f8b814bb37d1f5c2bbf3da
Author: Roberta Marton <ro...@esgyn.com>
Date: 2015-11-19T01:45:02Z
Trafodion-1100 Creator of view in private schema unable to select from view
For private schemas, all objects are owned by the schema owner. If an authID
has create component privilege, they can create objects in other schemas.
However, the owner of the new object is still the schema owner.
When the object creator is not the schema owner, then the schema owner
automatically becomes the owner and the object creator is granted all relevant
privileges on the object WGO.
For views, this was not working correctly.
Also found another issue where column privileges were not being handled
correctly when generating the privileges list.
Problem is described in more detail in the JIRA
Changes:
CmpSeabaseDDLview - changed the create view code to add privileges for both the
schema owner and the view creator, and fixes the privilege list issue.
PrivMgr - added a helper function to convert an authID to an authName
PrivMgrCommands - changed the API to send in the grantor ID
PrivMgrPrivileges - changed the code to use the passed in grantor
TEST141 - added a new regression test, it is currently skipped until
trafodion-1087 is resolved.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafodion pull request: [TRAFODION-1100] Creator of view...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/182#discussion_r45372231
--- Diff: core/sql/sqlcomp/CmpSeabaseDDLview.cpp ---
@@ -1404,17 +1456,22 @@ short CmpSeabaseDDL::dropMetadataViews(ExeCliInterface * cliInterface)
static bool checkAccessPrivileges(
const ParTableUsageList & vtul,
const ParViewColTableColsUsageList & vctcul,
+ NABoolean viewCreator,
PrivMgrBitmap & privilegesBitmap,
PrivMgrBitmap & grantableBitmap)
{
+ BindWA bindWA(ActiveSchemaDB(),CmpCommon::context(),FALSE/*inDDL*/);
--- End diff --
Thanks for changing the indentation. This is much easier to read. (At least for me.)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafodion pull request: [TRAFODION-1100] Creator of view...
Posted by robertamarton <gi...@git.apache.org>.
Github user robertamarton commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/182#discussion_r45418307
--- Diff: core/sql/sqlcomp/CmpSeabaseDDLview.cpp ---
@@ -1498,32 +1577,50 @@ PrivColumnBitmap colGrantableBitmap;
}
int32_t usedColNumber = naCol->getPosition();
- // Grab privileges from the NATable structure
- PrivMgrUserPrivs *privs = naTable->getPrivInfo();
- if (privs == NULL)
+ PrivMgrUserPrivs privs;
+ PrivMgrUserPrivs *pPrivInfo = NULL;
+ if (viewCreator)
+ pPrivInfo = naTable->getPrivInfo();
+ else
+ {
+ PrivStatus retcode = privInterface.getPrivileges((int64_t)naTable->objectUid().get_value(),
+ naTable->getObjectType(),
+ naTable->getOwner(),
+ privs);
+
+ if (retcode == STATUS_ERROR)
+ {
+ *CmpCommon::diags() << DgSqlCode(-CAT_UNABLE_TO_RETRIEVE_PRIVS);
+ return false;
+ }
+ pPrivInfo = &privs;
--- End diff --
As part of creating the natable, the list of privileges the current user has is allocated and stored in the class. This information is retained until the entry is removed. There should not be any memory leak.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafodion pull request: [TRAFODION-1100] Creator of view...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/182#discussion_r45370595
--- Diff: core/sql/regress/catman1/TEST141 ---
@@ -0,0 +1,394 @@
+-- ============================================================================
+-- Test: TEST141
+-- @@@ START COPYRIGHT @@@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one
+-- or more contributor license agreements. See the NOTICE file
+-- distributed with this work for additional information
+-- regarding copyright ownership. The ASF licenses this file
+-- to you under the Apache License, Version 2.0 (the
+-- "License"); you may not use this file except in compliance
+-- with the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing,
+-- software distributed under the License is distributed on an
+-- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+-- KIND, either express or implied. See the License for the
+-- specific language governing permissions and limitations
+-- under the License.
+--
+-- @@@ END COPYRIGHT @@@
+--
+-- ============================================================================
+-- Functionality: Extended support for views for all levels:
+-- column, object, component
+--
+-- Added in response to JIRA 1100
--- End diff --
Nit, might be more clear to say, JIRA TRAFODION-1100.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafodion pull request: [TRAFODION-1100] Creator of view...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-trafodion/pull/182
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafodion pull request: [TRAFODION-1100] Creator of view...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/182#discussion_r45418920
--- Diff: core/sql/sqlcomp/CmpSeabaseDDLview.cpp ---
@@ -1498,32 +1577,50 @@ PrivColumnBitmap colGrantableBitmap;
}
int32_t usedColNumber = naCol->getPosition();
- // Grab privileges from the NATable structure
- PrivMgrUserPrivs *privs = naTable->getPrivInfo();
- if (privs == NULL)
+ PrivMgrUserPrivs privs;
+ PrivMgrUserPrivs *pPrivInfo = NULL;
+ if (viewCreator)
+ pPrivInfo = naTable->getPrivInfo();
+ else
+ {
+ PrivStatus retcode = privInterface.getPrivileges((int64_t)naTable->objectUid().get_value(),
+ naTable->getObjectType(),
+ naTable->getOwner(),
+ privs);
+
+ if (retcode == STATUS_ERROR)
+ {
+ *CmpCommon::diags() << DgSqlCode(-CAT_UNABLE_TO_RETRIEVE_PRIVS);
+ return false;
+ }
+ pPrivInfo = &privs;
--- End diff --
I see... so in the one code path I alluded to, pPrivInfo is not pointing to a heap-allocated object but is pointing to a member of an object (possibly heap-allocated), and other code will take care of destroying that larger object.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafodion pull request: [TRAFODION-1100] Creator of view...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/182#discussion_r45372631
--- Diff: core/sql/sqlcomp/CmpSeabaseDDLview.cpp ---
@@ -1498,32 +1577,50 @@ PrivColumnBitmap colGrantableBitmap;
}
int32_t usedColNumber = naCol->getPosition();
- // Grab privileges from the NATable structure
- PrivMgrUserPrivs *privs = naTable->getPrivInfo();
- if (privs == NULL)
+ PrivMgrUserPrivs privs;
+ PrivMgrUserPrivs *pPrivInfo = NULL;
+ if (viewCreator)
+ pPrivInfo = naTable->getPrivInfo();
+ else
+ {
+ PrivStatus retcode = privInterface.getPrivileges((int64_t)naTable->objectUid().get_value(),
+ naTable->getObjectType(),
+ naTable->getOwner(),
+ privs);
+
+ if (retcode == STATUS_ERROR)
+ {
+ *CmpCommon::diags() << DgSqlCode(-CAT_UNABLE_TO_RETRIEVE_PRIVS);
+ return false;
+ }
+ pPrivInfo = &privs;
--- End diff --
So, in one code path pPrivInfo points to a heap-allocated structure while in this code path it points to a stack variable. Is there any issue with memory leaks in the first case?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---