chore(fxa-auth): Replace accountDevices call for refresh-token auth scheme

[nshirley]

Because

• The refesh-token auth scheme uses accountDevices_XX stored procedure
• And this sproc fetches all account devices to then just filter to the first one with a matching refreshTokenId

This pull request

• Creates a new dedicated flow to fetch devices by refreshTokenId
• Updates refresh-token auth scheme to use new flow
• Adds tests

Issue that this pull request solves

Closes: FXA-12692

Checklist

Put an x in the boxes that apply

• 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).

Screenshots (Optional)

Please attach the screenshots of the changes made in case of change in user interface.

Other information (Optional)

For testing this, I wanted to be a thorough as possible using batches of seed data to compare using the prior accountDevices_XX sproc to the new deviceFromRefreshTokenId_1. The main advantage the the new query is that we're offloading the filter to mysql, reducing the number of scanned and joined rows:

// refresh-token.js
    async authenticate(request, h) {
    // ...
        const devices = await db.devices(credentials.uid);

        const device = devices.filter(
          (device) => device.refreshTokenId === credentials.refreshTokenId
        )[0];
    // ...
    };

To this, I needed a way to insert a bunch of mock data in sessionTokens, refreshTokens, devices, etc. to cover a number of cases (noted below). I put all of the scripts necessary on a separate branch if you want to check it out!

Process:

• I started by defining each UID and necessary refreshTokenId for the tests as variables to prevent UNHEX() causing added overhead
• Then, both performance_schema.events_statements_summary_by_digest and performance_schema.events_statements_summary_by_program were truncated to start with a clean slate
• Each sproc was run 10 times (though a larger data set might be valuable!)
• After, I used the following query to get metrics on the 10 runs, showing run count, avg, min, max and total time etc

SELECT
    OBJECT_NAME AS 'name',
    COUNT_STAR AS 'run count',
    FORMAT_PICO_TIME(AVG_TIMER_WAIT) as 'avg time',
    FORMAT_PICO_TIME(SUM_TIMER_WAIT) as 'total time',
    FORMAT_PICO_TIME(MIN_TIMER_WAIT) as 'min time',
    FORMAT_PICO_TIME(MAX_TIMER_WAIT) as 'max time',
    SUM_ROWS_EXAMINED AS 'rows examined',
    SUM_ROWS_SENT AS 'rows sent'
FROM performance_schema.events_statements_summary_by_program
WHERE OBJECT_SCHEMA = 'fxa'
  AND (OBJECT_NAME = 'deviceFromRefreshTokenId_1' 
       OR OBJECT_NAME = 'accountDevices_17')
ORDER BY AVG_TIMER_WAIT DESC;

This shows a pretty clear picture that the new query improves execution time by about 2x. It's worth noting, most of these cases are extremes, with dozens or hundreds of devices, but even with low device/session accounts we're still seeing an improvement since there is no join to the sessionToken table necessary here.

Results:

• UID: 33333333333333333333333333333333 - account with 100 devices, each with refresh token but no sessionTokens

| name                        | run count  | avg time  | total time  | min time  | max time  | rows examined  | rows sent  |
|-----------------------------|------------|-----------|-------------|-----------|-----------|----------------|------------|
| accountDevices_17           | 10         | 1.26 ms   | 12.63 ms    | 832.62 µs | 1.92 ms   | 1000           | 1000       |
| deviceFromRefreshTokenId_1  | 10         | 532.70 µs | 5.33 ms     | 375.21 µs | 823.46 µs | 0              | 10         |

• UID: 55555555555555555555555555555555 - 50 devices with both sessionTokenId and refreshTokenIds, requiring join for all before sorting/filtering

| name                        | run count  | avg time  | total time  | min time  | max time  | rows examined  | rows sent  |
|-----------------------------|------------|-----------|-------------|-----------|-----------|----------------|------------|
| accountDevices_17           | 10         | 1.43 ms   | 14.32 ms    | 890.96 µs | 1.81 ms   | 1000           | 500        |
| deviceFromRefreshTokenId_1  | 10         | 668.89 µs | 6.69 ms     | 362.58 µs | 1.45 ms   | 0              | 10         |

• UID: 11111111111111111111111111111121 - HUGE number of devices, 500 all with refresh tokens and sessionTokens

| name                        | run count  | avg time  | total time  | min time  | max time  | rows examined  | rows sent  |
|-----------------------------|------------|------------|-------------|-----------|------------|----------------|------------|
| accountDevices_17           | 10         | 5.84 ms    | 58.43 ms    | 1.94 ms   | 10.24 ms   | 10000          | 5000       |
| deviceFromRefreshTokenId_1  | 10         | 477.84 µs  | 4.78 ms     | 380.88 µs | 591.67 µs  | 0              | 10         |

• UID: 99999999999999999999999999999999 - devices with a LOT of device commands (not very pratical, but demonstrates overhead when joining and fan-out from joins) 20 devices for account each with 15 commands

| name                        | run count  | avg time   | total time  | min time  | max time   | rows examined  | rows sent  |
|-----------------------------|------------|------------|-------------|-----------|------------|----------------|------------|
| accountDevices_17           | 10         | 2.36 ms    | 23.63 ms    | 1.26 ms   | 3.35 ms    | 6400           | 3000       |
| deviceFromRefreshTokenId_1  | 10         | 642.02 µs  | 6.42 ms     | 344.38 µs | 1.25 ms    | 300            | 150        |

• UID: 11111111111111111111111111111126 - 500 devices all with sessionTokens and refreshTokens

| name                        | run count  | avg time   | total time  | min time  | max time   | rows examined  | rows sent  |
|-----------------------------|------------|------------|-------------|-----------|------------|----------------|------------|
| accountDevices_17           | 10         | 5.93 ms    | 59.32 ms    | 4.32 ms   | 7.26 ms    | 10000          | 5000       |
| deviceFromRefreshTokenId_1  | 10         | 822.04 µs  | 8.22 ms     | 437.83 µs | 1.98 ms    | 0              | 10         |

• UID: CCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCC - devices with dangling refresh token id’s that don’t exist in the oauth db. Should have no impact, but worth testing

| name                        | run count  | avg time   | total time  | min time  | max time   | rows examined  | rows sent  |
|-----------------------------|------------|------------|-------------|-----------|------------|----------------|------------|
| accountDevices_17           | 10         | 818.96 µs  | 8.19 ms     | 291.79 µs | 1.26 ms    | 200            | 200        |
| deviceFromRefreshTokenId_1  | 10         | 637.63 µs  | 6.38 ms     | 392.67 µs | 1.18 ms    | 0              | 10         |

• UID: 11111111111111111111111111111122 - mixed commands, some devices have commands some don’t. our specific device requested does

| name                        | run count  | avg time   | total time  | min time  | max time   | rows examined  | rows sent  |
|-----------------------------|------------|------------|-------------|-----------|------------|----------------|------------|
| accountDevices_17           | 11         | 1.28 ms    | 14.03 ms    | 448.42 µs | 2.02 ms    | 1320           | 550        |
| deviceFromRefreshTokenId_1  | 11         | 434.93 µs  | 4.78 ms     | 280.46 µs | 696.33 µs  | 0              | 11         |

---

Additional Testing

I wanted to ensure that I didn't also break endpoints that accept this auth strategy. So, I leveraged the auth-client unit tests to call creating an OAuthToken and use that to request data from the various devices endpoints, here's an example.

Note

This test was not committed, but including here for purposes of showing testing!

// fxa-auth-client/test/client.ts
describe('lib/client', () => {
  //...
  it('does foo', async () => {
    // 1. Get a session token via login
    const sessionToken = await client.signUp(email, 'testpassword', {
      preVerified: 'true',
      lang: 'en',
    }); */
    const sessionToken = await client.signIn(
      '[email protected]',
      '[REDACTED]'
    );

    console.debug('sessionToken', sessionToken);

    // 2. Create OAuth token with refresh token
    const oauthResult = await client.createOAuthToken(
      sessionToken.sessionToken,
      '3c49430b43dfba77', // Android Components client ID (public client)
      {
        access_type: 'offline',
        scope: 'https://identity.mozilla.com/apps/oldsync',
      },
      new Headers()
    );

    console.debug('oauthResult', oauthResult);

    // 3. Use the refresh_token for authentication
    const refreshToken = oauthResult.refresh_token;

    // 4. Test endpoint with a Bearer token
    const response = await fetch('http://localhost:9000/v1/account/device', {
      method: 'POST',
      headers: {
        Authorization: `Bearer ${refreshToken}`, // this is run through the first auth strategy by hapi, which would return h.unauthenticated, and then moves onto the `refresh-token` strategy
        'Content-Type': 'application/json',
      },
      body: JSON.stringify({
        name: 'Test Device',
        type: 'mobile',
      }),
    });

    const responseData = await response.json();
    console.debug('response status', response.status);
    console.debug('response data', responseData);
  });
  //...
});

[nshirley]

Hm, I didn't see the tests had failed! I'll get them fixed

[dschom]

Any reason not to have these at the top of the file?

[nshirley]

uh, probably not! Not sure why I left that. I'll go through and clean up the tests in general for what I added, kinda rushed them

[dschom]

Why not assert is greater than? Also why not check exact number of commands.

[nshirley]

That's a good point. I have a habit of using greater/less but for no particular reason. Definitely better to assert the exact number here!

[dschom]

I'm guessing we don't, but do we need some DB clean after test suite runs?

[nshirley]

There's an existing "cleanup" in the setup step - it just drops the whole db when calling testDatabaseSetup. That said, I suppose we could have a beforeEach or afterEach that does a cleanup since we only run the setup function once per test module. I'll add something for these tests at least since they do need to insert data like they do

[dschom]

Thanks for keeping this updated too.

[nshirley]

yeahhh, I don't like this pattern but definitely want tests for it!

I put a ticket in the backlog to see if we can do this better. I THINK we should be able to use the same mysql migration tool that we do for dev/stage/prod when running tests and point it to our existing migration scripts. That way the testAdmin db that we're creating is always a mirror of the actual fxa db, and we don't have to duplicate scripts like this

[nshirley]

Found the issue I logged up! https://mozilla-hub.atlassian.net/browse/FXA-12691

[dschom]

Maybe just record if it was or was not found. Might make graphing more useful?