Improve fdc permissions setup (#8339)

* Run createUser in getSchemaMetadata for now, will reduce calls later

* Make setting up iam user explicit instead of implicit

* Add changlog

* improve schema diff

* Add quotes

* fix lint

* Don't block schema deployment

* Improve schema setup by making sure we execute changes in transaction, revoke users, and have sufficient permissions before diffing

* Fix lint issues

* Address code review comments

* Reuse setupSchemaIfNecessary in grant roles function (DRY)

Author: tammam-g

CHANGELOG.md

@@ -9,3 +9,4 @@
 - Fixed an issue where `ext:install` used POSIX file seperators on Windows machines. (#8326)
 - Updated the Firebase Data Connect local toolkit to v1.9.1, which adds support for generated Angular SDKs and updates Dart SDK fields to follow best practices. (#8340)
 - Fixed misleading comments in `firebase init dataconnect` `connector.yaml` template.
+- Improved Data Connect SQL permissions to better handle tables owned by IAM roles. (#8339)

src/commands/dataconnect-sql-diff.ts

@@ -23,6 +23,7 @@ export const command = new Command("dataconnect:sql:diff [serviceId]")
     const serviceInfo = await pickService(projectId, options.config, serviceId);
 
     const diffs = await diffSchema(
+      options,
       serviceInfo.schema,
       serviceInfo.dataConnectYaml.schema.datasource.postgresql?.schemaValidation,
     );

src/dataconnect/schemaMigration.ts

@@ -30,11 +30,39 @@ import * as cloudSqlAdminClient from "../gcp/cloudsql/cloudsqladmin";
 
 import * as errors from "./errors";
 
+async function setupSchemaIfNecessary(
+  instanceId: string,
+  databaseId: string,
+  options: Options,
+): Promise<SchemaSetupStatus.GreenField | SchemaSetupStatus.BrownField> {
+  await setupIAMUsers(instanceId, databaseId, options);
+  const schemaInfo = await getSchemaMetadata(instanceId, databaseId, DEFAULT_SCHEMA, options);
+  if (
+    schemaInfo.setupStatus !== SchemaSetupStatus.BrownField &&
+    schemaInfo.setupStatus !== SchemaSetupStatus.GreenField
+  ) {
+    return await setupSQLPermissions(
+      instanceId,
+      databaseId,
+      schemaInfo,
+      options,
+      /* silent=*/ true,
+    );
+  } else {
+    logger.info(
+      `Detected schema "${schemaInfo.name}" is setup in ${schemaInfo.setupStatus} mode. Skipping Setup.`,
+    );
+  }
+
+  return schemaInfo.setupStatus;
+}
+
 export async function diffSchema(
+  options: Options,
   schema: Schema,
   schemaValidation?: SchemaValidation,
 ): Promise<Diff[]> {
-  const { serviceName, instanceName, databaseId } = getIdentifiers(schema);
+  const { serviceName, instanceName, databaseId, instanceId } = getIdentifiers(schema);
   await ensureServiceIsConnectedToCloudSql(
     serviceName,
     instanceName,
@@ -43,6 +71,9 @@ export async function diffSchema(
   );
   let diffs: Diff[] = [];
 
+  // Make sure database is setup.
+  await setupSchemaIfNecessary(instanceId, databaseId, options);
+
   // If the schema validation mode is unset, we surface both STRICT and COMPATIBLE mode diffs, starting with COMPATIBLE.
   let validationMode: SchemaValidation = schemaValidation ?? "COMPATIBLE";
   setSchemaValidationMode(schema, validationMode);
@@ -128,6 +159,9 @@ export async function migrateSchema(args: {
   await setupIAMUsers(instanceId, databaseId, options);
   let diffs: Diff[] = [];
 
+  // Make sure database is setup.
+  await setupSchemaIfNecessary(instanceId, databaseId, options);
+
   // If the schema validation mode is unset, we surface both STRICT and COMPATIBLE mode diffs, starting with COMPATIBLE.
   let validationMode: SchemaValidation = schemaValidation ?? "COMPATIBLE";
   setSchemaValidationMode(schema, validationMode);
@@ -243,30 +277,16 @@ export async function grantRoleToUserInSchema(options: Options, schema: Schema)
   }
 
   // Make sure we have the right setup for the requested role grant.
-  const schemaInfo = await getSchemaMetadata(instanceId, databaseId, DEFAULT_SCHEMA, options);
-  let isGreenfieldSetup = schemaInfo.setupStatus === SchemaSetupStatus.GreenField;
-  switch (schemaInfo.setupStatus) {
-    case SchemaSetupStatus.NotSetup:
-    case SchemaSetupStatus.NotFound:
-      const newSetupStatus = await setupSQLPermissions(instanceId, databaseId, schemaInfo, options);
-      isGreenfieldSetup = newSetupStatus === SchemaSetupStatus.GreenField;
-      break;
-    default:
-      logger.info(
-        `Detected schema "${schemaInfo.name}" is setup in ${schemaInfo.setupStatus} mode. Skipping Setup.`,
-      );
-      break;
-  }
+  const schemaSetupStatus = await setupSchemaIfNecessary(instanceId, databaseId, options);
 
   // Edge case: we can't grant firebase owner unless database is greenfield.
-  if (!isGreenfieldSetup && fdcSqlRole === firebaseowner(databaseId, DEFAULT_SCHEMA)) {
-    const newSetupStatus = await setupSQLPermissions(instanceId, databaseId, schemaInfo, options);
-
-    if (newSetupStatus !== SchemaSetupStatus.GreenField) {
-      throw new FirebaseError(
-        `Can't grant owner rule for brownfield databases. Consider fully migrating your database to FDC using 'firebase dataconnect:sql:setup'`,
-      );
-    }
+  if (
+    schemaSetupStatus !== SchemaSetupStatus.GreenField &&
+    fdcSqlRole === firebaseowner(databaseId, DEFAULT_SCHEMA)
+  ) {
+    throw new FirebaseError(
+      `Owner rule isn't available in brownfield databases. If you would like Data Connect to manage and own your database schema, run 'firebase dataconnect:sql:setup'`,
+    );
   }
 
   // Upsert new user account into the database.
@@ -383,19 +403,11 @@ async function handleIncompatibleSchemaError(args: {
 
     const schemaInfo = await getSchemaMetadata(instanceId, databaseId, DEFAULT_SCHEMA, options);
     if (schemaInfo.setupStatus !== SchemaSetupStatus.GreenField) {
-      const newSetupStatus = await setupSQLPermissions(
-        instanceId,
-        databaseId,
-        schemaInfo,
-        options,
-        /* silent=*/ true,
+      throw new FirebaseError(
+        `Brownfield database are protected from SQL changes by Data Connect.\n` +
+          `You can use the SQL diff generated by 'firebase dataconnect:sql:diff' to assist you in applying the required changes to your CloudSQL database. Connector deployment will succeed when there is no required diff changes.\n` +
+          `If you would like Data Connect to manage your database schema, run 'firebase dataconnect:sql:setup'`,
       );
-
-      if (newSetupStatus !== SchemaSetupStatus.GreenField) {
-        throw new FirebaseError(
-          `Can't migrate brownfield databases. Consider fully migrating your database to FDC using 'firebase dataconnect:sql:setup'`,
-        );
-      }
     }
 
     // Test if iam user has access to the roles required for this migration

src/deploy/dataconnect/prepare.ts

@@ -68,6 +68,7 @@ export default async function (context: any, options: DeployOptions): Promise<vo
   if (options.dryRun) {
     for (const si of serviceInfos) {
       await diffSchema(
+        options,
         si.schema,
         si.dataConnectYaml.schema.datasource.postgresql?.schemaValidation,
       );

src/gcp/cloudsql/connect.ts

@@ -21,6 +21,7 @@ export async function execute(
     username: string;
     password?: string;
     silent?: boolean;
+    transaction?: boolean;
   },
 ): Promise<pg.QueryResult[]> {
   const logFn = opts.silent ? logger.debug : logger.info;
@@ -90,21 +91,32 @@ export async function execute(
     }
   }
 
+  const cleanUpFn = async () => {
+    conn.release();
+    await pool.end();
+    connector.close();
+  };
+
   const conn = await pool.connect();
   const results: pg.QueryResult[] = [];
   logFn(`Logged in as ${opts.username}`);
+  if (opts.transaction) {
+    sqlStatements.unshift("BEGIN;");
+    sqlStatements.push("COMMIT;");
+  }
   for (const s of sqlStatements) {
     logFn(`Executing: '${s}'`);
     try {
       results.push(await conn.query(s));
     } catch (err) {
+      logFn(`Rolling back transaction due to error ${err}}`);
+      await conn.query("ROLLBACK;");
+      await cleanUpFn();
       throw new FirebaseError(`Error executing ${err}`);
     }
   }
 
-  conn.release();
-  await pool.end();
-  connector.close();
+  await cleanUpFn();
   return results;
 }
 
@@ -114,6 +126,7 @@ export async function executeSqlCmdsAsIamUser(
   databaseId: string,
   cmds: string[],
   silent = false,
+  transaction = false,
 ): Promise<pg.QueryResult[]> {
   const projectId = needProjectId(options);
   const { user: iamUser } = await getIAMUser(options);
@@ -124,6 +137,7 @@ export async function executeSqlCmdsAsIamUser(
     databaseId,
     username: iamUser,
     silent: silent,
+    transaction: transaction,
   });
 }
 
@@ -136,6 +150,7 @@ export async function executeSqlCmdsAsSuperUser(
   databaseId: string,
   cmds: string[],
   silent = false,
+  transaction = false,
 ): Promise<pg.QueryResult[]> {
   const projectId = needProjectId(options);
   // 1. Create a temporary builtin user
@@ -156,6 +171,7 @@ export async function executeSqlCmdsAsSuperUser(
     username: superuser,
     password: temporaryPassword,
     silent: silent,
+    transaction: transaction,
   });
 }
 

src/gcp/cloudsql/permissions_setup.ts

@@ -105,7 +105,10 @@ export async function setupSQLPermissions(
 ): Promise<SchemaSetupStatus.BrownField | SchemaSetupStatus.GreenField> {
   const schema = schemaInfo.name;
   // Step 0: Check current user can run setup and upsert IAM / P4SA users
-  logger.info(`Attempting to Setup SQL schema "${schema}".`);
+  logger.info(
+    `Detected schema "${schema}" setup status is ${schemaInfo.setupStatus}. Running setup...`,
+  );
+
   const userIsCSQLAdmin = await iamUserIsCSQLAdmin(options);
   if (!userIsCSQLAdmin) {
     throw new FirebaseError(
@@ -114,20 +117,36 @@ export async function setupSQLPermissions(
   }
   await setupIAMUsers(instanceId, databaseId, options);
 
+  let runGreenfieldSetup = false;
   if (schemaInfo.setupStatus === SchemaSetupStatus.GreenField) {
+    runGreenfieldSetup = true;
     logger.info(
-      `Database ${databaseId} has already been setup. Rerunning setup to repair any missing permissions.`,
+      `Database ${databaseId} has already been setup as greenfield project. Rerunning setup to repair any missing permissions.`,
     );
-    await greenFieldSchemaSetup(instanceId, databaseId, schema, options, silent);
-    return SchemaSetupStatus.GreenField;
-  } else {
-    logger.info(`Detected schema "${schema}" setup status is ${schemaInfo.setupStatus}.`);
   }
 
-  // We need to setup the database
   if (schemaInfo.tables.length === 0) {
+    runGreenfieldSetup = true;
     logger.info(`Found no tables in schema "${schema}", assuming greenfield project.`);
-    await greenFieldSchemaSetup(instanceId, databaseId, schema, options, silent);
+  }
+
+  // We need to setup the database
+  if (runGreenfieldSetup) {
+    const greenfieldSetupCmds = await greenFieldSchemaSetup(
+      instanceId,
+      databaseId,
+      schema,
+      options,
+    );
+    await executeSqlCmdsAsSuperUser(
+      options,
+      instanceId,
+      databaseId,
+      greenfieldSetupCmds,
+      silent,
+      /** transaction=*/ true,
+    );
+
     logger.info(clc.green("Database setup complete."));
     return SchemaSetupStatus.GreenField;
   }
@@ -153,6 +172,12 @@ export async function setupSQLPermissions(
 
   if (shouldSetupGreenfield) {
     await setupBrownfieldAsGreenfield(instanceId, databaseId, schemaInfo, options, silent);
+    logger.info(clc.green("Database setup complete."));
+    logger.info(
+      clc.yellow(
+        "IMPORTANT: please uncomment 'schemaValidation: \"COMPATIBLE\"' in your dataconnect.yaml file to avoid dropping any existing tables by mistake.",
+      ),
+    );
     return SchemaSetupStatus.GreenField;
   } else {
     logger.info(
@@ -172,7 +197,6 @@ export async function greenFieldSchemaSetup(
   databaseId: string,
   schema: string,
   options: Options,
-  silent: boolean = false,
 ) {
   // Detect the minimal necessary revokes to avoid errors for users who used the old sql permissions setup.
   const revokes = [];
@@ -219,7 +243,7 @@ export async function greenFieldSchemaSetup(
     defaultPermissions(databaseId, schema, firebaseowner(databaseId, schema)),
   );
 
-  await executeSqlCmdsAsSuperUser(options, instanceId, databaseId, sqlRoleSetupCmds, silent);
+  return sqlRoleSetupCmds;
 }
 
 export async function getSchemaMetadata(
@@ -310,14 +334,23 @@ export async function setupBrownfieldAsGreenfield(
 ) {
   const schema = schemaInfo.name;
 
-  // Step 1: Run our usual setup which creates necessary roles, transfers schema ownership, and gives nessary grants.
-  await greenFieldSchemaSetup(instanceId, databaseId, schema, options, silent);
-
-  // Step 2: Grant non firebase owners the writer role before changing the table owners.
   const firebaseOwnerRole = firebaseowner(databaseId, schema);
   const nonFirebasetablesOwners = [...new Set(schemaInfo.tables.map((t) => t.owner))].filter(
     (owner) => owner !== firebaseOwnerRole,
   );
+
+  // Grant roles to firebase superuser to avoid missing permissions on tables
+  const grantOwnersToSuperuserCmds = nonFirebasetablesOwners.map(
+    (owner) => `GRANT "${owner}" TO "${FIREBASE_SUPER_USER}"`,
+  );
+  const revokeOwnersFromSuperuserCmds = nonFirebasetablesOwners.map(
+    (owner) => `REVOKE "${owner}" FROM "${FIREBASE_SUPER_USER}"`,
+  );
+
+  // Step 1: Our usual setup which creates necessary roles, transfers schema ownership, and gives nessary grants.
+  const greenfieldSetupCmds = await greenFieldSchemaSetup(instanceId, databaseId, schema, options);
+
+  // Step 2: Grant non firebase owners the writer role before changing the table owners.
   const grantCmds = nonFirebasetablesOwners.map(
     (owner) => `GRANT "${firebasewriter(databaseId, schema)}" TO "${owner}"`,
   );
@@ -327,13 +360,22 @@ export async function setupBrownfieldAsGreenfield(
     (table) => `ALTER TABLE "${schema}"."${table.name}" OWNER TO "${firebaseOwnerRole}";`,
   );
 
+  const setupCmds = [
+    ...grantOwnersToSuperuserCmds,
+    ...greenfieldSetupCmds,
+    ...grantCmds,
+    ...alterTableCmds,
+    ...revokeOwnersFromSuperuserCmds,
+  ];
+
   // Run sql commands
   await executeSqlCmdsAsSuperUser(
     options,
     instanceId,
     databaseId,
-    [...grantCmds, ...alterTableCmds],
+    setupCmds,
     silent,
+    /** transaction */ true,
   );
 }
 
@@ -351,6 +393,9 @@ export async function brownfieldSqlSetup(
   const grantOwnersToFirebasesuperuser = uniqueTablesOwners.map(
     (owner) => `GRANT "${owner}" TO "${FIREBASE_SUPER_USER}"`,
   );
+  const revokeOwnersFromFirebasesuperuser = uniqueTablesOwners.map(
+    (owner) => `REVOKE "${owner}" FROM "${FIREBASE_SUPER_USER}"`,
+  );
 
   // Step 2: Using firebasesuperuser, setup reader and writer permissions on existing tables and setup default permissions for future tables.
   const iamUser = (await getIAMUser(options)).user;
@@ -379,7 +424,17 @@ export async function brownfieldSqlSetup(
 
     // Insures firebase roles have access to future tables
     ...firebaseDefaultPermissions,
+
+    // Execute revokes to avoid builtin user becoming IAM role
+    ...revokeOwnersFromFirebasesuperuser,
   ];
 
-  await executeSqlCmdsAsSuperUser(options, instanceId, databaseId, brownfieldSetupCmds, silent);
+  await executeSqlCmdsAsSuperUser(
+    options,
+    instanceId,
+    databaseId,
+    brownfieldSetupCmds,
+    silent,
+    /** transaction=*/ true,
+  );
 }