From a14d4bf9c163d817b5294cf5769a0ceae593be13 Mon Sep 17 00:00:00 2001 From: Joey Perrott Date: Tue, 24 Mar 2026 03:52:12 +0000 Subject: [PATCH] fix(apps): address functions deployment compatibility and resolve callable signature issues --- apps/BUILD.bazel | 17 +++ apps/code-of-conduct/BUILD.bazel | 1 + .../app/block-user/block-user.component.ts | 2 +- apps/code-of-conduct/app/block.service.ts | 101 +++++++++--------- .../app/user-table/user-table.component.html | 50 ++++++--- apps/code-of-conduct/main.ts | 3 +- apps/deploy_wrapper_local.sh | 37 +++++++ apps/firestore.rules | 11 +- apps/functions/BUILD.bazel | 22 +++- apps/functions/code-of-conduct/BUILD.bazel | 2 + apps/functions/code-of-conduct/blockUser.ts | 8 +- .../code-of-conduct/getBlockedUsers.ts | 23 ++++ apps/functions/code-of-conduct/index.ts | 2 + apps/functions/code-of-conduct/shared.ts | 47 ++++++-- apps/functions/code-of-conduct/updateUser.ts | 40 +++++++ apps/functions/package.json | 2 +- 16 files changed, 288 insertions(+), 80 deletions(-) create mode 100755 apps/deploy_wrapper_local.sh create mode 100644 apps/functions/code-of-conduct/getBlockedUsers.ts create mode 100644 apps/functions/code-of-conduct/updateUser.ts diff --git a/apps/BUILD.bazel b/apps/BUILD.bazel index 9ed9dec5bd..b6f56b1b26 100644 --- a/apps/BUILD.bazel +++ b/apps/BUILD.bazel @@ -19,6 +19,23 @@ copy_to_bin( ], ) +copy_to_bin( + name = "firebase_assets", + srcs = [ + + # Firebase function files + "//apps/functions:functions_files", + + # Firebase hosted application files + "//apps/code-of-conduct:application_files", + ".firebaserc", + "deploy_wrapper_local.sh", + "firebase.json", + "firestore.indexes.json", + "firestore.rules", + ], +) + firebase.firebase_binary( name = "serve", chdir = package_name(), diff --git a/apps/code-of-conduct/BUILD.bazel b/apps/code-of-conduct/BUILD.bazel index 77e4c764d6..a2c790f459 100644 --- a/apps/code-of-conduct/BUILD.bazel +++ b/apps/code-of-conduct/BUILD.bazel @@ -17,6 +17,7 @@ ng_project( ":node_modules/@angular/compiler", ":node_modules/@angular/core", ":node_modules/@angular/fire", + ":node_modules/@angular/material", ":node_modules/@angular/platform-browser", ":node_modules/@angular/router", ":node_modules/zone.js", diff --git a/apps/code-of-conduct/app/block-user/block-user.component.ts b/apps/code-of-conduct/app/block-user/block-user.component.ts index b88d8a2bbe..42149cd1d3 100644 --- a/apps/code-of-conduct/app/block-user/block-user.component.ts +++ b/apps/code-of-conduct/app/block-user/block-user.component.ts @@ -102,7 +102,7 @@ export class BlockUserComponent { updateUser() { this.dialogRef.disableClose = true; this.blockService - .update(this.providedData.user!.username, this.blockUserForm.value) + .update({username: this.providedData.user!.username, data: this.blockUserForm.value}) .then(() => this.dialogRef.close(), console.error) .finally(() => { this.blockUserForm.enable(); diff --git a/apps/code-of-conduct/app/block.service.ts b/apps/code-of-conduct/app/block.service.ts index a53a1ff25a..d20b7f60ab 100644 --- a/apps/code-of-conduct/app/block.service.ts +++ b/apps/code-of-conduct/app/block.service.ts @@ -1,16 +1,8 @@ import {Injectable, inject} from '@angular/core'; -import { - updateDoc, - collection, - QueryDocumentSnapshot, - FirestoreDataConverter, - collectionSnapshots, - Firestore, - doc, - getDoc, -} from '@angular/fire/firestore'; +import {QueryDocumentSnapshot, FirestoreDataConverter} from '@angular/fire/firestore'; import {httpsCallable, Functions} from '@angular/fire/functions'; -import {map, shareReplay} from 'rxjs'; +import {map, shareReplay, from, switchMap, BehaviorSubject} from 'rxjs'; +import {MatSnackBar} from '@angular/material/snack-bar'; export interface BlockUserParams { /** The username of the user being blocked. */ @@ -46,55 +38,64 @@ export type BlockedUserFromFirestore = BlockedUser & { export class BlockService { /** Firebase functions instance, provided from the root. */ private functions = inject(Functions); - /** Firebase firestore instance, provided from the root. */ - private firestore = inject(Firestore); + /** Snackbar for displaying failure alerts. */ + private snackBar = inject(MatSnackBar); + /** Subject to trigger refreshing the blocked users list. */ + private refreshBlockedUsers$ = new BehaviorSubject(undefined); - /** Request a user to be blocked by the blocking service. */ - readonly block = httpsCallable(this.functions, 'blockUser'); - - /** Request a user to be unblocked by the blocking service. */ - readonly unblock = httpsCallable(this.functions, 'unblockUser'); - - /** Request a sync of all blocked users with the current Github blockings. */ - readonly syncUsersFromGithub = httpsCallable(this.functions, 'syncUsersFromGithub'); + /** Request all blocked users. */ + getBlockedUsers = this.asCallable('getBlockedUsers', true); /** All blocked users current blocked by the blocking service. */ - readonly blockedUsers = collectionSnapshots( - collection(this.firestore, 'blockedUsers').withConverter(converter), - ).pipe( + readonly blockedUsers = this.refreshBlockedUsers$.pipe( + switchMap(() => from(this.getBlockedUsers())), map((blockedUsers) => blockedUsers - .map((snapshot) => snapshot.data()) + .map((user) => ({ + ...user, + blockUntil: user.blockUntil === false ? false : new Date(user.blockUntil), + blockedOn: new Date(user.blockedOn), + })) .sort((a, b) => (a.username.toLowerCase() > b.username.toLowerCase() ? 1 : -1)), ), shareReplay(1), ); + /** Request a user to be blocked. */ + block = this.asCallable('blockUser'); + + /** Request a user to be unblocked. */ + unblock = this.asCallable('unblockUser'); + + /** Request a sync of all blocked users with the current Github blockings. */ + syncUsersFromGithub = this.asCallable('syncUsersFromGithub'); + /** Update the metadata for a blocked user. */ - async update(username: string, data: Partial) { - const userDoc = await getDoc( - doc(collection(this.firestore, 'blockedUsers').withConverter(converter), username), - ); - if (userDoc.exists()) { - return await updateDoc(userDoc.ref, data); - } - throw Error(`The entry for ${username} does not exist`); - } -} + update = this.asCallable<{username: string; data: Partial}, void>('updateUser'); -export const converter: FirestoreDataConverter = { - toFirestore: (user: BlockedUser) => { - return user; - }, - fromFirestore: (data: QueryDocumentSnapshot) => { - return { - username: data.get('username'), - context: data.get('context'), - comments: data.get('comments'), - blockedBy: data.get('blockedBy'), - blockUntil: - data.get('blockUntil') === false ? false : new Date(data.get('blockUntil').seconds * 1000), - blockedOn: new Date(data.get('blockedOn').seconds * 1000), + /** + * Helper function to create a callable function that automatically refreshes the blocked users list. + * @param callableName The name of the callable function to create. + * @returns A function that can be called to invoke the callable function. + */ + private asCallable( + callableName: string, + skipRefresh = false, + ): (callableArg: T) => Promise { + return async (callableArg: T) => { + try { + const result = await httpsCallable(this.functions, callableName)(callableArg); + if (!skipRefresh) { + this.refreshBlockedUsers$.next(); + } + return result.data; + } catch (error) { + const message = error instanceof Error ? error.message : 'Unknown error'; + this.snackBar.open(`Failed to execute ${callableName}: ${message}`, 'Dismiss', { + duration: 5000, + }); + throw error; + } }; - }, -}; + } +} diff --git a/apps/code-of-conduct/app/user-table/user-table.component.html b/apps/code-of-conduct/app/user-table/user-table.component.html index 69aaab5174..14e236e085 100644 --- a/apps/code-of-conduct/app/user-table/user-table.component.html +++ b/apps/code-of-conduct/app/user-table/user-table.component.html @@ -1,42 +1,66 @@ - + Username - username - {{user.username}} + username + {{ user.username }} Blocked Until - {{user.blockUntil === false ? 'Blocked Indefinitely' : user.blockUntil | date}} + + {{ user.blockUntil === false ? 'Blocked Indefinitely' : (user.blockUntil | date) }} + Blocked By - {{user.blockedBy}} + {{ user.blockedBy }} - - - - - - \ No newline at end of file + + diff --git a/apps/code-of-conduct/main.ts b/apps/code-of-conduct/main.ts index a2b0d5d1f1..77f4649fa3 100644 --- a/apps/code-of-conduct/main.ts +++ b/apps/code-of-conduct/main.ts @@ -11,6 +11,7 @@ import {routes} from './app/app.routes.js'; import {environment} from './environment.js'; import {provideFunctions, getFunctions} from '@angular/fire/functions'; import {provideFirestore, getFirestore} from '@angular/fire/firestore'; +import {MatSnackBarModule} from '@angular/material/snack-bar'; if (environment.production) { enableProdMode(); @@ -19,7 +20,7 @@ if (environment.production) { bootstrapApplication(AppComponent, { providers: [ provideRouter(routes), - importProvidersFrom([BrowserAnimationsModule]), + importProvidersFrom([BrowserAnimationsModule, MatSnackBarModule]), provideFirebaseApp(() => initializeApp(environment.firebase)), provideAuth(() => getAuth()), provideFunctions(() => getFunctions()), diff --git a/apps/deploy_wrapper_local.sh b/apps/deploy_wrapper_local.sh new file mode 100755 index 0000000000..ea33da09b5 --- /dev/null +++ b/apps/deploy_wrapper_local.sh @@ -0,0 +1,37 @@ +#!/usr/bin/env bash +set -e + +# Navigate to the directory where the script is running +cd "$(dirname "$0")" + +if [ -L functions/node_modules/firebase-functions ]; then + TARGET=$(readlink functions/node_modules/firebase-functions) + VIRTUAL_NODE_MODULES="functions/node_modules/$(dirname "$TARGET")" + echo "Materializing firebase-functions symlink..." + rm functions/node_modules/firebase-functions + cp -rL "functions/node_modules/$TARGET" functions/node_modules/firebase-functions + + echo "Creating symlinks for dependencies from $VIRTUAL_NODE_MODULES..." + for dep in "$VIRTUAL_NODE_MODULES"/*; do + dep_name=$(basename "$dep") + if [ "$dep_name" != "firebase-functions" ]; then + dep_target=$(readlink -f "$dep") + rm -rf "functions/node_modules/$dep_name" + ln -sf "$dep_target" "functions/node_modules/$dep_name" + fi + done +fi + +# Create .bin directory and symlink for firebase-functions executable +# firebase-tools explicitly searches for .bin/firebase-functions to locate the SDK +echo "Creating .bin/firebase-functions symlink..." +mkdir -p functions/node_modules/.bin +ln -sf ../firebase-functions/lib/bin/firebase-functions.js functions/node_modules/.bin/firebase-functions +chmod +x functions/node_modules/.bin/firebase-functions + +# Fix package.json to match CommonJS bundle format +echo "Removing type: module from package.json..." +sed -i '/"type": "module"/d' functions/package.json + +echo "Running firebase deploy..." +npx -p firebase-tools firebase deploy --project internal-200822 --config firebase.json diff --git a/apps/firestore.rules b/apps/firestore.rules index 8f722eb12f..5b613cf431 100644 --- a/apps/firestore.rules +++ b/apps/firestore.rules @@ -1,9 +1,16 @@ rules_version = '2'; service cloud.firestore { match /databases/{database}/documents { + match /blockedUsers/{user} { + // Deny all reads from client, must use cloud functions for authorization + allow read: if false; + // Deny all writes from client, must use cloud functions + allow write: if false; + } + + // Deny access to anything else by default match /{document=**} { - allow read, create: if request.auth != null; - allow update: if request.auth != null && request.resource.data.diff(resource.data).affectedKeys().hasOnly(['blockUntil', 'comments']); + allow read, write: if false; } } } \ No newline at end of file diff --git a/apps/functions/BUILD.bazel b/apps/functions/BUILD.bazel index 261ebd5eff..87606caad2 100644 --- a/apps/functions/BUILD.bazel +++ b/apps/functions/BUILD.bazel @@ -5,11 +5,29 @@ package(default_visibility = ["//visibility:private"]) npm_link_all_packages() +# TODO: Figure out a different way to deal with the file linking issues. +genrule( + name = "fixed_bundle", + srcs = [":raw_bundle"], + outs = ["bundle.js"], + cmd = """ + for f in $(locations :raw_bundle); do + if [[ $$f == *.js ]]; then + cp $$f $@ + break + fi + done + chmod +w $@ + sed -i 's/(0, import_node_module\\.createRequire)(import_meta\\.url)/(0, import_node_module.createRequire)(__filename)/g' $@ + sed -i -E 's/import_([a-zA-Z0-9_]+)\\.default/(import_\\1.default || import_\\1)/g' $@ + """, +) + copy_to_bin( name = "functions_files", srcs = [ "package.json", - ":bundle", + ":fixed_bundle", "//apps/functions:node_modules/firebase-tools", ], visibility = ["//apps:__pkg__"], @@ -28,7 +46,7 @@ ts_project( ) esbuild( - name = "bundle", + name = "raw_bundle", srcs = [ ":functions", "//apps/functions:node_modules/firebase-functions", diff --git a/apps/functions/code-of-conduct/BUILD.bazel b/apps/functions/code-of-conduct/BUILD.bazel index 8682931120..48f29fbacc 100644 --- a/apps/functions/code-of-conduct/BUILD.bazel +++ b/apps/functions/code-of-conduct/BUILD.bazel @@ -20,9 +20,11 @@ ts_project( name = "lib", srcs = [ "blockUser.ts", + "getBlockedUsers.ts", "shared.ts", "syncUsers.ts", "unblockUser.ts", + "updateUser.ts", ], deps = [ "//apps/functions:node_modules/@octokit/auth-app", diff --git a/apps/functions/code-of-conduct/blockUser.ts b/apps/functions/code-of-conduct/blockUser.ts index 0d9130ca40..36421929e7 100644 --- a/apps/functions/code-of-conduct/blockUser.ts +++ b/apps/functions/code-of-conduct/blockUser.ts @@ -15,13 +15,13 @@ export const blockUser = functions.https.onCall( }, async (request) => { const {comments, blockUntil, context, username} = request.data; - // Ensure that the request was authenticated. - checkAuthenticationAndAccess(request); + // Ensure that the request was authenticated and authorized. + const authRequest = await checkAuthenticationAndAccess(request); /** The Github client for performing Github actions. */ const github = await getAuthenticatedGithubClient(); /** The user performing the block action */ - const actor = await admin.auth().getUser(request.auth.uid); + const actor = await admin.auth().getUser(authRequest.auth.uid); /** The display name of the user. */ const actorName = actor.displayName || actor.email || 'Unknown User'; /** The Firestore Document for the user being blocked. */ @@ -47,7 +47,7 @@ export const blockUser = functions.https.onCall( username: username, blockedBy: actorName, blockedOn: new Date(), - blockUntil: new Date(blockUntil), + blockUntil: blockUntil === false ? false : new Date(blockUntil), }); }, ); diff --git a/apps/functions/code-of-conduct/getBlockedUsers.ts b/apps/functions/code-of-conduct/getBlockedUsers.ts new file mode 100644 index 0000000000..b473527c05 --- /dev/null +++ b/apps/functions/code-of-conduct/getBlockedUsers.ts @@ -0,0 +1,23 @@ +import * as functions from 'firebase-functions'; +import {checkAuthenticationAndAccess, blockedUsersCollection} from './shared.js'; + +/** Returns a list of all blocked users. */ +export const getBlockedUsers = functions.https.onCall( + { + secrets: ['ANGULAR_ROBOT_APP_PRIVATE_KEY', 'ANGULAR_ROBOT_APP_ID'], + }, + async (request) => { + // Ensure that the request was authenticated and authorized. + await checkAuthenticationAndAccess(request); + + const snapshot = await blockedUsersCollection().get(); + return snapshot.docs.map((doc) => { + const data = doc.data(); + return { + ...data, + blockUntil: data.blockUntil instanceof Date ? data.blockUntil.getTime() : data.blockUntil, + blockedOn: data.blockedOn instanceof Date ? data.blockedOn.getTime() : data.blockedOn, + }; + }); + }, +); diff --git a/apps/functions/code-of-conduct/index.ts b/apps/functions/code-of-conduct/index.ts index c1521b6789..4ff4c766c6 100644 --- a/apps/functions/code-of-conduct/index.ts +++ b/apps/functions/code-of-conduct/index.ts @@ -1,3 +1,5 @@ export {blockUser} from './blockUser.js'; export {unblockUser, dailyUnblock} from './unblockUser.js'; export {dailySync, syncUsersFromGithub} from './syncUsers.js'; +export {updateUser} from './updateUser.js'; +export {getBlockedUsers} from './getBlockedUsers.js'; diff --git a/apps/functions/code-of-conduct/shared.ts b/apps/functions/code-of-conduct/shared.ts index de1a0510f3..e84cd1f315 100644 --- a/apps/functions/code-of-conduct/shared.ts +++ b/apps/functions/code-of-conduct/shared.ts @@ -6,7 +6,7 @@ import * as functions from 'firebase-functions'; /** Parameters for blocking a user. */ export interface BlockUserParams { username: string; - blockUntil: string; + blockUntil: string | false; context: string; comments: string; } @@ -29,8 +29,9 @@ export const converter: admin.firestore.FirestoreDataConverter = { context: data.get('context'), comments: data.get('comments'), blockedBy: data.get('blockedBy'), - blockUntil: new Date(data.get('blockUntil')), - blockedOn: new Date(data.get('blockedOn')), + blockedOn: new Date(data.get('blockedOn').seconds * 1000), + blockUntil: + data.get('blockUntil') == false ? false : new Date(data.get('blockUntil').seconds * 1000), }; }, }; @@ -44,7 +45,7 @@ export interface BlockedUser extends admin.firestore.DocumentData { blockedBy: string; blockedOn: Date; username: string; - blockUntil: Date; + blockUntil: Date | false; context: string; comments: string; } @@ -55,15 +56,49 @@ interface AuthenticatedCallableContext extends functions.https.CallableRequest { } /** Verify that the incoming request is authenticated and authorized for access. */ -export function checkAuthenticationAndAccess( +export async function checkAuthenticationAndAccess( context: functions.https.CallableRequest, -): asserts context is AuthenticatedCallableContext { +): Promise { // Authentication is managed by firebase as this occurs within the Firebase functions context. // If the user is unauthenticted, the authorization object will be undefined. if (context.auth == undefined) { // Throwing an HttpsError so that the client gets the error details. throw new functions.https.HttpsError('unauthenticated', 'This action requires authentication'); } + + const user = await admin.auth().getUser(context.auth.uid); + const githubProvider = user.providerData.find((data) => data.providerId === 'github.com'); + + if (!githubProvider) { + throw new functions.https.HttpsError( + 'permission-denied', + 'You must link your Github account to use this application.', + ); + } + + const github = await getAuthenticatedGithubClient(); + + try { + // Resolve the github username from the connected UID + const {data: githubUser} = await github.request('GET /user/{user_id}', { + user_id: githubProvider.uid, + }); + + await github.orgs.checkMembershipForUser({ + org: 'angular', + username: githubUser.login, + }); + } catch (err: any) { + if (err.status === 404) { + throw new functions.https.HttpsError( + 'permission-denied', + 'You must be a member of the Angular Github organization.', + ); + } + throw err; + } + + return context as AuthenticatedCallableContext; } /** Retrieves a Github client instance authenticated as the Angular Robot Github App. */ diff --git a/apps/functions/code-of-conduct/updateUser.ts b/apps/functions/code-of-conduct/updateUser.ts new file mode 100644 index 0000000000..64f68ae674 --- /dev/null +++ b/apps/functions/code-of-conduct/updateUser.ts @@ -0,0 +1,40 @@ +import * as functions from 'firebase-functions'; +import {checkAuthenticationAndAccess, blockedUsersCollection, BlockedUser} from './shared.js'; + +export interface UpdateUserParams { + username: string; + data: { + blockUntil?: string | false; + comments?: string; + }; +} + +/** Updates the metadata for a blocked user. */ +export const updateUser = functions.https.onCall( + { + secrets: ['ANGULAR_ROBOT_APP_PRIVATE_KEY', 'ANGULAR_ROBOT_APP_ID'], + }, + async (request) => { + const {username, data} = request.data; + + // Ensure that the request was authenticated and authorized. + await checkAuthenticationAndAccess(request); + + const userDoc = await blockedUsersCollection().doc(username).get(); + + if (!userDoc.exists) { + throw new functions.https.HttpsError('not-found', `The entry for ${username} does not exist`); + } + + const updateData: Partial = {}; + if (data.comments !== undefined) { + updateData.comments = data.comments; + } + if (data.blockUntil !== undefined) { + // Support date string conversion + updateData.blockUntil = data.blockUntil === false ? false : new Date(data.blockUntil); + } + + await userDoc.ref.update(updateData); + }, +); diff --git a/apps/functions/package.json b/apps/functions/package.json index 77a8331006..d8266d0bcd 100644 --- a/apps/functions/package.json +++ b/apps/functions/package.json @@ -1,7 +1,7 @@ { "name": "functions", "engines": { - "node": "22" + "node": "24" }, "main": "bundle.js", "type": "module",