fix(security): default service account sentinel and partial deploy validation#10779
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces declarative security support and automated rolling IAM role grants/revocations for 2nd generation Cloud Functions. Key changes include refactoring the deployment planning to group changesets by codebase (CodebasePlan) and region, implementing sequential IAM role grants and revocations in the Fabricator to avoid GCP concurrency conflicts, and adding validation to prevent partial codebase deploys during enrollment/unenrollment. Additionally, helper methods in src/gcp/iam.ts were updated to support full service account email addresses, and a temporary warning was added for Dart functions. There are no review comments to evaluate.
e841de8 to
46bcc29
Compare
|
|
||
| if (isPartiallyFiltered && (isEnrolling || isUnenrolling)) { | ||
| throw new FirebaseError( | ||
| `Cannot deploy a partial codebase when enrolling or unenrolling from declarative security`, |
There was a problem hiding this comment.
Should the message explain why, to prevent this from feeling frustratingly arbitrary?
There was a problem hiding this comment.
IDK. I don't want an essay; maybe "because it is unsafe?"
ajperel
left a comment
ajperel
left a comment
There was a problem hiding this comment.
Leaving the substantive review to Victor, but asking some questions just so I can learn as y'all add all this functionality.
| if (requiredRoles) { | ||
| rolesToAdd = requiredRoles.filter((r) => !roles.includes(r)); | ||
| rolesToRemove = roles.filter((r) => !requiredRoles.includes(r)); | ||
| if (!existingManagedSA && managedSA) { |
There was a problem hiding this comment.
if we care about filters here why don't we also care about filters for enrollment in this instance? I thought you didn't want to do enrollment or unenrollment on partial deploys?
There was a problem hiding this comment.
Both are blocked in prepare.ts.
There was a problem hiding this comment.
yeah, that's why I was confused by any checking
4 participants
Description
This PR resolves the deployment crash when opting out of declarative security and refines unenrollment safety:
Scenarios Tested