Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.2.1.6, 10.2.2.0, 10.3.1.4, 10.3.2.1, 10.3.3.0, 10.4.1.3, 10.4.2.0, 10.5.1.1, 10.5.2.0, 10.5.3.0, 10.6.1.0, 10.6.2.1, 10.7.1.1, 10.8.1.2
    • Fix Version/s: 10.8.2.2, 10.9.1.0
    • Component/s: SQL
    • Labels:
      None
    • Environment:
      windows xp
    • Urgency:
      Normal
    • Issue & fix info:
      Repro attached
    • Bug behavior facts:
      Deviation from standard, Security, Seen in production

      Description

      Hi,

      in some cases there seem to be issues with SQLAuthorisation in conjunction with database VIEWS.
      Please see attached files as repro (script.txt has only the SQL I executed, output is the output resulting when running the script).
      I would think identical results should be returned in all cases and independent on how the view has been defined, but this to my surprise not being the case:

      1) Trying to select from view appl."VW_MyTasks" - which is a simple view defined on just one table - leads to expected results, i.e. "my" tasks are being returned.
      2) Trying to select from view appl."VW_MyPriorityTasks - which is a view defined on two joined tables without using an inner join clause - leads to expected results, i.e. "my" priority tasks are being returned.
      3) Trying to select from view appl."VW2_MyPriorityTasks - which is the same view but now the two tables joined using an inner join clause - leads to an error and no tasks returned (when the same results as in 2) above were expected).
      4) Trying to select from view appl."VW3_MyPriorityTasks" - which is a view defined using a subselect - also unexpectedly leads to an error.

      Note: While I could rewrite each inner join clause with changing the syntax like in 2) above, this provides a simple work-around for such cases. May be there is a work-around for subselects also (not sure if every subselect could be rewritten to a join?). However when depending on using EXISTS constructs in the query there unfortunately is no way (I would know of) to get around this problem. Unfortuanetly a view that makes use of EXISTS is also one I would need to define in my data base...

      Thanks

      1. derby5292d.stat
        0.7 kB
        Dag H. Wanvik
      2. derby5292d.diff
        13 kB
        Dag H. Wanvik
      3. derby5292c.stat
        0.8 kB
        Dag H. Wanvik
      4. derby5292c.diff
        13 kB
        Dag H. Wanvik
      5. derby5292b.stat
        0.6 kB
        Dag H. Wanvik
      6. derby5292b.diff
        6 kB
        Dag H. Wanvik
      7. derby5292a.diff
        0.6 kB
        Dag H. Wanvik
      8. script.txt
        4 kB
        Thomas Hill
      9. output.txt
        6 kB
        Thomas Hill

        Activity

        Hide
        Dag H. Wanvik added a comment -

        Thanks for reporting this, Thomas. I can reproduce on trunk. Looks like a bug to me.

        Show
        Dag H. Wanvik added a comment - Thanks for reporting this, Thomas. I can reproduce on trunk. Looks like a bug to me.
        Hide
        Dag H. Wanvik added a comment -

        The problem appears to be a missing definition in TableOperatorNode of disablePrivilegeCollection. For views the premissions collection is disabled from tables in the query from-list with this QueryTreeNode#disablePrivilegeCollection. In the repro, where explicit JOINs are used, the disabling is not propagated down to the left and right tables of the join. The same error would appear for other subclasses of TableOperatorNode, e.g. set operations.

        Show
        Dag H. Wanvik added a comment - The problem appears to be a missing definition in TableOperatorNode of disablePrivilegeCollection. For views the premissions collection is disabled from tables in the query from-list with this QueryTreeNode#disablePrivilegeCollection. In the repro, where explicit JOINs are used, the disabling is not propagated down to the left and right tables of the join. The same error would appear for other subclasses of TableOperatorNode, e.g. set operations.
        Hide
        Dag H. Wanvik added a comment -

        Uploading a patch which makes the repro script work.
        Not for commit, I still need to add new tests and check if there are other similar missing definitions.

        Show
        Dag H. Wanvik added a comment - Uploading a patch which makes the repro script work. Not for commit, I still need to add new tests and check if there are other similar missing definitions.
        Hide
        Dag H. Wanvik added a comment -

        Actually, the patch only solved the first failing case in the repro, so the patch is insufficient.

        Show
        Dag H. Wanvik added a comment - Actually, the patch only solved the first failing case in the repro, so the patch is insufficient.
        Hide
        Dag H. Wanvik added a comment -

        Uploading patch "b", which makes both failing cases work.
        The next missing pieces were sub-selects inside WHERE clause (and also HAVING clause, I found). I still need to add tests. Instead of populating the ValueNode hierarchy with new overrides, I used a CollectNodesVisitor so reach nested SELECT(s) to disable permission collection for them.

        Show
        Dag H. Wanvik added a comment - Uploading patch "b", which makes both failing cases work. The next missing pieces were sub-selects inside WHERE clause (and also HAVING clause, I found). I still need to add tests. Instead of populating the ValueNode hierarchy with new overrides, I used a CollectNodesVisitor so reach nested SELECT(s) to disable permission collection for them.
        Hide
        Knut Anders Hatlen added a comment -

        Would an alternative be to remove all the overrides and use the visitor for all nodes? Since the disablePrivilegeCollection() method is defined in QueryTreeNode, and ValueNode is a subclass of QueryTreeNode, it feels a bit inconsistent to require overrides only in the non-ValueNode subclasses. Removing the overrides and relying on the visitor might reduce this code to just a single invocation of the visitor (with QueryTreeNode.class as argument instead of SelectNode.class) in FromBaseTable.bindNonVTITables().

        Show
        Knut Anders Hatlen added a comment - Would an alternative be to remove all the overrides and use the visitor for all nodes? Since the disablePrivilegeCollection() method is defined in QueryTreeNode, and ValueNode is a subclass of QueryTreeNode, it feels a bit inconsistent to require overrides only in the non-ValueNode subclasses. Removing the overrides and relying on the visitor might reduce this code to just a single invocation of the visitor (with QueryTreeNode.class as argument instead of SelectNode.class) in FromBaseTable.bindNonVTITables().
        Hide
        Dag H. Wanvik added a comment -

        Yes, that could be a natural further improvement, I agree. I didn't go there yet in the interest of a minimal change, but I think it's worth trying. It would definitely make the code simpler.

        Show
        Dag H. Wanvik added a comment - Yes, that could be a natural further improvement, I agree. I didn't go there yet in the interest of a minimal change, but I think it's worth trying. It would definitely make the code simpler.
        Hide
        Dag H. Wanvik added a comment -

        Uploading a patch "c", which removes the overrides in favour of using a visitor always. This approach failed at first, because FromSubquery did not implement Visitable#acceptChildren, so I added that. I added a new test fixture to GrantRevokeTest: testViewDefinersRights based on the repro.

        Btw, it is not the first time we have had to add more implementations of acceptChildren to the node types. Maybe it's time to go through them to verify that all implement it correctly..

        Running regressions over again.

        Show
        Dag H. Wanvik added a comment - Uploading a patch "c", which removes the overrides in favour of using a visitor always. This approach failed at first, because FromSubquery did not implement Visitable#acceptChildren, so I added that. I added a new test fixture to GrantRevokeTest: testViewDefinersRights based on the repro. Btw, it is not the first time we have had to add more implementations of acceptChildren to the node types. Maybe it's time to go through them to verify that all implement it correctly.. Running regressions over again.
        Hide
        Knut Anders Hatlen added a comment -

        The patch looks fine to me. There are two added svn:ignore properties though, that you may want to revert before committing (#SelectNode.java# and #GrantRevokeTest.java#).

        Perhaps we should also make QueryTreeNode.disablePrivilegeCollection() final now, so that we get a compile-time check that we haven't forgotten it somewhere or accidentally overridden it. And possibly also make it package private since it's only used in impl.sql.compile.

        Show
        Knut Anders Hatlen added a comment - The patch looks fine to me. There are two added svn:ignore properties though, that you may want to revert before committing (#SelectNode.java# and #GrantRevokeTest.java#). Perhaps we should also make QueryTreeNode.disablePrivilegeCollection() final now, so that we get a compile-time check that we haven't forgotten it somewhere or accidentally overridden it. And possibly also make it package private since it's only used in impl.sql.compile.
        Hide
        Dag H. Wanvik added a comment -

        Yes, I saw those added properties, too.. Not sure how they got int there, maybe just the presence of those files is enough when I produced the diff? As for final, I had a plan to do that but forgot it. Thanks for spotting it! Regressions ran OK.

        Show
        Dag H. Wanvik added a comment - Yes, I saw those added properties, too.. Not sure how they got int there, maybe just the presence of those files is enough when I produced the diff? As for final, I had a plan to do that but forgot it. Thanks for spotting it! Regressions ran OK.
        Hide
        Dag H. Wanvik added a comment -

        Uploaded version "d" of this patch which removes the
        extraneous properties settings and adds a "final" to QTN#disablePrivilegeCollection and makes the method package private.

        Show
        Dag H. Wanvik added a comment - Uploaded version "d" of this patch which removes the extraneous properties settings and adds a "final" to QTN#disablePrivilegeCollection and makes the method package private.
        Hide
        Dag H. Wanvik added a comment -

        Committed version "d" as svn 1142635. This is a back-port candidate, so I am not closing it yet.

        Show
        Dag H. Wanvik added a comment - Committed version "d" as svn 1142635. This is a back-port candidate, so I am not closing it yet.
        Hide
        Dag H. Wanvik added a comment -

        Backported to 10.8 branch as svn 1142773. Thomas, feel free to close this issue if you believe the issue has been resolved.

        Show
        Dag H. Wanvik added a comment - Backported to 10.8 branch as svn 1142773. Thomas, feel free to close this issue if you believe the issue has been resolved.
        Hide
        Thomas Hill added a comment - - edited

        Amazed again about level of support and timely feedback I am receiving from you and Derby community - thanks for this.
        As I have never built Derby from source (and am not technical enough to start such an undertaking) I will need to wait for binaries before I can test. I think you advised me last time that there are periodic binary builds which are being produced? (but unfortunately I can't remember details). In regards to seeing the fix included in a production release, is 'fix version' 10.8.1.6 and 10.9.0 on this Jira indicating that this will be part of the next maintenance release 10.8.2 tentatively scheduled for middle Sep 2011?

        Show
        Thomas Hill added a comment - - edited Amazed again about level of support and timely feedback I am receiving from you and Derby community - thanks for this. As I have never built Derby from source (and am not technical enough to start such an undertaking) I will need to wait for binaries before I can test. I think you advised me last time that there are periodic binary builds which are being produced? (but unfortunately I can't remember details). In regards to seeing the fix included in a production release, is 'fix version' 10.8.1.6 and 10.9.0 on this Jira indicating that this will be part of the next maintenance release 10.8.2 tentatively scheduled for middle Sep 2011?
        Hide
        Dag H. Wanvik added a comment -

        Thanks, Thomas! Yes, 'fix version' 10.8.1.6 does mean it will be part of the next maintenance release You could pick up
        tinderbox jars for head of the 10.8 branch here (debug jars) and see if they solve your issue (they should!):

        https://builds.apache.org/job/Derby-branch-10.8/ws/10.8-head/jars/sane/

        (Please note that these jars are not intended for production.)

        Show
        Dag H. Wanvik added a comment - Thanks, Thomas! Yes, 'fix version' 10.8.1.6 does mean it will be part of the next maintenance release You could pick up tinderbox jars for head of the 10.8 branch here (debug jars) and see if they solve your issue (they should!): https://builds.apache.org/job/Derby-branch-10.8/ws/10.8-head/jars/sane/ (Please note that these jars are not intended for production.)
        Hide
        Knut Anders Hatlen added a comment -

        [bulk update] Close all resolved issues that haven't been updated for more than one year.

        Show
        Knut Anders Hatlen added a comment - [bulk update] Close all resolved issues that haven't been updated for more than one year.

          People

          • Assignee:
            Dag H. Wanvik
            Reporter:
            Thomas Hill
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development