Re: [HACKERS] Have REFRESH MATERIALIZED VIEW run as the MV owner

2013-07-06 Thread Hitoshi Harada
On Fri, Jul 5, 2013 at 9:45 AM, Noah Misch n...@leadboat.com wrote:
 REFRESH MATERIALIZED VIEW should temporarily switch the current user ID to the
 MV owner.  REINDEX and VACUUM do so to let privileged users safely maintain
 objects owned by others, and REFRESH MATERIALIZED VIEW belongs in that class
 of commands.

I was trying to understand why this is safe for a while.  REINDEX and
VACUUM make sense to me because they never contain side-effect as far
as I know, but MV can contain some volatile functions which could have
some unintended operation that shouldn't be invoked by no one but the
owner.  For example, if the function creates a permanent table per
call and doesn't clean it up, but later some other maintenance
operation is supposed to clean it up, and the owner schedules REFRESH
and maintenance once a day.  A non-owner user now can refresh it so
many times until the disk gets full.  Or is that operation supposed to
be restricted by the security context you are adding?

--
Hitoshi Harada


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Have REFRESH MATERIALIZED VIEW run as the MV owner

2013-07-06 Thread Noah Misch
On Fri, Jul 05, 2013 at 11:18:50PM -0700, Hitoshi Harada wrote:
 On Fri, Jul 5, 2013 at 9:45 AM, Noah Misch n...@leadboat.com wrote:
  REFRESH MATERIALIZED VIEW should temporarily switch the current user ID to 
  the
  MV owner.  REINDEX and VACUUM do so to let privileged users safely maintain
  objects owned by others, and REFRESH MATERIALIZED VIEW belongs in that class
  of commands.
 
 I was trying to understand why this is safe for a while.  REINDEX and
 VACUUM make sense to me because they never contain side-effect as far
 as I know, but MV can contain some volatile functions which could have
 some unintended operation that shouldn't be invoked by no one but the
 owner.  For example, if the function creates a permanent table per
 call and doesn't clean it up, but later some other maintenance
 operation is supposed to clean it up, and the owner schedules REFRESH
 and maintenance once a day.  A non-owner user now can refresh it so
 many times until the disk gets full.

I'm not proposing to expand the set of people *permitted* to refresh the MV.
That's still limited to the owning role (including other roles acquiring that
role by membership) and superusers.  My goal is to make it safe for a
superuser to refresh any MV, much like we've made it safe for a superuser to
REINDEX any index.

-- 
Noah Misch
EnterpriseDB http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] Have REFRESH MATERIALIZED VIEW run as the MV owner

2013-07-05 Thread Noah Misch
REFRESH MATERIALIZED VIEW should temporarily switch the current user ID to the
MV owner.  REINDEX and VACUUM do so to let privileged users safely maintain
objects owned by others, and REFRESH MATERIALIZED VIEW belongs in that class
of commands.  The MV query then runs as a security-restricted operation,
which forbids a few commands.  Most, e.g. UNLISTEN, are unlikely to arise in
practice.  The most interesting restriction is probably CREATE TEMP TABLE.
Consider a function that creates and later drops a temporary table that it
uses for intermediate storage during a complicated calculation.  That function
will no longer work in a MV query.  As a workaround, modify the function to
use a permanent table as its work area.

See attached patch.  The similar behavior of REINDEX et al. is undocumented.
Users are a bit more likely to notice limitations in the context of MVs, so I
added a brief documentation mention.  Seeing that this narrows the range of
valid MV queries, I bring it up now so MVs can debut with the restrictions
already in place.

Thanks,
nm

-- 
Noah Misch
EnterpriseDB http://www.enterprisedb.com
diff --git a/doc/src/sgml/ref/create_materialized_view.sgml 
b/doc/src/sgml/ref/create_materialized_view.sgml
index 0ed764b..b742e17 100644
--- a/doc/src/sgml/ref/create_materialized_view.sgml
+++ b/doc/src/sgml/ref/create_materialized_view.sgml
@@ -105,7 +105,9 @@ CREATE MATERIALIZED VIEW 
replaceabletable_name/replaceable
 listitem
  para
   A xref linkend=sql-select, link linkend=sql-tableTABLE/link,
-  or xref linkend=sql-values command.
+  or xref linkend=sql-values command.  This query will run within a
+  security-restricted operation; in particular, calls to functions that
+  themselves create temporary tables will fail.
  /para
 /listitem
/varlistentry
diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c
index 2bfe5fb..a3509d8 100644
--- a/src/backend/commands/createas.c
+++ b/src/backend/commands/createas.c
@@ -33,6 +33,7 @@
 #include commands/prepare.h
 #include commands/tablecmds.h
 #include commands/view.h
+#include miscadmin.h
 #include parser/parse_clause.h
 #include rewrite/rewriteHandler.h
 #include storage/smgr.h
@@ -69,7 +70,11 @@ ExecCreateTableAs(CreateTableAsStmt *stmt, const char 
*queryString,
 {
Query  *query = (Query *) stmt-query;
IntoClause *into = stmt-into;
+   boolis_matview = (into-viewQuery != NULL);
DestReceiver *dest;
+   Oid save_userid = InvalidOid;
+   int save_sec_context = 0;
+   int save_nestlevel = 0;
List   *rewritten;
PlannedStmt *plan;
QueryDesc  *queryDesc;
@@ -90,6 +95,7 @@ ExecCreateTableAs(CreateTableAsStmt *stmt, const char 
*queryString,
{
ExecuteStmt *estmt = (ExecuteStmt *) query-utilityStmt;
 
+   Assert(!is_matview);/* excluded by syntax */
ExecuteQuery(estmt, into, queryString, params, dest, 
completionTag);
 
return;
@@ -97,6 +103,21 @@ ExecCreateTableAs(CreateTableAsStmt *stmt, const char 
*queryString,
Assert(query-commandType == CMD_SELECT);
 
/*
+* For materialized views, lock down security-restricted operations and
+* arrange to make GUC variable changes local to this command.  This is
+* not necessary for security, but this keeps the behavior similar to
+* REFRESH MATERIALIZED VIEW.  Otherwise, one could create a 
materialized
+* view not possible to refresh.
+*/
+   if (is_matview)
+   {
+   GetUserIdAndSecContext(save_userid, save_sec_context);
+   SetUserIdAndSecContext(save_userid,
+  save_sec_context | 
SECURITY_RESTRICTED_OPERATION);
+   save_nestlevel = NewGUCNestLevel();
+   }
+
+   /*
 * Parse analysis was done already, but we still have to run the rule
 * rewriter.  We do not do AcquireRewriteLocks: we assume the query 
either
 * came straight from the parser, or suitable locks were acquired by
@@ -160,6 +181,15 @@ ExecCreateTableAs(CreateTableAsStmt *stmt, const char 
*queryString,
FreeQueryDesc(queryDesc);
 
PopActiveSnapshot();
+
+   if (is_matview)
+   {
+   /* Roll back any GUC changes */
+   AtEOXact_GUC(false, save_nestlevel);
+
+   /* Restore userid and security context */
+   SetUserIdAndSecContext(save_userid, save_sec_context);
+   }
 }
 
 /*
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index 2ffdca3..1c383ba 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -122,6 +122,9 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char 
*queryString,
RewriteRule *rule;
List