Skip to content

feat: add IAM and Resource Manager utilities for service accounts (PR 2)#10733

Open
inlined wants to merge 1 commit into
requires-role-1-discoveryfrom
requires-role-2-iam
Open

feat: add IAM and Resource Manager utilities for service accounts (PR 2)#10733
inlined wants to merge 1 commit into
requires-role-1-discoveryfrom
requires-role-2-iam

Conversation

@inlined

@inlined inlined commented Jun 29, 2026

Copy link
Copy Markdown
Member

Description

This is PR 2 in the requiresRole PR chain.
It implements low-level Cloud IAM and Resource Manager API utilities:

  1. generateManagedServiceAccountName for collision-resolved SA names.
  2. computeRolesEtag for salt-and-hash role etag calculations.
  3. addServiceAccountRoles and removeServiceAccountRoles to mutate project IAM policies.
  4. Static cache of standard roles in knownRoles.json to prevent API roundtrips.

Scenarios Tested

  • Run unit tests: npx mocha src/gcp/iam.spec.ts src/gcp/resourceManager.spec.ts
  • Run npm run build & npm run lint

Sample Commands

N/A

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces several IAM utility functions, including getRoleName, generateManagedServiceAccountName, computeRolesEtag, removeServiceAccountRoles, and getServiceAccountRoles, along with a local map of known roles and corresponding unit tests. The review feedback focuses on improving type safety and robustness: specifically, avoiding the use of any by utilizing the getErrStatus utility, using a default import for the JSON roles map, adhering to strict null checks for role lookups, and defensively handling potentially undefined bindings in the project policy to prevent runtime crashes.

Comment thread src/gcp/iam.ts Outdated
Comment on lines +316 to +321
} catch (err: any) {
if (err.status === 404) {
return accountId;
}
throw err;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Avoid using any as an escape hatch. Instead, use the getErrStatus utility from ../error to safely check the error status in a type-safe manner.

Suggested change
} catch (err: any) {
if (err.status === 404) {
return accountId;
}
throw err;
}
} catch (err) {
if (getErrStatus(err) === 404) {
return accountId;
}
throw err;
}
References
  1. Never use any or unknown as an escape hatch. Define proper interfaces/types or use type guards. (link)

Comment thread src/gcp/iam.ts Outdated
import { logger } from "../logger";
import { Client } from "../apiv2";
import * as utils from "../utils";
import { FirebaseError } from "../error";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Import getErrStatus from ../error to use it for safe error status checking in generateManagedServiceAccountName without resorting to any.

Suggested change
import { FirebaseError } from "../error";
import { FirebaseError, getErrStatus } from "../error";

Comment thread src/gcp/iam.ts
import { Client } from "../apiv2";
import * as utils from "../utils";
import { FirebaseError } from "../error";
import * as knownRoles from "./knownRoles.json";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Use a default import for the JSON file instead of a namespace import. When using import * as knownRoles, the imported object is a module namespace containing a default property (under esModuleInterop), which causes type mismatch issues when assigning it to Record<string, string>.

Suggested change
import * as knownRoles from "./knownRoles.json";
import knownRoles from "./knownRoles.json";

Comment thread src/gcp/iam.ts
* Short-circuits using a local map of known roles for speed, and falls back to the GCP API.
*/
export async function getRoleName(role: string): Promise<string> {
const map: Record<string, string> = knownRoles;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Type the map as Record<string, string | undefined> to adhere to strict null checks. Since not all arbitrary strings are valid keys in the knownRoles map, accessing map[role] can return undefined.

Suggested change
const map: Record<string, string> = knownRoles;
const map: Record<string, string | undefined> = knownRoles;
References
  1. Use strict null checks and handle undefined/null explicitly. (link)

Comment on lines +147 to +148
const projectPolicy = await getIamPolicy(projectId);
const memberName = `serviceAccount:${serviceAccountEmail}`;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Defensively initialize projectPolicy.bindings to an empty array if it is undefined. GCP's getIamPolicy API can return a policy without the bindings field if no bindings exist, which would cause a runtime crash (TypeError: Cannot read properties of undefined (reading 'length')) when attempting to iterate over it.

Suggested change
const projectPolicy = await getIamPolicy(projectId);
const memberName = `serviceAccount:${serviceAccountEmail}`;
const projectPolicy = await getIamPolicy(projectId);
projectPolicy.bindings = projectPolicy.bindings || [];
const memberName = `serviceAccount:${serviceAccountEmail}`;

Comment thread src/gcp/resourceManager.ts Outdated
Comment on lines +182 to +186
for (const binding of projectPolicy.bindings) {
if (binding.members.includes(memberName)) {
roles.push(binding.role);
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Defensively handle the case where projectPolicy.bindings is undefined to prevent a potential runtime crash when iterating over the bindings.

Suggested change
for (const binding of projectPolicy.bindings) {
if (binding.members.includes(memberName)) {
roles.push(binding.role);
}
}
for (const binding of projectPolicy.bindings || []) {
if (binding.members.includes(memberName)) {
roles.push(binding.role);
}
}

@inlined inlined force-pushed the requires-role-1-discovery branch from edc4799 to e0c76d9 Compare June 29, 2026 16:43
@inlined inlined force-pushed the requires-role-2-iam branch from 4a06f05 to 1ddf608 Compare June 29, 2026 16:49
@inlined inlined force-pushed the requires-role-1-discovery branch from e0c76d9 to 771a36b Compare June 29, 2026 17:33
### Description
This is PR 2 in the requiresRole PR chain.
It implements low-level Cloud IAM and Resource Manager API utilities:
1. generateManagedServiceAccountName for collision-resolved SA names.
2. computeRolesEtag for salt-and-hash role etag calculations.
3. addServiceAccountRoles and removeServiceAccountRoles to mutate project IAM policies.
4. Static cache of standard roles in knownRoles.json to prevent API roundtrips.

### Scenarios Tested
- Run unit tests: npx mocha src/gcp/iam.spec.ts src/gcp/resourceManager.spec.ts
- Run npm run build & npm run lint

### Sample Commands
N/A
@inlined inlined force-pushed the requires-role-2-iam branch from 1ddf608 to b4dd6a2 Compare June 29, 2026 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants