Details

Description

In Derby SQL authorization mode, invoking Java stored procedure that contains GRANT or REVOKE statement with CONTAINS SQL from a trigger should fail but in the following test, it successfully executed the trigger action.
Attaching repro patch for trunk.

Hmm... Correct. It should fail. Guess just invoking the procedure itself (outside a trigger) should fail. I was thinking about the link to trigger and GRANT statement. The problem is that procedure should always fail. Another missed negative test in the code.

Satheesh Bandaram
added a comment - 19/Aug/06 04:10 Hmm... Correct. It should fail. Guess just invoking the procedure itself (outside a trigger) should fail. I was thinking about the link to trigger and GRANT statement. The problem is that procedure should always fail. Another missed negative test in the code.

Yes, the GrantNode and RevokeNode should have derived from DDLStatementNode instead of MiscellaneousStatementNode. Subclassing DDLStatementNode will generate a call to GenericResultSetFactory's getDDLResultSet() in the activation class and invokes the GenericAuthorizer's
authorize() method with the proper parameters to enforce the correct semantics.

Attaching patch derby1729-trunk-diff01.txt for DERBY-1729. This patch applies what I stated on my previous comment + additional sql file for derbylang. The reason I didn't include this in grantRevokeDDL.sql is because of name collision and this testcase is one of the many additonal grant/revoke tests that I wrote and I'll like to append the rest of those testcases to this file(grantRevokeDDL2.sql) when I submit my patch for DERBY-1736. Running derbyall now, please review.

Yip Ng
added a comment - 04/Sep/06 20:12 Attaching patch derby1729-trunk-diff01.txt for DERBY-1729 . This patch applies what I stated on my previous comment + additional sql file for derbylang. The reason I didn't include this in grantRevokeDDL.sql is because of name collision and this testcase is one of the many additonal grant/revoke tests that I wrote and I'll like to append the rest of those testcases to this file(grantRevokeDDL2.sql) when I submit my patch for DERBY-1736 . Running derbyall now, please review.

Mamta A. Satoor
added a comment - 06/Sep/06 07:17 Yip, I am still reviewing the patch. The engine code changes look good. But grantRevokeDDL2.sql has following
+connect 'grantRevokeDDL2;create=true' user 'user1' as user1;
+connect 'grantRevokeDDL2;create=true' user 'user1' as user3;
+connect 'grantRevokeDDL2;create=true' user 'user1' as user3;
+
Did you intend to have same user name "as user3" for last 2 connections?

Further looking at the test, it seems like you were planning on creating another connection with user name as user2 and have the procedures grant and revoke privileges from that user.

Some feedback on the test code changes
1)Have a connection with user2 and test in the code that user2 can look at user1.t1 only after successful execution of grant_select_proc4() through the trigger code. Next, test that user2 can't look at user1.t1 anymore after successful execution of revoke_select_proc4() through the trigger code.

2)Some comments in the test code will be useful, explaining why 6 out of 8 procedure invocatiions from trigger will fail. It makes sense in the mind frame of this jira entry but it will be beneficial for future new users of this test to know what exactly the test is testing w/o having to go through DERBY-1729.

3)Comments in the test code explaining why we are checking SYSTABLEPERMS will be useful ie a row in SYSTABLEPERMS after delete means that grant statement inside the procedure worked correctly and user2 now has the SELECT privileges on user1.t1 Also, comment saying that the row in SYSTABLEPERMS should disappear after successful run of procedure with revoke statement in it.

And finally, I think we should consider changing the description of this jira entry to remove excess information about triggers. Like Satheesh mentioned, procedure failure is not related to invocation from trigger. This would happen with stand alone procedures too.

Mamta A. Satoor
added a comment - 06/Sep/06 07:48 Yip, I just finished reviewing the patch fully.
Further looking at the test, it seems like you were planning on creating another connection with user name as user2 and have the procedures grant and revoke privileges from that user.
Some feedback on the test code changes
1)Have a connection with user2 and test in the code that user2 can look at user1.t1 only after successful execution of grant_select_proc4() through the trigger code. Next, test that user2 can't look at user1.t1 anymore after successful execution of revoke_select_proc4() through the trigger code.
2)Some comments in the test code will be useful, explaining why 6 out of 8 procedure invocatiions from trigger will fail. It makes sense in the mind frame of this jira entry but it will be beneficial for future new users of this test to know what exactly the test is testing w/o having to go through DERBY-1729 .
3)Comments in the test code explaining why we are checking SYSTABLEPERMS will be useful ie a row in SYSTABLEPERMS after delete means that grant statement inside the procedure worked correctly and user2 now has the SELECT privileges on user1.t1 Also, comment saying that the row in SYSTABLEPERMS should disappear after successful run of procedure with revoke statement in it.
And finally, I think we should consider changing the description of this jira entry to remove excess information about triggers. Like Satheesh mentioned, procedure failure is not related to invocation from trigger. This would happen with stand alone procedures too.

"3)Comments in the test code explaining why we are checking SYSTABLEPERMS will be useful ie a row in SYSTABLEPERMS after delete means that grant statement inside the procedure worked correctly and user2 now has the SELECT privileges on user1.t1 Also, comment saying that the row in SYSTABLEPERMS should disappear after successful run of procedure with revoke statement in it. "

Why use SYSTABLEPERMS as the test mechanism, this is an indirect test of the expected behaviour? Why not perform the actual test of seeing if the user can or cannot access the table etc, along with checking the expected successful or exception thrown by the procedure?

Daniel John Debrunner
added a comment - 06/Sep/06 14:14 Mamta wrote:
"3)Comments in the test code explaining why we are checking SYSTABLEPERMS will be useful ie a row in SYSTABLEPERMS after delete means that grant statement inside the procedure worked correctly and user2 now has the SELECT privileges on user1.t1 Also, comment saying that the row in SYSTABLEPERMS should disappear after successful run of procedure with revoke statement in it. "
Why use SYSTABLEPERMS as the test mechanism, this is an indirect test of the expected behaviour? Why not perform the actual test of seeing if the user can or cannot access the table etc, along with checking the expected successful or exception thrown by the procedure?

Yip, I see that test changes still have the SYSTABLEPERMS checks to see if the test is behaving as expected. I think we should remove it especially now that we check the test validity by connecting as user2 and checking select privilege on user1.t1

Also, I wondered if you could address "Some comments in the test code will be useful, explaining why 6 out of 8 procedure invocatiions from trigger will fail. It makes sense in the mind frame of this jira entry but it will be beneficial for future new users of this test to know what exactly the test is testing w/o having to go through DERBY-1729. "? I do see an overall comment at the beginning of the test saying grant or revoke require MODIFIES SQL DATA, which is great, but a little more comments within the test explaining the reasons behind why something should fail or pass would be very nice.

Mamta A. Satoor
added a comment - 06/Sep/06 19:58 Yip, I see that test changes still have the SYSTABLEPERMS checks to see if the test is behaving as expected. I think we should remove it especially now that we check the test validity by connecting as user2 and checking select privilege on user1.t1
Also, I wondered if you could address "Some comments in the test code will be useful, explaining why 6 out of 8 procedure invocatiions from trigger will fail. It makes sense in the mind frame of this jira entry but it will be beneficial for future new users of this test to know what exactly the test is testing w/o having to go through DERBY-1729 . "? I do see an overall comment at the beginning of the test saying grant or revoke require MODIFIES SQL DATA, which is great, but a little more comments within the test explaining the reasons behind why something should fail or pass would be very nice.

I am going to run tests and look at committing this patch. If anyone else plans on reviewing the latest patch let me know - for this work I am counting on the review of others, but will verify that patch passes full set of tests before committing.

Mike Matrigali
added a comment - 07/Sep/06 01:03 I am going to run tests and look at committing this patch. If anyone else plans on reviewing the latest patch let me know - for this work I am counting on the review of others, but will verify that patch passes full set of tests before committing.

This is an issue with exception handling in jdk16. Reported as DERBY-1629. To temporarily resolve the regression test failure, we can add a new master file for jdk16. This was what was done for procedureInTrigger test.

Deepa Remesh
added a comment - 11/Sep/06 20:18 This is an issue with exception handling in jdk16. Reported as DERBY-1629 . To temporarily resolve the regression test failure, we can add a new master file for jdk16. This was what was done for procedureInTrigger test.

linking to DERBY-1629, adding jdk16 specific master to work around DERBY-1629. These jdk specific masters add future maintenance workload to getting clean test runs in all environments in the future, so would be good to get rid of the specific master when DERBY-1629 is resolved.

Mike Matrigali
added a comment - 15/Sep/06 17:33 linking to DERBY-1629 , adding jdk16 specific master to work around DERBY-1629 . These jdk specific masters add future maintenance workload to getting clean test runs in all environments in the future, so would be good to get rid of the specific master when DERBY-1629 is resolved.