Implement authentication through a Facebook flow by julienw · Pull Request #47 · mozilla-sensorweb/sensorweb-server

julienw · 2017-01-13T14:54:55Z

Closes #24

julienw · 2017-01-13T14:56:40Z

src/config.js

    port: {
      doc: 'Port where PostgreSQL is running',
-      format: 'port',
+      format: '*',


these changes are needed for me because I connect to psql through a unix socket (which is quite cool because it matches psql user with your user :) ).

but I'm fine not committing this and keeping the change locally

Ok, thanks for letting me know. Maybe add a comment explaining why the format is*.

I think it's probably better to do something in a separate patch; possibly with a custom verifier. I'll revert this from this patch for now.

julienw · 2017-01-13T14:58:01Z

src/middlewares/auth.js

-    const authHeader = req.headers['authorization'];
-    if (!authHeader) {
+    const authHeader = req.headers.authorization;
+    const authQuery = req.query.authorizationToken;


To be able to use a simple browser request, we can pass the JWT token in this query parameter. I haven't looked if there is anything kinda standardized for this yet.

julienw · 2017-01-13T14:59:20Z

src/middlewares/auth.js

+    if (!decoded || !decoded.id || !decoded.scope) {
+      return unauthorized(res);
+    }
+


note: directly calling unauthorized disallows the redirect to failureUrl; this will be fixed once we just pass an error to next() so that we can handle it (or not) in the next middleware.

julienw · 2017-01-13T15:00:22Z

src/models/db.js

+  port,
+  dialect: 'postgres',
+  logging: false
+});


extracting this out of the exported function makes it possible to export it so that we can use it elsewhere. (in my case: for the session store)

I don't think this makes a big perf difference, as I believe what makes the big perf hit is loading all the model and calling sync

julienw · 2017-01-13T15:01:37Z

src/models/users.js

+    return Promise.resolve({
+      id: 'admin',
+      scope: 'admin'
+    });


basic authentication is for admin only

julienw · 2017-01-13T15:01:47Z

src/models/users.js

+  AUTH_PROVIDER: (data) => {
+    return Promise.resolve({
+      id: data,
+      scope: 'user'


auth provider based login is for users only

julienw · 2017-01-13T15:02:36Z

src/models/users.js

+          attributes: [],
+          where: userData.id, // this contains all user attributes
+        })
+          .then(() => userData);


not sure whether I should put a real user object in userData.id

It is probably safer to create a new object only with the fields that we care about (i.e., opaqueId and provider). Sequelize adds some extra fields like createdAt and updatedAt that we should remove from the response.

Also, uber-nit: weird indentation.

Note that the result to authenticate is not returned to the user, rather it's stored in req.user for the next (express) middlewares.

Currently I'm not returning the sequelize's user at all. userData comes from the authentication methods. Depending on the methods we don't always have the same structure. My take at this is that the "scope" will give us the structure.

userData.scope === 'user' => userData.id is an object with opaqueId, provider and ClientKey. This looks like the sequelize object but it's not. It comes from the authentication methods to we rely on them giving us the right structure (but a wrong structure would make the find fail, so I think we're good).

My main issue with the current approach is that in this function we never know explicitely what's inside userData.id, that could make it more difficult to read the function.

userData.scope === 'admin' => userData.id is 'admin'

userData.scope === 'client' => userData.id is just the client's key

So once we know the scope we also know how to consult the id.

Note: the indentation comes from my work with gmarty :) each .then go a new line even if it was not completely needed. I liked the fact it was a systematic rule so I kind of adopted it too, but I can also adapt myself to another code style :D

Ok. Thanks for the explanation.

Regarding the indentation, it's not a big deal, but rather that introducing a new one, try to follow the existing style, please.

return User.findOrCreate({ ... }).then(...

or even

return User.findOrCreate({ ... }) .then(...

julienw · 2017-01-13T15:04:02Z

src/routes/auth/basic.js

+      userInfo => done(null, userInfo),
+      err => {
+        if (err.message === UNAUTHORIZED) {
+          done(null, false);


unauthorized errors means "no user", passing false is how passport understands this

Add this as a code comment, please.

julienw · 2017-01-13T15:05:34Z

src/routes/auth/facebook.js

+  (req, res, next) => {
+    passport.authenticate(
+      'facebook',
+      (err, user, _info) => {


using a callback is the only way I found to customize what to do when there is a failure

julienw · 2017-01-13T15:06:14Z

src/routes/auth/index.js

+
+const router = express.Router();
+
+router.use('/basic', basic);


no session for the basic auth, that's why I use it before doing the session stuff

Add an inline comment about this, please.

julienw · 2017-01-13T15:06:53Z

src/routes/auth/index.js

+    secure: isProduction,
+  },
+  name: 'connect.sid.auth',
+  proxy: isProduction, // TODO maybe better configure this in config


this is when node is in HTTP but there is a proxy in HTTPS, when we use secure: true above

julienw · 2017-01-13T15:07:19Z

src/routes/auth/utils.js

+  if (req.session && req.session.redirectUrl) {
+    const redirectUrl = url.parse(req.session.redirectUrl, true);
+    redirectUrl.query.token = token;
+    req.session.destroy();


I think I'll need to destroy this in case of failures too... but not so easy to do with the current way of managing errors :)

julienw · 2017-01-13T15:08:00Z

feedback? @ferjm

Now I need to work on tests and clean up some style :)

julienw · 2017-01-13T15:51:58Z

BTW I use https://github.com/julienw/sensorweb-login-website to start my login flow. Here are the informations for my client with the hardcoded JWT:

name: julien
key: 7c91e8d619046ad7
secret: efa0a85e98bfc7929e66a18e14db5cdae4fce6517dc48e3c138fcf3d207b94b3af883985a7134adef94790b59acdb1be220011f8aff903cab8729cbdb45005ff
authRedirectUrls: {http://localhost:10000/token.html}
authFailureRedirectUrls: {http://localhost:10000/tokenFailure.html}

ferjm

Thank you Julien. Great work!

Looks good overall. I only left a few comments.

I know you are working on tests now, but I am also missing some high level and API documentation.

ferjm · 2017-01-13T16:32:40Z

config/sample.json

    "user": "postgres",
    "password": "default"
  },
+  "facebook": {


nit: could we embed this within an userAuth object?

ferjm · 2017-01-13T16:33:46Z

package.json

+    "passport-facebook": "^2.1.1",
+    "passport-google-oauth": "^1.0.0",
+    "passport-http": "^0.3.0",
+    "passport-twitter": "^1.0.4",


I guess we don't need some of these for now (i.e. google, twitter)

ooops right :)

ferjm · 2017-01-13T16:34:09Z

src/config.js

    format: password,
    default: 'invalid'
  },
  adminSessionSecret: {


s/adminSessionSecret/sessionSecret/g

yeah, we discussed to do it in a separate patch because it's orthogonal and I didn't want to cram too much things into this patch. But I should probably revert the next change then.

ferjm · 2017-01-13T16:35:37Z

src/config.js

    port: {
      doc: 'Port where PostgreSQL is running',
-      format: 'port',
+      format: '*',


Ok, thanks for letting me know. Maybe add a comment explaining why the format is*.

ferjm · 2017-01-13T16:42:22Z

src/middlewares/auth.js

+      }
+    } else {
+      token = authQuery;
    }


Maybe:

const token = (authHeader && authHeader.split('Bearer ')[1]) || authQuery; if (!token) { return unauthorized(res); }

yeah why not; I can use a ternary which can be a tiny bit more readable.

ferjm · 2017-01-16T10:09:06Z

src/routes/auth/facebook.js

+const router = express.Router();
+
+const callbackURL =
+  `${config.get('publicHost')}/${config.get('version')}/auth/facebook/callback`;


Could we move the last part also to the config, please?

you mean the callback part ? In that case we can make callback the default ?

Just wondering: why do you think it's needed ?

Yes, basically all the hard-coded part. Having it in the config, in a common place for all IdP configuration, makes things a little bit easier, specially when trying to develop locally.

ferjm · 2017-01-16T10:09:52Z

src/routes/auth/facebook.js

+  },
+  function(req, accessToken, refreshToken, profile, cb) {
+    db()
+      .then(models => {


nit: weird indentation

ferjm · 2017-01-16T10:10:14Z

src/models/users.js

+          attributes: [],
+          where: userData.id, // this contains all user attributes
+        })
+          .then(() => userData);


Also, uber-nit: weird indentation.

ferjm · 2017-01-16T10:11:46Z

src/models/users.js

+  }, {
+    indexes: [{
+      unique: true,
+      fields: ['opaqueId', 'provider', 'ClientKey']


Could you add a comment about what's ClientKey for? Also, does it need to be in capital letters?

It's created automatically by sequelize with the association we create in associate.

I agree this is not really reader-friendly because the association is done quite a few lines below. A comment will definitely help here.

I can also configure explicitely the foreign key so that we control the name, and then we can have a lowercase as first letter. Do you think it's better even with the comment ?

If it is not too much effort, configuring the fk explicitly sounds good. It's not a big deal though.

ferjm · 2017-01-16T10:20:11Z

src/routes/auth/facebook.js

+
+function checkClientExists(req, res, next) {
+  db()
+    .then(({ Clients }) => Clients.findById(req.user.id))


Could we s/req.user/req.client/g?

actually, it seems to be a standard practice in express apps that the result of the authentications is in req.user. Also req.user can contains the information for clients, users or admin (and later: sensors).

The alternative is that instead of having a scope we could either set req.user req.client req.admin and req.sensor with the information we currently have in id. This looks quite more elegant to me. What do you think ?

Yes, this looks more elegant. Thanks.

Also, just to be extra careful, could you exclude the client secret from the query, please?

julienw · 2017-01-18T20:14:39Z

src/config.js

+          userAuth.cookieSecure. Also with this Express will trust the
+          X-Forwarded-For header. Set to 1 or auto if you're behind a proxy.`,
+    default: false,
+    format: '*',


format is "*" because otherwise convict infers it's a boolean from the default and will refuse 1 or "auto".

Could you add all these comments as in code comments, please?

julienw · 2017-01-18T20:17:00Z

src/middlewares/auth.js

-      const validScopes = ['admin'].filter(scopeIndex => {
-        return scopes.indexOf(scopeIndex) != -1;
+        req[decoded.scope] = decoded.id;
+        req.authScope = decoded.scope;


these 2 lines are repeated a few times, it's likely a good idea to factorize this, but maybe in a separate patch ?

Probably, but where are the repetitions? :)

there are 3 locations where we set req[XXX.scope] and req.authScope: here, and in all auth/<method>.js (basic, facebook, and later google and fxaccounts likely).

That said, maybe we don't want this in auth/<method>.js where it's only consumed by finalizeAuth; we could simply use req.user or even something else like req.auth.

I'll think of this and maybe we can rework it when implementing google and fx accounts.

File another issue and reference it in the code, please :)

after all I just did what I suggested in a separate commit. Easier than creating an issue :) Tell me what you think.

coveralls · 2017-01-18T20:17:42Z

Coverage decreased (-0.5%) to 85.561% when pulling df6d2a9 on julienw:user-management-3 into 4900a97 on mozilla-sensorweb:master.

julienw · 2017-01-18T20:20:26Z

src/server.js

 app.use(bodyParser.json());

-if (config.get('env') !== 'test') {
+if (config.get('env') !== 'test' || process.env.FORCE_OUTPUT) {


to be able to see the output in tests
The alternative is to have an env like "QUIET=1" to remove the logger, and set it in travis.

That said, I usually prefer to have more log than less log :)

I'm fine with both options as long as we have a way to keep the logging quite.

julienw · 2017-01-18T20:21:45Z

test/common.js

+const endpointPrefix = `/${config.get('version')}`;
+
+export function loginAsAdmin(server) {
+  return co(function*() {


if you're not familiar with co, it returns a Promise and takes a generator as argument; this makes it easier to deal with Promise-returning functions. yield will wait for the Promise to be resolved, rejection translates to exceptions, etc. This is so good :)

Here we could just use promise chains because it stays quite simple, still I think it's more readable like this.

julienw · 2017-01-18T20:23:12Z

test/mocha.opts

@@ -0,0 +1,2 @@
+--require co-mocha


This is a sugar helper to wrap the mocha tests in co if it's a generator.

julienw · 2017-01-18T20:24:24Z

test/test_auth_api.js

+              done();
+            });
+    });
+  });


I only grouped them inside the same describe + the endpoint, no other change. There are few things we could do though: template strings, returning a promise instead of using done, etc.

File issues for these suggestions and reference them in the code, please.

julienw · 2017-01-18T20:24:46Z

test/test_auth_api.js

+} from './common';
+
+const endpointPrefix = '/' + config.get('version');
+const server = supertest(app);


it's not an agent anymore; an agent keeps state between calls and I think we should do it only when needed.

julienw · 2017-01-18T20:25:38Z

test/test_clients_api.js

  before((done) => {
    const pass = btoa('admin:' + config.get('adminPass'));
-    server.post(endpointPrefix + '/users/auth')
+    server.post(endpointPrefix + '/auth/basic')


this "before" could use the common file now, but maybe later in a separate patch

Could you file an issue and add a XXX comment here referencing it, please?

julienw · 2017-01-18T20:25:54Z

test/test_users_api.js

@@ -1,76 +0,0 @@
-import btoa       from 'btoa';


renamed to test_auth_api

julienw · 2017-01-18T20:26:24Z

package.json

    "should": "^11.1.0",
-    "supertest": "^2.0.0"
+    "supertest": "^2.0.0",
+    "supertest-as-promised": "^4.0.2"


this is nice as we can work with promises instead of using end + done callback.

julienw · 2017-01-18T20:28:35Z

I haven't checked if travis/circleci works yet. I think circleci doesn't because I haven't adapted the config file yet.

also there is a breaking change: the endpoint to login changed, so we'll need to update the admin panel.

coveralls · 2017-01-18T20:31:30Z

Coverage decreased (-0.5%) to 85.561% when pulling 00649fa on julienw:user-management-3 into 4900a97 on mozilla-sensorweb:master.

julienw · 2017-01-18T20:34:38Z

CircleCi is green now, so @ferjm r?

If you review only the changes from last time, please note I reverted some of the lint changes I did in the first patch. So if you see semicolons disappearing, dont be surprised :)

ferjm

Thank you Julien. Great work!

ferjm · 2017-01-19T09:51:36Z

package.json

    "should": "^11.1.0",
-    "supertest": "^2.0.0"
+    "supertest": "^2.0.0",
+    "supertest-as-promised": "^4.0.2"


ferjm · 2017-01-19T09:52:07Z

src/config.js

+          userAuth.cookieSecure. Also with this Express will trust the
+          X-Forwarded-For header. Set to 1 or auto if you're behind a proxy.`,
+    default: false,
+    format: '*',


Could you add all these comments as in code comments, please?

ferjm · 2017-01-19T09:55:45Z

src/middlewares/auth.js

-      const validScopes = ['admin'].filter(scopeIndex => {
-        return scopes.indexOf(scopeIndex) != -1;
+        req[decoded.scope] = decoded.id;
+        req.authScope = decoded.scope;


Probably, but where are the repetitions? :)

ferjm · 2017-01-19T10:21:44Z

test/test_clients_api.js

  before((done) => {
    const pass = btoa('admin:' + config.get('adminPass'));
-    server.post(endpointPrefix + '/users/auth')
+    server.post(endpointPrefix + '/auth/basic')


Could you file an issue and add a XXX comment here referencing it, please?

ferjm · 2017-01-19T10:23:43Z

test/test_auth_api.js

+              done();
+            });
+    });
+  });


File issues for these suggestions and reference them in the code, please.

ferjm · 2017-01-19T10:24:51Z

test/test_auth_api.js

+                           .expect('location', /facebook\.com/)
+                           .expect('set-cookie', /^connect\.sid\.auth=/);
+
+      // This is Faacebook's anti-CSRF protection


typo: Facebook

coveralls · 2017-01-19T14:41:13Z

Coverage decreased (-0.5%) to 85.561% when pulling 2b93393 on julienw:user-management-3 into 4900a97 on mozilla-sensorweb:master.

coveralls · 2017-01-19T15:45:48Z

Coverage decreased (-0.4%) to 85.6% when pulling c5a5f6b on julienw:user-management-3 into 4900a97 on mozilla-sensorweb:master.

julienw commented Jan 13, 2017

View reviewed changes

src/models/users.js

return Promise.resolve({

id: 'admin',

scope: 'admin'

});

Copy link

Collaborator Author

julienw Jan 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

basic authentication is for admin only

julienw commented Jan 13, 2017

View reviewed changes

ferjm reviewed Jan 16, 2017

View reviewed changes

julienw added 15 commits January 18, 2017 18:27

first work

10a9c7d

facebook strategy

1b4a928

Facebook flow works

05d2eb3

store token in localStorage

002a80c

Better file naming

631688a

Add user in the DB

92085bf

Configuration precisions

0598b42

minor fix

f365472

makes user registration work

e8520e7

unique key includes a client

4e48c20

force a client in the schema

7eeb53d

some changes to get clients

590ead3

make things somewhat work

cb528f3

bad basic auth is a proprt 401

cf40a02

full facebook authentication works

cfdb882

julienw added 3 commits January 18, 2017 20:40

ClientKey -> clientKey

d4c32d5

move from req.user to req[scope]

89e8dfc

add some logs

07ba7f7

julienw force-pushed the user-management-3 branch from 7638745 to 07ba7f7 Compare January 18, 2017 20:13

julienw commented Jan 18, 2017

View reviewed changes

revert lint fix

df6d2a9

julienw commented Jan 18, 2017

View reviewed changes

test/test_users_api.js

@@ -1,76 +0,0 @@

import btoa from 'btoa';

Copy link

Collaborator Author

julienw Jan 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed to test_auth_api

julienw commented Jan 18, 2017

View reviewed changes

circleci config

00649fa

ferjm approved these changes Jan 19, 2017

View reviewed changes

julienw mentioned this pull request Jan 19, 2017

Use common's functions where possible #60

Open

Last review comments

2b93393

julienw changed the title ~~Implement authentication through a Facebook flow~~ Implement authentication through a Facebook flow (Closes #24) Jan 19, 2017

julienw changed the title ~~Implement authentication through a Facebook flow (Closes #24)~~ Implement authentication through a Facebook flow Jan 19, 2017

julienw mentioned this pull request Jan 19, 2017

Change the user authentication endpoint to follow latest server changes mozilla-sensorweb/sensorweb-admin-panel#11

Merged

Use only req.user during authentication

c5a5f6b

ferjm merged commit d17ece3 into mozilla-sensorweb:master Jan 19, 2017


		const router = express.Router();

		router.use('/basic', basic);

Conversation

julienw commented Jan 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julienw commented Jan 13, 2017

Uh oh!

julienw commented Jan 13, 2017

Uh oh!

ferjm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

julienw commented Jan 13, 2017 •

edited

Loading