feat: update unban logic/invocation and refactor User.bans module

* auto unban when User.getUsersFields is called and the user is banned but has expired

* cleanups and removal of expiry_readable

* expiry_readable make an alias for backward compatibility

* User.bans.func vs User.*ban*Func

* console.log cleanups, plus todo message added

* use code util.deprecate

* fix: remove ununsed winston require

src/controllers/authentication.js

@@ -446,7 +446,7 @@ authenticationController.localLogin = function (req, username, password, next) {
 					user.isAdminOrGlobalMod(uid, next);
 				},
 				banned: function (next) {
-					user.isBanned(uid, next);
+					user.bans.isBanned(uid, next);
 				},
 				hasLoginPrivilege: function (next) {
 					privileges.global.can('local:login', uid, next);
@@ -551,7 +551,7 @@ function getBanInfo(uid, callback) {
 			});
 		},
 		function (next) {
-			next(new Error(banInfo.expiry ? '[[error:user-banned-reason-until, ' + banInfo.expiry_readable + ', ' + banInfo.reason + ']]' : '[[error:user-banned-reason, ' + banInfo.reason + ']]'));
+			next(new Error(banInfo.banned_until ? '[[error:user-banned-reason-until, ' + banInfo.banned_until_readable + ', ' + banInfo.reason + ']]' : '[[error:user-banned-reason, ' + banInfo.reason + ']]'));
 		},
 	], function (err) {
 		if (err) {

src/middleware/header.js

@@ -108,8 +108,8 @@ module.exports = function (middleware) {
 					},
 					navigation: async.apply(navigation.get, req.uid),
 					tags: async.apply(meta.tags.parse, req, data, res.locals.metaTags, res.locals.linkTags),
-					banned: async.apply(user.isBanned, req.uid),
+					banned: async.apply(user.bans.isBanned, req.uid),
-					banReason: async.apply(user.getBannedReason, req.uid),
+					banReason: async.apply(user.bans.getReason, req.uid),
 
 					unreadData: async.apply(topics.getUnreadData, { uid: req.uid }),
 					unreadChatCount: async.apply(messaging.getUnreadCount, req.uid),

src/socket.io/user/ban.js

@@ -55,7 +55,7 @@ module.exports = function (SocketUser) {
 		toggleBan(socket.uid, uids, function (uid, next) {
 			async.waterfall([
 				function (next) {
-					user.unban(uid, next);
+					user.bans.unban(uid, next);
 				},
 				function (next) {
 					events.log({
@@ -124,7 +124,7 @@ module.exports = function (SocketUser) {
 				});
 			},
 			function (next) {
-				user.ban(uid, until, reason, next);
+				user.bans.ban(uid, until, reason, next);
 			},
 			function (banData, next) {
 				db.setObjectField('uid:' + uid + ':ban:' + banData.timestamp, 'fromUid', callerUid, next);

src/user/bans.js

@@ -1,11 +1,13 @@
 'use strict';
 
-var async = require('async');
+const util = require('util');
-
+const async = require('async');
-var db = require('../database');
+const db = require('../database');
 
 module.exports = function (User) {
-	User.ban = function (uid, until, reason, callback) {
+	User.bans = {};
+
+	User.bans.ban = function (uid, until, reason, callback) {
 		// "until" (optional) is unix timestamp in milliseconds
 		// "reason" (optional) is a string
 		if (!callback && typeof until === 'function') {
@@ -43,8 +45,6 @@ module.exports = function (User) {
 		if (until > now) {
 			tasks.push(async.apply(db.sortedSetAdd, 'users:banned:expire', until, uid));
 			tasks.push(async.apply(User.setUserField, uid, 'banned:expire', until));
-		} else {
-			until = 0;
 		}
 
 		async.series(tasks, function (err) {
@@ -52,7 +52,7 @@ module.exports = function (User) {
 		});
 	};
 
-	User.unban = function (uid, callback) {
+	User.bans.unban = function (uid, callback) {
 		async.waterfall([
 			function (next) {
 				User.setUserFields(uid, { banned: 0, 'banned:expire': 0 }, next);
@@ -63,36 +63,50 @@ module.exports = function (User) {
 		], callback);
 	};
 
-	User.isBanned = function (uid, callback) {
+	User.bans.getBannedAndExpired = function (uid, callback) {
 		if (parseInt(uid, 10) <= 0) {
 			return setImmediate(callback, null, false);
 		}
-		async.waterfall([
+		User.getUserFields(uid, ['banned', 'banned:expire'], function (err, userData) {
-			async.apply(User.getUserFields, uid, ['banned', 'banned:expire']),
+			if (err) {
-			function (userData, next) {
+				return callback(err);
-				var banned = userData && userData.banned;
-				if (!banned) {
-					return next(null, banned);
 			}
-
+			callback(null, User.bans.calcExpiredFromUserData(userData));
-				// If they are banned, see if the ban has expired
-				var stillBanned = !userData['banned:expire'] || Date.now() < userData['banned:expire'];
-
-				if (stillBanned) {
-					return next(null, true);
-				}
-				async.parallel([
-					async.apply(db.sortedSetRemove.bind(db), 'users:banned:expire', uid),
-					async.apply(db.sortedSetRemove.bind(db), 'users:banned', uid),
-					async.apply(User.setUserFields, uid, { banned: 0, 'banned:expire': 0 }),
-				], function (err) {
-					next(err, false);
 		});
-			},
-		], callback);
 	};
 
-	User.getBannedReason = function (uid, callback) {
+	User.bans.calcExpiredFromUserData = function (userData) {
+		return {
+			banned: !!userData.banned,
+			'banned:expire': userData['banned:expire'],
+			banExpired: userData['banned:expire'] <= Date.now() && userData['banned:expire'] !== 0,
+		};
+	};
+
+	User.bans.unbanIfExpired = function (uid, callback) {
+		User.bans.getBannedAndExpired(uid, function (err, result) {
+			if (err) {
+				return callback(err);
+			}
+			if (result.banned && result.banExpired) {
+				return User.bans.unban(uid, function (err) {
+					callback(err, { banned: false, banExpired: true, 'banned:expire': 0 });
+				});
+			}
+			callback(null, result);
+		});
+	};
+
+	User.bans.isBanned = function (uid, callback) {
+		if (parseInt(uid, 10) <= 0) {
+			return setImmediate(callback, null, false);
+		}
+		User.bans.unbanIfExpired(uid, function (err, result) {
+			callback(err, result.banned);
+		});
+	};
+
+	User.bans.getReason = function (uid, callback) {
 		if (parseInt(uid, 10) <= 0) {
 			return setImmediate(callback, null, '');
 		}
@@ -111,4 +125,14 @@ module.exports = function (User) {
 			},
 		], callback);
 	};
+
+	// TODO Remove in v1.13.0
+	const deprecatedMessage = (oldPath, newPath) => `function ${oldPath} is deprecated, please use ${newPath} instead`;
+	User.ban = util.deprecate(User.bans.ban, deprecatedMessage('User.ban', 'User.bans.ban'));
+	User.unban = util.deprecate(User.bans.unban, deprecatedMessage('User.unban', 'User.bans.unban'));
+	User.getBannedAndExpired = util.deprecate(User.bans.getBannedAndExpired, deprecatedMessage('User.getBannedAndExpired', 'User.bans.getBannedAndExpired'));
+	User.calcBanExpiredFromUserData = util.deprecate(User.bans.calcExpiredFromUserData, deprecatedMessage('User.calcBanExpiredFromUserData', 'User.bans.calcExpiredFromUserData'));
+	User.unbanIfBanExpired = util.deprecate(User.bans.unbanIfExpired, deprecatedMessage('User.unbanIfBanExpired', 'User.bans.unbanIfExpired'));
+	User.isBanned = util.deprecate(User.bans.isBanned, deprecatedMessage('User.isBanned', 'User.bans.isBanned'));
+	User.getBannedReason = util.deprecate(User.bans.getReason, deprecatedMessage('User.getBannedReason', 'User.bans.getReason'));
 };

src/user/data.js

@@ -74,6 +74,10 @@ module.exports = function (User) {
 			addField('lastonline');
 		}
 
+		if (fields.includes('banned') && !fields.includes('banned:expire')) {
+			addField('banned:expire');
+		}
+
 		var uniqueUids = _.uniq(uids).filter(uid => uid > 0);
 
 		async.waterfall([
@@ -148,9 +152,9 @@ module.exports = function (User) {
 	}
 
 	function modifyUserData(users, requestedFields, fieldsToRemove, callback) {
-		users.forEach(function (user) {
+		async.map(users, function (user, next) {
 			if (!user) {
-				return;
+				return next(null, user);
 			}
 
 			db.parseIntFields(user, intFields, requestedFields);
@@ -211,13 +215,28 @@ module.exports = function (User) {
 				user.lastonlineISO = utils.toISOString(user.lastonline) || user.joindateISO;
 			}
 
-			if (user.hasOwnProperty('banned:expire')) {
+			if (user.hasOwnProperty('banned') || user.hasOwnProperty('banned:expire')) {
-				user.banned_until = user['banned:expire'];
+				var result = User.bans.calcExpiredFromUserData(user);
-				user.banned_until_readable = user.banned_until ? new Date(user.banned_until).toString() : 'Not Banned';
+				var unban = result.banned && result.banExpired;
+				user.banned_until = unban ? 0 : user['banned:expire'];
+				user.banned_until_readable = user.banned_until && !unban ? utils.toISOString(user.banned_until) : 'Not Banned';
+				if (unban) {
+					return User.bans.unban(user.uid, function (err) {
+						if (err) {
+							return next(err);
 						}
+						user.banned = false;
+						next(null, user);
 					});
-
+				}
+			}
+			next(null, user);
+		}, function (err, users) {
+			if (err) {
+				return callback(err);
+			}
 			plugins.fireHook('filter:users.get', users, callback);
+		});
 	}
 
 	function parseGroupTitle(user) {

src/user/digest.js

@@ -79,7 +79,7 @@ Digest.getSubscribers = function (interval, callback) {
 		},
 		function (subscribers, next) {
 			async.filter(subscribers, function (uid, next) {
-				user.isBanned(uid, function (err, banned) {
+				user.bans.isBanned(uid, function (err, banned) {
 					next(err, !banned);
 				});
 			}, next);

src/user/info.js

@@ -24,13 +24,16 @@ module.exports = function (User) {
 				db.getObject(record[0], next);
 			},
 			function (banInfo, next) {
-				var expiry = banInfo.expire;
+				const expire = parseInt(banInfo.expire, 10);
+				const expire_readable = utils.toISOString(expire);
 
 				next(null, {
 					uid: uid,
 					timestamp: banInfo.timestamp,
-					expiry: parseInt(expiry, 10),
+					banned_until: expire,
-					expiry_readable: new Date(parseInt(expiry, 10)).toString(),
+					expiry: expire, /* backward compatible alias */
+					banned_until_readable: expire_readable,
+					expiry_readable: expire_readable, /* backward compatible alias */
 					reason: validator.escape(String(banInfo.reason || '')),
 				});
 			},

test/authentication.js

@@ -8,6 +8,7 @@ var async = require('async');
 
 var db = require('./mocks/databasemock');
 var user = require('../src/user');
+var utils = require('../src/utils');
 var meta = require('../src/meta');
 var privileges = require('../src/privileges');
 var helpers = require('./helpers');
@@ -456,21 +457,21 @@ describe('authentication', function () {
 	it('should prevent banned user from logging in', function (done) {
 		user.create({ username: 'banme', password: '123456', email: 'ban@me.com' }, function (err, uid) {
 			assert.ifError(err);
-			user.ban(uid, 0, 'spammer', function (err) {
+			user.bans.ban(uid, 0, 'spammer', function (err) {
 				assert.ifError(err);
 				loginUser('banme', '123456', function (err, res, body) {
 					assert.ifError(err);
 					assert.equal(res.statusCode, 403);
 					assert.equal(body, '[[error:user-banned-reason, spammer]]');
-					user.unban(uid, function (err) {
+					user.bans.unban(uid, function (err) {
 						assert.ifError(err);
 						var expiry = Date.now() + 10000;
-						user.ban(uid, expiry, '', function (err) {
+						user.bans.ban(uid, expiry, '', function (err) {
 							assert.ifError(err);
 							loginUser('banme', '123456', function (err, res, body) {
 								assert.ifError(err);
 								assert.equal(res.statusCode, 403);
-								assert.equal(body, '[[error:user-banned-reason-until, ' + (new Date(parseInt(expiry, 10)).toString()) + ', No reason given.]]');
+								assert.equal(body, '[[error:user-banned-reason-until, ' + utils.toISOString(expiry) + ', No reason given.]]');
 								done();
 							});
 						});

test/socket.io.js

@@ -132,8 +132,8 @@ describe('socket.io', function () {
 				assert.ifError(err);
 				assert(data.uid);
 				assert(data.timestamp);
-				assert(data.hasOwnProperty('expiry'));
+				assert(data.hasOwnProperty('banned_until'));
-				assert(data.hasOwnProperty('expiry_readable'));
+				assert(data.hasOwnProperty('banned_until_readable'));
 				assert.equal(data.reason, 'spammer');
 				done();
 			});
@@ -141,7 +141,7 @@ describe('socket.io', function () {
 	});
 
 	it('should return ban reason', function (done) {
-		user.getBannedReason(regularUid, function (err, reason) {
+		user.bans.getReason(regularUid, function (err, reason) {
 			assert.ifError(err);
 			assert.equal(reason, 'spammer');
 			done();
@@ -152,7 +152,7 @@ describe('socket.io', function () {
 		var socketUser = require('../src/socket.io/user');
 		socketUser.unbanUsers({ uid: adminUid }, [regularUid], function (err) {
 			assert.ifError(err);
-			user.isBanned(regularUid, function (err, isBanned) {
+			user.bans.isBanned(regularUid, function (err, isBanned) {
 				assert.ifError(err);
 				assert(!isBanned);
 				done();

test/topics.js

@@ -879,7 +879,7 @@ describe('Topic\'s', function () {
 				title: 'topic for controller test',
 				content: 'topic content',
 				cid: topic.categoryId,
-				thumb: 'http://i.imgur.com/64iBdBD.jpg',
+				// thumb: 'http://i.imgur.com/64iBdBD.jpg',
 			}, function (err, result) {
 				assert.ifError(err);
 				assert.ok(result);

test/user.js

@@ -1114,7 +1114,7 @@ describe('User', function () {
 		it('should return the correct ban reason', function (done) {
 			async.series([
 				function (next) {
-					User.ban(testUid, 0, '', function (err) {
+					User.bans.ban(testUid, 0, '', function (err) {
 						assert.ifError(err);
 						next(err);
 					});
@@ -1130,7 +1130,7 @@ describe('User', function () {
 				},
 			], function (err) {
 				assert.ifError(err);
-				User.unban(testUid, function (err) {
+				User.bans.unban(testUid, function (err) {
 					assert.ifError(err);
 					done();
 				});
@@ -1138,28 +1138,28 @@ describe('User', function () {
 		});
 
 		it('should ban user permanently', function (done) {
-			User.ban(testUid, function (err) {
+			User.bans.ban(testUid, function (err) {
 				assert.ifError(err);
-				User.isBanned(testUid, function (err, isBanned) {
+				User.bans.isBanned(testUid, function (err, isBanned) {
 					assert.ifError(err);
 					assert.equal(isBanned, true);
-					User.unban(testUid, done);
+					User.bans.unban(testUid, done);
 				});
 			});
 		});
 
 		it('should ban user temporarily', function (done) {
-			User.ban(testUid, Date.now() + 2000, function (err) {
+			User.bans.ban(testUid, Date.now() + 2000, function (err) {
 				assert.ifError(err);
 
-				User.isBanned(testUid, function (err, isBanned) {
+				User.bans.isBanned(testUid, function (err, isBanned) {
 					assert.ifError(err);
 					assert.equal(isBanned, true);
 					setTimeout(function () {
-						User.isBanned(testUid, function (err, isBanned) {
+						User.bans.isBanned(testUid, function (err, isBanned) {
 							assert.ifError(err);
 							assert.equal(isBanned, false);
-							User.unban(testUid, done);
+							User.bans.unban(testUid, done);
 						});
 					}, 3000);
 				});
@@ -1167,7 +1167,7 @@ describe('User', function () {
 		});
 
 		it('should error if until is NaN', function (done) {
-			User.ban(testUid, 'asd', function (err) {
+			User.bans.ban(testUid, 'asd', function (err) {
 				assert.equal(err.message, '[[error:ban-expiry-missing]]');
 				done();
 			});