Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.2.1.6
    • Fix Version/s: 10.2.1.6
    • Component/s: SQL
    • Labels:
      None
    • Urgency:
      Urgent

      Description

      Additional work needs to be done for grant/revoke to make sure that only users with required privileges can access various database objects. In order to do that, first we need to collect the privilege requirements for various database objects and store them in SYS.SYSREQUIREDPERM. Once we have this information then when a user tries to access an object, the required SYS.SYSREQUIREDPERM privileges for the object will be checked against the user privileges in SYS.SYSTABLEPERMS, SYS.SYSCOLPERMS and SYS.SYSROUTINEPERMS. The database object access will succeed only if the user has the necessary privileges.

      SYS.SYSTABLEPERMS, SYS.SYSCOLPERMS and SYS.SYSROUTINEPERMS are already populated by Satheesh's work on DERBY-464. But SYS.SYSREQUIREDPERM doesn't have any information in it at this point and hence no runtime privilege checking is getting done at this point.

      1. AuthorizationModelForDerbySQLStandardAuthorization.html
        14 kB
        Mamta A. Satoor
      2. AuthorizationModelForDerbySQLStandardAuthorizationV2.html
        13 kB
        Mamta A. Satoor
      3. Derby1330ViewPrivilegeCollectionV1diff.txt
        352 kB
        Mamta A. Satoor
      4. Derby1330ViewPrivilegeCollectionV1stat.txt
        4 kB
        Mamta A. Satoor
      5. Derby1330PrivilegeCollectionV2diff.txt
        362 kB
        Mamta A. Satoor
      6. Derby1330PrivilegeCollectionV2stat.txt
        4 kB
        Mamta A. Satoor
      7. Derby1330PrivilegeCollectionV3diff.txt
        363 kB
        Mamta A. Satoor
      8. Derby1330PrivilegeCollectionV3stat.txt
        4 kB
        Mamta A. Satoor
      9. Derby1330uuidIndexForPermsSystemTablesV4diff.txt
        39 kB
        Mamta A. Satoor
      10. Derby1330uuidIndexForPermsSystemTablesV4stat.txt
        1.0 kB
        Mamta A. Satoor
      11. Derby1330uuidIndexForPermsSystemTablesV5diff.txt
        85 kB
        Mamta A. Satoor
      12. Derby1330uuidIndexForPermsSystemTablesV5stat.txt
        1 kB
        Mamta A. Satoor
      13. Derby1330uuidIndexForPermsSystemTablesV6diff.txt
        19 kB
        Mamta A. Satoor
      14. Derby1330uuidIndexForPermsSystemTablesV6stat.txt
        0.5 kB
        Mamta A. Satoor
      15. Derby1330MinorCleanupV7diff.txt
        3 kB
        Mamta A. Satoor
      16. Derby1330MinorCleanupV7stat.txt
        0.1 kB
        Mamta A. Satoor
      17. Derby1330setUUIDinDataDictionaryV8diff.txt
        7 kB
        Mamta A. Satoor
      18. Derby1330setUUIDinDataDictionaryV8stat.txt
        0.6 kB
        Mamta A. Satoor
      19. DERBY1330javaDocWarningsDiffV9.txt
        2 kB
        Mamta A. Satoor
      20. DERBY1330javaDocWarningsStatV9.txt
        0.3 kB
        Mamta A. Satoor
      21. Derby1330setUUIDinDataDictionaryV10diff.txt
        6 kB
        Mamta A. Satoor
      22. Derby1330setUUIDinDataDictionaryV10stat.txt
        0.6 kB
        Mamta A. Satoor

        Issue Links

          Activity

          Hide
          Mamta A. Satoor added a comment -

          Attaching functional spec for grant/revoke authorization model. Appreciate any feedback

          Show
          Mamta A. Satoor added a comment - Attaching functional spec for grant/revoke authorization model. Appreciate any feedback
          Hide
          Daniel John Debrunner added a comment -

          Also have this comment in DERBY-464 http://issues.apache.org/jira/browse/DERBY-464#action_12376518

          • How does SYSREQUIREDPERM relate to the current dependency system. Could this functionality not be incorporated into
            the current dependency system so that we have a single system?
          Show
          Daniel John Debrunner added a comment - Also have this comment in DERBY-464 http://issues.apache.org/jira/browse/DERBY-464#action_12376518 How does SYSREQUIREDPERM relate to the current dependency system. Could this functionality not be incorporated into the current dependency system so that we have a single system?
          Hide
          Satheesh Bandaram added a comment -

          Interesting idea from Dan... about need to have SYSREQUIREDPERM system table. It does look possible, DEPENDENTID/PROVIDERID could be OPERATOR/OBJECT of new system table. Seems like a good fit with current dependency system.

          About the comment about including GRATOR in new system table index, seems useful as well, when WITH GRANT OPTION is added in the future, so I will make this change.

          Show
          Satheesh Bandaram added a comment - Interesting idea from Dan... about need to have SYSREQUIREDPERM system table. It does look possible, DEPENDENTID/PROVIDERID could be OPERATOR/OBJECT of new system table. Seems like a good fit with current dependency system. About the comment about including GRATOR in new system table index, seems useful as well, when WITH GRANT OPTION is added in the future, so I will make this change.
          Hide
          Mamta A. Satoor added a comment -

          I did a little research on Dan's suggestion about using existing system table SYS.SYSDEPENDS rather than introducing a new system table to keep track of views'/triggers'/constraints' dependencies on privileges. I think SYS.SYSDEPENDS should serve the purpose. Based on that, I have updated the functional spec(1st 2 paragraphs under section "DDL support for views, triggers and constraints") and attached it as AuthorizationModelForDerbySQLStandardAuthorizationV2.html. Any feedback will be appreciated.

          Show
          Mamta A. Satoor added a comment - I did a little research on Dan's suggestion about using existing system table SYS.SYSDEPENDS rather than introducing a new system table to keep track of views'/triggers'/constraints' dependencies on privileges. I think SYS.SYSDEPENDS should serve the purpose. Based on that, I have updated the functional spec(1st 2 paragraphs under section "DDL support for views, triggers and constraints") and attached it as AuthorizationModelForDerbySQLStandardAuthorizationV2.html. Any feedback will be appreciated.
          Hide
          Mamta A. Satoor added a comment -

          I have been working on storing privilege requirements for view (in Derby SQL Authorization mode only) during create view time in SYSDEPENDS table. This information in SYSDEPENDS will later be used at the revoke privilege time. That patch for this work is attached as Derby1330ViewPrivilegeCollectionV1diff.txt to this JIRA entry. All the views that depend on the privilege being revoked will be dropped. The revoke privilege work to drop affected views is not included in this patch.

          The patch has 2 major components to it
          1)If the query for create view is accessing view/s, then Derby engine flattens all those view/s. Once the view flattening is finished, Derby engine goes through all the columns for the flattened parent sql and it starts building the privilege requirement for those columns. But since views are always accessed with definer's privileges (which means that as long as a user has privilege to access a view, the user can select from the view. This works even if the user does not need direct privileges to the objects accessed by the view.), Derby engine should not collect privilege requirements for objects accessed by views. In order to implement this behavior, I have added a flag to QueryTreeNode which will be set to false if the object is going to be accessed with definer's privileges. During view flattening, the objects accessed by view will be marked with definer privileges. Later, when the Derby enging goes through all the columns, it will check if the column is marked with definer's privilege and if so, then do not look for privilege requirement for that column.
          2)Once the privileges have been collected for the create view in the bind phase, they need to be recorded in SYSDEPENDS table during the execution phase of the create view sql. SYSDEPENDS will add one row for every privilege requirement for the view. The DEPENDENTID will be view descriptor and PROVIDERID has to be a unique id to identify permission descriptor's uniquely. In order to do this, PermissionsDescriptor needs to implement interface Provider and it needs to have a UUID associated with it. This required addition of a UUID column in SYSTABLEPERMS, SYSCOLPERMS and SYSROUTINEPERMS. This UUID is required by the DependencyManager to uniquely identify a row for a given PROVIDERID.

          The patch mail to the derby developer list can be found at http://www.nabble.com/-PATCH-DERBY-1330-Collect-privilege-requirements-for-views-tf1858264.html#a5075008. This thread has more information about individual class changes.

          Show
          Mamta A. Satoor added a comment - I have been working on storing privilege requirements for view (in Derby SQL Authorization mode only) during create view time in SYSDEPENDS table. This information in SYSDEPENDS will later be used at the revoke privilege time. That patch for this work is attached as Derby1330ViewPrivilegeCollectionV1diff.txt to this JIRA entry. All the views that depend on the privilege being revoked will be dropped. The revoke privilege work to drop affected views is not included in this patch. The patch has 2 major components to it 1)If the query for create view is accessing view/s, then Derby engine flattens all those view/s. Once the view flattening is finished, Derby engine goes through all the columns for the flattened parent sql and it starts building the privilege requirement for those columns. But since views are always accessed with definer's privileges (which means that as long as a user has privilege to access a view, the user can select from the view. This works even if the user does not need direct privileges to the objects accessed by the view.), Derby engine should not collect privilege requirements for objects accessed by views. In order to implement this behavior, I have added a flag to QueryTreeNode which will be set to false if the object is going to be accessed with definer's privileges. During view flattening, the objects accessed by view will be marked with definer privileges. Later, when the Derby enging goes through all the columns, it will check if the column is marked with definer's privilege and if so, then do not look for privilege requirement for that column. 2)Once the privileges have been collected for the create view in the bind phase, they need to be recorded in SYSDEPENDS table during the execution phase of the create view sql. SYSDEPENDS will add one row for every privilege requirement for the view. The DEPENDENTID will be view descriptor and PROVIDERID has to be a unique id to identify permission descriptor's uniquely. In order to do this, PermissionsDescriptor needs to implement interface Provider and it needs to have a UUID associated with it. This required addition of a UUID column in SYSTABLEPERMS, SYSCOLPERMS and SYSROUTINEPERMS. This UUID is required by the DependencyManager to uniquely identify a row for a given PROVIDERID. The patch mail to the derby developer list can be found at http://www.nabble.com/-PATCH-DERBY-1330-Collect-privilege-requirements-for-views-tf1858264.html#a5075008 . This thread has more information about individual class changes.
          Hide
          Daniel John Debrunner added a comment -

          Just from a quick look at the patch there are many cases where you add a new method or field and add comments, but not as javadoc comments. E.g.

          + //A call to this method means stop collecting privilege requirements for
          + //this node because this node is executing under definer's privileges.
          + public void accessThisWithDefinerPrivileges()

          This means the comment does not make into the generated javadoc for the engine and (I think) will not be picked up by smart IDEs like Eclipse. In Eclipse "hovering" over a method call will show the javadoc comment for the called method, which can be useful when navigating unfamilar code. A javadoc comment uses the /** */ tokens, e.g.

          /**
          A call to this method means stop collecting privilege requirements for
          this node because this node is executing under definer's privileges.
          */
          public void accessThisWithDefinerPrivileges()

          Is it possible to change to javadoc methods and get into the habit of using them for new fields, classes and methods?

          Show
          Daniel John Debrunner added a comment - Just from a quick look at the patch there are many cases where you add a new method or field and add comments, but not as javadoc comments. E.g. + //A call to this method means stop collecting privilege requirements for + //this node because this node is executing under definer's privileges. + public void accessThisWithDefinerPrivileges() This means the comment does not make into the generated javadoc for the engine and (I think) will not be picked up by smart IDEs like Eclipse. In Eclipse "hovering" over a method call will show the javadoc comment for the called method, which can be useful when navigating unfamilar code. A javadoc comment uses the /** */ tokens, e.g. /** A call to this method means stop collecting privilege requirements for this node because this node is executing under definer's privileges. */ public void accessThisWithDefinerPrivileges() Is it possible to change to javadoc methods and get into the habit of using them for new fields, classes and methods?
          Hide
          Mamta A. Satoor added a comment -

          I will go ahead and change those comments to javadoc comments. Should have done them proactively I will wait for more comments on the patch before submitting a new patch.

          Show
          Mamta A. Satoor added a comment - I will go ahead and change those comments to javadoc comments. Should have done them proactively I will wait for more comments on the patch before submitting a new patch.
          Hide
          Daniel John Debrunner added a comment -

          Comment on QueryTreeNode changes and related items:
          I think the logic for what you are trying to do is correct, but the terminology and some of the descriptions in comments could be expanded.
          The code equates executing with definer's priviliges to not having to collect priviliges dependencies. I think the comments on isExecutingWithInovkerPrivileges() could be expanded to explain why collection is not needed. I think that maybe it's not generally true for definer mode and maybe the methods could be better named to relfect the actual situations, which I think are:

          • create view - collect privilege information for select statement
          • execute view (as definer) -collect privilege information for select statement
          • execute select (invoker) - do collect privilege information for select statement

          I'm just concerned that in the future when/if Derby supports definer mode for routines will the naming scheme for your methods continue to make sense, or can we ensure today that the method names (and comments) accurately reflect their purpose?

          Another aspect of this is the use of "executing" in the current tense for the QueryNodes (method name isExecutingWithInvokerPrivileges, comment "this node is executing"). The executing you are talking about relates to the runtime execution of the generated statement, not the execution of the node itself. Maybe be picky, but it's easy to get lost in the code because the present tense leads one to have the mindset the code is referring the current compilation, not the future execution.

          Show
          Daniel John Debrunner added a comment - Comment on QueryTreeNode changes and related items: I think the logic for what you are trying to do is correct, but the terminology and some of the descriptions in comments could be expanded. The code equates executing with definer's priviliges to not having to collect priviliges dependencies. I think the comments on isExecutingWithInovkerPrivileges() could be expanded to explain why collection is not needed. I think that maybe it's not generally true for definer mode and maybe the methods could be better named to relfect the actual situations, which I think are: create view - collect privilege information for select statement execute view (as definer) -collect privilege information for select statement execute select (invoker) - do collect privilege information for select statement I'm just concerned that in the future when/if Derby supports definer mode for routines will the naming scheme for your methods continue to make sense, or can we ensure today that the method names (and comments) accurately reflect their purpose? Another aspect of this is the use of "executing" in the current tense for the QueryNodes (method name isExecutingWithInvokerPrivileges, comment "this node is executing"). The executing you are talking about relates to the runtime execution of the generated statement, not the execution of the node itself. Maybe be picky, but it's easy to get lost in the code because the present tense leads one to have the mindset the code is referring the current compilation, not the future execution.
          Hide
          Mamta A. Satoor added a comment -

          Based on Dan's feedback on QueryNode, I am proposing the method name to be changed from "isExecutingWithInvokerPrivileges" to "isPrivilegeCollectionRequired". I tend to get too verbose with my method names, so if anyone has another suggestion for more appropriate method name, please suggest so. As for the comments for that method, I am suggesting following
          /**

          • Return true from this method means that we need to collect privilege
          • requirement for this node. For following cases, this method will
          • return true.
          • 1)execute view - collect privilege to access view but do not collect
          • privilege requirements for objects accessed by actual view uqery
          • 2)execute select - collect privilege requirements for objects accessed
          • by select statement
          • 3)create view - collect privileges for select statement : the select
          • statement for create view falls under 2) category above.
          • @return true if need to collect privilege requirement for this node
            */
          Show
          Mamta A. Satoor added a comment - Based on Dan's feedback on QueryNode, I am proposing the method name to be changed from "isExecutingWithInvokerPrivileges" to "isPrivilegeCollectionRequired". I tend to get too verbose with my method names, so if anyone has another suggestion for more appropriate method name, please suggest so. As for the comments for that method, I am suggesting following /** Return true from this method means that we need to collect privilege requirement for this node. For following cases, this method will return true. 1)execute view - collect privilege to access view but do not collect privilege requirements for objects accessed by actual view uqery 2)execute select - collect privilege requirements for objects accessed by select statement 3)create view - collect privileges for select statement : the select statement for create view falls under 2) category above. @return true if need to collect privilege requirement for this node */
          Hide
          Mamta A. Satoor added a comment -

          I am attaching an updated patch(Derby1330ViewPrivilegeCollectionV2diff.txt) which includes privilege collection for views, constraints and triggers. The earlier patch (Derby1330ViewPrivilegeCollectionV1diff.txt) had privilege collection changes for views only. The new patch is a superset of the earlier patch and hence, if anyone has spent time reviewing the earlier patch, that time spent won't be a waste. I have
          also changed comments around the method and field definition into java doc comments. Additionally, I have changed the method name isExecutingWithInvokerPrivileges to isPrivilegeCollectionRequired. Both these changes are based on Dan's feedback on the first review package.

          During view, trigger and constraint creation, in SQL Authorization mode, we need to collect all the privileges that are required by those objects. Using the Dependency Manager, these dependencies need to be saved in SYSDEPENDS table. If any of those privileges are later revoked by a revoke statement, then all the dependent objects(views, triggers and constraints) should get dropped.

          The patch has 4 majors components to it.
          1)If the query for create view/create trigger is accessing view/s, then Derby engine flattens all those view/s. Once the view flattening is finished, Derby engine goes through all the columns for the flattened parent sql and it starts building the privilege requirement for those columns. But since views are always accessed with definer's privileges (which means that as long as a user has privilege to access
          a view, the user can select from the view. This works even if the user does not have direct privileges to the objects accessed by the view.), When collecting privilege requirement for a given sql, Derby engine should not collect privilege requirements for objects accessed by views. In order to implement this behavior, I have added a flag to QueryTreeNode which will be set to false if the object is going to be accessed with definer's privileges. During view flattening, the objects accessed by view will be marked with definer privileges. Later, when the Derby enging goes through all the columns to collect the privilege requirements, it will check if a column is marked with definer's privilege and if so, then do not look for privilege requirement for that column.
          2)Once the privileges have been collected for the create view/create trigger/create constraint in the bind phase, they need to be recorded in SYSDEPENDS table during the execution phase of the create view/create trigger/create constraint sql. SYSDEPENDS will add one row for every privilege requirement for the view/trigger/constraint. The DEPENDENTID will be view/trigger/constraint descriptor and PROVIDERID has to be a unique id to identify permission descriptor's uniquely. In order to do this, PermissionsDescriptor needs to implement interface Provider and it needs to have a UUID associated with it. This required addition of a UUID column in SYSTABLEPERMS, SYSCOLPERMS and
          SYSROUTINEPERMS. This UUID is required by the DependencyManager to uniquely identify a row for a given PROVIDERID.
          3)Privilege collection during trigger creation is handled in a similar fashion as item 1. ie no privilege collection is required for objects that are accessed by objects running with definer privileges. For all the other objects, appropriate privilege is required by the trigger creator and those privilege requirements will be saved in SYSDEPENDS using the dependency manager. In addition, the trigger
          creator will need TRIGGER privilege on the table on which the trigger is getting defined. The dependency of the trigger on this TRIGGER privilege will be saved in SYSDEPENDS too.
          4)Among the different kinds of constraints, foreign key constraint is the only constraint in Derby that can access other objects in the database. And, anytime, an object accesses another object, appropriate privileges need to be in place. For foreign key constraints, we need to record these privilege dependencies in the system. This is so that when one of those privileges is revoked, the constraint
          should get dropped. The peculiar thing about constraint creation compared to views and triggers is one can define multiple foreign key constraints for a given table within a single sql statement. Derby collects all the privileges required for such a sql statement but foreign key constraint does not necessarily depend on all those privileges. eg
          create table t31ConstraintTest (c311 int references mamta1.t11ConstraintTest, c312 int references mamta2.t21ConstraintTest);
          For the above sql, Derby will collect REFERENCES privilege requirements for mamta1.t11ConstraintTest and mamta2.t21ConstraintTest. But the foreign key constraint on c311 depends only on REFERENCES privilege requirement for mamta1.t11ConstraintTest and foreign key constraint on c132 depenly only on REFERENCES privilege requirements for mamta2.t21ConstraintTest. I have made changes to CreateConstraintConstantAction.executeConstantAction such that only the necessary privileges get recorded for each foreign key constraint in SYSDEPENDS table.

          Following classes are impacted by this change. Compared to the first patch, there are 2 additional classes that got changed in Derby engine, when I added privilege collection for triggers and constraints. In addition, I added more tests to grantRevokeDDL.sql and to ProcedureTest.java The 2 additional classes that changed in Derby enginge compared to first patch are
          java/engine/org/apache/derby/impl/sql/execute/CreateTriggerConstantAction.java
          java/engine/org/apache/derby/impl/sql/execute/CreateConstraintConstantAction.java

          I have put some comment for all the classes that got changed/added below in this patch
          java/engine/org/apache/derby/impl/sql/compile/ResultSetNode.java
          Has method to mark this node to run with definer privileges (this could happen if the node is being accessed by an object(view/trigger/constraint) running with definer's privileges. This method also marks the result column list as running with definer privileges.

          java/engine/org/apache/derby/impl/sql/compile/DDLStatementNode.java
          Need to add a privilege requirement for the object access, only if the object is running with invoker's privileges.

          java/engine/org/apache/derby/impl/sql/compile/FromSubquery.java
          Has method to mark this node to run with definer privileges (this could happen if the node is being accessed by an object (view/trigger/constraint) running with definer's privileges. This method also marks the subquery underneath it as running with definer privileges.

          java/engine/org/apache/derby/impl/sql/compile/SelectNode.java
          Has method to mark this node to run with definer privileges (this could happen if the node is being accessed by an object (view/trigger/constraint) running with definer's privileges. This method also marks it's fromTableList as running with definer privileges.

          java/engine/org/apache/derby/impl/sql/compile/QueryTreeNode.java
          This is the super class that keeps the flag which determines if the object is getting accessed with invoker's or definer's privileges. It has methods to setter and getter methods on the flag.

          java/engine/org/apache/derby/impl/sql/compile/ResultColumn.java
          Need to add a privilege requirement for the object access, only if the object is running with invoker's privileges. In addition, it has method to mark this node to run with definer privileges (this could happen if the node is being accessed by an object(view/trigger/constraint) running with definer's privileges. This method also marks the expression underneath it as running with definer privileges.

          java/engine/org/apache/derby/impl/sql/compile/PrivilegeNode.java
          Grant was activated only on base tables in this class. I changed the code to allow grant on views along with the base tables.

          java/engine/org/apache/derby/impl/sql/compile/CreateTriggerNode.java
          Need to add a privilege requirement for the object access, only if the object is running with invoker's privileges.

          java/engine/org/apache/derby/impl/sql/compile/DeleteNode.java
          Need to add a privilege requirement for the object access, only if the object is running with invoker's privileges.

          java/engine/org/apache/derby/impl/sql/compile/CreateSchemaNode.java
          Need to add a privilege requirement for the object access, only if the object is running with invoker's privileges.

          java/engine/org/apache/derby/impl/sql/compile/JavaToSQLValueNode.java
          Has method to mark this node to run with definer privileges (this could happen if the node is being accessed by an object (view/trigger/constraint) running with definer's privileges. This method also marks it's javaNode as running with definer privileges.

          java/engine/org/apache/derby/impl/sql/compile/FromList.java
          Need to add a privilege requirement for the object access, only if the object is running with invoker's privileges.

          java/engine/org/apache/derby/impl/sql/compile/FKConstraintDefinitionNode.java
          Need to add a privilege requirement for the object access, only if the object is running with invoker's privileges.

          java/engine/org/apache/derby/impl/sql/compile/FromBaseTable.java
          If we are dealing with a view, we need to mark the query underneath it to run with definer privileges. This will make sure that we do not collect privilege requirements for that query.

          java/engine/org/apache/derby/impl/sql/compile/InsertNode.java
          Need to add a privilege requirement for the object access, only if the object is running with invoker's privileges.

          java/engine/org/apache/derby/impl/sql/compile/ResultColumnList.java
          Has method to mark this node to run with definer privileges (this could happen if the node is being accessed by an object (view/trigger/constraint) running with definer's privileges. This method also marks all the result columns in it as running with definer privileges.

          java/engine/org/apache/derby/impl/sql/compile/StaticMethodCallNode.java
          Need to add a privilege requirement for the object access, only if the object is running with invoker's privileges.

          java/engine/org/apache/derby/impl/sql/execute/CreateViewConstantAction.java
          Add rows in SYSDEPENDS to keep track of view's dependency on various privileges. If any of those privileges are later revoked, the view will be dropped automatically. If the view is getting created by the dba, then no privilege dependency is created because dba has access to all the objects in the database. If view is accessing an object that is owned by the view creator, then no need to put any privilege dependencies on such objects. If object accessed is not user owned, then check if there exists a privilege on that object for this user. If yes, then save that privilege in SYSDEPENDS table as one of the privileges required by the view. If user does not have the privilege on the object, then there has to exist a PUBLIC level privilege on that object. If that was not true, authorization checking for all the objects accessed by the views at the beginning of the execution phase would have failed. Column level privileges have a distinct behavior. For a given table, a user can have access to some of the columns via the privileges granted to the user explicitly. For the same table, the user may have access to some of the volumns via the privileged granted to the PUBLIC. an eg
          user1
          create table t11(c11 int, c12 int);
          grant select(c11) on t1 to user2;
          grant select(c12) on t1 to PUBLIC;
          user2
          create view v1 as select c11 from user1.t11 where c12=2;
          For the view above, there are 2 column level privilege depencies, one for column c11 which exists directly for user2 and one for column c12 which exists at PUBLIC level.
          java/engine/org/apache/derby/impl/sql/execute/CreateTriggerConstantAction.java (explanation for this node is same as for CreateViewConstantAction)

          java/engine/org/apache/derby/impl/sql/execute/CreateConstraintConstantAction.java
          Derby allows multiple foreign key creation within a single sql statement. The privileges collected for such a sql may end up having a superset of privileges required for individual foreign key constraints. eg
          create table t31ConstraintTest (c311 int references mamta1.t11ConstraintTest, c312 int references mamta2.t21ConstraintTest);
          For the above sql, Derby will collect REFERENCES privilege requirements for mamta1.t11ConstraintTest and mamta2.t21ConstraintTest. But the foreign key constraint on c311 depends only on REFERENCES privilege requirement for mamta1.t11ConstraintTest and foreign key constraint on c132 depenly only on REFERENCES privilege requirements for mamta2.t21ConstraintTest. The changes in the class above makes sure that right REFERENCES privilege gets picked up for each foreign key constraint. The logic for this went into executeConstantAction method.

          java/engine/org/apache/derby/impl/sql/catalog/SYSCOLPERMSRowFactory.java
          Added a column, uuid, (similar to one in SYSCONSTRAINTS). This column will be used as PROVIDERID in SYSDEPENDS table to keep track of view/triiger/constraint's dependency on column level constraints.

          java/engine/org/apache/derby/impl/sql/catalog/SYSROUTINEPERMSRowFactory.java
          Added a column, uuid, (similar to one in SYSCONSTRAINTS). This column will be used as PROVIDERID in SYSDEPENDS table to keep track of view/triiger/constraint's dependency on routine level constraints.

          java/engine/org/apache/derby/impl/sql/catalog/SYSTABLEPERMSRowFactory.java
          Added a column, uuid, (similar to one in SYSCONSTRAINTS). This column will be used as PROVIDERID in SYSDEPENDS table to keep track of view/triiger/constraint's dependency on table level constraints.

          java/engine/org/apache/derby/iapi/sql/dictionary/DataDictionary.java
          java/engine/org/apache/derby/impl/sql/catalog/DataDictionaryImpl.java
          Add a new method getColumnPermissions which is similar to another method by the same name in the class. But the new method accepts column privilege type in String format. This new method is called by ColPermsDescriptor.getDependableFinder and ColPermsDescriptor keeps privilege type in String format.

          java/engine/org/apache/derby/impl/sql/catalog/DDColumnPermissionsDependableFinder.java
          Need to add a new class because a Column Permission Descriptor needs a tableuuid and the column privilege type. The generic DDdependableFinder class can only deal with uuid and hence need a new subclass of DDdependableFinder. This new class is similar in nature to existing DDColumnDependableFinder

          java/engine/org/apache/derby/impl/sql/catalog/DDdependableFinder.java
          java/engine/org/apache/derby/impl/sql/catalog/CoreDDFinderClassInfo.java
          java/engine/org/apache/derby/catalog/Dependable.java
          java/engine/org/apache/derby/iapi/services/io/RegisteredFormatIds.java
          java/engine/org/apache/derby/iapi/services/io/StoredFormatIds.java
          Above few classes do the ground work for 3 permission descriptors so they can be used by the existing dependency system(to populate SYSDEPENDS table) to record view/triiger/constraint's dependency on them.

          java/engine/org/apache/derby/iapi/sql/dictionary/PermissionsDescriptor.java
          Permission Descriptor can now be the PROVIDER in SYSDEPENDS. ie a view/triiger/constraint might be dependent on an object level privilege. In order to allow this, PermissionsDescriptor implements Provider interface. Some of the methods of the interface are implemented by subclasses on PermissionsDescriptor. Also, each of the 3 Permission Descriptors now have a unique uuid. Added that uuid to PermissionsDescriptor and provided getter/setter on it. This uuid will be used as the PROVIDERID in SYSDEPENDS table. In
          addition, there is a method called checkOwner in PermissionsDescriptor which checks if the passed authorization id is same as the owner of the object on which the permission is defined. This gets called by create view/triiger/constraint while storing view/triiger/constraint's dependency on various permissions in SYSDEPENDS table. If the view/triiger/constraint and object being accessed
          is owned by the same authorization id, then no need to save view/triiger/constraint's dependency on object privilege in SYSDEPENDS table.

          java/engine/org/apache/derby/iapi/sql/dictionary/TablePermsDescriptor.java
          This class is modified to implement some of the PROVIDER interface methods. Another modification to this class was addition of checkOwner method (described under PermissionDescriptor class)

          java/engine/org/apache/derby/iapi/sql/dictionary/RoutinePermsDescriptor.java
          This class is modified to implement some of the PROVIDER interface methods. Another modification to this class was addition of checkOwner method (described under PermissionDescriptor class)

          java/engine/org/apache/derby/iapi/sql/dictionary/ColPermsDescriptor.java
          This class is modified to implement some of the PROVIDER interface methods. Another modification to this class was addition of checkOwner method (described under PermissionDescriptor class)

          java/engine/org/apache/derby/iapi/sql/dictionary/DataDescriptorGenerator.java
          Three methods in this class are modified to throw StandardException in their signature because they call constructor's of PermissionDescriptor subclasses and those constructors can throw StandardException.

          java/engine/org/apache/derby/iapi/sql/dictionary/StatementPermission.java
          java/engine/org/apache/derby/iapi/sql/dictionary/StatementTablePermission.java
          java/engine/org/apache/derby/iapi/sql/dictionary/StatementRoutinePermission.java
          java/engine/org/apache/derby/iapi/sql/dictionary/StatementSchemaPermission.java
          java/engine/org/apache/derby/iapi/sql/dictionary/StatementColumnPermission.java
          In the classes above, added a method called getPermissionDescriptor which will return the PermissionDescriptor for the passed authorization id for the StatementPermission object. This method gets during the execution phase of create view/trigger/constraint to track view/trigger/constraint's dependency on various permissions. getPermissionDescriptor for StatementSchemaPermission always returns null because views/triggers/constraints do not need to keep track of schema level privileges. They only need table, column and routine privileges as part of view/trigger/constraint dependency. They do need to have create permission is the schema, but once the view/trigger/constraint is created in that schema, there is no need to keep that schema permission in SYSDEPENDS. Special code is required to track column level privileges. It is possible that some column level privileges are available to the passed authorizer id but the rest required column level privileges are available at PUBLIC level. In order to record
          view/trigger/constraint dependency on user level and public level column privileges, I needed to add method getPUBLIClevelColPermsDescriptor to StatementColumnPermission.

          Test changes in following files
          java/testing/org/apache/derbyTesting/functionTests/tests/lang/grantRevokeDDL.sql
          java/testing/org/apache/derbyTesting/functionTests/master/DerbyNet/syscat.out
          java/testing/org/apache/derbyTesting/functionTests/master/grantRevokeDDL.out
          java/testing/org/apache/derbyTesting/functionTests/master/DerbyNetClient/syscat.out
          java/testing/org/apache/derbyTesting/functionTests/master/DerbyNetClient/jdk14/syscat.out
          java/testing/org/apache/derbyTesting/functionTests/master/syscat.out
          java/testing/org/apache/derbyTesting/functionTests/util/ProcedureTest.java

          I have attached the svn stat -q(Derby1330PrivilegeCollectionV2stat.txt) and svn diff(Derby1330PrivilegeCollectionV2diff.txt) output to DERBY-1330.

          As always, any feedback highly appreciated.

          Show
          Mamta A. Satoor added a comment - I am attaching an updated patch(Derby1330ViewPrivilegeCollectionV2diff.txt) which includes privilege collection for views, constraints and triggers. The earlier patch (Derby1330ViewPrivilegeCollectionV1diff.txt) had privilege collection changes for views only. The new patch is a superset of the earlier patch and hence, if anyone has spent time reviewing the earlier patch, that time spent won't be a waste. I have also changed comments around the method and field definition into java doc comments. Additionally, I have changed the method name isExecutingWithInvokerPrivileges to isPrivilegeCollectionRequired. Both these changes are based on Dan's feedback on the first review package. During view, trigger and constraint creation, in SQL Authorization mode, we need to collect all the privileges that are required by those objects. Using the Dependency Manager, these dependencies need to be saved in SYSDEPENDS table. If any of those privileges are later revoked by a revoke statement, then all the dependent objects(views, triggers and constraints) should get dropped. The patch has 4 majors components to it. 1)If the query for create view/create trigger is accessing view/s, then Derby engine flattens all those view/s. Once the view flattening is finished, Derby engine goes through all the columns for the flattened parent sql and it starts building the privilege requirement for those columns. But since views are always accessed with definer's privileges (which means that as long as a user has privilege to access a view, the user can select from the view. This works even if the user does not have direct privileges to the objects accessed by the view.), When collecting privilege requirement for a given sql, Derby engine should not collect privilege requirements for objects accessed by views. In order to implement this behavior, I have added a flag to QueryTreeNode which will be set to false if the object is going to be accessed with definer's privileges. During view flattening, the objects accessed by view will be marked with definer privileges. Later, when the Derby enging goes through all the columns to collect the privilege requirements, it will check if a column is marked with definer's privilege and if so, then do not look for privilege requirement for that column. 2)Once the privileges have been collected for the create view/create trigger/create constraint in the bind phase, they need to be recorded in SYSDEPENDS table during the execution phase of the create view/create trigger/create constraint sql. SYSDEPENDS will add one row for every privilege requirement for the view/trigger/constraint. The DEPENDENTID will be view/trigger/constraint descriptor and PROVIDERID has to be a unique id to identify permission descriptor's uniquely. In order to do this, PermissionsDescriptor needs to implement interface Provider and it needs to have a UUID associated with it. This required addition of a UUID column in SYSTABLEPERMS, SYSCOLPERMS and SYSROUTINEPERMS. This UUID is required by the DependencyManager to uniquely identify a row for a given PROVIDERID. 3)Privilege collection during trigger creation is handled in a similar fashion as item 1. ie no privilege collection is required for objects that are accessed by objects running with definer privileges. For all the other objects, appropriate privilege is required by the trigger creator and those privilege requirements will be saved in SYSDEPENDS using the dependency manager. In addition, the trigger creator will need TRIGGER privilege on the table on which the trigger is getting defined. The dependency of the trigger on this TRIGGER privilege will be saved in SYSDEPENDS too. 4)Among the different kinds of constraints, foreign key constraint is the only constraint in Derby that can access other objects in the database. And, anytime, an object accesses another object, appropriate privileges need to be in place. For foreign key constraints, we need to record these privilege dependencies in the system. This is so that when one of those privileges is revoked, the constraint should get dropped. The peculiar thing about constraint creation compared to views and triggers is one can define multiple foreign key constraints for a given table within a single sql statement. Derby collects all the privileges required for such a sql statement but foreign key constraint does not necessarily depend on all those privileges. eg create table t31ConstraintTest (c311 int references mamta1.t11ConstraintTest, c312 int references mamta2.t21ConstraintTest); For the above sql, Derby will collect REFERENCES privilege requirements for mamta1.t11ConstraintTest and mamta2.t21ConstraintTest. But the foreign key constraint on c311 depends only on REFERENCES privilege requirement for mamta1.t11ConstraintTest and foreign key constraint on c132 depenly only on REFERENCES privilege requirements for mamta2.t21ConstraintTest. I have made changes to CreateConstraintConstantAction.executeConstantAction such that only the necessary privileges get recorded for each foreign key constraint in SYSDEPENDS table. Following classes are impacted by this change. Compared to the first patch, there are 2 additional classes that got changed in Derby engine, when I added privilege collection for triggers and constraints. In addition, I added more tests to grantRevokeDDL.sql and to ProcedureTest.java The 2 additional classes that changed in Derby enginge compared to first patch are java/engine/org/apache/derby/impl/sql/execute/CreateTriggerConstantAction.java java/engine/org/apache/derby/impl/sql/execute/CreateConstraintConstantAction.java I have put some comment for all the classes that got changed/added below in this patch java/engine/org/apache/derby/impl/sql/compile/ResultSetNode.java Has method to mark this node to run with definer privileges (this could happen if the node is being accessed by an object(view/trigger/constraint) running with definer's privileges. This method also marks the result column list as running with definer privileges. java/engine/org/apache/derby/impl/sql/compile/DDLStatementNode.java Need to add a privilege requirement for the object access, only if the object is running with invoker's privileges. java/engine/org/apache/derby/impl/sql/compile/FromSubquery.java Has method to mark this node to run with definer privileges (this could happen if the node is being accessed by an object (view/trigger/constraint) running with definer's privileges. This method also marks the subquery underneath it as running with definer privileges. java/engine/org/apache/derby/impl/sql/compile/SelectNode.java Has method to mark this node to run with definer privileges (this could happen if the node is being accessed by an object (view/trigger/constraint) running with definer's privileges. This method also marks it's fromTableList as running with definer privileges. java/engine/org/apache/derby/impl/sql/compile/QueryTreeNode.java This is the super class that keeps the flag which determines if the object is getting accessed with invoker's or definer's privileges. It has methods to setter and getter methods on the flag. java/engine/org/apache/derby/impl/sql/compile/ResultColumn.java Need to add a privilege requirement for the object access, only if the object is running with invoker's privileges. In addition, it has method to mark this node to run with definer privileges (this could happen if the node is being accessed by an object(view/trigger/constraint) running with definer's privileges. This method also marks the expression underneath it as running with definer privileges. java/engine/org/apache/derby/impl/sql/compile/PrivilegeNode.java Grant was activated only on base tables in this class. I changed the code to allow grant on views along with the base tables. java/engine/org/apache/derby/impl/sql/compile/CreateTriggerNode.java Need to add a privilege requirement for the object access, only if the object is running with invoker's privileges. java/engine/org/apache/derby/impl/sql/compile/DeleteNode.java Need to add a privilege requirement for the object access, only if the object is running with invoker's privileges. java/engine/org/apache/derby/impl/sql/compile/CreateSchemaNode.java Need to add a privilege requirement for the object access, only if the object is running with invoker's privileges. java/engine/org/apache/derby/impl/sql/compile/JavaToSQLValueNode.java Has method to mark this node to run with definer privileges (this could happen if the node is being accessed by an object (view/trigger/constraint) running with definer's privileges. This method also marks it's javaNode as running with definer privileges. java/engine/org/apache/derby/impl/sql/compile/FromList.java Need to add a privilege requirement for the object access, only if the object is running with invoker's privileges. java/engine/org/apache/derby/impl/sql/compile/FKConstraintDefinitionNode.java Need to add a privilege requirement for the object access, only if the object is running with invoker's privileges. java/engine/org/apache/derby/impl/sql/compile/FromBaseTable.java If we are dealing with a view, we need to mark the query underneath it to run with definer privileges. This will make sure that we do not collect privilege requirements for that query. java/engine/org/apache/derby/impl/sql/compile/InsertNode.java Need to add a privilege requirement for the object access, only if the object is running with invoker's privileges. java/engine/org/apache/derby/impl/sql/compile/ResultColumnList.java Has method to mark this node to run with definer privileges (this could happen if the node is being accessed by an object (view/trigger/constraint) running with definer's privileges. This method also marks all the result columns in it as running with definer privileges. java/engine/org/apache/derby/impl/sql/compile/StaticMethodCallNode.java Need to add a privilege requirement for the object access, only if the object is running with invoker's privileges. java/engine/org/apache/derby/impl/sql/execute/CreateViewConstantAction.java Add rows in SYSDEPENDS to keep track of view's dependency on various privileges. If any of those privileges are later revoked, the view will be dropped automatically. If the view is getting created by the dba, then no privilege dependency is created because dba has access to all the objects in the database. If view is accessing an object that is owned by the view creator, then no need to put any privilege dependencies on such objects. If object accessed is not user owned, then check if there exists a privilege on that object for this user. If yes, then save that privilege in SYSDEPENDS table as one of the privileges required by the view. If user does not have the privilege on the object, then there has to exist a PUBLIC level privilege on that object. If that was not true, authorization checking for all the objects accessed by the views at the beginning of the execution phase would have failed. Column level privileges have a distinct behavior. For a given table, a user can have access to some of the columns via the privileges granted to the user explicitly. For the same table, the user may have access to some of the volumns via the privileged granted to the PUBLIC. an eg user1 create table t11(c11 int, c12 int); grant select(c11) on t1 to user2; grant select(c12) on t1 to PUBLIC; user2 create view v1 as select c11 from user1.t11 where c12=2; For the view above, there are 2 column level privilege depencies, one for column c11 which exists directly for user2 and one for column c12 which exists at PUBLIC level. java/engine/org/apache/derby/impl/sql/execute/CreateTriggerConstantAction.java (explanation for this node is same as for CreateViewConstantAction) java/engine/org/apache/derby/impl/sql/execute/CreateConstraintConstantAction.java Derby allows multiple foreign key creation within a single sql statement. The privileges collected for such a sql may end up having a superset of privileges required for individual foreign key constraints. eg create table t31ConstraintTest (c311 int references mamta1.t11ConstraintTest, c312 int references mamta2.t21ConstraintTest); For the above sql, Derby will collect REFERENCES privilege requirements for mamta1.t11ConstraintTest and mamta2.t21ConstraintTest. But the foreign key constraint on c311 depends only on REFERENCES privilege requirement for mamta1.t11ConstraintTest and foreign key constraint on c132 depenly only on REFERENCES privilege requirements for mamta2.t21ConstraintTest. The changes in the class above makes sure that right REFERENCES privilege gets picked up for each foreign key constraint. The logic for this went into executeConstantAction method. java/engine/org/apache/derby/impl/sql/catalog/SYSCOLPERMSRowFactory.java Added a column, uuid, (similar to one in SYSCONSTRAINTS). This column will be used as PROVIDERID in SYSDEPENDS table to keep track of view/triiger/constraint's dependency on column level constraints. java/engine/org/apache/derby/impl/sql/catalog/SYSROUTINEPERMSRowFactory.java Added a column, uuid, (similar to one in SYSCONSTRAINTS). This column will be used as PROVIDERID in SYSDEPENDS table to keep track of view/triiger/constraint's dependency on routine level constraints. java/engine/org/apache/derby/impl/sql/catalog/SYSTABLEPERMSRowFactory.java Added a column, uuid, (similar to one in SYSCONSTRAINTS). This column will be used as PROVIDERID in SYSDEPENDS table to keep track of view/triiger/constraint's dependency on table level constraints. java/engine/org/apache/derby/iapi/sql/dictionary/DataDictionary.java java/engine/org/apache/derby/impl/sql/catalog/DataDictionaryImpl.java Add a new method getColumnPermissions which is similar to another method by the same name in the class. But the new method accepts column privilege type in String format. This new method is called by ColPermsDescriptor.getDependableFinder and ColPermsDescriptor keeps privilege type in String format. java/engine/org/apache/derby/impl/sql/catalog/DDColumnPermissionsDependableFinder.java Need to add a new class because a Column Permission Descriptor needs a tableuuid and the column privilege type. The generic DDdependableFinder class can only deal with uuid and hence need a new subclass of DDdependableFinder. This new class is similar in nature to existing DDColumnDependableFinder java/engine/org/apache/derby/impl/sql/catalog/DDdependableFinder.java java/engine/org/apache/derby/impl/sql/catalog/CoreDDFinderClassInfo.java java/engine/org/apache/derby/catalog/Dependable.java java/engine/org/apache/derby/iapi/services/io/RegisteredFormatIds.java java/engine/org/apache/derby/iapi/services/io/StoredFormatIds.java Above few classes do the ground work for 3 permission descriptors so they can be used by the existing dependency system(to populate SYSDEPENDS table) to record view/triiger/constraint's dependency on them. java/engine/org/apache/derby/iapi/sql/dictionary/PermissionsDescriptor.java Permission Descriptor can now be the PROVIDER in SYSDEPENDS. ie a view/triiger/constraint might be dependent on an object level privilege. In order to allow this, PermissionsDescriptor implements Provider interface. Some of the methods of the interface are implemented by subclasses on PermissionsDescriptor. Also, each of the 3 Permission Descriptors now have a unique uuid. Added that uuid to PermissionsDescriptor and provided getter/setter on it. This uuid will be used as the PROVIDERID in SYSDEPENDS table. In addition, there is a method called checkOwner in PermissionsDescriptor which checks if the passed authorization id is same as the owner of the object on which the permission is defined. This gets called by create view/triiger/constraint while storing view/triiger/constraint's dependency on various permissions in SYSDEPENDS table. If the view/triiger/constraint and object being accessed is owned by the same authorization id, then no need to save view/triiger/constraint's dependency on object privilege in SYSDEPENDS table. java/engine/org/apache/derby/iapi/sql/dictionary/TablePermsDescriptor.java This class is modified to implement some of the PROVIDER interface methods. Another modification to this class was addition of checkOwner method (described under PermissionDescriptor class) java/engine/org/apache/derby/iapi/sql/dictionary/RoutinePermsDescriptor.java This class is modified to implement some of the PROVIDER interface methods. Another modification to this class was addition of checkOwner method (described under PermissionDescriptor class) java/engine/org/apache/derby/iapi/sql/dictionary/ColPermsDescriptor.java This class is modified to implement some of the PROVIDER interface methods. Another modification to this class was addition of checkOwner method (described under PermissionDescriptor class) java/engine/org/apache/derby/iapi/sql/dictionary/DataDescriptorGenerator.java Three methods in this class are modified to throw StandardException in their signature because they call constructor's of PermissionDescriptor subclasses and those constructors can throw StandardException. java/engine/org/apache/derby/iapi/sql/dictionary/StatementPermission.java java/engine/org/apache/derby/iapi/sql/dictionary/StatementTablePermission.java java/engine/org/apache/derby/iapi/sql/dictionary/StatementRoutinePermission.java java/engine/org/apache/derby/iapi/sql/dictionary/StatementSchemaPermission.java java/engine/org/apache/derby/iapi/sql/dictionary/StatementColumnPermission.java In the classes above, added a method called getPermissionDescriptor which will return the PermissionDescriptor for the passed authorization id for the StatementPermission object. This method gets during the execution phase of create view/trigger/constraint to track view/trigger/constraint's dependency on various permissions. getPermissionDescriptor for StatementSchemaPermission always returns null because views/triggers/constraints do not need to keep track of schema level privileges. They only need table, column and routine privileges as part of view/trigger/constraint dependency. They do need to have create permission is the schema, but once the view/trigger/constraint is created in that schema, there is no need to keep that schema permission in SYSDEPENDS. Special code is required to track column level privileges. It is possible that some column level privileges are available to the passed authorizer id but the rest required column level privileges are available at PUBLIC level. In order to record view/trigger/constraint dependency on user level and public level column privileges, I needed to add method getPUBLIClevelColPermsDescriptor to StatementColumnPermission. Test changes in following files java/testing/org/apache/derbyTesting/functionTests/tests/lang/grantRevokeDDL.sql java/testing/org/apache/derbyTesting/functionTests/master/DerbyNet/syscat.out java/testing/org/apache/derbyTesting/functionTests/master/grantRevokeDDL.out java/testing/org/apache/derbyTesting/functionTests/master/DerbyNetClient/syscat.out java/testing/org/apache/derbyTesting/functionTests/master/DerbyNetClient/jdk14/syscat.out java/testing/org/apache/derbyTesting/functionTests/master/syscat.out java/testing/org/apache/derbyTesting/functionTests/util/ProcedureTest.java I have attached the svn stat -q(Derby1330PrivilegeCollectionV2stat.txt) and svn diff(Derby1330PrivilegeCollectionV2diff.txt) output to DERBY-1330 . As always, any feedback highly appreciated.
          Hide
          Mamta A. Satoor added a comment -

          Wondered if anyone got a chance to look at the patch.

          Show
          Mamta A. Satoor added a comment - Wondered if anyone got a chance to look at the patch.
          Hide
          Daniel John Debrunner added a comment -

          I'm starting to look at the patch, I'm a little confused by your references to 'add a row to SYSDEPENDS'. Is your code doing this directly or is it using the dependency manager (as I hope)? Might be clearer if you described the change in terms of "adding a dependency on X" rather than "adding a row to SYSDEPENDS". Then the description and comments would match the code.

          The implementation of the dependency manager (ie. the current implementation uses SYSDEPENDS) should not be a factor in the design here.

          Show
          Daniel John Debrunner added a comment - I'm starting to look at the patch, I'm a little confused by your references to 'add a row to SYSDEPENDS'. Is your code doing this directly or is it using the dependency manager (as I hope)? Might be clearer if you described the change in terms of "adding a dependency on X" rather than "adding a row to SYSDEPENDS". Then the description and comments would match the code. The implementation of the dependency manager (ie. the current implementation uses SYSDEPENDS) should not be a factor in the design here.
          Hide
          Mamta A. Satoor added a comment -

          I am using the dependency manager and not touching SYSDEPENDS directly through my code changes. I will change the code comments to talk in terms of dependency manager rather than SYSDEPENDS but will wait for further review feedback before submiiting a new patch. Please let me know if a patch with comment changes will make the code review easier for you, in that case, I will go ahead and commit an updated patch.

          Show
          Mamta A. Satoor added a comment - I am using the dependency manager and not touching SYSDEPENDS directly through my code changes. I will change the code comments to talk in terms of dependency manager rather than SYSDEPENDS but will wait for further review feedback before submiiting a new patch. Please let me know if a patch with comment changes will make the code review easier for you, in that case, I will go ahead and commit an updated patch.
          Hide
          Daniel John Debrunner added a comment -

          What would be really helpful if was if this patch was split into multiple patches.

          I'm finally realizing the patch is making several changes:

          1) Add mechanism to stop collection of privilege information during compilation

          2) Use mechanism of 1) for view execution

          3) Add a UUID to the existing privilege systems tables.

          4) Change the PrivilegeDescriptor classes to be Providers as well as being
          TupleDescriptors

          5) Allowing GRANT to execute on views

          6) Make CREATE VIEW depend on the(existing) list of priviliges collected

          7) Stuff added for triggers added recently that I haven't managed to look at.

          8) Stuff added for constraints added recently that I haven't managed to look at.

          9) Others I've missed due to the size of the patch

          +) Test changes.

          So 8+) seperate changes "forced" into a single 352k patch. Personally I find patches like this really hard to review due to their size and multiple issues. I believe the risk of missing something in a review or in writing the code is increased by the size of the change, thus the quality of Derby can be degraded by accepting large patches. Incremental development is good.

          I know it's hard to anticipate & identify incremental changes from the start of implementing functionality, but it's a good habit to get into. As one develops something and realise that a new piece of functionality is needed, stop and think if it could be implemented in a separate clean code line and delivered as a separate patch. For example, with this change the requirement for the UUID to be added to the existing system tables could have been identified as a stand-alone item.
          Another technique I use is to separate items after the fact. Once a codeline becomes clogged with multiple changes, I pull individual changes into a separate code line and then test & submit incrementally. I will write this up in the wiki.

          Now, I do believe that the we, the community, could maybe do a better job to encourage smaller patches, faster turn around on submitted small patches etc., but I also believe, in general, the contributors can help a lot here. At least by following the guidelines from the wiki. I think we are in somewhat of a negative-feedback situation, patches are slow to be reviewed & committed due to time constraints and patch complexity leading to contributors trying to cram more things into a single patch to only have to undergo a single commit leading to longer review times etc. etc. If we could break the cycle and have fast turnaround on small patches it would move more people to the incremental development model.

          As for this specific patch, I'm not sure it's a good plan to have you submit 8+ patches & the testing associated with it, given we have a working patch here. There are some more changes I would like to see, and I would like you to refrain from adding any more functionality into the patch. I wll add specific review comments in another comment.

          Show
          Daniel John Debrunner added a comment - What would be really helpful if was if this patch was split into multiple patches. I'm finally realizing the patch is making several changes: 1) Add mechanism to stop collection of privilege information during compilation 2) Use mechanism of 1) for view execution 3) Add a UUID to the existing privilege systems tables. 4) Change the PrivilegeDescriptor classes to be Providers as well as being TupleDescriptors 5) Allowing GRANT to execute on views 6) Make CREATE VIEW depend on the(existing) list of priviliges collected 7) Stuff added for triggers added recently that I haven't managed to look at. 8) Stuff added for constraints added recently that I haven't managed to look at. 9) Others I've missed due to the size of the patch +) Test changes. So 8+) seperate changes "forced" into a single 352k patch. Personally I find patches like this really hard to review due to their size and multiple issues. I believe the risk of missing something in a review or in writing the code is increased by the size of the change, thus the quality of Derby can be degraded by accepting large patches. Incremental development is good. I know it's hard to anticipate & identify incremental changes from the start of implementing functionality, but it's a good habit to get into. As one develops something and realise that a new piece of functionality is needed, stop and think if it could be implemented in a separate clean code line and delivered as a separate patch. For example, with this change the requirement for the UUID to be added to the existing system tables could have been identified as a stand-alone item. Another technique I use is to separate items after the fact. Once a codeline becomes clogged with multiple changes, I pull individual changes into a separate code line and then test & submit incrementally. I will write this up in the wiki. Now, I do believe that the we, the community, could maybe do a better job to encourage smaller patches, faster turn around on submitted small patches etc., but I also believe, in general, the contributors can help a lot here. At least by following the guidelines from the wiki. I think we are in somewhat of a negative-feedback situation, patches are slow to be reviewed & committed due to time constraints and patch complexity leading to contributors trying to cram more things into a single patch to only have to undergo a single commit leading to longer review times etc. etc. If we could break the cycle and have fast turnaround on small patches it would move more people to the incremental development model. As for this specific patch, I'm not sure it's a good plan to have you submit 8+ patches & the testing associated with it, given we have a working patch here. There are some more changes I would like to see, and I would like you to refrain from adding any more functionality into the patch. I wll add specific review comments in another comment.
          Hide
          Daniel John Debrunner added a comment -

          Mamta wrote: "I am proposing the method name to be changed from "isExecutingWithInvokerPrivileges" to "isPrivilegeCollectionRequired". I tend to get too verbose with my method names"

          Not at all, that method name describes exactly what it does, a perfect name.

          One issue with the patch is that your change to the new (correct) name seems to be only half implemented, e.g.

          + public boolean isPrivilegeCollectionRequired()
          +

          { + return(accessWithInvokerPrivileges); + }

          Shouldn't the field name accessWithInvokerPrivileges be the same as the method name?

          Then for:

          + public void accessThisWithDefinerPrivileges()
          +

          { + accessWithInvokerPrivileges = false; + }

          Shouldn't this method be called something like, ' disablePrivilegeCollection'?

          Otherwise my earlier concerns still stand, in the future will we have to change the name of accessWithInvokerPrivileges /accessThisWithDefinerPrivileges because their usage (stop collection) might not match the access mode.

          Other comments on the patch are the similarity of the code in the create constant actions for triggers and views, in fact the one in trigger has only views as examples in the comments. This is the code that converts the list of privileges into a list of dependencies (or added dependencies). Could this code be shared, what about the one in create constraint?

          If you can address these & earlier comments I will commit the patch.

          Show
          Daniel John Debrunner added a comment - Mamta wrote: "I am proposing the method name to be changed from "isExecutingWithInvokerPrivileges" to "isPrivilegeCollectionRequired". I tend to get too verbose with my method names" Not at all, that method name describes exactly what it does, a perfect name. One issue with the patch is that your change to the new (correct) name seems to be only half implemented, e.g. + public boolean isPrivilegeCollectionRequired() + { + return(accessWithInvokerPrivileges); + } Shouldn't the field name accessWithInvokerPrivileges be the same as the method name? Then for: + public void accessThisWithDefinerPrivileges() + { + accessWithInvokerPrivileges = false; + } Shouldn't this method be called something like, ' disablePrivilegeCollection'? Otherwise my earlier concerns still stand, in the future will we have to change the name of accessWithInvokerPrivileges /accessThisWithDefinerPrivileges because their usage (stop collection) might not match the access mode. Other comments on the patch are the similarity of the code in the create constant actions for triggers and views, in fact the one in trigger has only views as examples in the comments. This is the code that converts the list of privileges into a list of dependencies (or added dependencies). Could this code be shared, what about the one in create constraint? If you can address these & earlier comments I will commit the patch.
          Hide
          Mamta A. Satoor added a comment -

          Dan, truly appreciate your time on the patch review.

          I think I have taken care of all your comments in the attached review package ( Derby1330PrivilegeCollectionV3diff.txt) Along with javadoc comments, method and variable renaming, I have abstracted the actual dependency saving code from CreateViewConstantAction, CreateConstraintConstantAction and CreateTriggerConstantAction into DDLConstantAction as 2 methods.

          Running the derbyall suite suite right noww for sanity check. Please let me know if I missed any of your feedback or if you/anyone else has any comments on the latest review package.

          svn stat -q output attached as Derby1330PrivilegeCollectionV3stat.txt

          Show
          Mamta A. Satoor added a comment - Dan, truly appreciate your time on the patch review. I think I have taken care of all your comments in the attached review package ( Derby1330PrivilegeCollectionV3diff.txt) Along with javadoc comments, method and variable renaming, I have abstracted the actual dependency saving code from CreateViewConstantAction, CreateConstraintConstantAction and CreateTriggerConstantAction into DDLConstantAction as 2 methods. Running the derbyall suite suite right noww for sanity check. Please let me know if I missed any of your feedback or if you/anyone else has any comments on the latest review package. svn stat -q output attached as Derby1330PrivilegeCollectionV3stat.txt
          Hide
          Mamta A. Satoor added a comment -

          derbyall suite finished with no new failures.

          Show
          Mamta A. Satoor added a comment - derbyall suite finished with no new failures.
          Hide
          Daniel John Debrunner added a comment -

          Patch Derby1330PrivilegeCollectionV3diff.txt Committed revision 420306 - Thanks Mamta

          Show
          Daniel John Debrunner added a comment - Patch Derby1330PrivilegeCollectionV3diff.txt Committed revision 420306 - Thanks Mamta
          Hide
          Knut Anders Hatlen added a comment -

          I have seen this regression test failure a couple of times lately. Could it be related to this issue?

                          • Diff file derbyall/derbylang/grantRevokeDDL.diff
              • Start: grantRevokeDDL jdk1.5.0_04 derbyall:derbylang 2006-07-10 09:56:26 ***
                560 del
                < ERROR 28508: User 'MAMTA3' does not have select permission on column 'C111' of table 'MAMTA2'.'V22'.
                560a560
                > ERROR 28508: User 'MAMTA3' does not have select permission on column 'C111' of table 'MAMTA2'.'V21'.
                Test Failed.
              • End: grantRevokeDDL jdk1.5.0_04 derbyall:derbylang 2006-07-10 09:56:37 ***

          It seems to happen on both Solaris and Linux, JVM 1.4 and 1.5:
          http://www.multinet.no/~solberg/public/Apache/Derby/testlog/Linux-2.6.9-34.ELsmp_x86_64-x86_64/420328-derbylang_diff.txt
          http://www.multinet.no/~solberg/public/Apache/Derby/testlog/SunOS-5.10_i86pc-i386/420328-derbylang_diff.txt
          http://www.multinet.no/~solberg/public/Apache/DerbyJvm1.4/testlog/Linux-2.6.9-34.ELsmp_x86_64-x86_64/420328-derbyall_diff.txt
          http://www.multinet.no/~solberg/public/Apache/DerbyJvm1.4/testlog/SunOS-5.10_i86pc-i386/420328-derbyall_diff.txt

          It does not happen on the tinderbox, though.

          Show
          Knut Anders Hatlen added a comment - I have seen this regression test failure a couple of times lately. Could it be related to this issue? Diff file derbyall/derbylang/grantRevokeDDL.diff Start: grantRevokeDDL jdk1.5.0_04 derbyall:derbylang 2006-07-10 09:56:26 *** 560 del < ERROR 28508: User 'MAMTA3' does not have select permission on column 'C111' of table 'MAMTA2'.'V22'. 560a560 > ERROR 28508: User 'MAMTA3' does not have select permission on column 'C111' of table 'MAMTA2'.'V21'. Test Failed. End: grantRevokeDDL jdk1.5.0_04 derbyall:derbylang 2006-07-10 09:56:37 *** It seems to happen on both Solaris and Linux, JVM 1.4 and 1.5: http://www.multinet.no/~solberg/public/Apache/Derby/testlog/Linux-2.6.9-34.ELsmp_x86_64-x86_64/420328-derbylang_diff.txt http://www.multinet.no/~solberg/public/Apache/Derby/testlog/SunOS-5.10_i86pc-i386/420328-derbylang_diff.txt http://www.multinet.no/~solberg/public/Apache/DerbyJvm1.4/testlog/Linux-2.6.9-34.ELsmp_x86_64-x86_64/420328-derbyall_diff.txt http://www.multinet.no/~solberg/public/Apache/DerbyJvm1.4/testlog/SunOS-5.10_i86pc-i386/420328-derbyall_diff.txt It does not happen on the tinderbox, though.
          Hide
          Mamta A. Satoor added a comment -

          Knut, the diffs you are noticing is in one of the new tests that I added to grantRevokeDDL.sql

          The diff is for following sql which is executed by mamta3(Note that mamta3 does not have SELECT privilege on mamta2.v22.c111 and on mamta2.v21.c111 but does have SELECT privilege on mamta1.t11)

          create view v33 as select v22.c111 as a, t11.c111 as b from mamta2.v22 v22, mamta1.t11 t11, mamta2.v21;

          When this create view is compiled by Derby, it collects the privileges required by the create view. Derby collects following privilege requirements for the create view sql above
          1)SELECT on mamta2.v22
          2)SELECT on mamta1.t11
          3)SELECT on mamta2.v21
          4)ownership of the schema in which create view is being issued

          Later on, at create view execution time, Derby grows through the required privileges list and checks if mamta3 has the required privileges or not. And since mamta3 does not have the required privileges on mamta2.v22 and mamta2.v21, the create view fails.

          In the diff that you posted above, both the error messages are correct. It seems like
          1)either the order in which the privilege requirements get added to the list in compile phase is not always the same
          2)or the order in which privileges get retrieved in the execution phase is not always the same
          and hence different error messages

          CompilerContextimpl.addRequiredTablePriv() has following
          requiredTablePrivileges.put(key, key);
          and requiredTablePrivileges is defined as follows in CompilerContextimpl
          private HashMap requiredTablePrivileges;

          Is this a Java behavior that you can count on the order in which items will be added and retrieved from HashMap?

          At this point, I am not sure, how to make sure that items get added and retrieved in the same order from the HashMap so that we will have consistent error message return from the create view sql above. Any insight from the list will be appreciated.

          Show
          Mamta A. Satoor added a comment - Knut, the diffs you are noticing is in one of the new tests that I added to grantRevokeDDL.sql The diff is for following sql which is executed by mamta3(Note that mamta3 does not have SELECT privilege on mamta2.v22.c111 and on mamta2.v21.c111 but does have SELECT privilege on mamta1.t11) create view v33 as select v22.c111 as a, t11.c111 as b from mamta2.v22 v22, mamta1.t11 t11, mamta2.v21; When this create view is compiled by Derby, it collects the privileges required by the create view. Derby collects following privilege requirements for the create view sql above 1)SELECT on mamta2.v22 2)SELECT on mamta1.t11 3)SELECT on mamta2.v21 4)ownership of the schema in which create view is being issued Later on, at create view execution time, Derby grows through the required privileges list and checks if mamta3 has the required privileges or not. And since mamta3 does not have the required privileges on mamta2.v22 and mamta2.v21, the create view fails. In the diff that you posted above, both the error messages are correct. It seems like 1)either the order in which the privilege requirements get added to the list in compile phase is not always the same 2)or the order in which privileges get retrieved in the execution phase is not always the same and hence different error messages CompilerContextimpl.addRequiredTablePriv() has following requiredTablePrivileges.put(key, key); and requiredTablePrivileges is defined as follows in CompilerContextimpl private HashMap requiredTablePrivileges; Is this a Java behavior that you can count on the order in which items will be added and retrieved from HashMap? At this point, I am not sure, how to make sure that items get added and retrieved in the same order from the HashMap so that we will have consistent error message return from the create view sql above. Any insight from the list will be appreciated.
          Hide
          Mamta A. Satoor added a comment -

          Mike responded following on the mailing list
          <quote>
          I do not believe you can count on the order of a HashMap, different
          JVM's may use different hash algo's which may result in different orders
          when you ask for the full list. I have seen this behavior in queries
          which use hash nodes in derby (I believe we first noticed a difference
          between j9 and other jvm's). In that case we added order by's as
          necessary to the tests, as either order of results was correct from
          SQL point of view.

          In your case is the order a code problem, or just a testing issue?
          </quote>

          First of all, thanks Mike for your response.

          As to your question, the order is not a testing issue because test is simply trying a scenario where a user is trying to create a object based on more than one object on which the user doesn't have access to. And depending on how items got into HashMap, the test fails with privilege error on one object vs the other. So, in this case, the order is a code problem.

          Show
          Mamta A. Satoor added a comment - Mike responded following on the mailing list <quote> I do not believe you can count on the order of a HashMap, different JVM's may use different hash algo's which may result in different orders when you ask for the full list. I have seen this behavior in queries which use hash nodes in derby (I believe we first noticed a difference between j9 and other jvm's). In that case we added order by's as necessary to the tests, as either order of results was correct from SQL point of view. In your case is the order a code problem, or just a testing issue? </quote> First of all, thanks Mike for your response. As to your question, the order is not a testing issue because test is simply trying a scenario where a user is trying to create a object based on more than one object on which the user doesn't have access to. And depending on how items got into HashMap, the test fails with privilege error on one object vs the other. So, in this case, the order is a code problem.
          Hide
          Mamta A. Satoor added a comment -

          The patch Derby1330PrivilegeCollectionV3diff.txt attached to this JIRA entry was recently committed into main trunk. I am working on revoke privilege which is based on that committed patch. While working on revoke privilege, I realized that I have incorrect logic for getting a permission descriptor from system tables based on the unique uuid. SYSTABLEPERMS, SYSCOLPERMS and SYSROUTINEPERMS each have a uuid column called TABLEPERMSID, COLPERMSID and ROUTINEPERMSID respectively. When a view\constraint\trigger is found to be dependent on a permission, we have the dependency manager track view\constraint\trigger's dependency on that permission's unique uuid. Later on, that unique uuid can be used to get the unique row from the system tables. The patch Derby1330PrivilegeCollectionV3diff.txt accidentally looked for combination of unique uuid and
          permission type to find the exact unique row from the system tables. The uuid column is sufficient to find the unique row form these 3 system tables and there is no need to keep track of permission type.

          I have attached a patch (Derby1330uuidIndexForPermsSystemTablesV4diff.txt) to fix that behavior. The patch gets rid of requirement of permission type along with the uuid to find a unique row. Fixing this has in fact cleaned up the supporting code quite a bit. For instance, there is no need for special class DDColumnPermissionDependableFinder or no need for a constructor in ColPermsDescriptor which takes permission types in the form of an int. DDdependableFinder is much cleaner too with this patch. In addition, I have also created a unique index on the uuid for these 3 system tables in order to quickly locate a row in those system tables.

          svn stat -q output is attached as Derby1330uuidIndexForPermsSystemTablesV4stat.txt

          Can someone please review this patch for me and submit it to the trunk if everything looks good?

          The grantRevokeDDL.sql test is ran fine and I have fired derbyall.

          Show
          Mamta A. Satoor added a comment - The patch Derby1330PrivilegeCollectionV3diff.txt attached to this JIRA entry was recently committed into main trunk. I am working on revoke privilege which is based on that committed patch. While working on revoke privilege, I realized that I have incorrect logic for getting a permission descriptor from system tables based on the unique uuid. SYSTABLEPERMS, SYSCOLPERMS and SYSROUTINEPERMS each have a uuid column called TABLEPERMSID, COLPERMSID and ROUTINEPERMSID respectively. When a view\constraint\trigger is found to be dependent on a permission, we have the dependency manager track view\constraint\trigger's dependency on that permission's unique uuid. Later on, that unique uuid can be used to get the unique row from the system tables. The patch Derby1330PrivilegeCollectionV3diff.txt accidentally looked for combination of unique uuid and permission type to find the exact unique row from the system tables. The uuid column is sufficient to find the unique row form these 3 system tables and there is no need to keep track of permission type. I have attached a patch (Derby1330uuidIndexForPermsSystemTablesV4diff.txt) to fix that behavior. The patch gets rid of requirement of permission type along with the uuid to find a unique row. Fixing this has in fact cleaned up the supporting code quite a bit. For instance, there is no need for special class DDColumnPermissionDependableFinder or no need for a constructor in ColPermsDescriptor which takes permission types in the form of an int. DDdependableFinder is much cleaner too with this patch. In addition, I have also created a unique index on the uuid for these 3 system tables in order to quickly locate a row in those system tables. svn stat -q output is attached as Derby1330uuidIndexForPermsSystemTablesV4stat.txt Can someone please review this patch for me and submit it to the trunk if everything looks good? The grantRevokeDDL.sql test is ran fine and I have fired derbyall.
          Hide
          Mamta A. Satoor added a comment -

          I have come across one test failure in derbyall suite with my changes, so please don't commit the latest patch just yet.

          Show
          Mamta A. Satoor added a comment - I have come across one test failure in derbyall suite with my changes, so please don't commit the latest patch just yet.
          Hide
          Mamta A. Satoor added a comment -

          With the earlier (uncommitted) patch, one of the tests in derbyall was failing for following sql
          grant execute on function f_abs to mamata2, mamata3;
          The execution of grant above needs to add 2 rows into SYSTABLEPERMS. And each one of those rows need to have a unique uuid. In my earlier patch, I had forgotten to reset the uuid in TablePrivilegeInfo.executeGrantRevoke and that is why the test above was failing. Similar change is required in RoutinePrivilegeInfo.executeGrantRevoke for grant on a function to multiple users in one statement.

          Also, I had said in the comments for earlier patch that there is no need for a constructor in ColPermsDescriptor which takes permission types in the form of an int. While prototyping revoke references privilege, I realized that we do need that constructor. So I am going to leave the constructor as it is and it's usage will be clear once I have the patch fo revoke references privilege ready.

          Can someone review the patch (Derby1330uuidIndexForPermsSystemTablesV5diff.txt) for me and commit it if it looks good?

          Show
          Mamta A. Satoor added a comment - With the earlier (uncommitted) patch, one of the tests in derbyall was failing for following sql grant execute on function f_abs to mamata2, mamata3; The execution of grant above needs to add 2 rows into SYSTABLEPERMS. And each one of those rows need to have a unique uuid. In my earlier patch, I had forgotten to reset the uuid in TablePrivilegeInfo.executeGrantRevoke and that is why the test above was failing. Similar change is required in RoutinePrivilegeInfo.executeGrantRevoke for grant on a function to multiple users in one statement. Also, I had said in the comments for earlier patch that there is no need for a constructor in ColPermsDescriptor which takes permission types in the form of an int. While prototyping revoke references privilege, I realized that we do need that constructor. So I am going to leave the constructor as it is and it's usage will be clear once I have the patch fo revoke references privilege ready. Can someone review the patch (Derby1330uuidIndexForPermsSystemTablesV5diff.txt) for me and commit it if it looks good?
          Hide
          Daniel John Debrunner added a comment -

          Mamta wrote:

          • In my earlier patch, I had forgotten to reset the uuid in TablePrivilegeInfo.executeGrantRevoke and that is why the test above was failing. Similar change is required in RoutinePrivilegeInfo.executeGrantRevoke for grant on a function to multiple users in one statement.

          Looking at the code for this it implies to me that the api is incorrect. Requiring the caller to reset the uuid is likely to lead to bugs, could the addRemovePermissionsDescriptor handle this automatically rather than relying on the caller? This could be addressed in a subsequent patch.

          Show
          Daniel John Debrunner added a comment - Mamta wrote: In my earlier patch, I had forgotten to reset the uuid in TablePrivilegeInfo.executeGrantRevoke and that is why the test above was failing. Similar change is required in RoutinePrivilegeInfo.executeGrantRevoke for grant on a function to multiple users in one statement. Looking at the code for this it implies to me that the api is incorrect. Requiring the caller to reset the uuid is likely to lead to bugs, could the addRemovePermissionsDescriptor handle this automatically rather than relying on the caller? This could be addressed in a subsequent patch.
          Hide
          Daniel John Debrunner added a comment -

          Derby1330uuidIndexForPermsSystemTablesV5diff.txt Patch committed - Committed revision 421118 - Thanks Mamta!

          Show
          Daniel John Debrunner added a comment - Derby1330uuidIndexForPermsSystemTablesV5diff.txt Patch committed - Committed revision 421118 - Thanks Mamta!
          Hide
          Mamta A. Satoor added a comment -

          As per Dan's suggestion, I have moved the resetting of permission descriptor's uuid into DataDictionary.addRemovePermissionsDescriptor method. The derbyall runs fine with no new failures. I have also added couple tests to lang\grantRevokeDDL.sql

          Please find this change in the attached Derby1330uuidIndexForPermsSystemTablesV6diff.txt. svn stat -q output is in Derby1330uuidIndexForPermsSystemTablesV6stat.txt

          Show
          Mamta A. Satoor added a comment - As per Dan's suggestion, I have moved the resetting of permission descriptor's uuid into DataDictionary.addRemovePermissionsDescriptor method. The derbyall runs fine with no new failures. I have also added couple tests to lang\grantRevokeDDL.sql Please find this change in the attached Derby1330uuidIndexForPermsSystemTablesV6diff.txt. svn stat -q output is in Derby1330uuidIndexForPermsSystemTablesV6stat.txt
          Hide
          Mamta A. Satoor added a comment -

          Just wondered if anyone got a chance to look at the patch Derby1330uuidIndexForPermsSystemTablesV6diff.txt. This one is a pretty localized change.

          Show
          Mamta A. Satoor added a comment - Just wondered if anyone got a chance to look at the patch Derby1330uuidIndexForPermsSystemTablesV6diff.txt. This one is a pretty localized change.
          Hide
          Daniel John Debrunner added a comment -

          Patch Derby1330uuidIndexForPermsSystemTablesV6diff.txt applied Committed revision 421981. Thanks Mamta

          Show
          Daniel John Debrunner added a comment - Patch Derby1330uuidIndexForPermsSystemTablesV6diff.txt applied Committed revision 421981. Thanks Mamta
          Hide
          Mamta A. Satoor added a comment -

          I have an extremely minor patch attached to this JIRA entry (svn diff attached as Derby1330MinorCleanupV7diff.txt and svn stat -q attached as Derby1330MinorCleanupV7stat.txt). There is a typo in SYSROUTINEPERMSRowFactory.java for one of the variables and this patch fixes just that. I have run the 2 grant revoke tests and they ran fine. I don't see a need to run derbyall but let me know if someone thinks it should be run.

          Show
          Mamta A. Satoor added a comment - I have an extremely minor patch attached to this JIRA entry (svn diff attached as Derby1330MinorCleanupV7diff.txt and svn stat -q attached as Derby1330MinorCleanupV7stat.txt). There is a typo in SYSROUTINEPERMSRowFactory.java for one of the variables and this patch fixes just that. I have run the 2 grant revoke tests and they ran fine. I don't see a need to run derbyall but let me know if someone thinks it should be run.
          Hide
          Mamta A. Satoor added a comment -

          Currently, in case of revoke privilege, DataDictionary.addRemovePermissionsDescriptor expects it's callers to set the permission descriptor's uuid so that DependencyManager can be invoked by addRemovePermissionsDescriptor to send REVOKE_PRIVILEGE action to permission descriptor's dependents(sending of REVOKE_PRIVILEGE work has not been finished yet. This patch is in preparation of that work). In order to set the uuid, the callers have to goto system tables to find the uuid and then set the permission descriptor's uuid.

          It will be less error prone if DataDictionary.addRemovePermissionsDescriptor did the setting of uuid of permission descriptor, rather than having every caller set the uuid correctly. DataDictionary.addRemovePermissionsDescriptor method has to goto the permission system tables anyways and has the uuid information handy and hence it will be more efficient for it to set the uuid of the permission descriptor. This is inline with the patch that was commited some time back so that resetting of the uuid happened in addRemovePermissionsDescriptor method rather than expecting the callers to do that.

          In order to do this, I have added an abstract method to PermissionsCatalogRowFactory called setUUIDOfThePassedDescriptor. This method will set the uuid of the passed permission descriptor to same value as the row corresponding to the permission system table which is also passed as a parameter. This method will be called by DataDictionary.addRemovePermissionsDescriptor.

          I have run derbyall suite on my Windows XP machine with Sun's jdk14 and there were no new failures. Can someon please review this patch(Derby1330setUUIDinDataDictionaryV8diff.txt) and if no comments, can a commiter please commit this patch?

          ps There is another patch pending on this JIRA entry(Derby1330MinorCleanupV7diff.txt). Can a commiter please commit that too?

          Show
          Mamta A. Satoor added a comment - Currently, in case of revoke privilege, DataDictionary.addRemovePermissionsDescriptor expects it's callers to set the permission descriptor's uuid so that DependencyManager can be invoked by addRemovePermissionsDescriptor to send REVOKE_PRIVILEGE action to permission descriptor's dependents(sending of REVOKE_PRIVILEGE work has not been finished yet. This patch is in preparation of that work). In order to set the uuid, the callers have to goto system tables to find the uuid and then set the permission descriptor's uuid. It will be less error prone if DataDictionary.addRemovePermissionsDescriptor did the setting of uuid of permission descriptor, rather than having every caller set the uuid correctly. DataDictionary.addRemovePermissionsDescriptor method has to goto the permission system tables anyways and has the uuid information handy and hence it will be more efficient for it to set the uuid of the permission descriptor. This is inline with the patch that was commited some time back so that resetting of the uuid happened in addRemovePermissionsDescriptor method rather than expecting the callers to do that. In order to do this, I have added an abstract method to PermissionsCatalogRowFactory called setUUIDOfThePassedDescriptor. This method will set the uuid of the passed permission descriptor to same value as the row corresponding to the permission system table which is also passed as a parameter. This method will be called by DataDictionary.addRemovePermissionsDescriptor. I have run derbyall suite on my Windows XP machine with Sun's jdk14 and there were no new failures. Can someon please review this patch(Derby1330setUUIDinDataDictionaryV8diff.txt) and if no comments, can a commiter please commit this patch? ps There is another patch pending on this JIRA entry(Derby1330MinorCleanupV7diff.txt). Can a commiter please commit that too?
          Hide
          Mamta A. Satoor added a comment -

          Please disregard change to RAFContainer.java in Derby1330setUUIDinDataDictionaryV8diff.txt. Let me know if I should submit another diff file w/o the RAFContainer change.

          Show
          Mamta A. Satoor added a comment - Please disregard change to RAFContainer.java in Derby1330setUUIDinDataDictionaryV8diff.txt. Let me know if I should submit another diff file w/o the RAFContainer change.
          Hide
          Daniel John Debrunner added a comment -

          Patch Derby1330MinorCleanupV7diff.txt applied - Committed revision 423232. - thanks Mamta

          Show
          Daniel John Debrunner added a comment - Patch Derby1330MinorCleanupV7diff.txt applied - Committed revision 423232. - thanks Mamta
          Hide
          Mamta A. Satoor added a comment -

          There are couple javadoc warnings for the code that got submitted as part of this JIRA entry. Can a commiter please commit this patch (svn diff o/p is attached as DERBY1330javaDocWarningsDiffV9.txt and svn stat -q o/p is attached as DERBY1330javaDocWarningsStatV9.txt) to fix the problem? Thanks

          Show
          Mamta A. Satoor added a comment - There are couple javadoc warnings for the code that got submitted as part of this JIRA entry. Can a commiter please commit this patch (svn diff o/p is attached as DERBY1330javaDocWarningsDiffV9.txt and svn stat -q o/p is attached as DERBY1330javaDocWarningsStatV9.txt) to fix the problem? Thanks
          Hide
          Andrew McIntyre added a comment -

          Committed DERBY-1330javaDocWarningsDiffV9.txt patch to trunk with revision 423352. Note that in two cases, the @param privType actually needed to be privTypeStr to match the name of the parameter.

          Show
          Andrew McIntyre added a comment - Committed DERBY-1330 javaDocWarningsDiffV9.txt patch to trunk with revision 423352. Note that in two cases, the @param privType actually needed to be privTypeStr to match the name of the parameter.
          Hide
          Daniel John Debrunner added a comment -

          I get this compile error applying this patch; Derby1330setUUIDinDataDictionaryV8diff.txt

          [javac] Compiling 1 source file to C:_work\svn_linq\trunk\classes
          [javac] C:_work\svn_linq\trunk\java\engine\org\apache\derby\impl\sql\catalo
          g\SYSROUTINEPERMSRowFactory.java:319: cannot resolve symbol
          [javac] symbol : variable ROUTINPERMSID_COL_NUM
          [javac] location: class org.apache.derby.impl.sql.catalog.SYSROUTINEPERMSRow
          Factory
          [javac] DataValueDescriptor existingPermDVD = row.getColumn(ROUTINPE
          RMSID_COL_NUM);
          [javac] ^
          [javac] 1 error

          Show
          Daniel John Debrunner added a comment - I get this compile error applying this patch; Derby1330setUUIDinDataDictionaryV8diff.txt [javac] Compiling 1 source file to C:_work\svn_linq\trunk\classes [javac] C:_work\svn_linq\trunk\java\engine\org\apache\derby\impl\sql\catalo g\SYSROUTINEPERMSRowFactory.java:319: cannot resolve symbol [javac] symbol : variable ROUTINPERMSID_COL_NUM [javac] location: class org.apache.derby.impl.sql.catalog.SYSROUTINEPERMSRow Factory [javac] DataValueDescriptor existingPermDVD = row.getColumn(ROUTINPE RMSID_COL_NUM); [javac] ^ [javac] 1 error
          Hide
          Mamta A. Satoor added a comment -

          Dan, the patch Derby1330setUUIDinDataDictionaryV8diff.txt failed for you because it was trying to reference a variable by a name which got changed by another patch that went in for this jira entry.

          I have attached a new patch Derby1330setUUIDinDataDictionaryV10diff.txt to resolve the problem. The svn stat -q is attached as Derby1330setUUIDinDataDictionaryV10stat.txt. At this point, this new patch is the only pending patch on this Jira entry and I will highly appreciate somebody taking the time to review and committer commiting it.

          Show
          Mamta A. Satoor added a comment - Dan, the patch Derby1330setUUIDinDataDictionaryV8diff.txt failed for you because it was trying to reference a variable by a name which got changed by another patch that went in for this jira entry. I have attached a new patch Derby1330setUUIDinDataDictionaryV10diff.txt to resolve the problem. The svn stat -q is attached as Derby1330setUUIDinDataDictionaryV10stat.txt. At this point, this new patch is the only pending patch on this Jira entry and I will highly appreciate somebody taking the time to review and committer commiting it.
          Hide
          Daniel John Debrunner added a comment -

          Patch Derby1330setUUIDinDataDictionaryV10diff.txt Committed revision 423949. - Thanks Mamta

          Show
          Daniel John Debrunner added a comment - Patch Derby1330setUUIDinDataDictionaryV10diff.txt Committed revision 423949. - Thanks Mamta
          Hide
          Laura Stewart added a comment -

          In the spec AuthorizationModelForDerbySQLStandardAuthorizationV2.html, I am not sure that I understand the sentences in this paragraph :

          "Authorization checking is of little value unless Derby authentication is turned on. By default, Derby's authentication is OFF and can be turned ON by setting derby.connection.requireAuthentication to TRUE. Attempts to set security mode to Derby SQL Standard Authorization mode without first requiring authentication will raise a warning."

          Questions:
          1. The text here refers to "derby.connection.requireAuthentication", how is that different from "derby.database.defaultConnectionMode" and "derby.database.sqlAuthorization" ?

          2. The last sentence in the paragraph from the spec is confusing. It is unclear what "requiring authentication" means. Is that "user" authentication, "Derby" authentication? What is clear is that "authentication" must be set before the security mode is set. The sentence implies that "Derby SQL Standard Authorization" is a mode that can be set for security. How is the "security mode" set?

          I'd appreciate any clarification that you can provide.

          Show
          Laura Stewart added a comment - In the spec AuthorizationModelForDerbySQLStandardAuthorizationV2.html, I am not sure that I understand the sentences in this paragraph : "Authorization checking is of little value unless Derby authentication is turned on. By default, Derby's authentication is OFF and can be turned ON by setting derby.connection.requireAuthentication to TRUE. Attempts to set security mode to Derby SQL Standard Authorization mode without first requiring authentication will raise a warning." Questions: 1. The text here refers to "derby.connection.requireAuthentication", how is that different from "derby.database.defaultConnectionMode" and "derby.database.sqlAuthorization" ? 2. The last sentence in the paragraph from the spec is confusing. It is unclear what "requiring authentication" means. Is that "user" authentication, "Derby" authentication? What is clear is that "authentication" must be set before the security mode is set. The sentence implies that "Derby SQL Standard Authorization" is a mode that can be set for security. How is the "security mode" set? I'd appreciate any clarification that you can provide.
          Hide
          Mamta A. Satoor added a comment -

          Laura, here are my attempts at your questions.

          derby.connection.requireAuthentication is a pre 10.2 property which enables user authentication. This property will require that a user name and password is supplied at database connection time. Chances are that in a multi-user environment, Derby would be run with authentication enabled (more info can be found in our existing docs in the
          Security->Derby and Security -> Working with user authentication -> Enabling user authentication section)

          derby.connection.requireAuthentication will ensures that a user with valid user name and password will be allowed to connect to Derby. A multi-user
          environment might also want to control what actions are allowed by different users once they are connected to Derby. This part falls under User authorization. More on this can be found under
          Security->Derby and Security -> User authorization -> Setting user authorization

          Prior to Derby 10.2, you could use derby.database.defaultConnectionMode to specify the default access mode to be noAccess, readOnlyAccess and fullAccess. There was no support for Grant/Revoke pre 10.2.

          With Derby 10.2, we are introducing grant revoke which is another scheme of user authorization and it is enabled by the property derby.database.sqlAuthorization. When you set this property to true, Derby is said to be running in Derby SQL Standard Authorization mode. So, in short, property derby.database.sqlAuthorization allows the grant revoke feature.

          There has been some talk about not requiring property property derby.database.sqlAuthorization and enabling grant revoke when user attempts to use grant/revoke sql statements for the first time. Someone more knowledgable about that discussion might want to add their comments here.

          So, I hope I have not gotten you confused with all the jargons. Existing manuals might be helpful in clearing out confusion on authentication vs authorization.

          If you have any further questions, feel free to post them,

          Show
          Mamta A. Satoor added a comment - Laura, here are my attempts at your questions. derby.connection.requireAuthentication is a pre 10.2 property which enables user authentication. This property will require that a user name and password is supplied at database connection time. Chances are that in a multi-user environment, Derby would be run with authentication enabled (more info can be found in our existing docs in the Security->Derby and Security -> Working with user authentication -> Enabling user authentication section) derby.connection.requireAuthentication will ensures that a user with valid user name and password will be allowed to connect to Derby. A multi-user environment might also want to control what actions are allowed by different users once they are connected to Derby. This part falls under User authorization. More on this can be found under Security->Derby and Security -> User authorization -> Setting user authorization Prior to Derby 10.2, you could use derby.database.defaultConnectionMode to specify the default access mode to be noAccess, readOnlyAccess and fullAccess. There was no support for Grant/Revoke pre 10.2. With Derby 10.2, we are introducing grant revoke which is another scheme of user authorization and it is enabled by the property derby.database.sqlAuthorization. When you set this property to true, Derby is said to be running in Derby SQL Standard Authorization mode. So, in short, property derby.database.sqlAuthorization allows the grant revoke feature. There has been some talk about not requiring property property derby.database.sqlAuthorization and enabling grant revoke when user attempts to use grant/revoke sql statements for the first time. Someone more knowledgable about that discussion might want to add their comments here. So, I hope I have not gotten you confused with all the jargons. Existing manuals might be helpful in clearing out confusion on authentication vs authorization. If you have any further questions, feel free to post them,
          Hide
          Laura Stewart added a comment -

          I want to be certain that I understand cascading object dependencies. Please correct any errors that I have in the scenarios below:

          1. Table T1 is owned by user u1 and the SELECT privilege on T1 is granted
          to user u2. Next, if u2 creates a view v1 on T1, When the SELECT privilege
          on T1 is revoked from u2, view v1 is dropped. Queries that access v1 are invalid.

          2. Table T1 is owned by user u1 and SELECT privilege on T1 is granted to user u2. Next,
          if u2 creates a view v1 on T1, Next, u2 creates a view v2 on v1. When the
          SELECT privilege on T1 is revoked from u2, Derby attempts to drop v1, but
          because v2 is dependent upon v1, the revoke fails.

          Show
          Laura Stewart added a comment - I want to be certain that I understand cascading object dependencies. Please correct any errors that I have in the scenarios below: 1. Table T1 is owned by user u1 and the SELECT privilege on T1 is granted to user u2. Next, if u2 creates a view v1 on T1, When the SELECT privilege on T1 is revoked from u2, view v1 is dropped. Queries that access v1 are invalid. 2. Table T1 is owned by user u1 and SELECT privilege on T1 is granted to user u2. Next, if u2 creates a view v1 on T1, Next, u2 creates a view v2 on v1. When the SELECT privilege on T1 is revoked from u2, Derby attempts to drop v1, but because v2 is dependent upon v1, the revoke fails.
          Hide
          Mamta A. Satoor added a comment -

          Laura, your understanding of the current behavior for revoke statement and views is correct.

          This behavior applies only to views. A trigger can't be dependent on another trigger. And hence when a revoke trigger is issued, you will not run into cascading problem. Same is true for foreign key constraints and hence one will not run into cascading problems with constraints.

          Hope this answers your question.

          Show
          Mamta A. Satoor added a comment - Laura, your understanding of the current behavior for revoke statement and views is correct. This behavior applies only to views. A trigger can't be dependent on another trigger. And hence when a revoke trigger is issued, you will not run into cascading problem. Same is true for foreign key constraints and hence one will not run into cascading problems with constraints. Hope this answers your question.
          Hide
          Laura Stewart added a comment -

          Another question. This is what I have to document the behavior
          for views, triggers, and constraints when only the PUBLIC privilege is active.
          Please let me know if there are errors in my understanding

          If you create a view, trigger, or constraint when only the PUBLIC privilege
          is active, the object that you create is dependent on the PUBLIC privilege.
          If you are subsequently granted the same user privileges as you have with
          PUBLIC, the objects that you created remain dependent on the PUBLIC privilege.
          If the PUBLIC privilege is later revoked, the objects that you created when
          only the PUBLIC privilege was active are dropped. Ensure that you have user
          level privileges before you create database objects to avoid this privilege
          dependency.

          Show
          Laura Stewart added a comment - Another question. This is what I have to document the behavior for views, triggers, and constraints when only the PUBLIC privilege is active. Please let me know if there are errors in my understanding If you create a view, trigger, or constraint when only the PUBLIC privilege is active, the object that you create is dependent on the PUBLIC privilege. If you are subsequently granted the same user privileges as you have with PUBLIC, the objects that you created remain dependent on the PUBLIC privilege. If the PUBLIC privilege is later revoked, the objects that you created when only the PUBLIC privilege was active are dropped. Ensure that you have user level privileges before you create database objects to avoid this privilege dependency.
          Hide
          Mamta A. Satoor added a comment -

          Basically, during an object create time, Derby will first look for the required privilege at the user level. If found, then the object will depend on that user-level privilege, for it's entire life. If privilege is not found at user-level, then Derby will look for that privilege at PUBLIC level. If found, then the object will depend on that PUBLIC level privilege, for it's entire life. After the object creation, if the privilege on which it depends is revoked, the object will simply drop itself. Derby will not try to see if it can find any other privilege to replace the one being dropped so the object does not have to drop itself. Ideally, during the privilege revoke time, the dependent object should see if there is any other privilege that they can start relying on so that they don't have to drop themselves.

          eg
          user1
          create t1
          grant select on t1 to user2
          grant select on t1 to public
          user2
          create view v1 as select * from user1.t1 – this view will depend on user-level select privilege on user1.t1
          user1
          revoke select on t1 from users
          – the above revoke will drop user2.v1, Ideally, rather than dropping user2.v1, Derby can make user2.v1 depend on available PUBLIC level privilege
          – this doesn't happen at this point and hence we should document the existing behavior to clear any confusion

          another eg
          user1
          create t1
          grant select on t1 to PUBLIC
          user2
          create view v1 as select * from user1.t1 – this view will depend on PUBLIC-level select privilege on user1.t1 since no user level privilege is around
          user1
          grant select on t1 to public
          revoke select on t1 from PUBLIC
          – the above revoke will drop user2.v1, Ideally, rather than dropping user2.v1, Derby can make user2.v1 depend on available user-level privilege
          – this doesn't happen at this point and hence we should document the existing behavior to clear any confusion

          Show
          Mamta A. Satoor added a comment - Basically, during an object create time, Derby will first look for the required privilege at the user level. If found, then the object will depend on that user-level privilege, for it's entire life. If privilege is not found at user-level, then Derby will look for that privilege at PUBLIC level. If found, then the object will depend on that PUBLIC level privilege, for it's entire life. After the object creation, if the privilege on which it depends is revoked, the object will simply drop itself. Derby will not try to see if it can find any other privilege to replace the one being dropped so the object does not have to drop itself. Ideally, during the privilege revoke time, the dependent object should see if there is any other privilege that they can start relying on so that they don't have to drop themselves. eg user1 create t1 grant select on t1 to user2 grant select on t1 to public user2 create view v1 as select * from user1.t1 – this view will depend on user-level select privilege on user1.t1 user1 revoke select on t1 from users – the above revoke will drop user2.v1, Ideally, rather than dropping user2.v1, Derby can make user2.v1 depend on available PUBLIC level privilege – this doesn't happen at this point and hence we should document the existing behavior to clear any confusion another eg user1 create t1 grant select on t1 to PUBLIC user2 create view v1 as select * from user1.t1 – this view will depend on PUBLIC-level select privilege on user1.t1 since no user level privilege is around user1 grant select on t1 to public revoke select on t1 from PUBLIC – the above revoke will drop user2.v1, Ideally, rather than dropping user2.v1, Derby can make user2.v1 depend on available user-level privilege – this doesn't happen at this point and hence we should document the existing behavior to clear any confusion
          Hide
          Yip Ng added a comment -

          It seems that the getTablePermissions( UUID ) method in DataDictionaryImpl might be problematic.
          This method is called when the dependency manager needs to get a dependable via its provider id. (tableperms id)
          (i.e.: getPersistentProviderInfos() of the dependency manager)

          In getTablePermissions(UUID), the TablePermsDescriptor constructor will set its member vars grantee and TableUUID
          to null since the row in sys.systableperms can be located via tablePermsUUID (instead of tableUUID, grantor and grantee). But during getPermissions() processing, the hashCode() of the TablePermsDescriptor will be invoked by the permissionCache which eventually will lead to a call to grantee.hashCode(); thus, resulting in NullPointerException.

          public TablePermsDescriptor getTablePermissions( UUID tablePermsUUID) throws StandardException

          { TablePermsDescriptor key = new TablePermsDescriptor( this, tablePermsUUID); return (TablePermsDescriptor) getPermissions( key); }
          Show
          Yip Ng added a comment - It seems that the getTablePermissions( UUID ) method in DataDictionaryImpl might be problematic. This method is called when the dependency manager needs to get a dependable via its provider id. (tableperms id) (i.e.: getPersistentProviderInfos() of the dependency manager) In getTablePermissions(UUID), the TablePermsDescriptor constructor will set its member vars grantee and TableUUID to null since the row in sys.systableperms can be located via tablePermsUUID (instead of tableUUID, grantor and grantee). But during getPermissions() processing, the hashCode() of the TablePermsDescriptor will be invoked by the permissionCache which eventually will lead to a call to grantee.hashCode(); thus, resulting in NullPointerException. public TablePermsDescriptor getTablePermissions( UUID tablePermsUUID) throws StandardException { TablePermsDescriptor key = new TablePermsDescriptor( this, tablePermsUUID); return (TablePermsDescriptor) getPermissions( key); }
          Hide
          Mamta A. Satoor added a comment -

          Yip, I would like to debug the behavior you mentioned. Do you have a test case that I could try to reproduce the NPE?

          Show
          Mamta A. Satoor added a comment - Yip, I would like to debug the behavior you mentioned. Do you have a test case that I could try to reproduce the NPE?
          Hide
          Rick Hillegas added a comment -

          Assign to 10.2 and bump urgency.

          Show
          Rick Hillegas added a comment - Assign to 10.2 and bump urgency.
          Hide
          Yip Ng added a comment -

          Mamta, one can reproduce this by reading the entries from SYSDEPENDS that has a row that have a tableperms as its provider id. For example, via getPersistentProviderInfos() of the dependency manager. This will eventually call the getTablePermissions(UUID) from DDdependableFinder object, which will lead to the NPE I described above. All the getXXXPermissions(UUID) methods also have the same problem.

          Show
          Yip Ng added a comment - Mamta, one can reproduce this by reading the entries from SYSDEPENDS that has a row that have a tableperms as its provider id. For example, via getPersistentProviderInfos() of the dependency manager. This will eventually call the getTablePermissions(UUID) from DDdependableFinder object, which will lead to the NPE I described above. All the getXXXPermissions(UUID) methods also have the same problem.
          Hide
          Yip Ng added a comment -

          I noticed getPersistentProviderInfos() are overloaded, so to be precise, call getPersistentProviderInfos(Dependent) with a dependent that depends on the table privilege from dependency manager to see the NPE symptom.

          Show
          Yip Ng added a comment - I noticed getPersistentProviderInfos() are overloaded, so to be precise, call getPersistentProviderInfos(Dependent) with a dependent that depends on the table privilege from dependency manager to see the NPE symptom.
          Hide
          Yip Ng added a comment -

          How does the PermissionsCache work with respect to passing two type of keys to represent the same PermissionDescriptor? If an object can be represented by two type of keys, how does it take advantage of the cache? Once one key's identity is set, its hash code should remain the same while it is stored in the cache, wouldn't the "other" key always miss then?

          For example, in the case of TablePermsDescriptor, there is one key that is composed of (grantee + tableUUID) and the other is the UUID of the TablePermsDescriptor. However, in its equals() and hashCode() implementation, it only takes in account for the (grantee + tableUUID) case.

          The getTablePermissions( UUID ) and other getXXXPermissions(UUID) seems to be mainly used by the dependency manager when it needs to load the stored dependencies which have PermissionDescriptor as a provider id but currently there is no code to force loading these stored dependencies. So in order to reproduce the NPE problem, one can call getPersistentProviderInfos(Dependent) of the dependency manager.

          Show
          Yip Ng added a comment - How does the PermissionsCache work with respect to passing two type of keys to represent the same PermissionDescriptor? If an object can be represented by two type of keys, how does it take advantage of the cache? Once one key's identity is set, its hash code should remain the same while it is stored in the cache, wouldn't the "other" key always miss then? For example, in the case of TablePermsDescriptor, there is one key that is composed of (grantee + tableUUID) and the other is the UUID of the TablePermsDescriptor. However, in its equals() and hashCode() implementation, it only takes in account for the (grantee + tableUUID) case. The getTablePermissions( UUID ) and other getXXXPermissions(UUID) seems to be mainly used by the dependency manager when it needs to load the stored dependencies which have PermissionDescriptor as a provider id but currently there is no code to force loading these stored dependencies. So in order to reproduce the NPE problem, one can call getPersistentProviderInfos(Dependent) of the dependency manager.
          Hide
          Mamta A. Satoor added a comment -

          Yip, the NPE behavrior you are describing is something I had run into before the dependency manager (DM) was changed by Dan with Revision: 425479 DERBY-1581. (You can find the disucssion of this very topic in DERBY-1539).

          Prior to Dan's changes, DM loaded the passed Provider while building the list of dependents for the passed Provider. But since the Provider object is already available to DM as parameter to the method, there really is not need to recreate Provider object. Before Dan's changes to not construct Provider object, the code path used to run into hashCode() and would get null pointer exception because at this point in DM, we only know about the UUID and not the tableUUID.

          After Dan's fix for the Provider objects, we do not go through the cache to construct the Provider object anymore and hence do not run into the hashCode() which expects that tableUUID be non-null.

          Hope this answers your question.

          Show
          Mamta A. Satoor added a comment - Yip, the NPE behavrior you are describing is something I had run into before the dependency manager (DM) was changed by Dan with Revision: 425479 DERBY-1581 . (You can find the disucssion of this very topic in DERBY-1539 ). Prior to Dan's changes, DM loaded the passed Provider while building the list of dependents for the passed Provider. But since the Provider object is already available to DM as parameter to the method, there really is not need to recreate Provider object. Before Dan's changes to not construct Provider object, the code path used to run into hashCode() and would get null pointer exception because at this point in DM, we only know about the UUID and not the tableUUID. After Dan's fix for the Provider objects, we do not go through the cache to construct the Provider object anymore and hence do not run into the hashCode() which expects that tableUUID be non-null. Hope this answers your question.
          Hide
          Yip Ng added a comment -

          Mamta, thanks for those jira links. My actual concern is the working of the permission cache. From what I interpret from those discussion, it seems that getXXXPermissions(UUID) are currently never called from the code line; thus, they are never exercised. Dan's patch addressed the issue of avoiding to reconstruct the "provider" from the stored dependencies; however, my example above is actually of the "dependent". There are cases where we need to exercise getXXXPermissions(UUID) to make use of the stored dependencies. For example, when granting a view that has underlying objects that is not own by the grantor, we need to go through all the view's providers to see if they are own by the grantor ( + grant option check, but since Derby does not support this yet, I'll skip the details). So, the getXXXPermission(UUID) needs to be called and currently the equals and hashCode methods of the various PermissionDescriptor types do need to address the nullibility of grantee and tableUUID or other related field that is used for the hashing. Even though the nullibility are addressed... back to my original question:

          How does the PermissionsCache work with respect to passing two type of keys to represent the same PermissionDescriptor? If an object can be represented by two type of keys, how does it take advantage of the cache? Once one key's identity is set, its hash code should remain the same while it is stored in the cache, wouldn't the "other" key always miss then?

          Show
          Yip Ng added a comment - Mamta, thanks for those jira links. My actual concern is the working of the permission cache. From what I interpret from those discussion, it seems that getXXXPermissions(UUID) are currently never called from the code line; thus, they are never exercised. Dan's patch addressed the issue of avoiding to reconstruct the "provider" from the stored dependencies; however, my example above is actually of the "dependent". There are cases where we need to exercise getXXXPermissions(UUID) to make use of the stored dependencies. For example, when granting a view that has underlying objects that is not own by the grantor, we need to go through all the view's providers to see if they are own by the grantor ( + grant option check, but since Derby does not support this yet, I'll skip the details). So, the getXXXPermission(UUID) needs to be called and currently the equals and hashCode methods of the various PermissionDescriptor types do need to address the nullibility of grantee and tableUUID or other related field that is used for the hashing. Even though the nullibility are addressed... back to my original question: How does the PermissionsCache work with respect to passing two type of keys to represent the same PermissionDescriptor? If an object can be represented by two type of keys, how does it take advantage of the cache? Once one key's identity is set, its hash code should remain the same while it is stored in the cache, wouldn't the "other" key always miss then?
          Hide
          Mamta A. Satoor added a comment -

          Yip, I thought you were running into NPE with the functionality implemented so far.

          As for when the PermissionDescriptor gets used as dependent, unfortunately, I think there can be NPE issue because of 2 types of keys associated with PermissionDescriptors and we will need to resolve this issue for what you are trying to accomplish. I do remember that some of the other Descriptors do not go through the cache when going through the DM(I think) and we could possibly do the same for PermissionDescriptors but I think more research is required in this area.

          Show
          Mamta A. Satoor added a comment - Yip, I thought you were running into NPE with the functionality implemented so far. As for when the PermissionDescriptor gets used as dependent, unfortunately, I think there can be NPE issue because of 2 types of keys associated with PermissionDescriptors and we will need to resolve this issue for what you are trying to accomplish. I do remember that some of the other Descriptors do not go through the cache when going through the DM(I think) and we could possibly do the same for PermissionDescriptors but I think more research is required in this area.
          Hide
          Yip Ng added a comment -

          Yes, it is unfortunate indeed. The permissions cache won't work properly when we are dealing with 2 types of keys associated with PermissionDescriptors. It looks like when calling getXXXPermissions(UUID), there is no choice but to load it from sys.sysdepends and bypass the cache mechanism since:

          1. The UUID of the PermissionDescriptor as a key will always miss the cache because the identity of the object is determined by the grantee and other related fields currently.

          2. The UUID of the PermissionDescriptor is not part of the equations of equals and hashCode method. It cannot because Derby do not know the "other" key value upfront during compilation of a statement until we retrieve it from the stored dependencies.

          Show
          Yip Ng added a comment - Yes, it is unfortunate indeed. The permissions cache won't work properly when we are dealing with 2 types of keys associated with PermissionDescriptors. It looks like when calling getXXXPermissions(UUID), there is no choice but to load it from sys.sysdepends and bypass the cache mechanism since: 1. The UUID of the PermissionDescriptor as a key will always miss the cache because the identity of the object is determined by the grantee and other related fields currently. 2. The UUID of the PermissionDescriptor is not part of the equations of equals and hashCode method. It cannot because Derby do not know the "other" key value upfront during compilation of a statement until we retrieve it from the stored dependencies.
          Hide
          Mamta A. Satoor added a comment -

          Closing this issue since I am finished with the work targeted for 10.2 release. The remaining work for this issue is getting tracked in following 2 jira entries
          1)DERBY-1782
          2)DERBY-1632
          3)DERBY-1631

          I will link the these issuess so we can keep track of the correlation between them.

          Show
          Mamta A. Satoor added a comment - Closing this issue since I am finished with the work targeted for 10.2 release. The remaining work for this issue is getting tracked in following 2 jira entries 1) DERBY-1782 2) DERBY-1632 3) DERBY-1631 I will link the these issuess so we can keep track of the correlation between them.

            People

            • Assignee:
              Mamta A. Satoor
              Reporter:
              Mamta A. Satoor
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development