Alexander Lakhin <exclus...@gmail.com> writes: > Can you please explain, is this a bug or intended behaviour?
I'd say it's a bug. The permissions restriction should apply even with the intermediate view. After some rooting around, it seems like this can be blamed on wrong order-of-operations in ApplyRetrieveRule(). It recursively expands sub-views, then moves the permissions bits on the RELATION RTE for the view being expanded to the view's "OLD" RTE entry, then (if the view is selected FOR UPDATE) applies markQueryForLocking which recursively marks relations referenced by the view as FOR UPDATE. markQueryForLocking knows that it should avoid touching the "OLD" and "NEW" entries in views, because they are treated specially. Unfortunately, that means we never mark sub-views as requiring a FOR UPDATE permission check; since we already expanded them, their RELATION RTEs aren't in the active jointree anymore, and we skip the OLD entry which is now the active entry for the sub-view's permissions check. We can fix this just by switching the order of operations so that markQueryForLocking is applied before recursive expansion. That way, by the time we go to expand a sub-view, its RELATION RTE has already been marked with any needed UPDATE permission bit, and that's correctly moved into the view's OLD entry, and you get the expected failure: regression=> SELECT datid, datname FROM pgsd FOR UPDATE; ERROR: permission denied for view pg_stat_database (Note that complaining about pg_stat_database is the correct thing; the pgsd owner's lack of UPDATE on that view is the missing permission.) It looks to me like we could dispense with the forUpdatePushedDown argument to ApplyRetrieveRule altogether, because with this approach a sub-view should always have a parse rowmark already by the time we try to expand it. I haven't tested that simplification though. I haven't included a regression test case in this patch, but Alexander's example can easily be converted into one. Although this is arguably a security bug, I'm not sure we should back-patch it. The consequences seem relatively minor, and the behavioral change carries a significant risk of breaking applications that worked as-intended up to now. Thoughts? regards, tom lane
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c index 88140bc..4ce50f3 100644 *** a/src/backend/rewrite/rewriteHandler.c --- b/src/backend/rewrite/rewriteHandler.c *************** ApplyRetrieveRule(Query *parsetree, *** 1560,1566 **** --- 1560,1584 ---- AcquireRewriteLocks(rule_action, true, forUpdatePushedDown); /* + * If FOR [KEY] UPDATE/SHARE of view, mark all the contained tables as + * implicit FOR [KEY] UPDATE/SHARE, the same as the parser would have done + * if the view's subquery had been written out explicitly. + * + * Note: we needn't consider forUpdatePushedDown for this; if there was an + * ancestor query level with a relevant FOR [KEY] UPDATE/SHARE clause, + * that's already been pushed down to here and is reflected in "rc". + */ + if (rc != NULL) + markQueryForLocking(rule_action, (Node *) rule_action->jointree, + rc->strength, rc->waitPolicy, true); + + /* * Recursively expand any view references inside the view. + * + * Note: this must happen after markQueryForLocking. That way, any UPDATE + * permission bits needed for sub-views are initially applied to their + * RTE_RELATION RTEs by markQueryForLocking, and then transferred to their + * OLD rangetable entries by this routine's action below. */ rule_action = fireRIRrules(rule_action, activeRIRs, forUpdatePushedDown); *************** ApplyRetrieveRule(Query *parsetree, *** 1594,1611 **** rte->insertedCols = NULL; rte->updatedCols = NULL; - /* - * If FOR [KEY] UPDATE/SHARE of view, mark all the contained tables as - * implicit FOR [KEY] UPDATE/SHARE, the same as the parser would have done - * if the view's subquery had been written out explicitly. - * - * Note: we don't consider forUpdatePushedDown here; such marks will be - * made by recursing from the upper level in markQueryForLocking. - */ - if (rc != NULL) - markQueryForLocking(rule_action, (Node *) rule_action->jointree, - rc->strength, rc->waitPolicy, true); - return parsetree; } --- 1612,1617 ----