firebase / firebase/firebase-tools

Declarative security: `grantNewRoles` has no retry for the SA propagation race, and reports the failure as a permissions error

Open
#10,903 0 comments 0 reactions 1 assignee Claimed by @inlined View on GitHub
api: functions type: bug
Dominant language
TypeScript
Stars
4.5k
Forks
1.3k
Avg merge
1d 12h
Merged PRs (30d)
84

Description

### Environment

**firebase-tools:** 15.25.1 and 15.26.0
**Platform:** macOS

### Summary

Deploying a `requiresRole()` codebase fails about 2 times in 5 during enrollment. Looks like one more surface of the SA propagation race from #10859 — this one in `grantNewRoles`, which #10871 didn't cover. Separately, the error message points at permissions rather than the actual cause, which made it slow to track down.

### Steps to reproduce

1. Deploy a codebase declaring roles via `requiresRole()` (ours declares 5).
2. Delete every function in the codebase, leaving `requiresRole()` in the source.
3. `firebase deploy --only functions`, and repeat.

Step 2 matters: `existingManagedSA` comes from a *deployed* endpoint's `serviceAccount` field (`prepare.ts:95`), so with none deployed each deploy creates a new SA and re-enters the race. Steady-state redeploys short-circuit at `prepare.ts:162` and never hit it.

### Actual behavior

```
Error: The declarative security roles for this codebase have changed, but you do not have access to see what has changed. Please ask an IAM administrator to perform the next deploy.
```

We held `roles/owner`, and the CLI's own `testIamPermissions` check at `prepare.ts:186` had already passed in the same run for `resourcemanager.projects.setIamPolicy` and `iam.serviceAccounts.create`. The real error only shows up in Cloud Audit Logs, on `SetIamPolicy`:

```
code 3 (INVALID_ARGUMENT)
"Exception calling IAM: Service account firebase-fn-2717501595@.iam.gserviceaccount.com does not exist."
```

`--debug` doesn't surface it either.

---

### Cause 1 — `grantNewRoles` doesn't retry the create-then-grant race

`fabricator.ts:113` creates the SA, `fabricator.ts:135` calls `addServiceAccountRoles(..., true)` 1-2s later. Resource Manager hasn't observed the account yet and rejects the member. The `true` is `skipAccountLookup` (`resourceManager.ts:67`), which also skips the IAM read that might otherwise have forced propagation.

That call isn't routed through the executor — bare `await` in a `try`, no predicate, no backoff. #10871 added `isServiceAccount404` and wired it into the function create/update sites (`fabricator.ts:476`, `:608`, `:759`, `:837`, `:873`), but `grantNewRoles` runs earlier and hits Resource Manager instead of Cloud Functions.

**One gotcha for the fix:** `isServiceAccount404` wouldn't match here anyway. It returns early unless `parseErrorCode(err) === 404` (`executor.ts:36`), and `parseErrorCode` reads `err.status`, which `responseToError` sets to the HTTP code — **400** for INVALID_ARGUMENT. The `code: 3` above is the gRPC canonical code in the audit log, not what the error object carries. So matching on 3, or reusing `isServiceAccount404` unchanged, won't fire.

**Suggested fix:** route `addServiceAccountRoles` through the executor with `isTransientError` plus a predicate for HTTP 400 whose message contains "does not exist" and an SA address (or widen `isServiceAccount404` to 400). A fixed sleep won't help — the create→grant gap is 1-2s on successes and failures alike, so there's no threshold to wait past.

### Cause 2 — the error message names a cause it hasn't established

That string is thrown unconditionally from four places, none of which inspect the error:

- `fabricator.ts:153` — catch-all around `addServiceAccountRoles`
- `fabricator.ts:165` — `testIamPermissions` failure (the one case where it's accurate)
- `fabricator.ts:209` — catch-all around `removeServiceAccountRoles`
- `prepare.ts:195` — catch-all around `getServiceAccountRoles`

The three catch-alls attach the real error as `{ original: e }` and never print it. Worth fixing on its own, since it'll misreport any future failure on these paths the same way.

**Suggested fix:** only claim a permissions cause on 403, otherwise rethrow or use a neutral message. Even just appending `original.message` would have made this self-diagnosing.

---

### Related

Retrying is only a partial workaround: `generateManagedServiceAccountName` (`iam.ts:307-313`) picks a random suffix, so each retry creates a fresh SA and starts the race over instead of converging. That's the naming question from #10860, which #10871 closed without changing naming or discovery — happy to file that separately if useful.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.