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

Merged
merged 3 commits into from
Jan 29, 2025
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
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 @@ -17,6 +17,10 @@ describe('accountDelete handler', () => {
}),
};

beforeEach(() => {
nock(config.endpoint).post(config.fxaRevokePath).reply(200);
});

afterEach(() => {
nock.cleanAll();
jest.restoreAllMocks();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { AccountDeleteEvent } from '../../schemas/accountDeleteEvent.ts';
import {
callQueueDeleteEndpoint,
callStripeDeleteEndpoint,
callFxARevokeEndpoint,
} from './postRequest.ts';
import { SQSRecord } from 'aws-lambda';
import { AggregateError } from '../../errors/AggregateError.ts';
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);
}
47 changes: 25 additions & 22 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 @@ -40,6 +40,7 @@
"csv-stringify": "^6.5.1",
"express": "4.21.2",
"express-validator": "^7.1.0",
"fetch-retry": "^5.0.6",
"knex": "3.1.0",
"lodash": "4.17.21",
"mysql2": "3.12.0",
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();
});
});
Loading
Loading