Use data models correctly

[ProZachJ]

Currently, many of our routes are using our mongoose models as simple references for interacting with the database. This causes a "mixing of concerns", and leads to complex functions that do way too many things (#callbackhell). The main concerns of a route should be:

• Define Inputs
• Define response logic (in the form of a callback or promise)
• Pass inputs to data model

If complex logic is needed to query and/or modify the db this should be handled in the model.

See http://closurelog.com/static-instance-methods-mongoose-mongodb/ for better understanding of how we should be doing things.

For example, the Host model should contain an "instance method" for adding an IP to the blacklist. So that given an instance (document) from the Host model, you can simply add the IP by calling something like:

myhost.blockIP(ip, cb)

It could also contain a "static method" so that given a hostname you could call a single method to fetch the needed document and add the block:

Host.blockIPHostname(hostname, ip, cb)

The above method would do the query for the host document and then call the blockIP instance method from that document.

[ProZachJ]

Also things might be made easier in this circumstance by putting the actual host instances into the users session instead of just an object containing the hostnames. Although this would likely require some more complex logic in our loadUser middle route since currently we query the User model here (you would need an additional query to the Host model to get the actual instances).

[ProZachJ]

Our user creation code is already using static and instance methods.

[ProZachJ]

Here is a good example of the three concerns:

exports.traffic = function getRange (req, res) {
  //Define Inputs
  var sitesArray = [];
  for (var site in req.session.user.sites) {
    if (req.session.user.sites.hasOwnProperty(site)) {
      var name = req.session.user.sites[site].name;
      sitesArray.push(name);
    }
  }
  //Define Response Logic
  var respond = function (err, docs) {
    res.send(docs);
  };
  //Pass inputs to data model
  RequestStore.find({'headers.host': { $in : sitesArray }, 'requestedtimestamp' : { $gte : new Date(req.body.start), $lt : new Date(req.body.end) } }, respond);
};

In this case the logic is simple enough to use the data model's build in instance methods (find), but in some cases we'll need to define custom instance methods to keep the logic out of the route.

[ProZachJ]

@mattjay I got this accomplished for blocking an IP. Still need to change a couple names and test.

Route

exports.toggleBlock = function toggleBlock (req, res) {
  var respond = function(err, doc, numaffected){
    if(numaffected){
      res.send({blocked: true});
    }else{
      console.log(err, err.doc);
      res.send({blocked: false});
    }
  };
  //needs validation
  var ip = req.body.ip;
  var hostname = req.body.host.replace(/\./g, "");
  var authorized = false;

  //make sure hostname is owned by user
  req.session.user.sites.forEach(function(site){
    if(site.hostname === req.body.host){
      authorized = true;
      Host.blockHostIP(hostname, ip, respond);
    }
  });
  if(!authorized){
    var err = 'Not authorized to modify blacklist for host' + req.session.user._id;
    respond(err);
  }

Schema Methods

hostSchema.statics.blockHostIP = function(hostname, ip, cb){
  this.findOne({hostname: hostname}, function(err, host){
    if(!err && host){
      host.blockIP(ip, cb)
    }else{
      err.doc = doc;
      cb(err);
    }
  });
};

hostSchema.methods.blockIP = function(ip, cb){
  var host = this;
  if(!host.blacklist){
    //create blacklist array
    host.set('blacklist',[{ip: ip, time: 1000}]);
  }else{
    //add ip if not already blocked
    host.blacklist.addToSet({ip: ip, time: 1000});
  }
  host.save(cb);
};

This is kinda MVPish code here

• The authorization check is weak since req.session.user.sites ultimately comes from the users registration and not some sort of verified list of sites.
• It could probably be made more readable by using promises but I don't want to undertake that learning and refactoring atm.

[mattjay]

err.doc = doc

should be:

err.doc = host