Fdc grant cmds by tammam-g · Pull Request #7656 · firebase/firebase-tools

tammam-g · 2024-09-13T20:13:22Z

Description

Add firebase dataconnect:sql:grant -R <role> -E <email> command. The command grants the corresponding role in the FDC sql instance.

Scenarios Tested

Not passing -R or --role -> Error: -R, --role <role> is required. Run the command with -h for more info.
Note passing -E or --email -> Error: -E, --email <email> is required. Run the command with -h for more info.
passing non existing role -> Error: Role should be one of owner | writer | reader.

Sample Commands

firebase dataconnect:sql:grant -E [email protected] -R writer
firebase dataconnect:sql:grant --email [email protected] --role reader
firebase dataconnect:sql:grant --email [email protected] --R owner

src/emulator/auth/handlers.ts

+        try {
+          const { newEmail } = setAccountInfoImpl(state, { oobCode });
+          if (continueUrl) {
+            return res.redirect(303, continueUrl);


src/emulator/auth/oob.spec.ts

+
+    await authApi()
+      .post("/identitytoolkit.googleapis.com/v1/accounts:sendOobCode")
+      .set("Authorization", "Bearer owner")


src/emulator/auth/oob.spec.ts

+
+    await authApi()
+      .post("/identitytoolkit.googleapis.com/v1/accounts:sendOobCode")
+      .set("Authorization", "Bearer owner")


src/emulator/auth/oob.spec.ts

+
+    await authApi()
+      .post("/identitytoolkit.googleapis.com/v1/accounts:sendOobCode")
+      .set("Authorization", "Bearer owner")


src/emulator/auth/oob.spec.ts

+
+    await authApi()
+      .post("/identitytoolkit.googleapis.com/v1/accounts:sendOobCode")
+      .set("Authorization", "Bearer owner")


src/emulator/auth/oob.spec.ts

+  it("should error when trying to verify and change email not associated with any user", async () => {
+    await authApi()
+      .post("/identitytoolkit.googleapis.com/v1/accounts:sendOobCode")
+      .set("Authorization", "Bearer owner")


src/emulator/auth/oob.spec.ts

+
+    await authApi()
+      .post("/identitytoolkit.googleapis.com/v1/accounts:sendOobCode")
+      .set("Authorization", "Bearer owner")


fredzqm

Can you update description with test commands and screenshots of results?

Both sunny case and error scenarios.

fredzqm · 2024-09-15T02:52:18Z

src/commands/dataconnect-sql-grant.ts

+    "-E, --email <email>",
+    "The email of the user or service account we would like to grant the role to.",
+  )
+  .before(requirePermissions, ["firebasedataconnect.services.list"])


and cloudsql.instance.connect

We can move cloudsql.users.create here for simplicity.

These are in the iamUserIsCSQLAdmin check so the user can get a nice error message. I'm ok with removing the iamUserIsCSQLAdmin check and just moving things here.

Fine to be checked later

fredzqm · 2024-09-15T02:55:36Z

src/dataconnect/schemaMigration.ts

+    throw new FirebaseError(
+      "-E, --email <email> is required. Run the command with -h for more info.",
+    );
+  }


Should these validations be at cmd file?

+1 - consider using https://github.com/tj/commander.js?tab=readme-ov-file#required-option instead of option to make these required.

Moved to cmd file.

@joehan, I tried using requiredOption but it wasn't possible. We have our own Command type and we don't define requiredOption on it. I also looked around and it seems like we don't have any usage of this.

fredzqm · 2024-09-15T02:56:55Z

src/dataconnect/schemaMigration.ts

+  // Make sure current user can perform this action.
+  const userIsCSQLAdmin = await iamUserIsCSQLAdmin(options);
+  if (!userIsCSQLAdmin) {
+    throw new FirebaseError(`Only users with 'roles/cloudsql.admin' can grant SQL roles.`);


Wonder if requiring cloudsql.users.create in the cmd permission will do it.

This error message is nice though.

This might be a good place to print out instructions telling users to ask their DB admin - ie:

Only users with 'roles/cloudsql.admin' can grant SQL roles. If you do not have this role, ask your database administrator to run this command or manually grant <role> to <user>

Updated the error message. The error message also will take care of the service account extension.

src/dataconnect/schemaMigration.ts

fredzqm · 2024-09-17T23:56:58Z

src/commands/dataconnect-sql-grant.ts

+    "-E, --email <email>",
+    "The email of the user or service account we would like to grant the role to.",
+  )
+  .before(requirePermissions, ["firebasedataconnect.services.list"])


Fine to be checked later

initial grant commands

11b377d

github-advanced-security bot found potential problems Sep 13, 2024

View reviewed changes

Merge master and fix conflict

28fe9a9

tammam-g force-pushed the fdc_grant_cmds branch from b08b2a9 to 28fe9a9 Compare September 13, 2024 20:24

Add some checks and fix linting

f0d4dea

tammam-g marked this pull request as ready for review September 13, 2024 22:42

tammam-g requested review from fredzqm and joehan September 13, 2024 22:46

fredzqm requested changes Sep 16, 2024

View reviewed changes

Address comments and cleanup / error messages

c8e6c8b

tammam-g requested a review from fredzqm September 17, 2024 23:33

fredzqm approved these changes Sep 17, 2024

View reviewed changes

tammam-g added 3 commits September 18, 2024 14:04

Merge branch 'master' into fdc_grant_cmds

234e07f

Add fdc grant commands changelog

ff6c3c6

Merge branch 'master' into fdc_grant_cmds

5d80dd2

tammam-g enabled auto-merge (squash) September 18, 2024 18:16

Fix merge conflict

7361fe4

tammam-g merged commit e1d1563 into master Sep 18, 2024

Conversation

tammam-g commented Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Scenarios Tested

Sample Commands

Uh oh!

Check warning

Check failure

Check failure

Check failure

Check failure

Check failure

Check failure

fredzqm left a comment

Choose a reason for hiding this comment

Uh oh!

fredzqm Sep 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tammam-g commented Sep 13, 2024 •

edited

Loading

fredzqm Sep 15, 2024 •

edited

Loading