Code review for GRANT CONNECT patch. Spell the privilege as CONNECT not

CONNECTION, fix a number of places that were missed (eg pg_dump support),
avoid executing an extra search of pg_database during startup.

6 files changed, 85 insertions, 59 deletions

diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 3b9701db077..a74c28e5b3d 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/catalog/aclchk.c,v 1.126 2006/04/30 02:09:07 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/catalog/aclchk.c,v 1.127 2006/04/30 21:15:33 tgl Exp $
  *
  * NOTES
  *	  See acl.h.
@@ -1368,7 +1368,7 @@ string_to_privilege(const char *privname)
 		return ACL_CREATE_TEMP;
 	if (strcmp(privname, "temp") == 0)
 		return ACL_CREATE_TEMP;
-	if (strcmp(privname, "connection") == 0)
+	if (strcmp(privname, "connect") == 0)
 		return ACL_CONNECT;
 	ereport(ERROR,
 			(errcode(ERRCODE_SYNTAX_ERROR),
@@ -1404,7 +1404,7 @@ privilege_to_string(AclMode privilege)
 		case ACL_CREATE_TEMP:
 			return "TEMP";
 		case ACL_CONNECT:
-			return "CONNECTION";
+			return "CONNECT";
 		default:
 			elog(ERROR, "unrecognized privilege: %d", (int) privilege);
 	}
@@ -1661,10 +1661,6 @@ pg_database_aclmask(Oid db_oid, Oid roleid,
 	ScanKeyData entry[1];
 	SysScanDesc scan;
 	HeapTuple	tuple;
-	Datum		aclDatum;
-	bool		isNull;
-	Acl		   *acl;
-	Oid			ownerId;
 
 	/* Superusers bypass all permission checking. */
 	if (superuser_arg(roleid))
@@ -1688,10 +1684,33 @@ pg_database_aclmask(Oid db_oid, Oid roleid,
 				(errcode(ERRCODE_UNDEFINED_DATABASE),
 				 errmsg("database with OID %u does not exist", db_oid)));
 
-	ownerId = ((Form_pg_database) GETSTRUCT(tuple))->datdba;
+	result = pg_database_tuple_aclmask(tuple, RelationGetDescr(pg_database),
+									   roleid, mask, how);
+
+	systable_endscan(scan);
+	heap_close(pg_database, AccessShareLock);
+
+	return result;
+}
+
+/*
+ * This is split out so that ReverifyMyDatabase can perform an ACL check
+ * without a whole extra search of pg_database
+ */
+AclMode
+pg_database_tuple_aclmask(HeapTuple db_tuple, TupleDesc tupdesc,
+						  Oid roleid, AclMode mask, AclMaskHow how)
+{
+	AclMode		result;
+	Datum		aclDatum;
+	bool		isNull;
+	Acl		   *acl;
+	Oid			ownerId;
+
+	ownerId = ((Form_pg_database) GETSTRUCT(db_tuple))->datdba;
 
-	aclDatum = heap_getattr(tuple, Anum_pg_database_datacl,
-							RelationGetDescr(pg_database), &isNull);
+	aclDatum = heap_getattr(db_tuple, Anum_pg_database_datacl,
+							tupdesc, &isNull);
 
 	if (isNull)
 	{
@@ -1711,9 +1730,6 @@ pg_database_aclmask(Oid db_oid, Oid roleid,
 	if (acl && (Pointer) acl != DatumGetPointer(aclDatum))
 		pfree(acl);
 
-	systable_endscan(scan);
-	heap_close(pg_database, AccessShareLock);
-
 	return result;
 }
 
diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index 4f64104ad3d..0fa61196ea7 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/utils/adt/acl.c,v 1.132 2006/04/30 02:09:07 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/utils/adt/acl.c,v 1.133 2006/04/30 21:15:33 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -553,7 +553,8 @@ acldefault(GrantObjectType objtype, Oid ownerId)
 			owner_default = ACL_ALL_RIGHTS_SEQUENCE;
 			break;
 		case ACL_OBJECT_DATABASE:
-			world_default = ACL_CREATE_TEMP | ACL_CONNECT;	/* not NO_RIGHTS! */
+			/* for backwards compatibility, grant some rights by default */
+			world_default = ACL_CREATE_TEMP | ACL_CONNECT;
 			owner_default = ACL_ALL_RIGHTS_DATABASE;
 			break;
 		case ACL_OBJECT_FUNCTION:
@@ -1341,6 +1342,8 @@ convert_priv_string(text *priv_type_text)
 		return ACL_CREATE_TEMP;
 	if (pg_strcasecmp(priv_type, "TEMPORARY") == 0)
 		return ACL_CREATE_TEMP;
+	if (pg_strcasecmp(priv_type, "CONNECT") == 0)
+		return ACL_CONNECT;
 
 	ereport(ERROR,
 			(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
@@ -1778,6 +1781,11 @@ convert_database_priv_string(text *priv_type_text)
 	if (pg_strcasecmp(priv_type, "TEMP WITH GRANT OPTION") == 0)
 		return ACL_GRANT_OPTION_FOR(ACL_CREATE_TEMP);
 
+	if (pg_strcasecmp(priv_type, "CONNECT") == 0)
+		return ACL_CONNECT;
+	if (pg_strcasecmp(priv_type, "CONNECT WITH GRANT OPTION") == 0)
+		return ACL_GRANT_OPTION_FOR(ACL_CONNECT);
+
 	ereport(ERROR,
 			(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 			 errmsg("unrecognized privilege type: \"%s\"", priv_type)));
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 63135d2d1e3..e89df5bb3af 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/utils/init/postinit.c,v 1.163 2006/04/30 02:09:07 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/utils/init/postinit.c,v 1.164 2006/04/30 21:15:33 tgl Exp $
  *
  *
  *-------------------------------------------------------------------------
@@ -51,7 +51,7 @@
 
 
 static bool FindMyDatabase(const char *name, Oid *db_id, Oid *db_tablespace);
-static void ReverifyMyDatabase(const char *name, const char *user_name);
+static void ReverifyMyDatabase(const char *name, bool am_superuser);
 static void InitCommunication(void);
 static void ShutdownPostgres(int code, Datum arg);
 static bool ThereIsAtLeastOneRole(void);
@@ -127,12 +127,11 @@ FindMyDatabase(const char *name, Oid *db_id, Oid *db_tablespace)
  * of pg_database.
  *
  * To avoid having to read pg_database more times than necessary
- * during session startup, this place is also fitting to set up any
- * database-specific configuration variables.
+ * during session startup, this place is also fitting to check CONNECT
+ * privilege and set up any database-specific configuration variables.
  */
- 
 static void
-ReverifyMyDatabase(const char *name, const char *user_name)
+ReverifyMyDatabase(const char *name, bool am_superuser)
 {
 	Relation	pgdbrel;
 	SysScanDesc pgdbscan;
@@ -196,6 +195,22 @@ ReverifyMyDatabase(const char *name, const char *user_name)
 					name)));
 
 		/*
+		 * Check privilege to connect to the database.  To avoid making
+		 * a whole extra search of pg_database here, we don't go through
+		 * pg_database_aclcheck, but instead use a lower-level routine
+		 * that we can pass the pg_database tuple to.
+		 */
+		if (!am_superuser &&
+			pg_database_tuple_aclmask(tup, RelationGetDescr(pgdbrel),
+									  GetUserId(),
+									  ACL_CONNECT, ACLMASK_ANY) == 0)
+			ereport(FATAL,
+					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+					 errmsg("permission denied for database %s",
+							NameStr(dbform->datname)),
+					 errdetail("User does not have CONNECT privilege.")));
+
+		/*
 		 * Check connection limit for this database.
 		 *
 		 * There is a race condition here --- we create our PGPROC before
@@ -206,29 +221,12 @@ ReverifyMyDatabase(const char *name, const char *user_name)
 		 * just document that the connection limit is approximate.
 		 */
 		if (dbform->datconnlimit >= 0 &&
-			!superuser() &&
+			!am_superuser &&
 			CountDBBackends(MyDatabaseId) > dbform->datconnlimit)
 			ereport(FATAL,
 					(errcode(ERRCODE_TOO_MANY_CONNECTIONS),
 					 errmsg("too many connections for database \"%s\"",
 							name)));
-
-		/*
-		 * Checking for privilege to connect to the database
-		 * We want to bypass the test if we are running in bootstrap mode
-		 */
-		if (!IsBootstrapProcessingMode())
-		{
-				if(pg_database_aclcheck(MyDatabaseId,GetUserId()
-					,ACL_CONNECT) != ACLCHECK_OK )
-				{
-					ereport(FATAL,
-                			(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-                 				errmsg("couldn't connect to database %s", NameStr(dbform->datname)),
-                 				errdetail("User %s doesn't have the CONNECTION privilege for database %s.",
-                                user_name, NameStr(dbform->datname))));				
-				}
-		}
 	}
 
 	/*
@@ -476,15 +474,20 @@ InitPostgres(const char *dbname, const char *username)
 	RelationCacheInitializePhase2();
 
 	/*
-	 * Figure out our postgres user id.  In standalone mode and in the
-	 * autovacuum process, we use a fixed id, otherwise we figure it out from
-	 * the authenticated user name.
+	 * Figure out our postgres user id, and see if we are a superuser.
+	 *
+	 * In standalone mode and in the autovacuum process, we use a fixed id,
+	 * otherwise we figure it out from the authenticated user name.
 	 */
 	if (bootstrap || autovacuum)
+	{
 		InitializeSessionUserIdStandalone();
+		am_superuser = true;
+	}
 	else if (!IsUnderPostmaster)
 	{
 		InitializeSessionUserIdStandalone();
+		am_superuser = true;
 		if (!ThereIsAtLeastOneRole())
 			ereport(WARNING,
 					(errcode(ERRCODE_UNDEFINED_OBJECT),
@@ -496,8 +499,12 @@ InitPostgres(const char *dbname, const char *username)
 	{
 		/* normal multiuser case */
 		InitializeSessionUserId(username);
+		am_superuser = superuser();
 	}
 
+	/* set up ACL framework (so ReverifyMyDatabase can check permissions) */
+	initialize_acl();
+
 	/*
 	 * Unless we are bootstrapping, double-check that InitMyDatabaseInfo() got
 	 * a correct result.  We can't do this until all the database-access
@@ -505,7 +512,7 @@ InitPostgres(const char *dbname, const char *username)
 	 * superuser, so the above stuff has to happen first.)
 	 */
 	if (!bootstrap)
-		ReverifyMyDatabase(dbname,username);
+		ReverifyMyDatabase(dbname, am_superuser);
 
 	/*
 	 * Final phase of relation cache startup: write a new cache file if
@@ -515,14 +522,6 @@ InitPostgres(const char *dbname, const char *username)
 	RelationCacheInitializePhase3();
 
 	/*
-	 * Check if user is a superuser.
-	 */
-	if (bootstrap || autovacuum)
-		am_superuser = true;
-	else
-		am_superuser = superuser();
-
-	/*
 	 * Check a normal user hasn't connected to a superuser reserved slot.
 	 */
 	if (!am_superuser &&
@@ -540,9 +539,6 @@ InitPostgres(const char *dbname, const char *username)
 	/* set default namespace search path */
 	InitializeSearchPath();
 
-	/* set up ACL framework (currently just sets RolMemCache callback) */
-	initialize_acl();
-
 	/* initialize client encoding */
 	InitializeClientEncoding();
 
diff --git a/src/bin/pg_dump/dumputils.c b/src/bin/pg_dump/dumputils.c
index 77e6d43fe02..86c930b8add 100644
--- a/src/bin/pg_dump/dumputils.c
+++ b/src/bin/pg_dump/dumputils.c
@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2006, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/bin/pg_dump/dumputils.c,v 1.26 2006/03/05 15:58:50 momjian Exp $
+ * $PostgreSQL: pgsql/src/bin/pg_dump/dumputils.c,v 1.27 2006/04/30 21:15:33 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -605,6 +605,7 @@ do { \
 	else if (strcmp(type, "DATABASE") == 0)
 	{
 		CONVERT_PRIV('C', "CREATE");
+		CONVERT_PRIV('c', "CONNECT");
 		CONVERT_PRIV('T', "TEMPORARY");
 	}
 	else if (strcmp(type, "TABLESPACE") == 0)
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index a90d7eb72cf..ad8f0f841b2 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -3,7 +3,7 @@
  *
  * Copyright (c) 2000-2006, PostgreSQL Global Development Group
  *
- * $PostgreSQL: pgsql/src/bin/psql/tab-complete.c,v 1.150 2006/04/02 09:02:41 alvherre Exp $
+ * $PostgreSQL: pgsql/src/bin/psql/tab-complete.c,v 1.151 2006/04/30 21:15:33 tgl Exp $
  */
 
 /*----------------------------------------------------------------------
@@ -1373,7 +1373,8 @@ psql_completion(char *text, int start, int end)
 	{
 		static const char *const list_privileg[] =
 		{"SELECT", "INSERT", "UPDATE", "DELETE", "RULE", "REFERENCES",
-		"TRIGGER", "CREATE", "TEMPORARY", "EXECUTE", "USAGE", "ALL", NULL};
+		 "TRIGGER", "CREATE", "CONNECT", "TEMPORARY", "EXECUTE", "USAGE",
+		 "ALL", NULL};
 
 		COMPLETE_WITH_LIST(list_privileg);
 	}
diff --git a/src/include/utils/acl.h b/src/include/utils/acl.h
index c329b6aab81..7651fba290c 100644
--- a/src/include/utils/acl.h
+++ b/src/include/utils/acl.h
@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2006, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/utils/acl.h,v 1.94 2006/04/30 02:09:07 momjian Exp $
+ * $PostgreSQL: pgsql/src/include/utils/acl.h,v 1.95 2006/04/30 21:15:33 tgl Exp $
  *
  * NOTES
  *	  An ACL array is simply an array of AclItems, representing the union
@@ -24,6 +24,8 @@
 #ifndef ACL_H
 #define ACL_H
 
+#include "access/htup.h"
+#include "access/tupdesc.h"
 #include "nodes/parsenodes.h"
 #include "utils/array.h"
 
@@ -145,7 +147,7 @@ typedef ArrayType Acl;
  */
 #define ACL_ALL_RIGHTS_RELATION		(ACL_INSERT|ACL_SELECT|ACL_UPDATE|ACL_DELETE|ACL_RULE|ACL_REFERENCES|ACL_TRIGGER)
 #define ACL_ALL_RIGHTS_SEQUENCE		(ACL_USAGE|ACL_SELECT|ACL_UPDATE)
-#define ACL_ALL_RIGHTS_DATABASE		(ACL_CREATE|ACL_CREATE_TEMP|ACL_CONNECT )
+#define ACL_ALL_RIGHTS_DATABASE		(ACL_CREATE|ACL_CREATE_TEMP|ACL_CONNECT)
 #define ACL_ALL_RIGHTS_FUNCTION		(ACL_EXECUTE)
 #define ACL_ALL_RIGHTS_LANGUAGE		(ACL_USAGE)
 #define ACL_ALL_RIGHTS_NAMESPACE	(ACL_USAGE|ACL_CREATE)
@@ -250,6 +252,8 @@ extern AclMode pg_class_aclmask(Oid table_oid, Oid roleid,
 				 AclMode mask, AclMaskHow how);
 extern AclMode pg_database_aclmask(Oid db_oid, Oid roleid,
 					AclMode mask, AclMaskHow how);
+extern AclMode pg_database_tuple_aclmask(HeapTuple db_tuple, TupleDesc tupdesc,
+					Oid roleid, AclMode mask, AclMaskHow how);
 extern AclMode pg_proc_aclmask(Oid proc_oid, Oid roleid,
 				AclMode mask, AclMaskHow how);
 extern AclMode pg_language_aclmask(Oid lang_oid, Oid roleid,