Skip to content

Commit 57eed28

Browse files
tammam-gtammam-g
authored
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)
1 parent 9b89121 commit 57eed28

File tree

6 files changed

+140
-54
lines changed

6 files changed

+140
-54
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,3 +9,4 @@
99
- Fixed an issue where `ext:install` used POSIX file seperators on Windows machines. (#8326)
1010
- 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)
1111
- Fixed misleading comments in `firebase init dataconnect` `connector.yaml` template.
12+
- Improved Data Connect SQL permissions to better handle tables owned by IAM roles. (#8339)

‎src/commands/dataconnect-sql-diff.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ export const command = new Command("dataconnect:sql:diff [serviceId]")
2323
const serviceInfo = await pickService(projectId, options.config, serviceId);
2424

2525
const diffs = await diffSchema(
26+
options,
2627
serviceInfo.schema,
2728
serviceInfo.dataConnectYaml.schema.datasource.postgresql?.schemaValidation,
2829
);

‎src/dataconnect/schemaMigration.ts‎

Lines changed: 47 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -30,11 +30,39 @@ import * as cloudSqlAdminClient from "../gcp/cloudsql/cloudsqladmin";
3030

3131
import * as errors from "./errors";
3232

33+
async function setupSchemaIfNecessary(
34+
instanceId: string,
35+
databaseId: string,
36+
options: Options,
37+
): Promise<SchemaSetupStatus.GreenField | SchemaSetupStatus.BrownField> {
38+
await setupIAMUsers(instanceId, databaseId, options);
39+
const schemaInfo = await getSchemaMetadata(instanceId, databaseId, DEFAULT_SCHEMA, options);
40+
if (
41+
schemaInfo.setupStatus !== SchemaSetupStatus.BrownField &&
42+
schemaInfo.setupStatus !== SchemaSetupStatus.GreenField
43+
) {
44+
return await setupSQLPermissions(
45+
instanceId,
46+
databaseId,
47+
schemaInfo,
48+
options,
49+
/* silent=*/ true,
50+
);
51+
} else {
52+
logger.info(
53+
`Detected schema "${schemaInfo.name}" is setup in ${schemaInfo.setupStatus} mode. Skipping Setup.`,
54+
);
55+
}
56+
57+
return schemaInfo.setupStatus;
58+
}
59+
3360
export async function diffSchema(
61+
options: Options,
3462
schema: Schema,
3563
schemaValidation?: SchemaValidation,
3664
): Promise<Diff[]> {
37-
const { serviceName, instanceName, databaseId } = getIdentifiers(schema);
65+
const { serviceName, instanceName, databaseId, instanceId } = getIdentifiers(schema);
3866
await ensureServiceIsConnectedToCloudSql(
3967
serviceName,
4068
instanceName,
@@ -43,6 +71,9 @@ export async function diffSchema(
4371
);
4472
let diffs: Diff[] = [];
4573

74+
// Make sure database is setup.
75+
await setupSchemaIfNecessary(instanceId, databaseId, options);
76+
4677
// If the schema validation mode is unset, we surface both STRICT and COMPATIBLE mode diffs, starting with COMPATIBLE.
4778
let validationMode: SchemaValidation = schemaValidation ?? "COMPATIBLE";
4879
setSchemaValidationMode(schema, validationMode);
@@ -128,6 +159,9 @@ export async function migrateSchema(args: {
128159
await setupIAMUsers(instanceId, databaseId, options);
129160
let diffs: Diff[] = [];
130161

162+
// Make sure database is setup.
163+
await setupSchemaIfNecessary(instanceId, databaseId, options);
164+
131165
// If the schema validation mode is unset, we surface both STRICT and COMPATIBLE mode diffs, starting with COMPATIBLE.
132166
let validationMode: SchemaValidation = schemaValidation ?? "COMPATIBLE";
133167
setSchemaValidationMode(schema, validationMode);
@@ -243,30 +277,16 @@ export async function grantRoleToUserInSchema(options: Options, schema: Schema)
243277
}
244278

245279
// Make sure we have the right setup for the requested role grant.
246-
const schemaInfo = await getSchemaMetadata(instanceId, databaseId, DEFAULT_SCHEMA, options);
247-
let isGreenfieldSetup = schemaInfo.setupStatus === SchemaSetupStatus.GreenField;
248-
switch (schemaInfo.setupStatus) {
249-
case SchemaSetupStatus.NotSetup:
250-
case SchemaSetupStatus.NotFound:
251-
const newSetupStatus = await setupSQLPermissions(instanceId, databaseId, schemaInfo, options);
252-
isGreenfieldSetup = newSetupStatus === SchemaSetupStatus.GreenField;
253-
break;
254-
default:
255-
logger.info(
256-
`Detected schema "${schemaInfo.name}" is setup in ${schemaInfo.setupStatus} mode. Skipping Setup.`,
257-
);
258-
break;
259-
}
280+
const schemaSetupStatus = await setupSchemaIfNecessary(instanceId, databaseId, options);
260281

261282
// Edge case: we can't grant firebase owner unless database is greenfield.
262-
if (!isGreenfieldSetup && fdcSqlRole === firebaseowner(databaseId, DEFAULT_SCHEMA)) {
263-
const newSetupStatus = await setupSQLPermissions(instanceId, databaseId, schemaInfo, options);
264-
265-
if (newSetupStatus !== SchemaSetupStatus.GreenField) {
266-
throw new FirebaseError(
267-
`Can't grant owner rule for brownfield databases. Consider fully migrating your database to FDC using 'firebase dataconnect:sql:setup'`,
268-
);
269-
}
283+
if (
284+
schemaSetupStatus !== SchemaSetupStatus.GreenField &&
285+
fdcSqlRole === firebaseowner(databaseId, DEFAULT_SCHEMA)
286+
) {
287+
throw new FirebaseError(
288+
`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'`,
289+
);
270290
}
271291

272292
// Upsert new user account into the database.
@@ -383,19 +403,11 @@ async function handleIncompatibleSchemaError(args: {
383403

384404
const schemaInfo = await getSchemaMetadata(instanceId, databaseId, DEFAULT_SCHEMA, options);
385405
if (schemaInfo.setupStatus !== SchemaSetupStatus.GreenField) {
386-
const newSetupStatus = await setupSQLPermissions(
387-
instanceId,
388-
databaseId,
389-
schemaInfo,
390-
options,
391-
/* silent=*/ true,
406+
throw new FirebaseError(
407+
`Brownfield database are protected from SQL changes by Data Connect.\n` +
408+
`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` +
409+
`If you would like Data Connect to manage your database schema, run 'firebase dataconnect:sql:setup'`,
392410
);
393-
394-
if (newSetupStatus !== SchemaSetupStatus.GreenField) {
395-
throw new FirebaseError(
396-
`Can't migrate brownfield databases. Consider fully migrating your database to FDC using 'firebase dataconnect:sql:setup'`,
397-
);
398-
}
399411
}
400412

401413
// Test if iam user has access to the roles required for this migration

‎src/deploy/dataconnect/prepare.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@ export default async function (context: any, options: DeployOptions): Promise<vo
6868
if (options.dryRun) {
6969
for (const si of serviceInfos) {
7070
await diffSchema(
71+
options,
7172
si.schema,
7273
si.dataConnectYaml.schema.datasource.postgresql?.schemaValidation,
7374
);

‎src/gcp/cloudsql/connect.ts‎

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ export async function execute(
2121
username: string;
2222
password?: string;
2323
silent?: boolean;
24+
transaction?: boolean;
2425
},
2526
): Promise<pg.QueryResult[]> {
2627
const logFn = opts.silent ? logger.debug : logger.info;
@@ -90,21 +91,32 @@ export async function execute(
9091
}
9192
}
9293

94+
const cleanUpFn = async () => {
95+
conn.release();
96+
await pool.end();
97+
connector.close();
98+
};
99+
93100
const conn = await pool.connect();
94101
const results: pg.QueryResult[] = [];
95102
logFn(`Logged in as ${opts.username}`);
103+
if (opts.transaction) {
104+
sqlStatements.unshift("BEGIN;");
105+
sqlStatements.push("COMMIT;");
106+
}
96107
for (const s of sqlStatements) {
97108
logFn(`Executing: '${s}'`);
98109
try {
99110
results.push(await conn.query(s));
100111
} catch (err) {
112+
logFn(`Rolling back transaction due to error ${err}}`);
113+
await conn.query("ROLLBACK;");
114+
await cleanUpFn();
101115
throw new FirebaseError(`Error executing ${err}`);
102116
}
103117
}
104118

105-
conn.release();
106-
await pool.end();
107-
connector.close();
119+
await cleanUpFn();
108120
return results;
109121
}
110122

@@ -114,6 +126,7 @@ export async function executeSqlCmdsAsIamUser(
114126
databaseId: string,
115127
cmds: string[],
116128
silent = false,
129+
transaction = false,
117130
): Promise<pg.QueryResult[]> {
118131
const projectId = needProjectId(options);
119132
const { user: iamUser } = await getIAMUser(options);
@@ -124,6 +137,7 @@ export async function executeSqlCmdsAsIamUser(
124137
databaseId,
125138
username: iamUser,
126139
silent: silent,
140+
transaction: transaction,
127141
});
128142
}
129143

@@ -136,6 +150,7 @@ export async function executeSqlCmdsAsSuperUser(
136150
databaseId: string,
137151
cmds: string[],
138152
silent = false,
153+
transaction = false,
139154
): Promise<pg.QueryResult[]> {
140155
const projectId = needProjectId(options);
141156
// 1. Create a temporary builtin user
@@ -156,6 +171,7 @@ export async function executeSqlCmdsAsSuperUser(
156171
username: superuser,
157172
password: temporaryPassword,
158173
silent: silent,
174+
transaction: transaction,
159175
});
160176
}
161177

‎src/gcp/cloudsql/permissions_setup.ts‎

Lines changed: 71 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,10 @@ export async function setupSQLPermissions(
105105
): Promise<SchemaSetupStatus.BrownField | SchemaSetupStatus.GreenField> {
106106
const schema = schemaInfo.name;
107107
// Step 0: Check current user can run setup and upsert IAM / P4SA users
108-
logger.info(`Attempting to Setup SQL schema "${schema}".`);
108+
logger.info(
109+
`Detected schema "${schema}" setup status is ${schemaInfo.setupStatus}. Running setup...`,
110+
);
111+
109112
const userIsCSQLAdmin = await iamUserIsCSQLAdmin(options);
110113
if (!userIsCSQLAdmin) {
111114
throw new FirebaseError(
@@ -114,20 +117,36 @@ export async function setupSQLPermissions(
114117
}
115118
await setupIAMUsers(instanceId, databaseId, options);
116119

120+
let runGreenfieldSetup = false;
117121
if (schemaInfo.setupStatus === SchemaSetupStatus.GreenField) {
122+
runGreenfieldSetup = true;
118123
logger.info(
119-
`Database ${databaseId} has already been setup. Rerunning setup to repair any missing permissions.`,
124+
`Database ${databaseId} has already been setup as greenfield project. Rerunning setup to repair any missing permissions.`,
120125
);
121-
await greenFieldSchemaSetup(instanceId, databaseId, schema, options, silent);
122-
return SchemaSetupStatus.GreenField;
123-
} else {
124-
logger.info(`Detected schema "${schema}" setup status is ${schemaInfo.setupStatus}.`);
125126
}
126127

127-
// We need to setup the database
128128
if (schemaInfo.tables.length === 0) {
129+
runGreenfieldSetup = true;
129130
logger.info(`Found no tables in schema "${schema}", assuming greenfield project.`);
130-
await greenFieldSchemaSetup(instanceId, databaseId, schema, options, silent);
131+
}
132+
133+
// We need to setup the database
134+
if (runGreenfieldSetup) {
135+
const greenfieldSetupCmds = await greenFieldSchemaSetup(
136+
instanceId,
137+
databaseId,
138+
schema,
139+
options,
140+
);
141+
await executeSqlCmdsAsSuperUser(
142+
options,
143+
instanceId,
144+
databaseId,
145+
greenfieldSetupCmds,
146+
silent,
147+
/** transaction=*/ true,
148+
);
149+
131150
logger.info(clc.green("Database setup complete."));
132151
return SchemaSetupStatus.GreenField;
133152
}
@@ -153,6 +172,12 @@ export async function setupSQLPermissions(
153172

154173
if (shouldSetupGreenfield) {
155174
await setupBrownfieldAsGreenfield(instanceId, databaseId, schemaInfo, options, silent);
175+
logger.info(clc.green("Database setup complete."));
176+
logger.info(
177+
clc.yellow(
178+
"IMPORTANT: please uncomment 'schemaValidation: \"COMPATIBLE\"' in your dataconnect.yaml file to avoid dropping any existing tables by mistake.",
179+
),
180+
);
156181
return SchemaSetupStatus.GreenField;
157182
} else {
158183
logger.info(
@@ -172,7 +197,6 @@ export async function greenFieldSchemaSetup(
172197
databaseId: string,
173198
schema: string,
174199
options: Options,
175-
silent: boolean = false,
176200
) {
177201
// Detect the minimal necessary revokes to avoid errors for users who used the old sql permissions setup.
178202
const revokes = [];
@@ -219,7 +243,7 @@ export async function greenFieldSchemaSetup(
219243
defaultPermissions(databaseId, schema, firebaseowner(databaseId, schema)),
220244
);
221245

222-
await executeSqlCmdsAsSuperUser(options, instanceId, databaseId, sqlRoleSetupCmds, silent);
246+
return sqlRoleSetupCmds;
223247
}
224248

225249
export async function getSchemaMetadata(
@@ -310,14 +334,23 @@ export async function setupBrownfieldAsGreenfield(
310334
) {
311335
const schema = schemaInfo.name;
312336

313-
// Step 1: Run our usual setup which creates necessary roles, transfers schema ownership, and gives nessary grants.
314-
await greenFieldSchemaSetup(instanceId, databaseId, schema, options, silent);
315-
316-
// Step 2: Grant non firebase owners the writer role before changing the table owners.
317337
const firebaseOwnerRole = firebaseowner(databaseId, schema);
318338
const nonFirebasetablesOwners = [...new Set(schemaInfo.tables.map((t) => t.owner))].filter(
319339
(owner) => owner !== firebaseOwnerRole,
320340
);
341+
342+
// Grant roles to firebase superuser to avoid missing permissions on tables
343+
const grantOwnersToSuperuserCmds = nonFirebasetablesOwners.map(
344+
(owner) => `GRANT "${owner}" TO "${FIREBASE_SUPER_USER}"`,
345+
);
346+
const revokeOwnersFromSuperuserCmds = nonFirebasetablesOwners.map(
347+
(owner) => `REVOKE "${owner}" FROM "${FIREBASE_SUPER_USER}"`,
348+
);
349+
350+
// Step 1: Our usual setup which creates necessary roles, transfers schema ownership, and gives nessary grants.
351+
const greenfieldSetupCmds = await greenFieldSchemaSetup(instanceId, databaseId, schema, options);
352+
353+
// Step 2: Grant non firebase owners the writer role before changing the table owners.
321354
const grantCmds = nonFirebasetablesOwners.map(
322355
(owner) => `GRANT "${firebasewriter(databaseId, schema)}" TO "${owner}"`,
323356
);
@@ -327,13 +360,22 @@ export async function setupBrownfieldAsGreenfield(
327360
(table) => `ALTER TABLE "${schema}"."${table.name}" OWNER TO "${firebaseOwnerRole}";`,
328361
);
329362

363+
const setupCmds = [
364+
...grantOwnersToSuperuserCmds,
365+
...greenfieldSetupCmds,
366+
...grantCmds,
367+
...alterTableCmds,
368+
...revokeOwnersFromSuperuserCmds,
369+
];
370+
330371
// Run sql commands
331372
await executeSqlCmdsAsSuperUser(
332373
options,
333374
instanceId,
334375
databaseId,
335-
[...grantCmds, ...alterTableCmds],
376+
setupCmds,
336377
silent,
378+
/** transaction */ true,
337379
);
338380
}
339381

@@ -351,6 +393,9 @@ export async function brownfieldSqlSetup(
351393
const grantOwnersToFirebasesuperuser = uniqueTablesOwners.map(
352394
(owner) => `GRANT "${owner}" TO "${FIREBASE_SUPER_USER}"`,
353395
);
396+
const revokeOwnersFromFirebasesuperuser = uniqueTablesOwners.map(
397+
(owner) => `REVOKE "${owner}" FROM "${FIREBASE_SUPER_USER}"`,
398+
);
354399

355400
// Step 2: Using firebasesuperuser, setup reader and writer permissions on existing tables and setup default permissions for future tables.
356401
const iamUser = (await getIAMUser(options)).user;
@@ -379,7 +424,17 @@ export async function brownfieldSqlSetup(
379424

380425
// Insures firebase roles have access to future tables
381426
...firebaseDefaultPermissions,
427+
428+
// Execute revokes to avoid builtin user becoming IAM role
429+
...revokeOwnersFromFirebasesuperuser,
382430
];
383431

384-
await executeSqlCmdsAsSuperUser(options, instanceId, databaseId, brownfieldSetupCmds, silent);
432+
await executeSqlCmdsAsSuperUser(
433+
options,
434+
instanceId,
435+
databaseId,
436+
brownfieldSetupCmds,
437+
silent,
438+
/** transaction=*/ true,
439+
);
385440
}

0 commit comments

Comments
 (0)