issue 451 - MakeDrive auth should be pluggable, configurable, optional #463

alicoding · 2014-11-10T19:11:26Z

No description provided.

alicoding · 2014-11-11T00:26:35Z

@humphd can you r?

humphd · 2014-11-11T15:51:32Z

server/middleware.js

-    }
-
-    req.params.username = username;
+    passport.authenticate(env.get('PASSPORT_PROVIDER'), function(err, user, info) {


I think we need to abstract this a bit more. In order to use custom authentication strategies, you have to load other modules, modify the Passport object (e.g., passport.use(...)), and register a provider type. I think we should create a dir server/authentication and have a few files in there:

server/authentication/index.js - this will be a module that will require/initialize passport, figure out which provider to use based on the env settings, then dynamically load (i.e., require) the appropriate module, which will do the provider specific stuff. If you look at how you use Facebook, for example:

http://passportjs.org/guide/facebook/

You need code that can do all that extra stuff for Facebook, which needs to live in a separate module.

server/authentication/passport-webmaker.js - this is the default for webmaker.org, and it will work sort of like the Passport Local provider, but inspect the cookie and make sure we have the username attached.

I'm happy to sketch this out in more detail over video if you want.

humphd · 2014-11-11T15:57:03Z

server/web-server.js

@@ -15,6 +15,30 @@ var middleware = require('./middleware');
 var routes = require('./routes');
 var log = require('./lib/logger.js');
 var nunjucks = require('nunjucks');
+var passport = require('passport');


All of the changes below are an example of what I am talking about above. This should all live in a file like server/authentication/passport-basic.js or something. We should be able to do all of this in that file, and swap it easily with another provider without having to modify the server code in any way. Right now that's impossible.

humphd · 2014-11-11T15:57:47Z

server/web-server.js

+    });
+  }
+));
+

 var app = express();
 var webmakerAuth = new WebmakerAuth({


This code is going to need to move to the server/authentication/passport-webmaker.js file, since we'll only do it for that provider type.

humphd · 2014-11-11T15:58:16Z

server/web-server.js

@@ -58,6 +82,7 @@ app.use(express.json());
 app.use(express.urlencoded());
 app.use(webmakerAuth.cookieParser());
 app.use(webmakerAuth.cookieSession());


I wonder if all this cookie parsing should get added by individual passport provider code when needed?

humphd · 2014-11-11T16:00:11Z

tests/lib/util.js

@@ -30,21 +30,6 @@ var app = server.app;
 var serverURL = 'http://127.0.0.1:' + env.get('PORT'),
    socketURL = serverURL.replace( 'http', 'ws' );

-// Mock Webmaker auth


I don't understand why you've changed all this? Why not have the tests configured to use a passport-webmaker.js provider, and then this fakes authentication for you. Nothing would need to change in this code, or the tests.

humphd · 2014-11-11T16:01:29Z

tests/lib/util.js

@@ -952,11 +892,10 @@ function ensureRemoteFilesystem(layout, jar, callback) {
 * FileSystem options on the options object.
 */
 function setupSyncClient(options, callback) {
-  authenticateAndConnect(options, function(err, result) {
+  getWebsocketToken(options, function(err, result) {


If you'd kept the same name, you wouldn't have needed to update all the tests. Why make work for yourself?

humphd · 2014-11-11T16:02:39Z

tests/unit/http-routes-tests.js

          if(err) throw err;

          // /z/ should come back as export.zip with one dir and file
          // in the archive.
          request.get({
            url: util.serverURL + '/z/',
-            jar: result.jar,
+            qs: {
+              username: env.get("PASSPORT_USERS"), password: env.get("PASSPORT_PASSWORD")


I don't like how we're causing these tests to have to know things about the environment in order to work. That means we'll get failures with misconfigured env files.

humphd · 2014-11-11T16:03:14Z

r-, let's discuss when you're around.

alicoding · 2014-11-13T19:16:36Z

tests/lib/util.js

@@ -115,23 +102,6 @@ function comparePaths(a, b) {
  return 0;
 }

-// Ensure that the file is downloadable via /p/ route


I don't see we use this anywhere at all, so I removed this.

humphd · 2014-11-13T19:17:09Z

README.md

+export GITHUB_CLIENTID="clientId"
+export GITHUB_CLIENTSECRET="clientSecret"
+export GITHUB_CALLBACKURL="http://callbackurl"
+export PASSPORT_PROVIDER="passport-github"


Remove, duplicate line.

humphd · 2014-11-13T19:17:28Z

README.md

+export PASSPORT_PROVIDER="passport-github"
+```
+
+You can also check other provider listed in the [providers directory](./server/authentication/providers) for more information.


s/other provider/other providers/ (plural)

humphd · 2014-11-13T19:18:12Z

env.dist

+# NOTE: You must include this provider in package.json or manually require it.
+export PASSPORT_PROVIDER="json"
+
+# PASSPORT Github authentication configurations


I'd remove the Github stuff, since it's not something we'll use. We can just document it in the code/readme.

humphd · 2014-11-13T19:19:31Z

server/authentication/index.js

@@ -0,0 +1,56 @@
+var env = require('../lib/environment');


Throughout this file, remove the padding spaces around arguments. Do f(a,b) vs. f( a, b )

humphd · 2014-11-13T19:20:30Z

server/authentication/index.js

+  passport.authenticate(passportProvider.name, function(err, user, info) {
+    // If something went wrong with authenticate method
+    if(err) {
+      return next( 500, err );


Add a log.error() call here too.

humphd · 2014-11-13T19:20:57Z

server/authentication/index.js

+    // Otherwise try to login, so we can have user's information added to the session
+    req.login(user, function(err) {
+      if(err) {
+        return next(err);


Add a log.error() call here too.

humphd · 2014-11-13T19:21:32Z

server/authentication/index.js

+      if (!req.user) {
+        return next( generateError( 401, "Unauthorized" ) );
+      }
+      // at this point put username to params


"Add username to params so it can be accessed elsewhere by MakeDrive"

humphd · 2014-11-13T19:22:08Z

server/authentication/index.js

+        return next( generateError( 401, "Unauthorized" ) );
+      }
+      // at this point put username to params
+      req.params.username = req.user;


Maybe add a log.debug() here to indicate which user just authenticated.

humphd · 2014-11-13T19:22:26Z

server/authentication/passport-strategies/passport-json-strategy.js

+var passport = require('passport-strategy');
+var env = require('../../lib/environment');
+
+/**


Move this comment block to the top of the file.

humphd · 2014-11-13T19:23:40Z

server/authentication/passport-strategies/passport-json-strategy.js

+}
+
+/**
+ * Authenticate request based on the environment configurations.


s/configurations/configuration/ (singular)

humphd · 2014-11-13T19:24:23Z

server/authentication/passport-strategies/passport-json-strategy.js

@@ -0,0 +1,86 @@
+// Module dependencies.


I don't think this should be called -json- since there's no JSON involved. You could call it passport-env to indicate it's reading info from the env.

humphd · 2014-11-13T19:25:01Z

server/authentication/passport-strategies/passport-json-strategy.js

+ */
+Strategy.prototype.authenticate = function(req, options) {
+  // reading username and password from environment configuration.
+  var username =  env.get("PASSPORT_LOCAL_USER");


You could read these at startup (i.e., outside this function, near the top of this file) since they won't change after startup.

humphd · 2014-11-13T19:25:36Z

server/authentication/passport-strategies/passport-json-strategy.js

+  var self = this;
+
+  function verified(err, user, info) {
+    if (err) { return self.error(err); }


Prefer newlines after brackes. Also, add logging for the failure cases.

humphd · 2014-11-13T19:25:49Z

server/authentication/passport-strategies/passport-json-strategy.js

+      this._verify(username, password, verified);
+    }
+  } catch (ex) {
+    return self.error(ex);


humphd · 2014-11-13T19:26:01Z

server/authentication/passport-strategies/passport-json-strategy.js

+};
+
+
+/**


This comment block isn't necessary.

humphd · 2014-11-13T19:26:19Z

server/authentication/passport-strategies/passport-webmaker-strategy.js

+// Module dependency.
+var passport = require('passport-strategy');
+
+/**


Move to the top of the file

humphd · 2014-11-13T19:27:16Z

server/authentication/passport-strategies/passport-webmaker-strategy.js

+ * @api protected
+ */
+Strategy.prototype.authenticate = function(req, options) {
+  // Webmaker Auth should already added user object to the cookie session.


"Webmaker Auth should have already added a user object to the cookie session."

humphd · 2014-11-13T19:27:34Z

server/authentication/passport-strategies/passport-webmaker-strategy.js

+  var password = "";
+  // if no username found we can assume that the user is not logged in to Webmaker
+  if (!username) {
+    return this.fail({ message: 'No user found in Webmaker session' }, 400);


This will be return to the authenticate method in the middleware which I'm doing that there.

humphd · 2014-11-13T20:11:25Z

server/authentication/index.js

+    if(err) {
+      log.error(err, 'Unable to authenticate with %s', passportProvider.name);
+      return next(500, err);
+    // If missing credentials or unknown user


Move this to line 41 (i.e., inside the {...)

humphd · 2014-11-13T20:12:12Z

server/authentication/passport-strategies/passport-env-strategy.js

+ * `Strategy` constructor.
+ *
+ * The local authentication strategy authenticates requests based on the
+ * credentials in the environment configuration option.


Get rid of "option" at the end of this.

humphd · 2014-11-13T20:12:35Z

server/authentication/passport-strategies/passport-env-strategy.js

+// reading username and password from environment configuration.
+var username =  env.get("PASSPORT_USER");
+var password = env.get("PASSPORT_PASSWORD");
+function Strategy(options, verify) {


Newline above this.

humphd · 2014-11-13T20:14:26Z

server/authentication/passport-strategies/passport-env-strategy.js

+      return self.error(err);
+    }
+    if (!user) {
+      log.error('failed: ', info);


This looks wrong. What does not having a user here mean? That they failed authentication? If so, you want something like, log.debug('Authentication failed for user %s', ...)

humphd · 2014-11-13T20:15:23Z

server/authentication/passport-strategies/passport-webmaker-strategy.js

+/**
+ * `Strategy` constructor.
+ *
+ * The webmaker authentication strategy authenticates requests based on the Webmaker session.


s/webmaker authentication/Webmaker Authentication/. Also add a link to the Webmaker Auth client repo, which has more docs.

humphd · 2014-11-13T20:16:16Z

server/authentication/passport-strategies/passport-webmaker-strategy.js

+
+  function verified(err, user, info) {
+    if (err) {
+      return self.error(err);


Same thing here, logging? You did it above for this and the next case.

humphd · 2014-11-13T20:16:46Z

server/authentication/providers/passport-env.js

@@ -0,0 +1,21 @@
+/**
+ * passport-env:
+ *   It allows single user to be authenticated for MakeDrive testing.


"Allow a single user to be authenticated for MakeDrive testing, using credentials in the env file."

humphd · 2014-11-13T20:17:20Z

server/authentication/providers/passport-local.js

@@ -0,0 +1,24 @@
+/**
+ * passport-local:


passport-query-string ?

humphd · 2014-11-13T20:31:37Z

tests/lib/util.js

@@ -142,87 +112,30 @@ function toSyncMessage(string) {
  return string;
 }

+function jar() {


Does this get used anywhere now?

I think it's being used in some of the test.

humphd · 2014-11-13T20:33:50Z

tests/lib/util.js

-
+  options.username = options.username || uniqueUsername();
+  env.set("PASSPORT_USERNAME", options.username);
+  env.set("PASSPORT_PASSWORD", 'secret');


The way you're doing this is wrong--you're forced to change the environment every time you authenticate. That speaks to the fact that we need a list of users. Basically, this is what the existing code was doing, by adding a route to register a new user so they would exist when this part of the code was called. We need a better solution here. In an ideal world, we'd do something for our tests where the value returned by uniqueUsername() would follow a pattern, and we'd allow any username with that pattern and the right password (i.e., the one set in PASSPORT_PASSWORD).

humphd · 2014-11-13T21:15:41Z

env.dist

+
+# PASSPORT query-string and env setup
+export PASSPORT_USERNAME="any_user_name"
+export PASSPORT_PASSWORD="secret"


Remove all this, since zeroconfig shouldn't need them.

humphd · 2014-11-13T21:15:52Z

README.md

+--------  | --------------
+`passport-github`   | Passport strategy for authenticating with GitHub using the OAuth 2.0 API.
+`passport-env`     | Passport strategy for authenticating using environment configuration.
+`passport-zeroconfig`     | Passport strategy for always authenticated root user.


"passport-zeroconfig (default)"

humphd · 2014-11-13T21:17:11Z

server/authentication/passport-strategies/passport-webmaker-strategy.js

+/**
+ * `Strategy` constructor.
+ *
+ * The Webmaker Authentication strategy authenticates requests based on the Webmaker session.


Add link to Webmaker Auth client repo for README.

humphd · 2014-11-13T21:18:14Z

server/authentication/passport-strategies/passport-zeroconfig-strategy.js

+/**
+ * `Strategy` constructor.
+ *
+ * The root strategy authenticates requests based on single username (root).


"The zeroconfig strategy always automatically authenticates requests for a single 'root' user. Only use this for development."

humphd · 2014-11-13T21:18:53Z

server/authentication/providers/passport-local.js

@@ -0,0 +1,24 @@
+/**


Change the filename to match passport-query-string.

humphd · 2014-11-13T21:19:13Z

server/authentication/providers/passport-webmaker.js

@@ -0,0 +1,42 @@
+/**
+ * passport-webmaker:
+ *   This provider is meant to be used for either local development or production, and requires a Webmaker Login server to be running.


Same thing here, add link to Webmaker Auth client repo.

humphd · 2014-11-13T21:19:45Z

server/authentication/providers/passport-zeroconfig.js

+ */
+
+var passport = require('passport');
+var ZEROCONFIGStrategy = require('../passport-strategies/passport-env-strategy').Strategy;


passport-env-strategy? Shouldn't that be zeroconfig?

humphd · 2014-11-13T21:20:38Z

server/web-server.js

 var Path = require('path');
 var http = require('http');
+var auth = require('./authentication');


./authentication.js (always use .js to signal it's a file vs. module name).

humphd · 2014-11-13T21:25:33Z

server/authentication/index.js

+      log.error(err, 'Unable to authenticate with %s', passportProvider.name);
+      return next(500, err);
+    } else if (info) {
+    // If missing credentials or unknown user


Indent this 2 spaces.

humphd · 2014-11-13T21:26:49Z

server/authentication/providers/passport-env.js

+ *   Allow a single user to be authenticated for MakeDrive testing, using credentials in the env file.
+ *   to use this provider make sure to set these environment configurations in your .env file
+ *   export PASSPORT_USERNAME="someusername"
+ *   export PASSPORT_PASSWORD="someusername"


s/someusername/somepassword/

humphd · 2014-11-13T21:28:30Z

server/authentication/providers/passport-webmaker.js

+  loginURL: env.get('LOGIN'),
+  secretKey: env.get('SESSION_SECRET'),
+  forceSSL: env.get('FORCE_SSL'),
+  domain: env.get('COOKIE_DOMAIN')


Should we log.error or throw if these env vars are missing? It might be a good idea, so you can figure out why things are broken.

Yeah, let's do it, here and in all the other's too.

humphd · 2014-11-13T21:29:14Z

server/authentication/passport-strategies/passport-env-strategy.js

+// reading username and password from environment configuration.
+var username =  env.get("PASSPORT_USERNAME");
+var password = env.get("PASSPORT_PASSWORD");
+


Let's check for username/password here, and log.error if they are missing so it's easier to debug.

humphd · 2014-11-13T21:30:25Z

server/authentication/providers/passport-github.js

+ *   to use this provider make sure to set these environment configurations in your .env file
+ *   export GITHUB_CLIENTID="clientId"
+ *   export GITHUB_CLIENTSECRET="clientSecret"
+ *   export GITHUB_CALLBACKURL="http://callbackurl"


Let's add a log.error() if any of these variables are missing from env.

humphd · 2014-11-13T21:31:44Z

server/authentication/providers/util.js

+
+function findByUsername(username, fn) {
+  if (env.get("PASSPORT_USERNAME") === username) {
+    return fn(null, env.get("PASSPORT_USERNAME"));


Instead of reading this from env all the time (here and below), just do it at startup and cache in a variable.

humphd · 2014-11-13T21:32:43Z

Fix the logging for missing env stuff, and the other things I just found. r=me with travis. Awesome work Ali!

alicoding force-pushed the alicoding:passport branch from fac9c3c to 05ba065 Nov 11, 2014

alicoding changed the title ~~WIP~~ issue 451 - MakeDrive auth should be pluggable, configurable, optional Nov 11, 2014

humphd reviewed Nov 11, 2014
View changes

alicoding reviewed Nov 13, 2014
View changes

tests/lib/util.js

@@ -115,23 +102,6 @@ function comparePaths(a, b) {

return 0;

}

// Ensure that the file is downloadable via /p/ route

This comment has been minimized.

Sign in to view

alicoding Nov 13, 2014
Author Member

I don't see we use this anywhere at all, so I removed this.