diff --git a/.github/workflows/lambda-tests.yml b/.github/workflows/lambda-tests.yml index 92676d9e..fac7ff00 100644 --- a/.github/workflows/lambda-tests.yml +++ b/.github/workflows/lambda-tests.yml @@ -213,19 +213,25 @@ jobs: for f in $files; do [ -f "$f" ] || continue # '-- allow-destructive: ' opts a file out, deliberately loudly. + # Read from the raw file: this opt-out is itself a comment. if grep -qi '^-- allow-destructive:' "$f"; then echo "::warning file=$f::opted out of the destructive-SQL check" continue fi - if grep -inE '\b(DROP[[:space:]]+SCHEMA|DROP[[:space:]]+DATABASE|TRUNCATE|DROP[[:space:]]+TABLE|DROP[[:space:]]+COLUMN)\b' "$f"; then + # Scan SQL, not prose. `make new-migration` scaffolds a header that + # says "CREATE INDEX CONCURRENTLY / VACUUM will not work here", and + # matching that failed every generated migration. Blanking comments + # rather than dropping the lines keeps grep -n line numbers honest. + sql=$(sed 's/--.*//' "$f") + if printf '%s\n' "$sql" | grep -inE '\b(DROP[[:space:]]+SCHEMA|DROP[[:space:]]+DATABASE|TRUNCATE|DROP[[:space:]]+TABLE|DROP[[:space:]]+COLUMN)\b'; then echo "::error file=$f::destructive statement. Migrations run against production BEFORE the new lambda code deploys, so the currently deployed code would hit the changed schema. See the expand/contract rules in apps/backend/db/README.md. Add '-- allow-destructive: ' only for a contract-phase migration whose expand phase is already live." fail=1 fi - if grep -inE 'CONCURRENTLY' "$f"; then + if printf '%s\n' "$sql" | grep -inE 'CONCURRENTLY'; then echo "::error file=$f::CONCURRENTLY cannot run inside a transaction, and the migrator wraps the run in one. This database is tiny -- use a plain CREATE INDEX." fail=1 fi - if grep -inE '^[[:space:]]*INSERT[[:space:]]+INTO[[:space:]]+(branch\.)?users\b' "$f"; then + if printf '%s\n' "$sql" | grep -inE '^[[:space:]]*INSERT[[:space:]]+INTO[[:space:]]+(branch\.)?users\b'; then echo "::warning file=$f::inserting users in a migration puts rows in PRODUCTION. Seeded users with a NULL cognito_sub are claimable by POST /auth/register, so this can hand someone an account. Dev seed rows belong in apps/backend/db/seed.sql." fi done diff --git a/apps/backend/db/migrations/20260812012951_add_expenditure_admin_notes.sql b/apps/backend/db/migrations/20260812012951_add_expenditure_admin_notes.sql new file mode 100644 index 00000000..8cf39f6a --- /dev/null +++ b/apps/backend/db/migrations/20260812012951_add_expenditure_admin_notes.sql @@ -0,0 +1,16 @@ +-- add_expenditure_admin_notes +-- +-- Every pending migration runs inside a SINGLE transaction, with +-- search_path = branch, public -- so table names can be unqualified, and +-- CREATE INDEX CONCURRENTLY / VACUUM will not work here. +-- +-- This migration is applied to PRODUCTION automatically when the PR merges, +-- BEFORE the new lambda code is deployed. It must be safe for the code that is +-- live right now: additive changes only. See apps/backend/db/README.md for the +-- expand/contract rules that destructive changes need. +-- +-- Forward-only: there is no rollback. Fix a mistake with a new migration, and +-- never edit a migration that has been merged -- someone has already run it. +-- Do not use IF NOT EXISTS: you want a failure, not silent drift. + +ALTER TABLE expenditures ADD COLUMN admin_notes TEXT; diff --git a/infrastructure/aws/README.md b/infrastructure/aws/README.md index 890a502f..d83d4dc2 100644 --- a/infrastructure/aws/README.md +++ b/infrastructure/aws/README.md @@ -49,6 +49,7 @@ No modules. | [aws_iam_role_policy.ci_plan_state_lock](https://registry.terraform.io/providers/hashicorp/aws/6.14.1/docs/resources/iam_role_policy) | resource | | [aws_iam_role_policy.ci_preview](https://registry.terraform.io/providers/hashicorp/aws/6.14.1/docs/resources/iam_role_policy) | resource | | [aws_iam_role_policy.lambda_cognito_admin](https://registry.terraform.io/providers/hashicorp/aws/6.14.1/docs/resources/iam_role_policy) | resource | +| [aws_iam_role_policy.lambda_s3_objects](https://registry.terraform.io/providers/hashicorp/aws/6.14.1/docs/resources/iam_role_policy) | resource | | [aws_iam_role_policy_attachment.ci_apply_admin](https://registry.terraform.io/providers/hashicorp/aws/6.14.1/docs/resources/iam_role_policy_attachment) | resource | | [aws_iam_role_policy_attachment.ci_plan_readonly](https://registry.terraform.io/providers/hashicorp/aws/6.14.1/docs/resources/iam_role_policy_attachment) | resource | | [aws_iam_role_policy_attachment.lambda_basic](https://registry.terraform.io/providers/hashicorp/aws/6.14.1/docs/resources/iam_role_policy_attachment) | resource | diff --git a/infrastructure/aws/lambda.tf b/infrastructure/aws/lambda.tf index 8bddf3d3..f5111f5a 100644 --- a/infrastructure/aws/lambda.tf +++ b/infrastructure/aws/lambda.tf @@ -46,6 +46,27 @@ resource "aws_iam_role_policy" "lambda_cognito_admin" { }) } +# The expenditures lambda presigns receipt uploads and downloads. A presigned +# URL carries the signer's permissions, so the role needs both PutObject and +# GetObject or the browser's PUT/GET fails AccessDenied. +resource "aws_iam_role_policy" "lambda_s3_objects" { + name = "branch-lambda-s3-objects" + role = aws_iam_role.lambda_role.id + + policy = jsonencode({ + Version = "2012-10-17" + Statement = [{ + Sid = "LambdaReportsBucketObjects" + Effect = "Allow" + Action = [ + "s3:PutObject", + "s3:GetObject", + ] + Resource = "${aws_s3_bucket.reports_bucket.arn}/*" + }] + }) +} + # Get AWS account ID for unique bucket naming data "aws_caller_identity" "current" {} diff --git a/shared/types/db-types.d.ts b/shared/types/db-types.d.ts index a1ffe4ed..00f906ae 100644 --- a/shared/types/db-types.d.ts +++ b/shared/types/db-types.d.ts @@ -30,6 +30,7 @@ export interface BranchDonors { } export interface BranchExpenditures { + admin_notes: string | null; amount: Numeric; category: string | null; created_at: Generated;