feat(server): add basket proxy server #2460

zaach · 2015-05-24T03:10:53Z

zaach · 2015-05-24T03:13:07Z

I wasn't quite sure how to test this... Perhaps a mock basket server that we configure the proxy to talk to in dev/test mode.

coveralls · 2015-05-24T03:18:57Z

Coverage decreased (-0.11%) to 98.25% when pulling 4cbfed3 on issue-2443-basket-proxy into ebc3bee on master.

shane-tomlinson · 2015-05-25T10:39:13Z

@zaach - this app will need to be able to respond to CORS requests.

shane-tomlinson · 2015-05-25T11:15:41Z

When token arrives here, it still has Bearer prepended, which causes the OAuth server to reject it.

A simple fix is:

token = token.replace(/^Bearer /, '');

rfk · 2015-05-25T11:33:17Z

server/bin/basket-proxy-server.js

+    verifyOAuthToken(req.headers.authorization)
+      .then(function (creds) {
+        basketRequest('/subscribe', 'post', {
+          email: creds.email,


This creds.email property will only exist if the token had profile:email scope. @shane-tomlinson the simplest thing might be to request scope="basket:write profile:email" for the basket token, although when this functionality merges into basket proper that will no longer be necessary.

rfk · 2015-05-25T11:49:59Z

server/bin/basket-proxy-server.js

+      .then(function (creds) {
+        basketRequest('/unsubscribe', 'post', {
+          email: creds.email,
+          newsletter_id: req.params.newsletter_id


For basket API compatibility, this needs to be { "newsletters": "comma,separated,list,of,ids" } here and in the client code.

rfk · 2015-05-25T11:54:07Z

server/bin/basket-proxy-server.js

+    if (body.code >= 400) {
+      logger.debug('unauthorized', body);
+      return defer.reject(new Error(body.message));
+    }


Need to check for body.scope.indexOf("basket:write") >= 0 in the response body here :-)

rfk · 2015-05-25T12:04:27Z

server/bin/basket-proxy-server.js

+    });
+  });
+
+  app.post('/unsubscribe', function (req, res) {


I had to add a trailing slash to /subscribe/ and /unsubscribe/ when talking to the dev basket server by hand, otherwise it would give me a 405.

Ah, this is probably basket trying to redirect me to the trailing-slash variant, and my HTTP client following it via GET rather than retrying the POST there.

rfk · 2015-05-25T12:46:22Z

server/bin/basket-proxy-server.js

+
+function initApp() {
+  var app = express();
+  app.use(bodyParser.json());


AFAICT basket only accepts form-encded input, not JSON. We could use JSON for our own needs but should support form-encoded here as well.

I'm a bit "meh" on supporting form-encoded input if our other services do not. Basket is a bit of an implementation detail, and AFAICT, all our other services only accept JSON input.

Well, hopefully long-term this oauth functionality will live directly in basket rather than in a proxy. In which case we'll be talking directly to basket and limited by whatever input formats it supports.

But yeah, JSON FTW, so let's upstream this: https://bugzilla.mozilla.org/show_bug.cgi?id=1168218

shane-tomlinson · 2015-05-25T16:47:04Z

@zaach - this is great stuff. I had to make some updates to perform end to end testing, so I opened a PR (#2467) against this PR.

shane-tomlinson · 2015-05-26T13:34:11Z

ref mozilla/fxa-content-experiments#6

* Strip the "Bearer " portion of a token before verification. * Enable CORS from the content server. * Handle errors from both the OAuth and Basket servers.

Update the basket proxy server to support the full flow.

jrgm · 2015-05-27T22:00:07Z

FYI, neither @pdehaan or myself can do a 'git checkout clean && npm install && npm start' with this branch on osx. It dies on a hard to explain error about loading eventemitter3 when calling require('http-proxy'). Might be due to the stale version of http-proxy@1.2.

pdehaan · 2015-05-27T22:49:20Z

Yeah, what @jrgm said. But it worked for me after I deleted the npm-shrinkwrap.json and did npm-next i http-proxy@latest -S and then npm start (using node 0.10.38 and npm 2.11.0).

¯_(ツ)_/¯

rfk · 2015-05-28T00:20:20Z

server/bin/basket-proxy-server.js

+}
+
+function listen(app) {
+  var serverUrl = url.parse(config.get('marketing_email.api_url'));


Per discussion with @jrgm in IRC; the marketing_email.api_url is the publicly-visible URL of this API. For hosting purposes we will want to listen on a localhost port. I suggest making a new basket.proxy_url config option to set the local listen address for this proxy to bind to, and keeping marketing_email.api_url as the public-facing one.

In practice, we'll host with some sort of nginx proxy so the settings might be something like:

marketing_email.api_url: "https://accounts.firefox.com/basket/news"

basket.proxy_url: "http://localhost:3034"

@zaach thoughts?

rfk · 2015-05-28T00:26:02Z

server/bin/basket-proxy-server.js

+      return;
+    }
+
+    var token = authHeader.replace(/^Bearer /, '');


IMHO it would be better to explicitly check for "Bearer" and error out if there's some other weirdy auth type in the header, e.g.:

var authz = authHeader..split(/\s+/); if (authz.length !== 2 || authz[0].toLowerCase() !== "bearer") { // error out }

rfk · 2015-05-28T00:28:33Z

server/bin/basket-proxy-server.js

+
+      logger.debug('auth.valid', body);
+
+      res.locals.creds = body;


We should check that we actually got the email back as part of the response; the oauth server will only send it if the token had correct scopes

We need to check body.scopes for the presence of basket:write before allowing this through.

rfk · 2015-05-28T00:33:08Z

Those notes aside, this is looking really solid!

jrgm · 2015-05-28T06:09:21Z

server/bin/basket-proxy-server.js

+    var params = req.body;
+    var creds = res.locals.creds;
+
+    basketRequest('/lookup-user/?email=' + creds.email, 'get', params).pipe(res);


Yeah, so this needs to change here and other places where we just pipe through the response. It needs to log errors at minimum, but really should be standard HTTP response code logging for success and failure for every request.

Aye. We should be able to reuse the route logger we have for the main server.

rfk · 2015-05-28T07:27:13Z

server/bin/basket-proxy-server.js

+        return;
+      }
+
+      if (! body.scopes.match(/basket:write/)) {


IIUC body.scopes is supposed to come back as an array; if it's a string here then we need to cross-check behaviour with oauth server

Ah, you're right. Fixing.

shane-tomlinson · 2015-05-28T08:58:02Z

server/bin/basket-proxy-server.js

+    }
+
+    if (! authHeader.match(/^Bearer /)) {
+      res.status(400).json(errorResponse('invalid authorization header', BASKET_ERRORS.USAGE_ERROR));


Add error logging.

shane-tomlinson · 2015-05-28T08:58:53Z

server/bin/basket-proxy-server.js

+        return;
+      }
+
+      if (body.scopes.indexOf('basket:write') === -1) {


Do we need to ensure scopes exists before calling indexOf?

We do, while testing with the null proxy server:

fxa-content-server.CRITICAL: TypeError: Cannot call method 'match' of undefined at Request._callback (/Users/stomlinson/development/fxa-content-server/server/bin/basket-proxy-server.js:103:25) at Request.self.callback (/Users/stomlinson/development/fxa-content-server/node_modules/request/request.js:123:22) at Request.emit (events.js:98:17) at Request.<anonymous> (/Users/stomlinson/development/fxa-content-server/node_modules/request/request.js:1047:14) at Request.emit (events.js:117:20) at IncomingMessage.<anonymous> (/Users/stomlinson/development/fxa-content-server/node_modules/request/request.js:998:12) at IncomingMessage.emit (events.js:117:20) at _stream_readable.js:943:16 at process._tickCallback (node.js:419:13)

ahha, the oauth server returns scope, not scopes.

ugh, that's unfortunate; its API docs say it should return scopes but the code indeed returns scope.

shane-tomlinson · 2015-05-28T09:02:53Z

server/lib/configuration.js

@@ -283,6 +283,31 @@ var conf = module.exports = convict({
      doc: 'Location of "report-uri"',
      default: '/_/csp-violation',
    }
+  },
+  marketing_email: {


We might as well take the marketing_email section out of this PR and add it to #2452.

…elongs in #2452

@zaach

Contains @zaach's proxy from #2460 issue #2443

@zaach

Contains @zaach's proxy from #2460 issue #2443

@zaach

feat(server): add basket proxy server Thank you @zaach for this. Awesome. r=@shane-tomlinson

@zaach

Contains @zaach's proxy from #2460 issue #2443

@zaach

Contains @zaach's proxy from #2460 issue #2443

@zaach

Contains @zaach's proxy from #2460 issue #2443 (cherry picked from commit 8c4246e)

@zaach

Contains @zaach's proxy from #2460 issue #2443 (cherry picked from commit 8c4246e)

feat(server): add basket proxy server

4cbfed3

zaach added waffle:needs review WIP labels May 24, 2015

zaach mentioned this pull request May 24, 2015

feat(content-server): add basket proxy server mozilla/fxa-local-dev#15

Merged

rfk reviewed May 25, 2015
View changes

shane-tomlinson mentioned this pull request May 26, 2015

feat: Add communications preferences feature toggle. mozilla/fxa-content-experiments#6

Merged

shane-tomlinson added this to the train 38 milestone May 26, 2015

Shane Tomlinson and others added 2 commits May 25, 2015

Update the basket proxy server to support the full flow.

cc0dfdb

* Strip the "Bearer " portion of a token before verification. * Enable CORS from the content server. * Handle errors from both the OAuth and Basket servers.

Merge pull request #2467 from shane-tomlinson/issue-2443-basket-proxy

ada9023

Update the basket proxy server to support the full flow.

rfk reviewed May 28, 2015
View changes

jrgm reviewed May 28, 2015
View changes

chore(npm): update http-proxy and shrinkwrap

dcb48de

zaach force-pushed the issue-2443-basket-proxy branch from 8c9b2f9 to dcb48de May 28, 2015

rfk reviewed May 28, 2015
View changes

fix(basket-proxy): tighten up param checking and logging

fa5aa01

zaach force-pushed the issue-2443-basket-proxy branch from a3da6da to fa5aa01 May 28, 2015

shane-tomlinson reviewed May 28, 2015
View changes

server/bin/basket-proxy-server.js

}

if (! authHeader.match(/^Bearer /)) {

res.status(400).json(errorResponse('invalid authorization header', BASKET_ERRORS.USAGE_ERROR));

This comment has been minimized.

Sign in to view

shane-tomlinson May 28, 2015
Member

Add error logging.

shane-tomlinson reviewed May 28, 2015
View changes

Shane Tomlinson added 3 commits May 28, 2015

scopes=>scope

3d9d882

Remove the "marketing_email" configuration section from this PR, it b…

51fbf8c

…elongs in #2452

Log an invalid authorization header.

f6a1690

shane-tomlinson pushed a commit that referenced this pull request May 28, 2015

feat: Email opt-in.

d8ad411

Contains @zaach's proxy from #2460 issue #2443

shane-tomlinson pushed a commit that referenced this pull request May 28, 2015

feat: Email opt-in.

9ef2b7a

Contains @zaach's proxy from #2460 issue #2443

shane-tomlinson pushed a commit that referenced this pull request May 28, 2015

Merge pull request #2460 from mozilla/issue-2443-basket-proxy

8040fcf

feat(server): add basket proxy server Thank you @zaach for this. Awesome. r=@shane-tomlinson

shane-tomlinson removed waffle:needs review WIP labels May 28, 2015

shane-tomlinson pushed a commit that referenced this pull request May 28, 2015

feat: Email opt-in.

517387d

Contains @zaach's proxy from #2460 issue #2443

shane-tomlinson pushed a commit that referenced this pull request May 28, 2015

feat: Email opt-in.

8c4246e

Contains @zaach's proxy from #2460 issue #2443

shane-tomlinson pushed a commit that referenced this pull request Jun 5, 2015

feat: Email opt-in.

5e84a9b

Contains @zaach's proxy from #2460 issue #2443 (cherry picked from commit 8c4246e)

shane-tomlinson pushed a commit that referenced this pull request Jun 5, 2015

feat: Email opt-in.

c540126

Contains @zaach's proxy from #2460 issue #2443 (cherry picked from commit 8c4246e)

mozilla / fxa-content-server Archived

feat(server): add basket proxy server #2460

feat(server): add basket proxy server #2460

zaach commented May 24, 2015

zaach commented May 24, 2015

coveralls commented May 24, 2015

This comment has been minimized.

shane-tomlinson commented on `server/bin/basket-proxy-server.js` in `4cbfed3` May 25, 2015

This comment has been minimized.

shane-tomlinson commented on `server/bin/basket-proxy-server.js` in `4cbfed3` May 25, 2015

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

shane-tomlinson commented May 25, 2015

shane-tomlinson commented May 26, 2015

jrgm commented May 27, 2015

pdehaan commented May 27, 2015

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

rfk commented May 28, 2015

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mozilla / fxa-content-server Archived

feat(server): add basket proxy server #2460

feat(server): add basket proxy server #2460

Conversation

zaach commented May 24, 2015

zaach commented May 24, 2015

coveralls commented May 24, 2015

This comment has been minimized.

shane-tomlinson commented on server/bin/basket-proxy-server.js in 4cbfed3 May 25, 2015

This comment has been minimized.

shane-tomlinson commented on server/bin/basket-proxy-server.js in 4cbfed3 May 25, 2015

This comment has been minimized.

rfk May 25, 2015 Member

This comment has been minimized.

rfk May 25, 2015 Member

This comment has been minimized.

rfk May 25, 2015 Member

This comment has been minimized.

rfk May 25, 2015 Member

This comment has been minimized.

rfk May 25, 2015 Member

This comment has been minimized.

rfk May 25, 2015 Member

This comment has been minimized.

shane-tomlinson May 25, 2015 Member

This comment has been minimized.

rfk May 25, 2015 Member

shane-tomlinson commented May 25, 2015

shane-tomlinson commented May 26, 2015

jrgm commented May 27, 2015

pdehaan commented May 27, 2015

This comment has been minimized.

rfk May 28, 2015 Member

This comment has been minimized.

zaach May 28, 2015 Author Contributor

This comment has been minimized.

rfk May 28, 2015 Member

This comment has been minimized.

rfk May 28, 2015 Member

This comment has been minimized.

rfk May 28, 2015 Member

rfk commented May 28, 2015

This comment has been minimized.

jrgm May 28, 2015 Contributor

This comment has been minimized.

zaach May 28, 2015 Author Contributor

This comment has been minimized.

rfk May 28, 2015 Member

This comment has been minimized.

zaach May 28, 2015 Author Contributor

This comment has been minimized.

shane-tomlinson May 28, 2015 Member

This comment has been minimized.

shane-tomlinson May 28, 2015 Member

This comment has been minimized.

shane-tomlinson May 28, 2015 Member

This comment has been minimized.

shane-tomlinson May 28, 2015 Member

This comment has been minimized.

rfk May 28, 2015 Member

This comment has been minimized.

shane-tomlinson May 28, 2015 Member

shane-tomlinson commented on `server/bin/basket-proxy-server.js` in `4cbfed3` May 25, 2015

shane-tomlinson commented on `server/bin/basket-proxy-server.js` in `4cbfed3` May 25, 2015

rfk May 25, 2015
Member

rfk May 25, 2015
Member

rfk May 25, 2015
Member

rfk May 25, 2015
Member

rfk May 25, 2015
Member

rfk May 25, 2015
Member

shane-tomlinson May 25, 2015
Member

rfk May 25, 2015
Member

rfk May 28, 2015
Member

zaach May 28, 2015
Author Contributor

rfk May 28, 2015
Member

rfk May 28, 2015
Member

rfk May 28, 2015
Member

jrgm May 28, 2015
Contributor

zaach May 28, 2015
Author Contributor

rfk May 28, 2015
Member

zaach May 28, 2015
Author Contributor

shane-tomlinson May 28, 2015
Member

shane-tomlinson May 28, 2015
Member

shane-tomlinson May 28, 2015
Member

shane-tomlinson May 28, 2015
Member

rfk May 28, 2015
Member

shane-tomlinson May 28, 2015
Member