Address review

Rename ScrubIcebergUserMappingHandler to RedactRestCatalogUserMappingSecrets
and move its registration from pg_lake_iceberg to pg_lake_table, alongside
BlockDDLOnExtensionCatalogs.

Reorder credential resolution in GetRestCatalogConnectionFromServer so the
code reads in ascending priority: GUCs (defaults) → catalogs.conf → user
mapping. Previously catalogs.conf was checked after user mapping with
NULL guards to avoid overriding it.

Guard catalogs.conf parser against malformed entries like "rest." (dot
with no key after it) by checking item->name[serverNameLen + 1] != '\0'.

Add test for catalogs.conf on the extension-owned rest server.

Add DDL-level test for per-user scope isolation: two roles with different
scopes on the same server, verified via pg_user_mapping.

Signed-off-by: sfc-gh-npuka <naisila.puka@snowflake.com>

Author: sfc-gh-npuka

pg_lake_iceberg/include/pg_lake/rest_catalog/rest_catalog.h

@@ -127,4 +127,4 @@ extern PGDLLEXPORT RestCatalogRequest * GetRemoveSnapshotCatalogRequest(List *re
 /* protects extension-owned catalog servers */
 extern PGDLLEXPORT bool BlockDDLOnExtensionCatalogs(ProcessUtilityParams *processUtilityParams, void *arg);
 /* scrubs user mapping secrets in-place */
-extern PGDLLEXPORT bool ScrubIcebergUserMappingHandler(ProcessUtilityParams *processUtilityParams, void *arg);
+extern PGDLLEXPORT bool RedactRestCatalogUserMappingSecrets(ProcessUtilityParams *processUtilityParams, void *arg);

pg_lake_iceberg/src/init.c

@@ -330,8 +330,6 @@ _PG_init(void)
 							 NULL, NULL, NULL);
 
 	AvroInit();
-
-	RegisterUtilityStatementHandler(ScrubIcebergUserMappingHandler, NULL);
 }
 
 

pg_lake_iceberg/src/rest_catalog/rest_catalog.c

@@ -295,15 +295,14 @@ ScrubUserMappingSecrets(const char *queryString, List *options)
 
 
 /*
- * ScrubIcebergUserMappingHandler is a ProcessUtility handler registered via
- * pg_lake_engine's RegisterUtilityStatementHandler.  When it detects a
- * CREATE/ALTER USER MAPPING targeting an iceberg_catalog server, it scrubs
- * secret values in the queryString in-place and returns false so normal
+ * RedactRestCatalogUserMappingSecrets is a ProcessUtility handler that detects
+ * CREATE/ALTER USER MAPPING targeting an iceberg_catalog server and scrubs
+ * secret values in the queryString in-place. Returns false so normal
  * processing continues.
  */
 bool
-ScrubIcebergUserMappingHandler(ProcessUtilityParams *processUtilityParams,
-							   void *arg)
+RedactRestCatalogUserMappingSecrets(ProcessUtilityParams *processUtilityParams,
+									void *arg)
 {
 	Node	   *parsetree = processUtilityParams->plannedStmt->utilityStmt;
 	const char *serverName = NULL;
@@ -564,7 +563,8 @@ ReadCatalogsConfCredentials(const char *serverName,
 		 * The dot separates the server name from the property name.
 		 */
 		if (strncmp(item->name, serverName, serverNameLen) != 0 ||
-			item->name[serverNameLen] != '.')
+			item->name[serverNameLen] != '.' ||
+			item->name[serverNameLen + 1] == '\0')
 			continue;
 
 		key = item->name + serverNameLen + 1;
@@ -663,8 +663,25 @@ GetRestCatalogConnectionFromServer(const char *serverName)
 	/*
 	 * Phase 2: Resolve credentials and scope.
 	 *
-	 * Priority: user mapping > catalogs.conf > GUC fallback (set above).
+	 * Priority (ascending): GUC (set above) < catalogs.conf < user mapping.
 	 */
+	char	   *confClientId = NULL;
+	char	   *confClientSecret = NULL;
+	char	   *confScope = NULL;
+
+	if (ReadCatalogsConfCredentials(serverName,
+									&confClientId, &confClientSecret,
+									&confScope))
+	{
+		if (confClientId != NULL)
+			conn->clientId = confClientId;
+		if (confClientSecret != NULL)
+			conn->clientSecret = confClientSecret;
+		if (confScope != NULL)
+			conn->scope = confScope;
+	}
+
+	/* User mapping has highest priority — overrides everything above */
 	List	   *umOptions = LookupUserMappingOptions(server->serverid);
 
 	foreach(lc, umOptions)
@@ -679,23 +696,6 @@ GetRestCatalogConnectionFromServer(const char *serverName)
 			conn->scope = pstrdup(defGetString(def));
 	}
 
-	/* catalogs.conf overrides GUCs but not user mapping */
-	char	   *confClientId = NULL;
-	char	   *confClientSecret = NULL;
-	char	   *confScope = NULL;
-
-	if (ReadCatalogsConfCredentials(serverName,
-									&confClientId, &confClientSecret,
-									&confScope))
-	{
-		if (conn->clientId == NULL && confClientId != NULL)
-			conn->clientId = confClientId;
-		if (conn->clientSecret == NULL && confClientSecret != NULL)
-			conn->clientSecret = confClientSecret;
-		if (conn->scope == NULL && confScope != NULL)
-			conn->scope = confScope;
-	}
-
 	if (conn->clientId == NULL || conn->clientSecret == NULL)
 		ereport(ERROR,
 				(errcode(ERRCODE_FDW_OPTION_NAME_NOT_FOUND),

pg_lake_table/src/init.c

@@ -351,7 +351,10 @@ _PG_init(void)
 
 	MarkGUCPrefixReserved(PG_LAKE_TABLE);
 
+	/* iceberg_catalog server / user mapping guards */
 	RegisterUtilityStatementHandler(BlockDDLOnExtensionCatalogs, NULL);
+	RegisterUtilityStatementHandler(RedactRestCatalogUserMappingSecrets, NULL);
+
 	RegisterUtilityStatementHandler(ProcessVacuumPgLakeTable, NULL);
 	RegisterUtilityStatementHandler(ProcessCreatePgLakeTable, NULL);
 	RegisterUtilityStatementHandler(ProcessCreateAsSelectPgLakeTable, NULL);

pg_lake_table/tests/pytests/test_iceberg_catalog_server.py

@@ -290,6 +290,59 @@ def test_reject_server_options_in_user_mapping(superuser_conn, extension):
     superuser_conn.rollback()
 
 
+def test_per_user_scope_in_user_mapping(superuser_conn, extension):
+    """Different users can have different scopes on the same server."""
+    run_command(
+        """
+        CREATE SERVER test_scope_srv TYPE 'rest'
+            FOREIGN DATA WRAPPER iceberg_catalog
+            OPTIONS (rest_endpoint 'http://localhost:8181')
+        """,
+        superuser_conn,
+    )
+    run_command("CREATE ROLE analyst LOGIN", superuser_conn)
+    run_command("CREATE ROLE admin_user LOGIN", superuser_conn)
+    run_command(
+        """
+        CREATE USER MAPPING FOR analyst SERVER test_scope_srv
+            OPTIONS (client_id 'a-id', client_secret 'a-secret',
+                     scope 'PRINCIPAL_ROLE:READER')
+        """,
+        superuser_conn,
+    )
+    run_command(
+        """
+        CREATE USER MAPPING FOR admin_user SERVER test_scope_srv
+            OPTIONS (client_id 'b-id', client_secret 'b-secret',
+                     scope 'PRINCIPAL_ROLE:ALL')
+        """,
+        superuser_conn,
+    )
+
+    result = run_query(
+        """
+        SELECT u.rolname, array_to_string(um.umoptions, ', ') AS opts
+        FROM pg_user_mapping um
+        JOIN pg_roles u ON u.oid = um.umuser
+        WHERE um.umserver = (SELECT oid FROM pg_foreign_server WHERE srvname = 'test_scope_srv')
+        ORDER BY u.rolname
+        """,
+        superuser_conn,
+    )
+    assert len(result) == 2
+    assert "PRINCIPAL_ROLE:ALL" in result[0]["opts"]  # admin_user
+    assert "PRINCIPAL_ROLE:READER" in result[1]["opts"]  # analyst
+
+    run_command("DROP USER MAPPING FOR analyst SERVER test_scope_srv", superuser_conn)
+    run_command(
+        "DROP USER MAPPING FOR admin_user SERVER test_scope_srv", superuser_conn
+    )
+    run_command("DROP SERVER test_scope_srv", superuser_conn)
+    run_command("DROP ROLE analyst", superuser_conn)
+    run_command("DROP ROLE admin_user", superuser_conn)
+    superuser_conn.rollback()
+
+
 # ── CREATE FOREIGN TABLE on iceberg_catalog servers is blocked ──────────────
 
 
@@ -748,6 +801,57 @@ def test_scope_from_catalogs_conf(
     superuser_conn.commit()
 
 
+def test_credentials_from_catalogs_conf_for_rest_server(
+    pg_conn,
+    superuser_conn,
+    s3,
+    extension,
+    with_default_location,
+    catalogs_conf,
+):
+    """catalogs.conf credentials for the extension-owned 'rest' server"""
+
+    # Clearing the GUCs shouldn't matter when catalogs.conf is set (it should
+    # take priority), but there is currently no way to verify that credentials
+    # are actually coming from catalogs.conf rather than GUC fallback.
+    run_command("SET pg_lake_iceberg.rest_catalog_client_id TO ''", superuser_conn)
+    run_command("SET pg_lake_iceberg.rest_catalog_client_secret TO ''", superuser_conn)
+
+    # Without catalogs.conf, credentials should be missing
+    err = run_command(
+        """
+        CREATE TABLE test_rest_conf_tbl ()
+            USING iceberg
+            WITH (catalog = 'rest', read_only = 'true',
+                  catalog_namespace = 'ns', catalog_table_name = 'tbl')
+        """,
+        pg_conn,
+        raise_error=False,
+    )
+    assert err is not None
+    assert "no credentials found" in str(err)
+    pg_conn.rollback()
+
+    # Now write catalogs.conf — credentials should resolve
+    catalogs_conf(
+        "rest.client_id = 'platform-id'\n" "rest.client_secret = 'platform-secret'\n"
+    )
+
+    err = run_command(
+        """
+        CREATE TABLE test_rest_conf_tbl ()
+            USING iceberg
+            WITH (catalog = 'rest', read_only = 'true',
+                  catalog_namespace = 'ns', catalog_table_name = 'tbl')
+        """,
+        pg_conn,
+        raise_error=False,
+    )
+    assert err is not None
+    assert "no credentials found" not in str(err)
+    pg_conn.rollback()
+
+
 # ── Backward compatibility ─────────────────────────────────────────────────