Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove AppConstants #5044

Merged
merged 1 commit into from
Sep 16, 2024
Merged

Remove AppConstants #5044

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 0 additions & 2 deletions .prettierignore
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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([
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that's pretty cool, nice idea. I didn't like how non-standard AppConstants was but I really like the behavior of throwing upfront.

"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
View file
Open in desktop
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
View file
Open in desktop
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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious why you don't use the getEnvVarsOrThrow function here? Seems like you could avoid having to do the ?? "/", is this fallback ever desirable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good question, and I was (still am) on the fence. My reasoning was as follows: AppConstants only logged an error without throwing, so if this variable wouldn't be set, this would just result in a log and a call to NextResponse.redirect(undefined, 301). So if I were to use that function instead, maybe I would be causing API routes that used to succeed but return a malformed response, to start failing upfront now. Not in practice, because the variable should be set, I think - but I might be thinking wrong.

It looks like .redirect supports relative URLs, so judging by the comment above this line, this fallback is the desired behaviour. Though given that this is a POST request handler, I don't see why that makes sense - so presumably, we're never hitting this.

}
}
3 changes: 1 addition & 2 deletions src/app/api/v1/user/remove-email/route.ts
View file
Open in desktop
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",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question re: getEnvVarsOrThrow vs. process.env

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same reasoning as #5044 (comment) - in this case, if it's unset, it would fall back to redirecting to /user/settings anyway, I think.

301,
);
} catch (e) {
Expand Down
72 changes: 0 additions & 72 deletions src/appConstants.js
View file
Open in desktop

This file was deleted.

6 changes: 4 additions & 2 deletions src/db/tables/subscribers.ts
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like you could use getEnvVarsOrThrow and catch it + process.exit (I think throwing from here would the process to exit anyway but being explicit is probably better just in case it's refactored later)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was actually considering that too, but then I figured I'd have to wrap the entire file in a try {} block, which... Isn't that big of a deal, but felt a little bit icky, so I stuck with mostly matching the existing code. I hadn't realised yet that throwing would cause an exit anyway, so that could actually be a good compromise too. What do you think?

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
View file
Open in desktop
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
View file
Open in desktop
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
Loading