From b54401274edccd8b0b2fcb2c438bc43c7331369e Mon Sep 17 00:00:00 2001 From: Anubhav Jain Date: Thu, 25 Jun 2015 15:38:42 -0700 Subject: [PATCH] Fixed Rate Limit and DDP Rate Limit packages. Added end to end tests which test removing rules. --- .../ddp-rate-limiter-server-tests.js | 20 +++++ .../ddp-rate-limiter-tests.js | 81 ++++++++++++++++--- packages/ddp-rate-limiter/ddp-rate-limiter.js | 34 ++------ packages/ddp-rate-limiter/package.js | 4 +- packages/rate-limit/package.js | 2 + packages/rate-limit/rate-limit-tests.js | 34 ++++---- packages/rate-limit/rate-limit.js | 22 +++-- 7 files changed, 133 insertions(+), 64 deletions(-) create mode 100644 packages/ddp-rate-limiter/ddp-rate-limiter-server-tests.js diff --git a/packages/ddp-rate-limiter/ddp-rate-limiter-server-tests.js b/packages/ddp-rate-limiter/ddp-rate-limiter-server-tests.js new file mode 100644 index 0000000000..87f0b96c1c --- /dev/null +++ b/packages/ddp-rate-limiter/ddp-rate-limiter-server-tests.js @@ -0,0 +1,20 @@ +Meteor.methods({ + resetAndAddRuleToDDPRateLimiter : function(intervalTimeInMillis) { + DDPRateLimiter.rateLimiter.rules = {}; + this.ruleId = DDPRateLimiter.addRule({ + userId: null, + ipAddr: function() {return true}, + type: 'method', + name: 'login' + }, 5, intervalTimeInMillis); + return this.ruleId; + }, + + removeRuleFromDDPRateLimiter : function(id) { + return DDPRateLimiter.removeRule(id); + }, + + printCurrentListOfRules : function () { + console.log('Current list of rules :', DDPRateLimiter.rateLimiter.rules); + } +}); \ No newline at end of file diff --git a/packages/ddp-rate-limiter/ddp-rate-limiter-tests.js b/packages/ddp-rate-limiter/ddp-rate-limiter-tests.js index a0551e2b2f..27d1f7b074 100644 --- a/packages/ddp-rate-limiter/ddp-rate-limiter-tests.js +++ b/packages/ddp-rate-limiter/ddp-rate-limiter-tests.js @@ -1,14 +1,58 @@ -DDPRateLimiter.config([]); -DDPRateLimiter.addRule({ - userId: null, - IPAddr: null, - type: 'method', - name: 'login' -}, 5, 1000); +testAsyncMulti("passwords - basic login with password", [ + function (test, expect) { + var self = this; + // Setup the rate limiter rules + Meteor.call('resetAndAddRuleToDDPRateLimiter', 1000, expect(function(error, result) { + self.ruleId = result; + })); + // setup + this.username = Random.id(); + this.email = Random.id() + '-intercept@example.com'; + this.password = 'password'; -if (Meteor.isClient) { - testAsyncMulti("passwords - basic login with password", [ - function (test, expect) { + Accounts.createUser({ + username: this.username, + email: this.email, + password: this.password + }, + expect(function () {})); + }, + function (test, expect) { + test.notEqual(Meteor.userId(), null); + }, + function (test, expect) { + Meteor.logout(expect(function (error) { + test.equal(error, undefined); + test.equal(Meteor.user(), null); + })); + }, + function (test, expect) { + var self = this; + for (var i = 0; i < 5; i++) { + Meteor.loginWithPassword(self.username, 'fakePassword', expect( + function (error) { + // Get 5 'User not found' 403 messages before rate limit is hit + test.equal(error.error, 403); + })); + } + Meteor.loginWithPassword(self.username, 'fakePassword', expect( + function (error) { + test.equal(error.error, 'too-many-requests'); + })); + // Cleanup + Meteor.call('removeRuleFromDDPRateLimiter', self.ruleId, expect(function(error, result) { + test.equal(result,true); + })); + } +]); + +testAsyncMulti("test removing rule with rateLimited client lets them send new queries", [ + function(test, expect) { + var self = this; + // Setup the rate limiter rules + Meteor.call('resetAndAddRuleToDDPRateLimiter', 5000, expect(function(error, result) { + self.ruleId = result; + })); // setup this.username = Random.id(); this.email = Random.id() + '-intercept@example.com'; @@ -35,7 +79,8 @@ if (Meteor.isClient) { for (var i = 0; i < 5; i++) { Meteor.loginWithPassword(self.username, 'fakePassword', expect( function (error) { - // Get 5 'User not found' 403 messages before rate limit is hit + // Call printCurrentListofRules to see all the rules on the server + // Meteor.call('printCurrentListOfRules'); test.equal(error.error, 403); })); } @@ -43,6 +88,16 @@ if (Meteor.isClient) { function (error) { test.equal(error.error, 'too-many-requests'); })); - } + // By removing the rule from the DDP rate limiter, we no longer restrict them even though they were rate limited + Meteor.call('removeRuleFromDDPRateLimiter', self.ruleId, expect(function(error, result) { + test.equal(result,true); + })); + // + for (var i = 0; i < 10; i++) { + Meteor.loginWithPassword(self.username, 'fakePassword', expect( + function (error) { + test.equal(error.error, 403); + })); + } + } ]); -}; \ No newline at end of file diff --git a/packages/ddp-rate-limiter/ddp-rate-limiter.js b/packages/ddp-rate-limiter/ddp-rate-limiter.js index af647e3ec4..355ff6840d 100644 --- a/packages/ddp-rate-limiter/ddp-rate-limiter.js +++ b/packages/ddp-rate-limiter/ddp-rate-limiter.js @@ -3,20 +3,10 @@ DDPRateLimiter = { errorMessage : function (rateLimitResult) { return "Error, too many requests. Please slow down. You must wait " + Math.ceil( rateLimitResult.timeToReset / 1000) + " seconds before trying again."; - } + }, + rateLimiter : new RateLimiter() } -DDPRateLimiter.rateLimiter = new RateLimiter(); - -// DDPRateLimiter.rateLimiter.addRule( { -// userId: null, -// ipAddr: function (ipAddr) { -// return true; -// }, -// type: 'sub', -// name: null -// }, 5, 10000); - DDPRateLimiter.getErrorMessage = function (rateLimitResult) { if (typeof this.errorMessage === 'function') return this.errorMessage(rateLimitResult); @@ -28,22 +18,10 @@ DDPRateLimiter.setErrorMessage = function (message) { this.errorMessage = message; } -DDPRateLimiter.setRules = function (rules) { - DDPRateLimiter.rateLimiter.rules = rules; -}; - DDPRateLimiter.addRule = function (rule, numRequests, intervalTime) { - DDPRateLimiter.rateLimiter.addRule(rule, numRequests, intervalTime); + return DDPRateLimiter.rateLimiter.addRule(rule, numRequests, intervalTime); }; - -// Add a default rule of limiting logins to 5 times per 10 seconds by IP address. -// Override using DDPRateLimiter.config -DDPRateLimiter.addRule({ - userId: null, - ipAddr: function (ipAddr) { - return true; - }, - type: 'method', - name: 'login' -}, 5, 10000); +DDPRateLimiter.removeRule = function (id) { + return DDPRateLimiter.rateLimiter.removeRule(id); +} \ No newline at end of file diff --git a/packages/ddp-rate-limiter/package.js b/packages/ddp-rate-limiter/package.js index c1971d35d2..7c2fb06678 100644 --- a/packages/ddp-rate-limiter/package.js +++ b/packages/ddp-rate-limiter/package.js @@ -18,9 +18,11 @@ Package.onUse(function(api) { }); Package.onTest(function(api) { + api.use('underscore'); api.use(['accounts-password', 'tinytest', 'test-helpers', 'tracker', 'accounts-base', 'random', 'email', 'underscore', 'check', 'ddp']); api.use('ddp-rate-limiter'); - api.addFiles('ddp-rate-limiter-tests.js'); + api.addFiles('ddp-rate-limiter-server-tests.js', 'server'); + api.addFiles('ddp-rate-limiter-tests.js', 'client'); }); diff --git a/packages/rate-limit/package.js b/packages/rate-limit/package.js index 2e11608700..950efed10d 100644 --- a/packages/rate-limit/package.js +++ b/packages/rate-limit/package.js @@ -12,6 +12,7 @@ Package.describe({ Package.onUse(function(api) { api.use('underscore'); + api.use('random'); api.addFiles('rate-limit.js'); api.export("RateLimiter"); }); @@ -19,6 +20,7 @@ Package.onUse(function(api) { Package.onTest(function(api) { api.use('test-helpers', ['client', 'server']); api.use('underscore'); + api.use('random'); api.use('ddp-rate-limiter'); api.use('tinytest'); api.use('rate-limit'); diff --git a/packages/rate-limit/rate-limit-tests.js b/packages/rate-limit/rate-limit-tests.js index 60442e815e..59b65ec1c0 100644 --- a/packages/rate-limit/rate-limit-tests.js +++ b/packages/rate-limit/rate-limit-tests.js @@ -1,6 +1,6 @@ Tinytest.add('Check empty constructor creation', function (test) { r = new RateLimiter(); - test.equal(r.rules, []); + test.equal(r.rules, {}); }); Tinytest.add( @@ -220,7 +220,7 @@ Tinytest.add("test matchRule method", function (test) { type: null, name: null } - r.addRule(globalRule); + var globalRuleId = r.addRule(globalRule); var RateLimiterInput = { userId: 1023, @@ -229,7 +229,7 @@ Tinytest.add("test matchRule method", function (test) { name: 'getSubLists' }; - test.equal(r.rules[0].match(RateLimiterInput), true); + test.equal(r.rules[globalRuleId].match(RateLimiterInput), true); var oneNotNullRule = { userId: 102, @@ -238,18 +238,18 @@ Tinytest.add("test matchRule method", function (test) { name: null } - r.addRule(oneNotNullRule); - test.equal(r.rules[1].match(RateLimiterInput), false); + var oneNotId = r.addRule(oneNotNullRule); + test.equal(r.rules[oneNotId].match(RateLimiterInput), false); oneNotNullRule.userId = 1023; - test.equal(r.rules[1].match(RateLimiterInput), true); + test.equal(r.rules[oneNotId].match(RateLimiterInput), true); var notCompleteInput = { userId: 102, IPAddr: '127.0.0.1' }; - test.equal(r.rules[0].match(notCompleteInput), true); - test.equal(r.rules[1].match(notCompleteInput), false); + test.equal(r.rules[globalRuleId].match(notCompleteInput), true); + test.equal(r.rules[oneNotId].match(notCompleteInput), false); }); Tinytest.add('test generateMethodKey string', function (test) { @@ -260,7 +260,7 @@ Tinytest.add('test generateMethodKey string', function (test) { type: null, name: null } - r.addRule(globalRule); + var globalRuleId = r.addRule(globalRule); var RateLimiterInput = { userId: 1023, @@ -268,11 +268,11 @@ Tinytest.add('test generateMethodKey string', function (test) { type: 'sub', name: 'getSubLists' }; - // test.equal(r._generateKeyString(globalRule, RateLimiterInput), ""); - test.equal(r.rules[0]._generateKeyString(RateLimiterInput), ""); + + test.equal(r.rules[globalRuleId]._generateKeyString(RateLimiterInput), ""); globalRule.userId = 1023; - test.equal(r.rules[0]._generateKeyString(RateLimiterInput), + test.equal(r.rules[globalRuleId]._generateKeyString(RateLimiterInput), "userId1023"); var ruleWithFuncs = { @@ -282,16 +282,16 @@ Tinytest.add('test generateMethodKey string', function (test) { IPAddr: null, type: null }; - r.addRule(ruleWithFuncs); - test.equal(r.rules[1]._generateKeyString(RateLimiterInput), ""); + var funcRuleId = r.addRule(ruleWithFuncs); + test.equal(r.rules[funcRuleId]._generateKeyString(RateLimiterInput), ""); RateLimiterInput.userId = 1024; - test.equal(r.rules[1]._generateKeyString(RateLimiterInput), + test.equal(r.rules[funcRuleId]._generateKeyString(RateLimiterInput), "userId1024"); var multipleRules = ruleWithFuncs; multipleRules.IPAddr = '127.0.0.1'; - r.addRule(multipleRules); - test.equal(r.rules[2]._generateKeyString(RateLimiterInput), + var multipleRuleId = r.addRule(multipleRules); + test.equal(r.rules[multipleRuleId]._generateKeyString(RateLimiterInput), "userId1024IPAddr127.0.0.1") }) diff --git a/packages/rate-limit/rate-limit.js b/packages/rate-limit/rate-limit.js index df92debe75..d0dcdc9477 100644 --- a/packages/rate-limit/rate-limit.js +++ b/packages/rate-limit/rate-limit.js @@ -6,6 +6,7 @@ var DEFAULT_REQUESTS_PER_INTERVAL = 10; var Rule = function (options, matchers) { var self = this; + self.id = Random.id(); // Options contains the timeToReset and intervalTime self.options = options; @@ -98,10 +99,10 @@ _.extend(Rule.prototype, { RateLimiter = function () { var self = this; - // List of all rules associated with this RateLimiter. Each rule object stores - // the rule pattern, number of requests allowed, last reset time and the rule - // reset interval in milliseconds. - self.rules = []; + // Dictionary of all rules associated with this RateLimiter, keyed by their + // id. Each rule object stores the rule pattern, number of requests allowed, + // last reset time and the rule reset interval in milliseconds. + self.rules = {}; } /** @@ -175,7 +176,8 @@ RateLimiter.prototype.addRule = function (rule, numRequestsAllowed, intervalTime } var newRule = new Rule(options, rule); - this.rules.push(newRule); + this.rules[newRule.id] = newRule; + return newRule.id; } /** @@ -214,4 +216,14 @@ RateLimiter.prototype._findAllMatchingRules = function (input) { matchingRules.push(rule); }); return matchingRules; +} + +RateLimiter.prototype.removeRule = function (id) { + var self = this; + if (self.rules[id]) { + delete self.rules[id]; + return true; + } else { + return false; + } } \ No newline at end of file