Skip to content

Commit

Permalink
Remove AppConstants
Browse files Browse the repository at this point in the history
To make the migration as safe as possible, in places where it was
used to access environment variables that were absolutely required,
I replaced it with a function that forces the caller to explicitly
enumerate the environment variables they specifically expect,
allows them to access them in a type-safe way, and throws if the
named environment variables aren't set. (This is slightly different
than the previous behaviour, where AppConstants would only log the
missing variable without throwing, but I checked that all these
variables should be set in production.)
  • Loading branch information
Vinnl committed Sep 16, 2024
1 parent 3e66114 commit cda0f5a
Show file tree
Hide file tree
Showing 15 changed files with 115 additions and 136 deletions.
2 changes: 0 additions & 2 deletions .prettierignore
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@ src/db/**/*.js
src/emails/**/*.js

# TODO NEXT.JS MIGRATION:
# These files are remnants of our Express app:
src/appConstants.js

# These should be ignored anyway
coverage/
Expand Down
2 changes: 0 additions & 2 deletions jest.config.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,6 @@ const customJestConfig = {
"<rootDir>/src/apiMocks/mockData.ts",
"<rootDir>/src/(.+).stories.(ts|tsx)",
"<rootDir>/.storybook/",
// Old, pre-Next.js code assumed to be working:
"<rootDir>/src/appConstants.js",
],

// Indicates which provider should be used to instrument code for coverage
Expand Down
8 changes: 3 additions & 5 deletions jest.setup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,13 +47,11 @@ afterEach(() => {

global.TextEncoder = TextEncoder;

// Jest doesn't like the top-level await in AppConstants, so we mock it. In
// time we can hopefully phase out the entire file and just use dotenv-flow
// and process.env directly.
jest.mock("./src/appConstants.js", () => {
// Jest doesn't like the top-level await in envVars.ts, so we mock it.
jest.mock("./src/envVars", () => {
// eslint-disable-next-line @typescript-eslint/no-var-requires
require("dotenv-flow").config();
return {
...process.env,
getEnvVarsOrThrow: () => process.env,
};
});
24 changes: 16 additions & 8 deletions src/app/api/utils/auth.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import { AuthOptions, Profile as FxaProfile, User } from "next-auth";
import { SubscriberRow } from "knex/types/tables";
import { logger } from "../../functions/server/logging";

import AppConstants from "../../../appConstants.js";
import {
getSubscriberByFxaUid,
updateFxAData,
Expand All @@ -26,6 +25,15 @@ import { SerializedSubscriber } from "../../../next-auth";
import { record } from "../../functions/server/glean";
import { renderEmail } from "../../../emails/renderEmail";
import { SignupReportEmail } from "../../../emails/templates/signupReport/SignupReportEmail";
import { getEnvVarsOrThrow } from "../../../envVars";

const envVars = getEnvVarsOrThrow([
"OAUTH_AUTHORIZATION_URI",
"OAUTH_TOKEN_URI",
"OAUTH_CLIENT_ID",
"OAUTH_CLIENT_SECRET",
"OAUTH_PROFILE_URI",
]);

const fxaProviderConfig: OAuthConfig<FxaProfile> = {
// As per https://mozilla.slack.com/archives/C4D36CAJW/p1683642497940629?thread_ts=1683642325.465929&cid=C4D36CAJW,
Expand All @@ -36,7 +44,7 @@ const fxaProviderConfig: OAuthConfig<FxaProfile> = {
name: "Mozilla accounts",
type: "oauth",
authorization: {
url: AppConstants.OAUTH_AUTHORIZATION_URI,
url: envVars.OAUTH_AUTHORIZATION_URI,
params: {
scope: "profile https://identity.mozilla.com/account/subscriptions",
access_type: "offline",
Expand All @@ -45,20 +53,20 @@ const fxaProviderConfig: OAuthConfig<FxaProfile> = {
max_age: 0,
},
},
token: AppConstants.OAUTH_TOKEN_URI,
// userinfo: AppConstants.OAUTH_PROFILE_URI,
token: envVars.OAUTH_TOKEN_URI,
// userinfo: envVars.OAUTH_PROFILE_URI,
userinfo: {
request: async (context) => {
const response = await fetch(AppConstants.OAUTH_PROFILE_URI, {
const response = await fetch(envVars.OAUTH_PROFILE_URI, {
headers: {
Authorization: `Bearer ${context.tokens.access_token ?? ""}`,
},
});
return (await response.json()) as FxaProfile;
},
},
clientId: AppConstants.OAUTH_CLIENT_ID,
clientSecret: AppConstants.OAUTH_CLIENT_SECRET,
clientId: envVars.OAUTH_CLIENT_ID,
clientSecret: envVars.OAUTH_CLIENT_SECRET,
// Parse data returned by FxA's /userinfo/
profile: (profile) => {
return convertFxaProfile(profile);
Expand Down Expand Up @@ -309,6 +317,6 @@ export function bearerToken(req: NextRequest) {
}

export function isAdmin(email: string) {
const admins = AppConstants.ADMINS?.split(",") ?? [];
const admins = (process.env.ADMINS ?? "").split(",") ?? [];
return admins.includes(email);
}
3 changes: 1 addition & 2 deletions src/app/api/v1/fxa-rp-events/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import {
} from "../../../functions/server/onerep";
import { bearerToken } from "../../utils/auth";
import { revokeOAuthTokens } from "../../../../utils/fxa";
import appConstants from "../../../../appConstants";
import { changeSubscription } from "../../../functions/server/changeSubscription";
import { deleteAccount } from "../../../functions/server/deleteAccount";
import { record } from "../../../functions/server/glean";
Expand All @@ -43,7 +42,7 @@ const MONITOR_PREMIUM_CAPABILITY = "monitor";
* @returns {Promise<Array<jwt.JwtPayload> | undefined>} keys an array of FxA JWT keys
*/
const getJwtPubKey = async () => {
const jwtKeyUri = `${appConstants.OAUTH_ACCOUNT_URI}/jwks`;
const jwtKeyUri = `${process.env.OAUTH_ACCOUNT_URI}/jwks`;
try {
const response = await fetch(jwtKeyUri, {
headers: {
Expand Down
3 changes: 1 addition & 2 deletions src/app/api/v1/user/email/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

import { getToken } from "next-auth/jwt";
import { NextRequest, NextResponse } from "next/server";
import AppConstants from "../../../../../appConstants";

import { getSubscriberByFxaUid } from "../../../../../db/tables/subscribers";
import { addSubscriberUnverifiedEmailHash } from "../../../../../db/tables/emailAddresses";
Expand Down Expand Up @@ -108,6 +107,6 @@ export async function POST(req: NextRequest) {
}
} else {
// Not Signed in, redirect to home
return NextResponse.redirect(AppConstants.SERVER_URL, 301);
return NextResponse.redirect(process.env.SERVER_URL ?? "/", 301);
}
}
3 changes: 1 addition & 2 deletions src/app/api/v1/user/remove-email/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import { getToken } from "next-auth/jwt";
import { NextRequest, NextResponse } from "next/server";

import { logger } from "../../../../functions/server/logging";
import AppConstants from "../../../../../appConstants";
import {
getSubscriberByFxaUid,
deleteResolutionsWithEmail,
Expand Down Expand Up @@ -50,7 +49,7 @@ export async function POST(req: NextRequest) {
existingEmail.email,
);
return NextResponse.redirect(
AppConstants.SERVER_URL + "/user/settings",
process.env.SERVER_URL + "/user/settings",
301,
);
} catch (e) {
Expand Down
72 changes: 0 additions & 72 deletions src/appConstants.js

This file was deleted.

6 changes: 4 additions & 2 deletions src/db/tables/subscribers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,14 @@
import type { Profile } from "next-auth";
import type { EmailAddressRow, SubscriberRow } from "knex/types/tables";
import createDbConnection from "../connect";
import AppConstants from "../../appConstants.js";
import { SerializedSubscriber } from "../../next-auth.js";
import { getFeatureFlagData } from "./featureFlags";
import { getEnvVarsOrThrow } from "../../envVars";

const knex = createDbConnection();
const { DELETE_UNVERIFIED_SUBSCRIBERS_TIMER } = AppConstants;
const { DELETE_UNVERIFIED_SUBSCRIBERS_TIMER } = getEnvVarsOrThrow([
"DELETE_UNVERIFIED_SUBSCRIBERS_TIMER",
]);
const MONITOR_PREMIUM_CAPABILITY = "monitor";

// Not covered by tests; mostly side-effects. See test-coverage.md#mock-heavy
Expand Down
29 changes: 29 additions & 0 deletions src/envVars.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

if (
typeof process.env.NEXT_RUNTIME === "undefined" &&
typeof process.env.STORYBOOK === "undefined"
) {
// Next.js already loads env vars by itself, and dotenv-flow will throw an
// error if loaded in that context (about `fs` not existing), so only load
// it if we're not running in a Next.js-context (e.g. cron jobs):
await import("dotenv-flow/config");
}

export function getEnvVarsOrThrow<EnvVarNames extends string>(
envVars: EnvVarNames[],
): Record<EnvVarNames, string> {
const envVarsRecord: Record<EnvVarNames, string> = {} as never;
for (const varName of envVars) {
const value = process.env[varName];
if (typeof value !== "string") {
throw new Error(
`Required environment variable was not set: [${varName}].`,
);
}
envVarsRecord[varName] = value;
}
return envVarsRecord;
}
36 changes: 21 additions & 15 deletions src/scripts/cronjobs/updateBreachesInRemoteSettings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,19 +33,36 @@
*
*/

import AppConstants from "../../appConstants";
import * as HIBP from "../../utils/hibp";

type RemoteSettingsBreach = Pick<
HIBP.HibpGetBreachesResponse[number],
"Name" | "Domain" | "BreachDate" | "PwnCount" | "AddedDate" | "DataClasses"
>;

const FX_REMOTE_SETTINGS_WRITER_USER =
process.env.FX_REMOTE_SETTINGS_WRITER_USER;
const FX_REMOTE_SETTINGS_WRITER_PASS =
process.env.FX_REMOTE_SETTINGS_WRITER_PASS;
const FX_REMOTE_SETTINGS_WRITER_SERVER =
process.env.FX_REMOTE_SETTINGS_WRITER_SERVER;

if (
!FX_REMOTE_SETTINGS_WRITER_USER ||
!FX_REMOTE_SETTINGS_WRITER_PASS ||
!FX_REMOTE_SETTINGS_WRITER_SERVER
) {
console.error(
"updatebreaches requires FX_REMOTE_SETTINGS_WRITER_SERVER, FX_REMOTE_SETTINGS_WRITER_USER, FX_REMOTE_SETTINGS_WRITER_PASS.",
);
process.exit(1);
}

const BREACHES_COLLECTION = "fxmonitor-breaches";
const FX_RS_COLLECTION = `${AppConstants.FX_REMOTE_SETTINGS_WRITER_SERVER}/buckets/main-workspace/collections/${BREACHES_COLLECTION}`;
const FX_RS_COLLECTION = `${FX_REMOTE_SETTINGS_WRITER_SERVER}/buckets/main-workspace/collections/${BREACHES_COLLECTION}`;
const FX_RS_RECORDS = `${FX_RS_COLLECTION}/records`;
const FX_RS_WRITER_USER = AppConstants.FX_REMOTE_SETTINGS_WRITER_USER;
const FX_RS_WRITER_PASS = AppConstants.FX_REMOTE_SETTINGS_WRITER_PASS;
const FX_RS_WRITER_USER = FX_REMOTE_SETTINGS_WRITER_USER;
const FX_RS_WRITER_PASS = FX_REMOTE_SETTINGS_WRITER_PASS;

async function whichBreachesAreNotInRemoteSettingsYet(
breaches: HIBP.HibpGetBreachesResponse,
Expand Down Expand Up @@ -90,17 +107,6 @@ async function requestReviewOnBreachesCollection() {
return response.json();
}

if (
!AppConstants.FX_REMOTE_SETTINGS_WRITER_USER ||
!AppConstants.FX_REMOTE_SETTINGS_WRITER_PASS ||
!AppConstants.FX_REMOTE_SETTINGS_WRITER_SERVER
) {
console.error(
"updatebreaches requires FX_REMOTE_SETTINGS_WRITER_SERVER, FX_REMOTE_SETTINGS_WRITER_USER, FX_REMOTE_SETTINGS_WRITER_PASS.",
);
process.exit(1);
}

(async () => {
const allHibpBreaches = await HIBP.fetchHibpBreaches();
const verifiedSiteBreaches = allHibpBreaches.filter((breach) => {
Expand Down
5 changes: 2 additions & 3 deletions src/utils/dockerflow.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@

import fs from "fs";
import path from "path";
import AppConstants from "../appConstants";
import packageJson from "../../package.json";

export type VersionData = {
Expand All @@ -23,7 +22,7 @@ if (!fs.existsSync(versionJsonPath)) {
const versionJson = {
source: packageJson.homepage,
version: packageJson.version,
NODE_ENV: AppConstants.NODE_ENV,
NODE_ENV: process.env.NODE_ENV,
};

fs.writeFileSync(
Expand All @@ -33,7 +32,7 @@ if (!fs.existsSync(versionJsonPath)) {
}

export function vers(): VersionData {
if (AppConstants.APP_ENV === "heroku") {
if (process.env.APP_ENV === "heroku") {
/* eslint-disable no-process-env */
return {
commit: process.env.HEROKU_SLUG_COMMIT!,
Expand Down
10 changes: 6 additions & 4 deletions src/utils/email.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,16 @@

import { createTransport, Transporter } from "nodemailer";

import AppConstants from "../appConstants.js";
import { SentMessageInfo } from "nodemailer/lib/smtp-transport/index.js";
import { getEnvVarsOrThrow } from "../envVars";

// The SMTP transport object. This is initialized to a nodemailer transport
// object while reading SMTP credentials, or to a dummy function in debug mode.
let gTransporter: Transporter<SentMessageInfo>;

async function initEmail(smtpUrl = AppConstants.SMTP_URL) {
const envVars = getEnvVarsOrThrow(["SMTP_URL", "EMAIL_FROM", "SES_CONFIG_SET"]);

async function initEmail(smtpUrl = envVars.SMTP_URL) {
// Allow a debug mode that will log JSON instead of sending emails.
if (!smtpUrl) {
console.info("smtpUrl-empty", {
Expand Down Expand Up @@ -42,14 +44,14 @@ async function sendEmail(
throw new Error("SMTP transport not initialized");
}

const emailFrom = AppConstants.EMAIL_FROM;
const emailFrom = envVars.EMAIL_FROM;
const mailOptions = {
from: emailFrom,
to: recipient,
subject,
html,
headers: {
"x-ses-configuration-set": AppConstants.SES_CONFIG_SET,
"x-ses-configuration-set": envVars.SES_CONFIG_SET,
},
};

Expand Down
Loading

0 comments on commit cda0f5a

Please sign in to comment.