feat(apple): Add apple user migrations script #15406

vbudhram · 2023-06-02T15:09:40Z

Because

Ops team needs the ability to run migration script and import users into FxA from Pocket

This pull request

Adds script

Issue that this pull request solves

Closes: https://mozilla-hub.atlassian.net/browse/FXA-7490

Checklist

My commit is GPG signed.
If applicable, I have modified or added tests which pass locally.
I have added necessary documentation (if appropriate).
I have verified that my changes render correctly in RTL (if appropriate).

Other information (Optional)

I need to find a better way to test this script. Since it is making network calls from another Node process, I can't use nock. I'm putting this up for initial review, it is still missing a few things

Better test coverage
Write csv file with status of imigration
Emit a new notification when user is migrated

vbudhram · 2023-06-05T21:12:01Z

packages/fxa-auth-server/scripts/apple-transfer-users/apple-transfer-users.js

+      this.appleUserInfo = res.data;
+      return res.data;
+    } catch (err) {
+      this.setFailure(err);


I took a look at the retry later ability and kinda went down a rabbit hole. Instead, we mark the user as failed and can reattempt later.

vbudhram · 2023-06-05T21:12:40Z

packages/fxa-auth-server/scripts/apple-transfer-users/apple-transfer-users.js

+
+  async createLinkedAccount(accountRecord, sub) {
+    // If the user already has a linked account, delete it and create a new one.
+    await this.db.deleteLinkedAccount(accountRecord.uid, APPLE_PROVIDER);


There shouldn't be any harm in deleting a linked third party auth account. The feature is not broadcasted so no real users should have this set.

packages/fxa-auth-server/test/scripts/apple-transfer-users.js

+
+  it('should exchange identifiers correctly and update user', async function() {
+    const stub = sandbox.stub(axios, 'post').resolves({ data: { sub: 'sub', email: 'email@example.com', is_private_email: true } });
+    const data = await user.exchangeIdentifiers('accessToken');


packages/fxa-auth-server/test/scripts/apple-transfer-users.js

+  it('should transfer user correctly', async function() {
+    sandbox.stub(user, 'exchangeIdentifiers').resolves();
+    sandbox.stub(user, 'createUpdateFxAUser').resolves();
+    await user.transferUser('accessToken');


pdehaan · 2023-06-06T23:12:31Z

packages/fxa-auth-server/scripts/apple-transfer-users.js

+  .option('-d, --delimiter [delimiter]', 'Delimiter for input file', ',')
+  .option('-o, --output <filename>', 'Output filename to save results to')


Q: Does the program.delimiter and program.output get used?

pdehaan · 2023-06-06T23:17:54Z

packages/fxa-auth-server/scripts/apple-transfer-users/apple-transfer-users.js

+      return input.split(/\n/).map((s, index) => {
+        if (index === 0) return;
+
+        const delimiter = program.delimiter || ',';


This was the only other instance of program i found in this file:

fxa/packages/fxa-auth-server/scripts/apple-transfer-users/apple-transfer-users.js

Line 15 in ea9bb53

const program = require('commander');

Should this be using the unused(?) delimiter from packages/fxa-auth-server/scripts/apple-transfer-users.js#L7 above?

pdehaan · 2023-06-06T23:34:32Z

packages/fxa-auth-server/scripts/apple-transfer-users/apple-transfer-users.js

+      const err = (user.err && user.err.message) || '';
+      return `${transferSub},${uid},${fxaEmail},${appleEmail},${success},${err}`;
+    }).join('\n');
+    fs.writeFileSync(path.resolve(`${Date.now()}_output.csv`), output);


Q: Should this accept an output filename?
packages/fxa-auth-server/scripts/apple-transfer-users.js#L20 specifies an output filename in the commander config, but I don't think it's used currently.

pdehaan · 2023-06-06T23:36:38Z

packages/fxa-auth-server/scripts/apple-transfer-users/apple-transfer-users.js

+      const transferSub = user.transferSub;
+      const success = user.success;
+      const err = (user.err && user.err.message) || '';
+      return `${transferSub},${uid},${fxaEmail},${appleEmail},${success},${err}`;


Q: Would there be any potential issues if the ${err} string had a comma (thus throwing off column counts, etc)?

pdehaan · 2023-06-06T23:52:01Z

packages/fxa-auth-server/scripts/apple-transfer-users/apple-transfer-users.js

+    const tokenRes = await axios.post(config.appleAuthConfig.tokenEndpoint,
+      new URLSearchParams(tokenOptions).toString(),
+    );


Q: does this need a try..catch in the unlikely event of a server/fetch error?

pdehaan · 2023-06-07T00:20:10Z

packages/fxa-auth-server/scripts/apple-transfer-users.js

+  const migration = new ApplePocketFxAMigration(program.input);
+
+  await migration.load();
+  await migration.printUsers();


I don't think printUsers() is an async method:

fxa/packages/fxa-auth-server/scripts/apple-transfer-users/apple-transfer-users.js

Lines 176 to 178 in ea9bb53

printUsers() {

console.table(this.users);

}

pdehaan · 2023-06-07T00:22:19Z

packages/fxa-auth-server/scripts/apple-transfer-users.js

+  await migration.load();
+  await migration.printUsers();
+  await migration.transferUsers();
+  await migration.saveResults();


Same w/ non-async saveResults():

fxa/packages/fxa-auth-server/scripts/apple-transfer-users/apple-transfer-users.js

Lines 180 to 181 in ea9bb53

saveResults() {

const output = this.users.map((user) => {

dschom

Just a couple nits. Would be good to address the comments in saveResults.

dschom · 2023-06-06T22:06:49Z

packages/fxa-auth-server/scripts/apple-transfer-users/apple-transfer-users.js

@@ -0,0 +1,259 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public


Nit: Might be nice to just call this /transfer-users/apple.js. That way if there are other types of user transfers in the future, we can just add more scripts here.

dschom · 2023-06-06T22:07:40Z

packages/fxa-auth-server/test/scripts/apple-transfer-users.js

+    AUTH_FIRESTORE_EMULATOR_HOST: 'localhost:9090',
+  },
+};
+const axios = require('axios');


Nit: Any reason to axios instead of fetch?

dschom · 2023-06-06T22:09:41Z

packages/fxa-auth-server/scripts/apple-transfer-users/apple-transfer-users.js

+      return res.data;
+    } catch (err) {
+      this.setFailure(err);
+      if (err.response && err.response.status === 429) {


Perhaps we want to add a delay option, so we can throttle?

dschom · 2023-06-06T22:10:00Z

packages/fxa-auth-server/scripts/apple-transfer-users/apple-transfer-users.js

+
+  // Exchanges the Apple `transfer_sub` for the user's profile information and
+  // moves the user to the new team.
+  // Ref: https://developer.apple.com/documentation/sign_in_with_apple/bringing_new_apps_and_users_into_your_team#3559300


dschom · 2023-06-06T22:11:15Z

packages/fxa-auth-server/scripts/apple-transfer-users.js

+
+program
+  .option('-d, --delimiter [delimiter]', 'Delimiter for input file', ',')
+  .option('-o, --output <filename>', 'Output filename to save results to')


Should this have a default value?

dschom · 2023-06-06T22:12:37Z

packages/fxa-auth-server/scripts/apple-transfer-users/apple-transfer-users.js

+      const err = (user.err && user.err.message) || '';
+      return `${transferSub},${uid},${fxaEmail},${appleEmail},${success},${err}`;
+    }).join('\n');
+    fs.writeFileSync(path.resolve(`${Date.now()}_output.csv`), output);


Should this be program.output?

dschom · 2023-06-07T06:27:54Z

packages/fxa-auth-server/scripts/apple-transfer-users/apple-transfer-users.js

+  }
+
+  saveResults() {
+    const output = this.users.map((user) => {


This could be a really big string. Perhaps write the file line by line?

vbudhram · 2023-06-07T16:03:04Z

@dschom @pdehaan Thanks for reviews, I cleaned up the code a bit and updated it to stream results to a file.

vbudhram marked this pull request as ready for review June 5, 2023 21:10

vbudhram requested a review from a team as a code owner June 5, 2023 21:10

vbudhram requested a review from dschom June 5, 2023 21:10

vbudhram commented Jun 5, 2023

View reviewed changes

github-advanced-security bot found potential problems Jun 5, 2023

View reviewed changes

pdehaan reviewed Jun 7, 2023

View reviewed changes

dschom approved these changes Jun 7, 2023

View reviewed changes

feat(apple): Add apple user migrations script

68b8504

vbudhram force-pushed the fxa-7490 branch from ea9bb53 to 68b8504 Compare June 7, 2023 15:48

vbudhram merged commit d714f5b into main Jun 7, 2023

vbudhram deleted the fxa-7490 branch June 7, 2023 16:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(apple): Add apple user migrations script #15406

feat(apple): Add apple user migrations script #15406

vbudhram commented Jun 2, 2023

vbudhram Jun 5, 2023

vbudhram Jun 5, 2023 •

edited

Loading

pdehaan Jun 6, 2023

pdehaan Jun 6, 2023

pdehaan Jun 6, 2023

pdehaan Jun 6, 2023

pdehaan Jun 6, 2023

pdehaan Jun 7, 2023

pdehaan Jun 7, 2023

dschom left a comment •

edited

Loading

dschom Jun 6, 2023

dschom Jun 6, 2023

dschom Jun 6, 2023

dschom Jun 6, 2023

dschom Jun 6, 2023

dschom Jun 6, 2023

dschom Jun 7, 2023

vbudhram commented Jun 7, 2023

		.option('-d, --delimiter [delimiter]', 'Delimiter for input file', ',')
		.option('-o, --output <filename>', 'Output filename to save results to')

		@@ -0,0 +1,259 @@
		/* This Source Code Form is subject to the terms of the Mozilla Public

feat(apple): Add apple user migrations script #15406

feat(apple): Add apple user migrations script #15406

Conversation

vbudhram commented Jun 2, 2023

Because

This pull request

Issue that this pull request solves

Checklist

Other information (Optional)

Choose a reason for hiding this comment

vbudhram Jun 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dschom left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbudhram commented Jun 7, 2023

vbudhram Jun 5, 2023 •

edited

Loading

dschom left a comment •

edited

Loading