From 3fbb6faf28543d17954e7eb7aa6bb4519c21aab0 Mon Sep 17 00:00:00 2001 From: Aziz Khoury Date: Fri, 5 Apr 2019 21:14:49 +0300 Subject: [PATCH] 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 | 4 +- src/middleware/header.js | 4 +- src/socket.io/user/ban.js | 4 +- src/user/bans.js | 82 ++++++++++++++++++++----------- src/user/data.js | 33 ++++++++++--- src/user/digest.js | 2 +- src/user/info.js | 9 ++-- test/authentication.js | 9 ++-- test/socket.io.js | 8 +-- test/topics.js | 2 +- test/user.js | 20 ++++---- 11 files changed, 112 insertions(+), 65 deletions(-) diff --git a/src/controllers/authentication.js b/src/controllers/authentication.js index fa27ecaa31..061359394c 100644 --- a/src/controllers/authentication.js +++ b/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) { diff --git a/src/middleware/header.js b/src/middleware/header.js index 6f9fdb7321..5bdab71f55 100644 --- a/src/middleware/header.js +++ b/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), - banReason: async.apply(user.getBannedReason, req.uid), + banned: async.apply(user.bans.isBanned, 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), diff --git a/src/socket.io/user/ban.js b/src/socket.io/user/ban.js index 1084a82625..d84c672442 100644 --- a/src/socket.io/user/ban.js +++ b/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); diff --git a/src/user/bans.js b/src/user/bans.js index b82e64312b..5832dfea59 100644 --- a/src/user/bans.js +++ b/src/user/bans.js @@ -1,11 +1,13 @@ 'use strict'; -var async = require('async'); - -var db = require('../database'); +const util = require('util'); +const async = require('async'); +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([ - async.apply(User.getUserFields, uid, ['banned', 'banned:expire']), - function (userData, next) { - var banned = userData && userData.banned; - if (!banned) { - return next(null, banned); - } + User.getUserFields(uid, ['banned', 'banned:expire'], function (err, userData) { + if (err) { + return callback(err); + } + 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']; + User.bans.calcExpiredFromUserData = function (userData) { + return { + banned: !!userData.banned, + 'banned:expire': userData['banned:expire'], + banExpired: userData['banned:expire'] <= Date.now() && userData['banned:expire'] !== 0, + }; + }; - 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); + 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); + } + 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.getBannedReason = function (uid, callback) { + 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')); }; diff --git a/src/user/data.js b/src/user/data.js index c411aef9c1..3ad1db6433 100644 --- a/src/user/data.js +++ b/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')) { - user.banned_until = user['banned:expire']; - user.banned_until_readable = user.banned_until ? new Date(user.banned_until).toString() : 'Not Banned'; + if (user.hasOwnProperty('banned') || user.hasOwnProperty('banned:expire')) { + var result = User.bans.calcExpiredFromUserData(user); + 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); }); - - plugins.fireHook('filter:users.get', users, callback); } function parseGroupTitle(user) { diff --git a/src/user/digest.js b/src/user/digest.js index 881805940c..9b01126891 100644 --- a/src/user/digest.js +++ b/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); diff --git a/src/user/info.js b/src/user/info.js index 742ebbbbb0..5315abdb68 100644 --- a/src/user/info.js +++ b/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), - expiry_readable: new Date(parseInt(expiry, 10)).toString(), + banned_until: expire, + expiry: expire, /* backward compatible alias */ + banned_until_readable: expire_readable, + expiry_readable: expire_readable, /* backward compatible alias */ reason: validator.escape(String(banInfo.reason || '')), }); }, diff --git a/test/authentication.js b/test/authentication.js index c5a44e4ee8..23b9f97e72 100644 --- a/test/authentication.js +++ b/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(); }); }); diff --git a/test/socket.io.js b/test/socket.io.js index 4e71fd7a8d..20dcdd7ed3 100644 --- a/test/socket.io.js +++ b/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('expiry_readable')); + assert(data.hasOwnProperty('banned_until')); + 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(); diff --git a/test/topics.js b/test/topics.js index 19dfe90d81..2a1dd87a95 100644 --- a/test/topics.js +++ b/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); diff --git a/test/user.js b/test/user.js index f2ff257951..857f909a5a 100644 --- a/test/user.js +++ b/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(); });