oauth->auth phase 1 #2497

dannycoates · 2019-09-13T04:04:48Z

No description provided.

dannycoates

Looking for r?

What this does...

It starts using the code from oauth-server directly in the auth-server instead of making a http request. One goal here was minimal changes, I think I almost got that. Cleaning up orphaned code will be in a later phase.

I removed the local/oauthdb tests since they looked pretty specific to the http api calls and functionality seemed pretty well covered by the remote tests. lmk if that isn't correct.

In prod, or any other non-local-dev setup we'll need to start passing some oauth-server config into auth-server, mainly DB and a few others. I'll be making config PRs to fxa-dev and clouds-deployment. In local dev and ci this is handled in mysql_servers.json. Unfortunately servers.json won't work during the transition since multiple processes now need access to the data.

dannycoates · 2019-09-17T02:39:12Z

packages/fxa-auth-server/lib/oauthdb/grant-tokens-from-refresh-token.js

@@ -30,6 +30,7 @@ module.exports = config => {
      }),
      response: Joi.object({
        access_token: validators.accessToken.required(),
+        id_token: validators.assertion.optional(),


I had to add this for this line in the test to work https://github.com/mozilla/fxa/blob/master/packages/fxa-auth-server/test/remote/oauth_tests.js#L188

I don't know if it's ok for this to be returning an id_token or not, and I don't know why it would be different from before.

Yeah, this is right. If you look at the test, the original scope requested for the grant contained openid. With @rfk's PR to make refresh token grants default to the original grant scopes, openid would have been propagated when using the refresh token to request the new access token. Now, for why this test wouldn't have failed before your change here is a mystery though it does look like the test on line 196 can be re-instated because @rfk's PR fixed #1867.

res2 still has an empty scope in this test 🤷‍♂️

Oh... Strange. And unexpected. @rfk - with your fix we should expect scope to be populated there, wouldn't we?

@dannycoates - I don't think this is related to your PR, so carry on.

Hmm, actually I think it's super weird to return an id_token in a refresh grant and we should instead update #2410 to skip the "openid" scope. I filed #2548 to follow up.

I agree that @dannycoates shouldn't worry about this for his PR though.

dannycoates · 2019-09-17T02:41:12Z

packages/fxa-auth-server/fxa-oauth-server/test/routes/token.js

@@ -23,6 +15,9 @@ const CODE = 'df6dcfe7bf6b54a65db5742cbcdce5c0a84a5da81a0bb6bdf5fc793eef041fc6';
 const PKCE_CODE_VERIFIER = 'au3dqDz2dOB0_vSikXCUf4S8Gc-37dL-F7sGxtxpR3R';
 const DISABLED_CLIENT_ID = 'd15ab1edd15ab1ed';

+realConfig.set('disabledClients', [DISABLED_CLIENT_ID]);
+delete require.cache[require.resolve(routeModulePath)];


Moving config.js into config/index.js broke the sandbox/proxyquire thing and I was too lazy to figure out how to fix it, so I just did this instead.

I think this is fine for now. Since there will be more changes for the config, we can probably revisit it later.

dannycoates · 2019-09-17T02:44:54Z

packages/fxa-auth-server/fxa-oauth-server/lib/config/index.js

@@ -0,0 +1,100 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */


I moved config.js here because I thought I was going to have 2 config sets based on whether oauth or the key server was loading it and using separate files for each, but they ended up both being pretty much the same. I moved the schema out for the same reason but idk if leaving it there is better or not.

dannycoates · 2019-09-17T02:46:51Z

packages/fxa-auth-server/fxa-oauth-server/lib/config/index.js

+);
+
+// This is sneaky and gross, but temporary.
+if (process.mainModule.filename.includes('key_server')) {


these are the differences when loading from auth-server, luckily I don't think any of the necessary ENVs in the schema overlap auth-server's

Is there an easy way to check this to make sure?

im doing this now as i make the config PRs to cloudops and fxa-dev

dannycoates · 2019-09-17T02:48:07Z

.circleci/test-content-server.sh

@@ -53,7 +53,7 @@ if grep -e "$MODULE" -e 'all' $DIR/../packages/test.list; then
  fi

  cd ../../
-  npx pm2 delete servers.json && npx pm2 start servers.json
+  npx pm2 delete mysql_servers.json && npx pm2 start mysql_servers.json


tests require mysql for now since two processes access the oauth data

dannycoates · 2019-09-17T02:51:10Z

packages/fxa-auth-server/lib/oauthdb/index.js

+
+const routes = new Map(
+  oauthRoutes.map(route => [route.path, route.config.handler])
+);

 module.exports = (log, config) => {
  const OAuthAPI = createBackendServiceAPI(log, config, 'oauth', {


I left this for the validators

Oh, I see. Should those validators just be moved to the routes in oauth.js? If not, can you leave a note about accessing the validators via the exported api property? If we keep the validators where they are, should we consider removing path and method from the oauthdb files since the route handlers are called directly?

This will all change in phase 2. I left most things in place to keep the diff smaller

dannycoates · 2019-09-17T02:55:37Z

packages/fxa-content-server/tests/functional/support.js

@@ -28,7 +28,9 @@ const {
 registerSuite('support form without valid session', {
  tests: {
    'go to support form, redirects to signin': function() {
-      return this.remote.then(openPage(SUPPORT_URL, selectors.SIGNIN.HEADER));
+      return this.remote
+        .then(clearBrowserState())


This test started failing without clearing the state first. The screenshot looked like it was already logged in. idk why this changed?

Probably a timing issue. I imagine things are ever so slightly faster now with fewer HTTP requests to make. You might want to add a {force: true} parameter, the default way of clearing state is also somewhat brittle to timing issues.

shane-tomlinson

This is very exiting @dannycoates and the overall direction looks good. When locally testing using the memory database, it seems the auth and oauth servers create two separate memory instances. If a token is created in the Auth server's memory instance, that same token cannot be verified if the OAuth server's /verify endpoint is used.

You can see this in action by applying this diff: https://paste.mozilla.org/GfiGcdUv

Then sign up & verify an account and try to go to the /settings page. The token keeps coming back as invalid. I'm not sure what the best way forward is because IIUC at some point soon, all the routes for both services will be served by one process. We could say in the meantime MySQL is the only way forward, or we could start a small shared memory DB service?

shane-tomlinson · 2019-09-18T09:27:54Z

packages/fxa-auth-server/fxa-oauth-server/lib/config/index.js

+  `${conf.get('env')}.json`
+);
+
+// This is sneaky and gross, but temporary.


What is the plan to get rid of the sneaky/grossness?

Phase 2 will copy oauth config into auth-server's main config.

shane-tomlinson · 2019-09-18T09:30:40Z

packages/fxa-auth-server/fxa-oauth-server/lib/config/index.js

+// This is sneaky and gross, but temporary.
+if (process.mainModule.filename.includes('key_server')) {
+  if (fs.existsSync(envConfig)) {
+    conf.loadFile(envConfig);


Should we yell loudly if the environment config file does not exist?

This file doesn't exist in prod, it's all environment variables.

shane-tomlinson · 2019-09-18T09:33:58Z

packages/fxa-auth-server/fxa-oauth-server/lib/config/index.js

+);
+
+// This is sneaky and gross, but temporary.
+if (process.mainModule.filename.includes('key_server')) {


Is there an easy way to check this to make sure?

shane-tomlinson · 2019-09-18T09:35:13Z

packages/fxa-auth-server/fxa-oauth-server/lib/config/index.js

+    conf.loadFile(envConfig);
+  }
+  conf.set('mysql.createSchema', false);
+  conf.validate();


How come the key server doesn't enforce strict mode but the oauth server does?

auth-server only requires a subset of the oauth config in this phase. Strict makes us set more variables than we need in this case.

shane-tomlinson · 2019-09-18T09:41:36Z

packages/fxa-auth-server/fxa-oauth-server/test/routes/authorization.js

@@ -27,6 +19,9 @@ const PKCE_CODE_CHALLENGE = 'iyW5ScKr22v_QL-rcW_EGlJrDSOymJvrlXlw4j7JBiQ';
 const PKCE_CODE_CHALLENGE_METHOD = 'S256';
 const DISABLED_CLIENT_ID = 'd15ab1edd15ab1ed';

+realConfig.set('disabledClients', [DISABLED_CLIENT_ID]);
+delete require.cache[require.resolve(routeModulePath)];


TIL. This looks much easier than using proxyquire and the mocks module.

Yea, however I vaguely remember running into subtle problems when using this. It puts the puts the pressure on the developer to know which version of the module has been loaded where.

noted. imho we ought to be injecting the config instead of requireing it, but that's a debate for another time

shane-tomlinson · 2019-09-18T10:05:04Z

packages/fxa-auth-server/lib/oauthdb/index.js

+
+const routes = new Map(
+  oauthRoutes.map(route => [route.path, route.config.handler])
+);

 module.exports = (log, config) => {
  const OAuthAPI = createBackendServiceAPI(log, config, 'oauth', {


Oh, I see. Should those validators just be moved to the routes in oauth.js? If not, can you leave a note about accessing the validators via the exported api property? If we keep the validators where they are, should we consider removing path and method from the oauthdb files since the route handlers are called directly?

shane-tomlinson · 2019-09-18T10:25:55Z

packages/fxa-auth-server/test/local/oauthdb.js

-
-'use strict';
-
-const { assert } = require('chai');


Do we need any portion of these tests or is the code exercised elsewhere?

This gets covered by remote/oauth_tests.js and the content-server functional tests. I didn't do an exhaustive comparison because the new callRoute and backendService.sendRequest are so similar

shane-tomlinson · 2019-09-18T10:26:30Z

packages/fxa-auth-server/test/remote/oauth_tests.js

  let password;
  let server;

  before(async () => {
    testUtils.disableLogs();
-    oauthServer = await oauthServerModule.create();


🕺 amazing!

shane-tomlinson · 2019-09-18T10:28:18Z

packages/fxa-content-server/tests/functional/support.js

@@ -28,7 +28,9 @@ const {
 registerSuite('support form without valid session', {
  tests: {
    'go to support form, redirects to signin': function() {
-      return this.remote.then(openPage(SUPPORT_URL, selectors.SIGNIN.HEADER));
+      return this.remote
+        .then(clearBrowserState())


Probably a timing issue. I imagine things are ever so slightly faster now with fewer HTTP requests to make. You might want to add a {force: true} parameter, the default way of clearing state is also somewhat brittle to timing issues.

shane-tomlinson · 2019-09-18T10:29:51Z

packages/fxa-auth-server/fxa-oauth-server/lib/config/schema.json

@@ -0,0 +1,300 @@
+{


Is the intent of using a separate schema here so that one config module can be used to load configs for both services?

vbudhram

@dannycoates Nice work! I don't really have any issues with approach here. Although, it is a little unclear what the next steps, maybe create a checklist of items?

We could say in the meantime MySQL is the only way forward

I think this is fair, I really don't know who uses the memory database (except for local dev) and we have talked about nuking it for a while.

vbudhram · 2019-09-18T15:47:09Z

packages/fxa-auth-server/fxa-oauth-server/test/routes/token.js

@@ -23,6 +15,9 @@ const CODE = 'df6dcfe7bf6b54a65db5742cbcdce5c0a84a5da81a0bb6bdf5fc793eef041fc6';
 const PKCE_CODE_VERIFIER = 'au3dqDz2dOB0_vSikXCUf4S8Gc-37dL-F7sGxtxpR3R';
 const DISABLED_CLIENT_ID = 'd15ab1edd15ab1ed';

+realConfig.set('disabledClients', [DISABLED_CLIENT_ID]);
+delete require.cache[require.resolve(routeModulePath)];


I think this is fine for now. Since there will be more changes for the config, we can probably revisit it later.

vbudhram · 2019-09-18T15:54:10Z

packages/fxa-auth-server/fxa-oauth-server/test/routes/authorization.js

@@ -27,6 +19,9 @@ const PKCE_CODE_CHALLENGE = 'iyW5ScKr22v_QL-rcW_EGlJrDSOymJvrlXlw4j7JBiQ';
 const PKCE_CODE_CHALLENGE_METHOD = 'S256';
 const DISABLED_CLIENT_ID = 'd15ab1edd15ab1ed';

+realConfig.set('disabledClients', [DISABLED_CLIENT_ID]);
+delete require.cache[require.resolve(routeModulePath)];


Yea, however I vaguely remember running into subtle problems when using this. It puts the puts the pressure on the developer to know which version of the module has been loaded where.

dannycoates · 2019-09-18T22:01:10Z

@shane-tomlinson I removed servers.json to prevent confusion about which to run. We ought to be able to restore it in phase 2, or we can use this as an excuse to deprecate the memory db for good.

shane-tomlinson

Down with the in-memory database! Down with the separation of the auth and oauth servers! r+

dannycoates · 2019-09-19T17:22:24Z

Once config PRs are good I'll merge this

shane-tomlinson · 2019-09-24T09:45:36Z

@dannycoates looks like some merge conflicts were introduced. Mind rebasing? Other than a rebase, is there more you want to do here?

vladikoff · 2019-09-24T14:32:47Z

servers.json

@@ -1,236 +0,0 @@
-{
-  "apps": [


do we need to update the docs that servers.json is not a thing anymore? or should we take mysql_servers and call it servers now?

dannycoates · 2019-09-24T17:36:27Z

@dannycoates looks like some merge conflicts were introduced. Mind rebasing? Other than a rebase, is there more you want to do here?

rebased. I'd like to get the config PRs reviewed before I merge.

dannycoates · 2019-10-11T18:29:21Z

Deployment risks:

code: LOW (no logic changes, only glue code. running by devs and on latest for 2 weeks, all tests passing)
config: LOW (will get caught in stage if misconfigured)
operations: LOW-MEDIUM (slightly reduced load on oauth-server, reduced network overall, slightly increased load on auth-server, more db connections to oauth db)

dannycoates · 2019-10-11T18:42:30Z

Testing

These areas are most affected by this change:

Ensure logging in with oauth works (123done)
Ensure oauth sessions (123done) display on the settings page in 'Devices & apps'

ValentinaPC · 2020-07-29T09:35:44Z

Verified as fixed in stage, using Fx78.0.2 and Windows 10 (64-bit).
Both scenarios work as expected.
Postfix screenshot below.

dannycoates force-pushed the auth-oauth branch 10 times, most recently from d1aa649 to da5bf7b Compare September 16, 2019 22:56

dannycoates commented Sep 17, 2019

View reviewed changes

dannycoates marked this pull request as ready for review September 17, 2019 03:11

dannycoates changed the title ~~wip on oauth->auth phase 1~~ oauth->auth phase 1 Sep 17, 2019

dannycoates requested a review from a team September 17, 2019 03:16

vbudhram added this to the Train 147: FxA milestone Sep 17, 2019

shane-tomlinson suggested changes Sep 18, 2019

View reviewed changes

vbudhram reviewed Sep 18, 2019

View reviewed changes

dannycoates force-pushed the auth-oauth branch from daf1602 to 928e5d7 Compare September 18, 2019 18:33

shane-tomlinson approved these changes Sep 19, 2019

View reviewed changes

dannycoates force-pushed the auth-oauth branch from 928e5d7 to 56a56fb Compare September 19, 2019 15:45

rfk mentioned this pull request Sep 19, 2019

Don't generate id_tokens in a refresh token grant #2548

Closed

dannycoates mentioned this pull request Sep 20, 2019

added oauth env variables to auth-server mozilla/fxa-dev#487

Merged

rfk mentioned this pull request Sep 23, 2019

Add RFC7009-compatible token revocation endpoint. #2567

Merged

vladikoff reviewed Sep 24, 2019

View reviewed changes

dannycoates force-pushed the auth-oauth branch from 56a56fb to 5a9f3e8 Compare September 24, 2019 17:33

dannycoates force-pushed the auth-oauth branch from 5a9f3e8 to a078265 Compare September 24, 2019 19:38

dannycoates force-pushed the auth-oauth branch from a078265 to 4338e97 Compare September 24, 2019 20:14

clouserw assigned dannycoates Sep 26, 2019

dannycoates force-pushed the auth-oauth branch 3 times, most recently from a320d7d to e1df82d Compare September 28, 2019 01:28

clouserw modified the milestones: Train 147: FxA, Train 148: FxA Sep 30, 2019

chore(oauth): Phase 1 of oauth->auth server consolidation #1922

f7431df

dannycoates force-pushed the auth-oauth branch from e1df82d to f7431df Compare October 1, 2019 23:31

dannycoates merged commit 7ad1f3b into master Oct 2, 2019

clouserw added needs:qa Development work is done. This is ready for QA. qa+ Critical flow or high chance of regression. QA should focus on testing this issue. labels Oct 11, 2019

shane-tomlinson deleted the auth-oauth branch October 14, 2019 11:56

ValentinaPC removed the needs:qa Development work is done. This is ready for QA. label Jul 29, 2020

oauth->auth phase 1 #2497

oauth->auth phase 1 #2497

Conversation

dannycoates commented Sep 13, 2019

dannycoates left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dannycoates Sep 17, 2019 • edited

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

Choose a reason for hiding this comment

shane-tomlinson left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbudhram left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dannycoates commented Sep 18, 2019

shane-tomlinson left a comment

Choose a reason for hiding this comment

dannycoates commented Sep 19, 2019

shane-tomlinson commented Sep 24, 2019

Choose a reason for hiding this comment

dannycoates commented Sep 24, 2019

dannycoates commented Oct 11, 2019

dannycoates commented Oct 11, 2019 • edited

Testing

ValentinaPC commented Jul 29, 2020

dannycoates Sep 17, 2019 •

edited

vbudhram left a comment •

edited

dannycoates commented Oct 11, 2019 •

edited