Skip to content
Open
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
2 changes: 2 additions & 0 deletions locales/en/settings.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ settings-delete-monitor-free-account-dialog-cta-label = Delete account
settings-delete-monitor-free-account-dialog-cancel-button-label = Never mind, take me back
settings-delete-monitor-account-confirmation-toast-label-2 = Your { -brand-monitor } account is now deleted.
settings-delete-monitor-account-confirmation-toast-dismiss-label = Dismiss
settings-delete-account-recent-auth-required = For your safety, please sign in again (within the last hour) before deleting your account.
settings-delete-account-error-generic = Something went wrong while trying to delete your account. Try again.
Comment on lines +61 to +62
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wordsmithing isn't my expertise, but "within the last hour" is confusing to me for something the user still has to do.

Suggested change
settings-delete-account-recent-auth-required = For your safety, please sign in again (within the last hour) before deleting your account.
settings-delete-account-error-generic = Something went wrong while trying to delete your account. Try again.
settings-delete-account-recent-auth-required = For your safety, please sign in again before deleting your account.
settings-delete-account-error-generic = Something went wrong while trying to delete your account. Please try again.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it mean "log in again within the next hour", or you "must have done the log in within the last hour"?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think that the timing is particularly helpful to the user here. Why not just say, "For your safety, please sign in again before deleting your account." ?


## Monthly Monitor Report

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { signOut } from "next-auth/react";
import { Button } from "../../../../../../components/client/Button";
import { type onDeleteAccount } from "./actions";
import { useTelemetry } from "../../../../../../hooks/useTelemetry";
import { useL10n } from "../../../../../../hooks/l10n";

/**
* Since <Button> is a client component, the `onDeleteAccount` handler can only
Expand All @@ -21,29 +22,57 @@ export const DeleteAccountButton = (
onDeleteAccount: typeof onDeleteAccount;
},
) => {
const l10n = useL10n();
const recordTelemetry = useTelemetry();
const [isSubmitting, setIsSubmitting] = useState(false);
const [errorMessage, setErrorMessage] = useState<string | null>(null);
return (
<Button
{...props}
isLoading={isSubmitting}
isDisabled={isSubmitting}
onPress={() => {
recordTelemetry("ctaButton", "click", {
button_id: "confirm_delete_account",
});
setIsSubmitting(true);
// It's currently unclear if and how we should mock our server action:
/* c8 ignore next 8 */
void props
.onDeleteAccount()
.then(() => {
void signOut({ callbackUrl: "/" });
})
.catch(() => {
setIsSubmitting(false);
<>
<Button
{...props}
isLoading={isSubmitting}
isDisabled={isSubmitting}
onPress={() => {
recordTelemetry("ctaButton", "click", {
button_id: "confirm_delete_account",
});
}}
/>
setIsSubmitting(true);
setErrorMessage(null);
// It's currently unclear if and how we should mock our server action:
/* c8 ignore next 13 */
void props
.onDeleteAccount()
.then((result) => {
if (result && typeof result === "object" && "success" in result) {
if (result.success === false) {
setIsSubmitting(false);
setErrorMessage(
result.errorMessage ??
l10n.getString("settings-delete-account-error-generic"),
);
return;
}
}
void signOut({ callbackUrl: "/" });
})
.catch(() => {
setIsSubmitting(false);
setErrorMessage(
l10n.getString("settings-delete-account-error-generic"),
);
});
}}
/>
{errorMessage && (
<p
role="status"
style={{
marginTop: "0.5rem",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably doesn't matter for such a small and rarely visible surface, but we tend to avoid margins in CSS because they hurt the composability of individual elements. Instead, we try to be consistent in having layout "flow down", e.g. having a flexbox parent container with a gap.

Again, probably not a biggie for this specific code, but good to know.

}}
>
{errorMessage}
</p>
)}
</>
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ import { type NormalizedProfileData } from "./panels/SettingsPanelEditProfile/Ed
import { OnerepUsPhoneNumber } from "../../../../../../functions/server/onerep";
import { getEnabledFeatureFlags } from "../../../../../../../db/tables/featureFlags";

const RECENT_AUTH_WINDOW_MS = 60 * 60 * 1000;

export type AddEmailFormState =
| { success?: never }
| { success: true; submittedAddress: string }
Expand Down Expand Up @@ -192,6 +194,17 @@ export async function onRemoveEmail(email: SanitizedEmailAddressRow) {
}
}

const createRecentAuthErrorResponse = async () => {
const l10n = getL10n(await getAcceptLangHeaderInServerComponents());
return {
success: false,
error: "delete-account-auth-too-old",
errorMessage: l10n.getString(
"settings-delete-account-recent-auth-required",
),
} as const;
};

export async function onDeleteAccount() {
const session = await getServerSession();
if (!session?.user.subscriber?.fxa_uid) {
Expand All @@ -203,6 +216,22 @@ export async function onDeleteAccount() {
};
}

const authenticatedAt = session.authenticatedAt
? Date.parse(session.authenticatedAt)
: NaN;
if (!Number.isFinite(authenticatedAt)) {
logger.warn("delete-account-missing-authenticated-at", {
subscriber_id: session.user.subscriber.id,
});
return createRecentAuthErrorResponse();
}
if (Date.now() - authenticatedAt > RECENT_AUTH_WINDOW_MS) {
logger.warn("delete-account-authenticated-at-too-old", {
subscriber_id: session.user.subscriber.id,
});
return createRecentAuthErrorResponse();
}

const subscriber = await getSubscriberByFxaUid(
session.user.subscriber.fxa_uid,
);
Expand All @@ -229,6 +258,7 @@ export async function onDeleteAccount() {
// possibile, so instead the sign-out and redirect needs to happen on the
// client side after this action completes.
// See https://github.com/nextauthjs/next-auth/discussions/5334.
return { success: true } as const;
}

export async function onHandleUpdateProfileData(
Expand Down
6 changes: 6 additions & 0 deletions src/app/api/utils/auth.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,9 @@ export const authOptions: AuthOptions = {
},
// Unused arguments also listed to show what's available:
async jwt({ token, account, profile, trigger }) {
if (account) {
token.authenticatedAt = Date.now();
}
if (trigger === "update") {
// Refresh the user data from FxA, in case e.g. new subscriptions got added:
const subscriberFromDb = await getSubscriberByFxaUid(
Expand Down Expand Up @@ -298,6 +301,9 @@ export const authOptions: AuthOptions = {
}
}
}
if (token.authenticatedAt) {
session.authenticatedAt = new Date(token.authenticatedAt).toISOString();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't it safer to just keep the UNIX timestamp here? I don't think we need to preserve the timezone for this use case, so it would be good to be able to be able to avoid date string parsing.

}

return session;
},
Expand Down
2 changes: 2 additions & 0 deletions src/next-auth.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ declare module "next-auth" {
/** Session data available after deserialising the JWT */
interface Session {
error?: "RefreshAccessTokenError";
authenticatedAt?: ISO8601DateString;
Copy link
Collaborator

Choose a reason for hiding this comment

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

As above, wouldn't it make sense to use a UNIX timestamp here as well?

user: {
fxa?: {
/** The value of the Accept-Language header when the user signed up for their Firefox Account */
Expand Down Expand Up @@ -76,5 +77,6 @@ declare module "next-auth/jwt" {
subscriptions: Array<string>;
};
subscriber?: SerializedSubscriber;
authenticatedAt?: number;
}
}
Loading