Merge pull request #713 from Gym/0.4.0

BUG: fix non-admin user edit route.  Broke with admin feature

modules/users/server/controllers/admin.server.controller.js

@@ -74,6 +74,12 @@ exports.list = function (req, res) {
  * User middleware
  */
 exports.userByID = function (req, res, next, id) {
+	if (!mongoose.Types.ObjectId.isValid(id)) {
+		return res.status(400).send({
+			message: 'User is invalid'
+		});
+	}
+
 	User.findById(id, '-salt -password').exec(function (err, user) {
 		if (err) return next(err);
 		if (!user) return next(new Error('Failed to load user ' + id));

modules/users/server/controllers/users/users.authorization.server.controller.js

@@ -10,10 +10,16 @@ var _ = require('lodash'),
 /**
  * User middleware
  */
-exports.userByID = function(req, res, next, id) {
+exports.userByID = function (req, res, next, id) {
+	if (!mongoose.Types.ObjectId.isValid(id)) {
+		return res.status(400).send({
+			message: 'User is invalid'
+		});
+	}
+
 	User.findOne({
 		_id: id
-	}).exec(function(err, user) {
+	}).exec(function (err, user) {
 		if (err) return next(err);
 		if (!user) return next(new Error('Failed to load User ' + id));
 		req.profile = user;

modules/users/server/policies/admin.server.policies.js → modules/users/server/policies/admin.server.policy.js

@@ -9,7 +9,7 @@ var acl = require('acl');
 acl = new acl(new acl.memoryBackend());
 
 /**
- * Invoke Articles Permissions
+ * Invoke Admin Permissions
  */
 exports.invokeRolesPolicies = function () {
 	acl.allow([{

modules/users/server/routes/admin.server.routes.js

@@ -3,19 +3,22 @@
 /**
  * Module dependencies.
  */
-var adminPolicy = require('../policies/admin.server.policies'),
+var adminPolicy = require('../policies/admin.server.policy'),
 	admin = require('../controllers/admin.server.controller');
 
 module.exports = function (app) {
+	// User route registration first. Ref: #713
+	require('./users.server.routes.js')(app);
+
 	// Users collection routes
-	app.route('/api/users').all(adminPolicy.isAllowed)
-		.get(admin.list);
+	app.route('/api/users')
+		.get(adminPolicy.isAllowed, admin.list);
 
 	// Single user routes
-	app.route('/api/users/:userId').all(adminPolicy.isAllowed)
-		.get(admin.read)
-		.put(admin.update)
-		.delete(admin.delete);
+	app.route('/api/users/:userId')
+		.get(adminPolicy.isAllowed, admin.read)
+		.put(adminPolicy.isAllowed, admin.update)
+		.delete(adminPolicy.isAllowed, admin.delete);
 
 	// Finish by binding the user middleware
 	app.param('userId', admin.userByID);

modules/users/server/routes/users.server.routes.js

@@ -1,6 +1,6 @@
 'use strict';
 
-module.exports = function(app) {
+module.exports = function (app) {
 	// User Routes
 	var users = require('../controllers/users.server.controller');
 

modules/users/tests/server/user.server.routes.tests.js

@@ -0,0 +1,107 @@
+'use strict';
+
+var should = require('should'),
+	request = require('supertest'),
+	path = require('path'),
+	mongoose = require('mongoose'),
+	User = mongoose.model('User'),
+	express = require(path.resolve('./config/lib/express'));
+
+/**
+ * Globals
+ */
+var app, agent, credentials, user, admin;
+
+/**
+ * User routes tests
+ */
+describe('User CRUD tests', function () {
+	before(function (done) {
+		// Get application
+		app = express.init(mongoose);
+		agent = request.agent(app);
+
+		done();
+	});
+
+	beforeEach(function (done) {
+		// Create user credentials
+		credentials = {
+			username: 'username',
+			password: 'password'
+		};
+
+		// Create a new user
+		user = new User({
+			firstName: 'Full',
+			lastName: 'Name',
+			displayName: 'Full Name',
+			email: '[email protected]',
+			username: credentials.username,
+			password: credentials.password,
+			provider: 'local'
+		});
+
+		// Save a user to the test db and create new article
+		user.save(function () {
+			done();
+		});
+	});
+
+	it('should not be able to retrieve a list of users if not admin', function (done) {
+		agent.post('/api/auth/signin')
+			.send(credentials)
+			.expect(200)
+			.end(function (signinErr, signinRes) {
+				// Handle signin error
+				if (signinErr) {
+					return done(signinErr);
+				}
+
+				// Save a new article
+				agent.get('/api/users')
+					.expect(403)
+					.end(function (usersGetErr, usersGetRes) {
+						if (usersGetErr) {
+							return done(usersGetErr);
+						}
+
+						return done();
+					});
+			});
+	});
+
+	it('should be able to retrieve a list of users if admin', function (done) {
+		user.roles = ['user', 'admin'];
+
+		user.save(function () {
+			agent.post('/api/auth/signin')
+				.send(credentials)
+				.expect(200)
+				.end(function (signinErr, signinRes) {
+					// Handle signin error
+					if (signinErr) {
+						return done(signinErr);
+					}
+
+					// Save a new article
+					agent.get('/api/users')
+						.expect(200)
+						.end(function (usersGetErr, usersGetRes) {
+							if (usersGetErr) {
+								return done(usersGetErr);
+							}
+
+							usersGetRes.body.should.be.instanceof(Array).and.have.lengthOf(1);
+
+							// Call the assertion callback
+							done();
+						});
+				});
+		});
+	});
+
+	afterEach(function (done) {
+		User.remove().exec(done);
+	});
+});