Document that CREATE OPERATOR CLASS amounts to granting public execute

permissions on the functions and operators contained in the opclass.
Since we already require superuser privilege to create an operator class,
there's no expansion-of-privilege hazard here, but if someone were to get
the idea of building an opclass containing functions that need security
restrictions, we'd better warn them off.  Also, change the permission
checks from have-execute-privilege to have-ownership, and then comment
them all out since they're dead code anyway under the superuser restriction.

diff --git a/doc/src/sgml/ref/create_opclass.sgml b/doc/src/sgml/ref/create_opclass.sgml
index 575672371ee3a90860d0c39765395ebd18edcb46..472bac003de52c0ddaa2c2a37b009cb5689ad1fd 100644 (file)
--- a/doc/src/sgml/ref/create_opclass.sgml
+++ b/doc/src/sgml/ref/create_opclass.sgml
@@ -1,5 +1,5 @@
 <!--
-$PostgreSQL: pgsql/doc/src/sgml/ref/create_opclass.sgml,v 1.13 2005/01/14 01:16:52 tgl Exp $
+$PostgreSQL: pgsql/doc/src/sgml/ref/create_opclass.sgml,v 1.14 2006/01/13 18:10:25 tgl Exp $
 PostgreSQL documentation
 -->
 
@@ -59,8 +59,9 @@ CREATE OPERATOR CLASS <replaceable class="parameter">name</replaceable> [ DEFAUL
 
   <para>
    <command>CREATE OPERATOR CLASS</command> does not presently check
-   whether the operator class definition includes all the operators and functions
-   required by the index method.  It is the user's
+   whether the operator class definition includes all the operators and
+   functions required by the index method, nor whether the operators and
+   functions form a self-consistent set.  It is the user's
    responsibility to define a valid operator class.
   </para>
 
@@ -208,6 +209,14 @@ CREATE OPERATOR CLASS <replaceable class="parameter">name</replaceable> [ DEFAUL
  <refsect1>
   <title>Notes</title>
 
+  <para>
+   Because the index machinery does not check access permissions on functions
+   before using them, including a function or operator in an operator class
+   is tantamount to granting public execute permission on it.  This is usually
+   not an issue for the sorts of functions that are useful in an operator
+   class.
+  </para>
+
   <para>
    The operators should not be defined by SQL functions.  A SQL function
    is likely to be inlined into the calling query, which will prevent

diff --git a/src/backend/commands/opclasscmds.c b/src/backend/commands/opclasscmds.c
index 378421033dcf41b3a77ec68518a73c0fcedfd3ec..1e3c2547025d1d3c5c1bef3364d628d23c93b0a1 100644 (file)
--- a/src/backend/commands/opclasscmds.c
+++ b/src/backend/commands/opclasscmds.c
@@ -9,7 +9,7 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/commands/opclasscmds.c,v 1.40 2005/11/22 18:17:09 momjian Exp $
+ *       $PostgreSQL: pgsql/src/backend/commands/opclasscmds.c,v 1.41 2006/01/13 18:10:25 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -119,11 +119,24 @@ DefineOpClass(CreateOpClassStmt *stmt)
        ReleaseSysCache(tup);
 
        /*
+        * The question of appropriate permissions for CREATE OPERATOR CLASS is
+        * interesting.  Creating an opclass is tantamount to granting public
+        * execute access on the functions involved, since the index machinery
+        * generally does not check access permission before using the functions.
+        * A minimum expectation therefore is that the caller have execute
+        * privilege with grant option.  Since we don't have a way to make the
+        * opclass go away if the grant option is revoked, we choose instead to
+        * require ownership of the functions.  It's also not entirely clear what
+        * permissions should be required on the datatype, but ownership seems
+        * like a safe choice.
+        *
         * Currently, we require superuser privileges to create an opclass. This
         * seems necessary because we have no way to validate that the offered set
         * of operators and functions are consistent with the AM's expectations.
         * It would be nice to provide such a check someday, if it can be done
         * without solving the halting problem :-(
+        *
+        * XXX re-enable NOT_USED code sections below if you remove this test.
         */
        if (!superuser())
                ereport(ERROR,
@@ -156,7 +169,6 @@ DefineOpClass(CreateOpClassStmt *stmt)
                Oid                     operOid;
                Oid                     funcOid;
                OpClassMember *member;
-               AclResult       aclresult;
 
                Assert(IsA(item, CreateOpClassItem));
                switch (item->itemtype)
@@ -184,13 +196,19 @@ DefineOpClass(CreateOpClassStmt *stmt)
                                        operOid = LookupOperName(item->name, typeoid, typeoid,
                                                                                         false);
                                }
-                               /* Caller must have execute permission on operators */
+
+#ifdef NOT_USED
+                               /* XXX this is unnecessary given the superuser check above */
+                               /* Caller must own operator and its underlying function */
+                               if (!pg_oper_ownercheck(operOid, GetUserId()))
+                                       aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_OPER,
+                                                                  get_opname(operOid));
                                funcOid = get_opcode(operOid);
-                               aclresult = pg_proc_aclcheck(funcOid, GetUserId(),
-                                                                                        ACL_EXECUTE);
-                               if (aclresult != ACLCHECK_OK)
-                                       aclcheck_error(aclresult, ACL_KIND_PROC,
+                               if (!pg_proc_ownercheck(funcOid, GetUserId()))
+                                       aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_PROC,
                                                                   get_func_name(funcOid));
+#endif
+
                                /* Save the info */
                                member = (OpClassMember *) palloc0(sizeof(OpClassMember));
                                member->object = operOid;
@@ -208,12 +226,14 @@ DefineOpClass(CreateOpClassStmt *stmt)
                                                                        item->number, numProcs)));
                                funcOid = LookupFuncNameTypeNames(item->name, item->args,
                                                                                                  false);
-                               /* Caller must have execute permission on functions */
-                               aclresult = pg_proc_aclcheck(funcOid, GetUserId(),
-                                                                                        ACL_EXECUTE);
-                               if (aclresult != ACLCHECK_OK)
-                                       aclcheck_error(aclresult, ACL_KIND_PROC,
+#ifdef NOT_USED
+                               /* XXX this is unnecessary given the superuser check above */
+                               /* Caller must own function */
+                               if (!pg_proc_ownercheck(funcOid, GetUserId()))
+                                       aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_PROC,
                                                                   get_func_name(funcOid));
+#endif
+
                                /* Save the info */
                                member = (OpClassMember *) palloc0(sizeof(OpClassMember));
                                member->object = funcOid;
@@ -227,6 +247,14 @@ DefineOpClass(CreateOpClassStmt *stmt)
                                                        (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
                                                   errmsg("storage type specified more than once")));
                                storageoid = typenameTypeId(item->storedtype);
+
+#ifdef NOT_USED
+                               /* XXX this is unnecessary given the superuser check above */
+                               /* Check we have ownership of the datatype */
+                               if (!pg_type_ownercheck(storageoid, GetUserId()))
+                                       aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_TYPE,
+                                                                  format_type_be(storageoid));
+#endif
                                break;
                        default:
                                elog(ERROR, "unrecognized item type: %d", item->itemtype);