Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 16 additions & 6 deletions .github/workflows/preview-env.yml
Original file line number Diff line number Diff line change
Expand Up @@ -115,19 +115,29 @@ jobs:
exit 1
fi

# Build the preview lambdas' env from the ACTUAL prod config (single source
# of truth) + the live RDS endpoint. Reserved / credential keys are dropped;
# the module adds NODE_ENV. Only needed when creating the stack.
# Prod config is the single source of truth, DB_HOST included. Never re-derive
# it from `DBInstances[0]` -- this account hosts other C4C databases and that
# picked an unreachable one. Reserved / credential keys are dropped; the module
# adds NODE_ENV. Only needed when creating the stack.
- name: Resolve preview lambda env
if: steps.mode.outputs.create == 'true'
run: |
DB_HOST=$(aws rds describe-db-instances --query "DBInstances[0].Endpoint.Address" --output text)
set -euo pipefail
AUTH_ENV=$(aws lambda get-function-configuration --function-name branch-auth --query 'Environment.Variables' --output json)
REPORTS_ENV=$(aws lambda get-function-configuration --function-name branch-reports --query 'Environment.Variables' --output json)
ENV_JSON=$(jq -n --argjson a "$AUTH_ENV" --argjson r "$REPORTS_ENV" --arg dbh "$DB_HOST" '
(($a // {}) + ($r // {}) + { "DB_HOST": $dbh })
ENV_JSON=$(jq -n --argjson a "$AUTH_ENV" --argjson r "$REPORTS_ENV" '
(($a // {}) + ($r // {}))
| del(.NODE_ENV, .AWS_REGION, .AWS_DEFAULT_REGION, .AWS_ACCESS_KEY_ID, .AWS_SECRET_ACCESS_KEY, .AWS_SESSION_TOKEN)
')
# Fail loudly rather than shipping a preview that 401s on every DB call.
for key in DB_HOST DB_NAME DB_USER DB_PASSWORD COGNITO_USER_POOL_ID COGNITO_CLIENT_ID; do
value=$(jq -r --arg k "$key" '.[$k] // ""' <<<"$ENV_JSON")
if [ -z "$value" ]; then
echo "::error::branch-auth/branch-reports did not provide $key; preview lambdas would be misconfigured."
exit 1
fi
done
echo "Preview DB_HOST: $(jq -r '.DB_HOST' <<<"$ENV_JSON")"
# Stash for the terraform step (multiline-safe).
printf 'LAMBDA_ENV<<EOF\n%s\nEOF\n' "$ENV_JSON" >> "$GITHUB_ENV"

Expand Down
6 changes: 3 additions & 3 deletions infrastructure/preview/variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@ variable "pr_number" {
}

# Full runtime environment for the preview lambdas, resolved by the workflow
# from the ACTUAL prod lambda config (branch-auth + branch-reports) plus the
# shared RDS endpoint. Passing it in keeps prod as the single source of truth
# for DB creds / Cognito ids / reports bucket rather than duplicating them here.
# from the ACTUAL prod lambda config (branch-auth + branch-reports). Passing it in
# keeps prod as the single source of truth for DB_HOST / DB creds / Cognito ids /
# reports bucket rather than duplicating -- or re-deriving -- them here.
# Preview envs deliberately reuse the shared RDS + Cognito pool (data risk is
# accepted); migrations are NEVER run from this module -- the generated DB types
# hardcode the `branch.` schema prefix, so a per-PR schema would require per-PR
Expand Down
67 changes: 37 additions & 30 deletions shared/lambda-auth/src/authenticate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,39 +49,46 @@ export async function authenticateRequest(
const token = extractToken(event);
if (!token) return { isAuthenticated: false };

try {
const payload = await getVerifier().verify(token);

const dbUser = await db
.selectFrom('branch.users')
.where('cognito_sub', '=', payload.sub)
.selectAll()
.executeTakeFirst();

if (!dbUser) {
console.warn(
'User authenticated with Cognito but not found in database:',
payload.sub,
);
return { isAuthenticated: false };
}

const user: AuthenticatedUser = {
cognitoSub: payload.sub,
userId: dbUser.user_id,
email: payload.email as string | undefined,
isAdmin: dbUser.is_admin === true,
// Informational only. We deliberately do NOT promote on a Cognito
// "Admins" group: branch.users.is_admin is the single source of truth.
// A second source would make demotion via PATCH /users/{userId} silently
// ineffective, nothing in this codebase writes group membership, and no
// aws_cognito_user_group is defined in infrastructure/aws/cognito.tf.
cognitoGroups: payload['cognito:groups'] as string[] | undefined,
};
// Outside the try: missing config is a broken deployment, not a bad token.
const jwtVerifier = getVerifier();

return { user, isAuthenticated: true };
let payload: any;
try {
payload = await jwtVerifier.verify(token);
} catch (error) {
// Only an unverifiable token is genuinely unauthenticated.
console.error('Token verification failed:', error);
return { isAuthenticated: false };
}

// Uncaught on purpose: a DB outage is not a 401. Catching it hid an unreachable
// RDS behind "Authentication required" and logged users out. Handlers map to 500.
const dbUser = await db
.selectFrom('branch.users')
.where('cognito_sub', '=', payload.sub)
.selectAll()
.executeTakeFirst();

if (!dbUser) {
console.warn(
'User authenticated with Cognito but not found in database:',
payload.sub,
);
return { isAuthenticated: false };
}

const user: AuthenticatedUser = {
cognitoSub: payload.sub,
userId: dbUser.user_id,
email: payload.email as string | undefined,
isAdmin: dbUser.is_admin === true,
// Informational only. We deliberately do NOT promote on a Cognito
// "Admins" group: branch.users.is_admin is the single source of truth.
// A second source would make demotion via PATCH /users/{userId} silently
// ineffective, nothing in this codebase writes group membership, and no
// aws_cognito_user_group is defined in infrastructure/aws/cognito.tf.
cognitoGroups: payload['cognito:groups'] as string[] | undefined,
};

return { user, isAuthenticated: true };
}
24 changes: 13 additions & 11 deletions shared/lambda-auth/test/authenticate.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -197,34 +197,36 @@ describe('authenticateRequest', () => {
expect(mockCreate).toHaveBeenCalledWith(expect.objectContaining({ clientId: null }));
});

it('degrades to unauthenticated (not a throw) when COGNITO_USER_POOL_ID is unset', async () => {
// This is why a missing env var manifests as blanket silent 401s across all
// six lambdas rather than a loud 500: getVerifier() throws inside the try.
it('throws (not a silent 401) when COGNITO_USER_POOL_ID is unset', async () => {
// Swallowing this gave blanket silent 401s across all six lambdas.
delete process.env.COGNITO_USER_POOL_ID;
const { authenticateRequest } = await loadModule();
const { db } = makeDb({ user_id: 7, is_admin: true });

await expect(authenticateRequest(db, bearerEvent('good'))).resolves.toEqual({
isAuthenticated: false,
});
await expect(authenticateRequest(db, bearerEvent('good'))).rejects.toThrow(
'COGNITO_USER_POOL_ID',
);
expect(mockCreate).not.toHaveBeenCalled();
});

it('returns unauthenticated when the database query throws', async () => {
it('propagates a database failure instead of reporting it as unauthenticated', async () => {
// Regression guard for the preview-env outage (PR #316).
mockVerify.mockResolvedValue({ sub: 'sub-1' });
const { authenticateRequest } = await loadModule();
const db = {
selectFrom: () => ({
where: () => ({
selectAll: () => ({
executeTakeFirst: jest.fn().mockRejectedValue(new Error('db down')),
executeTakeFirst: jest
.fn()
.mockRejectedValue(new Error('timeout exceeded when trying to connect')),
}),
}),
}),
};

await expect(authenticateRequest(db, bearerEvent('good'))).resolves.toEqual({
isAuthenticated: false,
});
await expect(authenticateRequest(db, bearerEvent('good'))).rejects.toThrow(
'timeout exceeded when trying to connect',
);
});
});
Loading