Making onCall Dart functions Public when deployed#10390
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the Fabricator class in src/deploy/functions/release/fabricator.ts to support callable-triggered functions. It adds logic to identify these endpoints and uses the executor to set their IAM invoker permissions to public via the run.setInvokerUpdate method. I have no feedback to provide.
|
I think we probably want to be consistent with Node here. I was asking an agent to compare the two implementations (because I'm assuming Node is doing the right things) and I think there are additional bugs in the behavior (though not caused by you, still something we should fix and just get it right at once). Here's a lightly edited summary from an agent: I reviewed your recent commits regarding Dart function visibility and noticed a few inconsistencies between the new Dart behavior and the existing Node (GCFv1/v2) behavior. To ensure a consistent developer experience across runtimes, we should align them. Here are the two main areas to address:
I notice for all the other languages they use separate methods for create & update which allows there to be fairly separate logic. We could go that route for dart too? Or make a better setInvoker method? Thoughts? |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the invoker configuration logic by inlining the previously shared setInvoker method into createRunFunction and updateRunFunction. While createRunFunction now correctly handles both HTTPS and callable triggers, the updateRunFunction implementation is missing the logic for callable triggers and the check to respect private invoker settings. Additionally, feedback suggests adding unit tests to verify the new callable trigger logic during function creation to ensure comprehensive test coverage.
|
Great points, Andy! The Dart block did things slightly differently to save code lines, but that shared method is exactly what caused these bugs to sneak into updates. I have removed the shared setInvoker method exclusively created for Dart. While inlining instructions creates slightly longer files, it eliminates the update bugs you raised and brings in the semantic consistency with the Node and Python sections in this file I also updated the test suite stubs to listen for setInvokerCreate to account for this newly refactored creation flow. |
ajperel
left a comment
There was a problem hiding this comment.
Thanks for adding tests too!
| await this.setInvoker(endpoint); | ||
| const serviceName = `projects/${endpoint.project}/locations/${endpoint.region}/services/${endpoint.runServiceId}`; | ||
| if (backend.isHttpsTriggered(endpoint)) { | ||
| const invoker = endpoint.httpsTrigger.invoker || ["public"]; |
There was a problem hiding this comment.
If you look at v2 update ~line 603 you'll notice they do it slightly differently
invoker = endpoint.httpsTrigger.invoker === null ? ["public"] : endpoint.httpsTrigger.invoker;
This is distinguishing between null and undefined
AI calls out this difference:
HTTPS Functions: They are aligned on creation, but there is a difference on update:
- Node (updateV2Function): If the invoker field is omitted (undefined), it does not touch the IAM policy. If it is explicitly set to ["public"] or ["private"], it updates it.
- Dart (updateRunFunction): If the invoker field is omitted (undefined), it falls back to ["public"] and calls setInvokerUpdate.
Impact of the difference: If a user manually makes a Dart HTTPS function private in the Cloud Console and does not specify invoker in their configuration, the next firebase deploy will force it back to public. For Node, it would stay private.
We're getting close to very duplicated code between Dart and gen2. I think since we only handle 2 event types that duplication is maybe OK for now. But we should consider unifying the invoker logic to only be in one place in the future. Otherwise it'll be really easy to again subtly introduce bugs like this. Or we do it now and have more complex logic for only supporting a subset of triggers for Dart.
|
Code changes look good. Can you also add tests. Mostly for Dart, but it looks like we could also use a test for Node that ensures the null vs undefined behavior on update. |
| await fab.updateRunFunction(update); | ||
|
|
||
| expect(run.setInvokerUpdate).to.not.have.been.called; | ||
| }); |
There was a problem hiding this comment.
Can you add one last test for https where we've explicitly configured it to private for the invoker.
ajperel
left a comment
There was a problem hiding this comment.
Some lint issues to fix but LG.
* Making Http and Callable Dart functions Public when deployed
…ploy" flow as well (#10406) * Add support for BUILD-available secrets for Local Builds (#10229) * Enable secret resolution during local App Hosting builds * fix build error * Handle env vars (especially secrets) with a Promise.all so it can be parallelized * Add a warning/confirmation when using BUILD-available secrets for local builds (#10337) * Enable secret resolution during local App Hosting builds * fix build error * Handle env vars (especially secrets) with a Promise.all so it can be parallelized * Add a warning so that we do not use build-available secrets unless the user confirms * fix undefined boolean flag behavior * fix tests, address some review comments * fix test mocking * Consolidate the ABIU prompts in the CLI so it matches the UI behavior (#10352) * Consolidate the ABIU prompts in the CLI so it matches the UI more closely * Address linter errors * Run tests automatically against next (#10365) * feat: add SSE mode support for MCP server (#10258) * feat: add SSE mode support for MCP server ### Description Adds support for running the MCP server in SSE (HTTP) mode, in addition to the default Stdio transport. This allows clients to connect over network or via tools that support SSE. ### Scenarios Tested - Started server in SSE mode and verified log output. * fix: add progressToken to McpContext interface to fix build error ### Description Fixes a type error where progressToken was not defined on McpContext. ### Scenarios Tested - Verified build succeeds. * refactor: address PR comments on SSE support ### Description Addresses PR comments by: - Moving inline require calls to top-level imports. - Replacing any types with specific interfaces or unknown. ### Scenarios Tested - Verified build succeeds. * fix: address remaining review comments on SSE support ### Description - Reverts accidental GA4 tracking change in mcpListResources. - Replaces console.error with this.logger calls for better logging. - Changes default server binding from 0.0.0.0 to 127.0.0.1 for security. ### Scenarios Tested - Verified build succeeds. * style: lint and format fixes for SSE support ### Description - Applied auto-formatting fixes from npm run format. ### Scenarios Tested - Verified build succeeds. * feat: add infrastructure for MCP Apps (#10259) * feat: add infrastructure for MCP Apps ### Description Adds support for returning structured content from tools, which is used by MCP Apps to pass complex data to the host. Also updates the resource index. ### Scenarios Tested - Verified build and file changes. * fix: resolve build errors and address review comments on infra ### Description - Removes imports and registry entries for UI resources that are not yet available in this branch (login, update_environment, deploy, init). - Replaces as any in toContent with an intersection type for better type safety. ### Scenarios Tested - Verified build succeeds. * chore: avoid any for sessionId in SSE transport ### Description - Defines a local interface extending SSEServerTransport to avoid using when accessing . ### Scenarios Tested - Build succeeds. - Lint passes for modified lines. * feat: change sse flag to mode flag and fix build errors ### Description - Replaced boolean flag with string flag (defaults to 'stdio'). - Added validation for to accept only 'stdio' or 'sse'. - Fixed build errors by adding to interface and removing missing resource. ### Scenarios Tested - Build succeeds. - Lint passes with no new errors. * feat: add Update Environment MCP App (#10260) * feat: add mcpapps experiment flag and helper ### Description - Adds mcpapps experiment flag to src/experiments.ts. - Adds applyAppMeta helper function to src/mcp/util.ts to conditionally add UI metadata. - Adds unit tests for applyAppMeta in src/mcp/util.spec.ts. ### Scenarios Tested - Unit tests passed. - Build succeeds. * chore: address PR comments on experiments and util ### Description - Fixes applyAppMeta to preserve existing metadata. - Moves mcpapps flag to be grouped with other MCP experiments. - Removes as any in util.spec.ts by importing CallToolResult. ### Scenarios Tested - Build succeeds. - Lint passes for modified files (ignoring pre-existing warnings). - Unit tests for applyAppMeta pass. * feat: add infrastructure for MCP Apps Adds support for returning structured content from tools, which is used by MCP Apps to pass complex data to the host. Also updates the resource index. - Verified build and file changes. * feat: add Update Environment MCP App ### Description Adds the Update Environment MCP App, allowing users to switch projects and directories from the UI. ### Scenarios Tested - Verified build and file changes. * fix: resolve compilation errors in mcp-update-env-app * fix: resolve remaining lint errors in mcp-update-env-app * refactor: extract app MIME type shared constant * added changelog' * fix: prevent hosting deploy to site in wrong project (#10380) * fix: prevent hosting deploy to site in wrong project ### Description Prevent accidental deployments to a hosting site that does not belong to the active project. The CLI now verifies that the site belongs to the project before creating a version. Fixes #10376 ### Scenarios Tested - Verified that error is thrown when site does not belong to project. - Verified that deploy passes when site belongs to project. - Verified that check is skipped for demo projects. ### Sample Commands `firebase deploy --project project-b` (where site in firebase.json belongs to project-a) -> should fail. * chore: remove excessive site validation check and update tests * Comment * chore: remove demo project check and corresponding test * chore: remove unused Constants import in prepare.ts * test: improve coverage for appdistribution distribution (#10353) * test: improve coverage for appdistribution distribution ### Description Add hermetic uploading and mock polling asserts for app distribution workflows. ### Scenarios Tested - Extracting sequential binary structures * test: fix unused imports in distribution tests * test: improve coverage for dataconnect names (#10350) * test: improve coverage for dataconnect names ### Description Add parsing and validation regex checks for Data Connect entities. ### Scenarios Tested - Extracting groups for service and connectors * fix: correct instances literal in CloudSQL toString * update valid python runtimes (#10393) * Silence punycode deprecation notice (#10395) * feat: ignore punycode deprecation warning on Node 22 ### Description Added a warning handler in the CLI entry point to ignore the `DEP0040` (punycode deprecation) warning. This warning is triggered by dependencies (like `tr46`) on Node 22 because Node 22 deprecates the built-in `punycode` module. Since we cannot easily update the dependencies to avoid it without risking regressions or security issues, ignoring the specific warning is the safest approach. Fixes #10385 ### Scenarios Tested - Verified that unit tests pass (4269 passing). - Verified that the changes are limited to `src/bin/firebase.ts` and `CHANGELOG.md`. ### Sample Commands None (this is an internal warning suppression). * Better changelog * Automatically tag previous image during release (#10367) * chore: add .gitignore entries for the Dart/Flutter files (#10399) In the template directory * test: improve coverage for apphosting yaml configs (#10355) * test: improve coverage for apphosting yaml configs ### Description Implement loading, merging, storing assertions and fix environment map formatting bugs for app hosting setup. ### Scenarios Tested - Loading basic strings and merging overlapping targets * fix: use destructuring to remove variable property in toEnvMap * Prompt for ABIU runtime settings when creating a backend from the "deploy" flow as well * clarify runtime parameter meaning * Update src/deploy/apphosting/prepare.ts Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Pass in rootDir more explicitly * Making onCall Dart functions Public when deployed (#10390) * Making Http and Callable Dart functions Public when deployed * Clean up 'any' typing in src/emulators (#10405) * refactor: refactor catch clauses in emulator directory to use unknown ### Description This PR refactors `catch (err: any)` to `catch (err: unknown)` in the `src/emulator/` directory to improve type safety and reduce weak typing in the codebase. Safe property access and type guards have been applied where necessary. ### Scenarios Tested Ran full test suite with `npm test`. 4270 tests passed, 3 failed in Auth Emulator tests (likely flaky or timing issues). ### Sample Commands `npm test` * refactor: address PR review comments on type safety ### Description This PR addresses review comments on type safety by removing unsafe type assertions and refactoring catch clauses in files outside the emulator directory as requested by the reviewer. ### Scenarios Tested Ran full test suite with `npm test`. All 4273 tests passed successfully. ### Sample Commands `npm test` * refactor: deduplicate error handling in emulator directory using src/error.ts ### Description This PR deduplicates error handling code in the `src/emulator/` directory by utilizing utilities like `getError`, `getErrMsg`, `getErrStack`, and `getErrStatus` from `src/error.ts` as requested by the user. ### Scenarios Tested Ran full test suite with `npm test`. All 4273 tests passed successfully. ### Sample Commands `npm test` * PR fixes * test: improve coverage for firestore deploy (#10346) * test: improve coverage for firestore deploy ### Description Add hermetic tests validating Firestore index and security rule dispatch flows within `src/deploy/firestore/deploy.ts`. ### Scenarios Tested - Sequential initialization of defined configurations - Intermittent state retries ### Sample Commands `npm run mocha -- 'src/deploy/firestore/deploy.spec.ts'` * fix: remove as unknown as type assertions in firestore deploy tests * test: improve test coverage for resourceManager (#10344) * test: improve test coverage for resourceManager ### Description Add unit tests for src/gcp/resourceManager.ts, specifically testing addServiceAccountToRoles and serviceAccountHasRoles. ### Scenarios Tested - Validating success outcomes of role additions - Preventing duplicate role assignment ### Sample Commands npm run mocha -- 'src/gcp/resourceManager.spec.ts' * fix: resolve ESLint errors in tests * fix: remove type escape hatches in nock configurations in resourceManager tests * fix: use arrow functions for nock body matching in resourceManager tests * format * chore: address dependabot alerts in subprojects (#10413) * chore: address dependabot alerts in subprojects ### Description Addressed multiple Dependabot alerts in subprojects by running `npm audit fix` and adding manual overrides for `protobufjs` to fix critical vulnerabilities. Also updated `next` to a safe version in test templates. Avoided breaking changes and major updates as requested. ### Scenarios Tested Ran `npm install` in all affected directories to verify dependency resolution. Did not run full test suite due to time and environment constraints, but changes are isolated to subprojects and test fixtures. ### Sample Commands npm audit fix * fix: update Next.js chunk patterns in tests ### Description Updated the regex patterns in `webframeworks-deploy-tests/tests.ts` to match alphanumeric chunk names generated by `next@16.2.4`. ### Scenarios Tested Verified that chunk names in failure logs match the new pattern. * fix: update Next.js chunk patterns to allow dots and handle .js middleware manifest ### Description Updated the regex patterns in `webframeworks-deploy-tests/tests.ts` to allow dots in chunk names (e.g., `0.zhcmd__c9_v.js`) and updated the expected extension for `_clientMiddlewareManifest` from `.json` to `.js` to match output from newer Next.js versions. ### Scenarios Tested Running `npm run test:frameworks` locally. * test: improve coverage for use command (#10345) * test: improve coverage for use command ### Description Add comprehensive unit tests for `src/commands/use.ts` including alias manipulation, output validation, and input parsing. ### Scenarios Tested - Successful execution of `--add` and `--clear` - Verification of interactive mode fallbacks ### Sample Commands `npm run mocha -- 'src/commands/use.spec.ts'` * fix: resolve ESLint errors in tests * test: address review comments and fix type errors in use.spec.ts * nit * Revert "test: improve coverage for use command (#10345)" (#10418) This reverts commit 9ae31c3. * Make dartfunctions experiment public This boolean flip will make it visible to CLI users when running `firebase experiments:list`. * Cli prompts fixed to respect non interactive flag (#10401) * Cli prompts fixed to respect non interactive flag * fix edge case when project have existing csql and cli is called in non-interactive mode * add changlong * improve defaults and make sdk prompt support non interactive mode * make agent skills not installed by default in non interactive mode * fix lint errors --------- Co-authored-by: Joe Hanley <joehanley@google.com> Co-authored-by: aalej <alejandromarco@google.com> Co-authored-by: Kevin Moore <kevmoo@users.noreply.github.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: shettyvarun268 <shettyvarun@google.com> Co-authored-by: Wanda Mora <morawand@google.com> Co-authored-by: tammam-g <tammam@google.com>
Fixes #10225
This Pull Request ensures that newly deployed Gen 2 Dart callable functions correctly default to public access on Cloud Run instead of requiring authentication by default.