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.

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

feat(deletion): revoke fxa tokens when deleting accounts #897

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
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
12 changes: 12 additions & 0 deletions infrastructure/account-data-deleter/src/dataDeleterApp.ts
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,18 @@ export class DataDeleterApp extends Construct {
name: 'EXPORT_SIGNEDURL_USER_SECRET_KEY',
valueFrom: `arn:aws:secretsmanager:${region.name}:${caller.accountId}:secret:${config.name}/${config.environment}/EXPORT_USER_CREDS:secretAccessKey::`,
},
{
name: 'FXA_CLIENT_ID',
valueFrom: `arn:aws:ssm:${region.name}:${caller.accountId}:parameter/Web/${config.environment}/FIREFOX_WEB_AUTH_CLIENT_ID`,
},
{
name: 'FXA_CLIENT_SECRET',
valueFrom: `arn:aws:ssm:${region.name}:${caller.accountId}:parameter/Web/${config.environment}/FIREFOX_WEB_AUTH_CLIENT_SECRET`,
},
{
name: 'FXA_OAUTH_URL',
valueFrom: `arn:aws:ssm:${region.name}:${caller.accountId}:parameter/Web/${config.environment}/FIREFOX_AUTH_OAUTH_URL`,
},
],
},
],
Expand Down
1 change: 1 addition & 0 deletions lambdas/account-data-deleter-events/src/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ export const config = {
'https://account-data-deleter-api.getpocket.dev',
queueDeletePath: '/queueDelete',
stripeDeletePath: '/stripeDelete',
fxaRevokePath: '/revokeFxa',
sentry: {
// these values are inserted into the environment in
// .aws/src/.ts
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { AccountDeleteEvent } from '../../schemas/accountDeleteEvent';
import {
callQueueDeleteEndpoint,
callStripeDeleteEndpoint,
callFxARevokeEndpoint,
} from './postRequest';
import { SQSRecord } from 'aws-lambda';
import { AggregateError } from '../../errors/AggregateError';
Expand Down Expand Up @@ -47,6 +48,7 @@ export function validatePostBody(
* rows from Pocket's internal database
* * stripe API - deletes stripe customer data (and internal
* stripe-related data in database)
* * FxA Auth API - revokes access tokens from FxA (Mozilla Accounts)
* @param record the event forwarded from event bridge via SQS
* @throws Error if the record body does not conform to expected schema
* @throws AggregateError if any errors encountered making
Expand All @@ -59,10 +61,12 @@ export async function accountDeleteHandler(record: SQSRecord): Promise<void> {
const postBody = validatePostBody(message);
const queueRes = await callQueueDeleteEndpoint(postBody);
const stripeRes = await callStripeDeleteEndpoint(postBody);
const fxaRes = await callFxARevokeEndpoint(postBody);
const errors: Error[] = [];
for await (const { endpoint, res } of [
{ endpoint: 'queueDelete', res: queueRes },
{ endpoint: 'stripeDelete', res: stripeRes },
{ endpoint: 'fxaDelete', res: fxaRes },
]) {
if (!res.ok) {
const data = await res.json();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,3 +51,12 @@ export async function callQueueDeleteEndpoint(body: any): Promise<any> {
export async function callStripeDeleteEndpoint(body: any): Promise<any> {
return postRequest(body, config.stripeDeletePath, config.endpoint);
}

/**
* Revoke FxA Access Token when a user deletes their account
* @param body
* @returns
*/
export async function callFxARevokeEndpoint(body: any): Promise<any> {
return postRequest(body, config.fxaRevokePath, config.endpoint);
}
32 changes: 7 additions & 25 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions servers/account-data-deleter/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
"csv-stringify": "^6.5.1",
"express": "4.20.0",
"express-validator": "^7.1.0",
"fetch-retry": "^5.0.6",
"knex": "3.1.0",
"lodash": "4.17.21",
"mysql2": "3.11.3",
Expand Down
6 changes: 6 additions & 0 deletions servers/account-data-deleter/src/config/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,12 @@ export const config = {
apiVersion: '2024-06-20' as const,
productId: 7,
},
fxa: {
clientId: process.env.FXA_CLIENT_ID || 'somefakeclientid',
secret: process.env.FXA_CLIENT_SECRET || 'somefakesecret',
oauthEndpoint: process.env.FXA_OAUTH_URL || 'https://localhost/',
version: 'v1',
},
database: {
// contains tables for user, list, tags, annotations, etc.
read: {
Expand Down
52 changes: 52 additions & 0 deletions servers/account-data-deleter/src/dataService/FxaRevoker.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import { FxaRevoker } from './FxaRevoker';

describe('FxARevoker', () => {
afterEach(() => {
jest.restoreAllMocks();
});
describe('with an access token', () => {
beforeEach(() => {
jest
.spyOn(FxaRevoker.prototype, 'fetchAccessToken')
.mockResolvedValue('accessme123');
});
it('returns false if revoking throws an error', async () => {
jest
.spyOn(FxaRevoker.prototype, 'requestRevokeToken')
.mockRejectedValueOnce(new Error('error fetching'));
const res = await new FxaRevoker('abc123').revokeToken();
expect(res).toBeFalse();
});
it('returns false if revoking response is not ok', async () => {
jest
.spyOn(FxaRevoker.prototype, 'requestRevokeToken')
.mockResolvedValueOnce(new Response(null, { status: 500 }));
const res = await new FxaRevoker('abc123').revokeToken();
expect(res).toBeFalse();
});
it('returns false if db delete method throws error', async () => {
jest
.spyOn(FxaRevoker.prototype, 'deleteAuthRecord')
.mockRejectedValueOnce(new Error('error deleting'));
const res = await new FxaRevoker('abc123').revokeToken();
expect(res).toBeFalse();
});
it('returns true if process does not error', async () => {
jest
.spyOn(FxaRevoker.prototype, 'requestRevokeToken')
.mockResolvedValueOnce(new Response(null, { status: 200 }));
jest
.spyOn(FxaRevoker.prototype, 'deleteAuthRecord')
.mockResolvedValueOnce(1);
const res = await new FxaRevoker('abc123').revokeToken();
expect(res).toBeTrue();
});
});
it('returns true if no FxA tokens exist', async () => {
jest
.spyOn(FxaRevoker.prototype, 'fetchAccessToken')
.mockResolvedValueOnce(undefined);
const res = await new FxaRevoker('abc123').revokeToken();
expect(res).toBeTrue();
});
});
109 changes: 109 additions & 0 deletions servers/account-data-deleter/src/dataService/FxaRevoker.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
import { Knex } from 'knex';
import { readClient, writeClient } from './clients';
import * as Sentry from '@sentry/node';
import { serverLogger } from '@pocket-tools/ts-logger';
import fetchRetry from 'fetch-retry';
import { config } from '../config';
const newFetch = fetchRetry(fetch);

export class FxaRevoker {
private write: Knex;
private read: Knex;
constructor(private readonly userId: string) {
this.write = writeClient();
this.read = readClient();
}

/**
* Get the current FxA access token for a user, to be
* revoked during the account deletion process. We delete
* all records of auth creds in the account deletion process.
* This will notify Mozilla Accounts to remove Pocket from
* the integrated apps, so it no longer shows up on the
* account overview page.
*/
async fetchAccessToken(): Promise<string | undefined> {
const token = await this.read('user_firefox_account')
.where({
user_id: this.userId,
})
.pluck('firefox_access_token');
// The length assertion is technically unnecessary since
// user_id is the PK on this table
if (token != null && token.length === 1) {
return token[0];
} else {
return undefined;
}
}

/**
* Revoke an FxA access token, removing Pocket from integrated
* apps in Mozilla account, and delete the database record
* associated with Mozilla auth.
* @returns true if successful, false otherwise
*/
async revokeToken(): Promise<boolean> {
try {
const token = await this.fetchAccessToken();
// No FxA account data exists (old, unmigrated accounts)
if (token == null) {
return true;
} else {
const res = await this.requestRevokeToken(token);
if (!res.ok) {
throw new Error(
`Failed to revoke FxA access token [${res.status} - ${res.statusText}]`,
);
}
await this.deleteAuthRecord();
return true;
}
} catch (error) {
serverLogger.error({
message: 'Failed to revoke and/or delete FxA access token',
errorData: error,
errorMessage: error.message,
userId: this.userId,
});
Sentry.captureException(error);
return false;
}
}

/**
* Make a request to the oauth endpoint to revoke FxA access token
* @param token the token to revoke
* @returns
*/
async requestRevokeToken(token: string): Promise<Response> {
const body = {
client_id: config.fxa.clientId,
client_secret: config.fxa.secret,
token,
token_type_hint: 'access_token',
};
const fetchPath = `${config.fxa.oauthEndpoint}${config.fxa.version}/oauth/destroy`;
return newFetch(fetchPath, {
retryOn: [500, 502, 503],
retryDelay: (attempt, error, response) => {
return Math.pow(2, attempt) * 500;
},
retries: 3,
method: 'post',
headers: {
'Content-Type': 'application/json',
},
body: JSON.stringify(body),
});
}

/**
* Delete the auth record in Pocket's DB
*/
async deleteAuthRecord(): Promise<number> {
return await this.write('user_firefox_account')
.where({ user_id: this.userId })
.del();
}
}
33 changes: 33 additions & 0 deletions servers/account-data-deleter/src/routes/revokeFxa.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import { Request, Response, Router } from 'express';
import { checkSchema } from 'express-validator';
import { validate } from './validator';
import { nanoid } from 'nanoid';
import { accountDeleteSchema } from './schemas';
import { FxaRevoker } from '../dataService/FxaRevoker';

const router = Router();

/**
* This endpoint is called by the lambda that consumes account delete
* events forwarded from the event bus. When a user deletes their account,
* revoke any FxA access tokens and delete all FxA records in Pocket.
*/
router.post(
'/',
checkSchema(accountDeleteSchema),
validate,
async (req: Request, res: Response) => {
const requestId = req.body.traceId ?? nanoid();
new FxaRevoker(req.body.userId).revokeToken();
// Error handling and logging is done asynchronously; just respond OK
// if we received the message and kicked off the work
return res.send({
status: 'OK',
message: `received message body ${JSON.stringify(
req.body,
)} (requestId='${requestId}')`,
});
},
);

export default router;
1 change: 1 addition & 0 deletions servers/account-data-deleter/src/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ export async function startServer(port: number): Promise<{
});
app.use('/queueDelete', queueDeleteRouter);
app.use('/stripeDelete', stripeDeleteRouter);
app.use('/revokeFxa', revokeFxaRouter);

Sentry.setupExpressErrorHandler(app);

Expand Down
6 changes: 5 additions & 1 deletion servers/user-api/src/config/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ const tables = {
tablesWithSensitivePii: [
'users',
'user_google_account',
'user_firefox_account',
'user_twitter_auth',
'users_social_tokens',
'users_tokens',
Expand Down Expand Up @@ -72,6 +71,11 @@ export default {
user_id: [...tables.tablesWithSensitivePii],
created_by_user_id: ['channels'],
},
// For test seeds
userPIITestSeedTables: {
user_id: [...tables.tablesWithSensitivePii, 'user_firefox_account'],
created_by_user_id: ['channels'],
},
},
sentry: {
dsn: process.env.SENTRY_DSN || '',
Expand Down
Loading
Loading