Route options #34

yonjah · 2017-08-15T04:46:01Z

This pull request will allow to override settings for each route.
I also added a few useful settings to override -
enabled - to enable rate limiting only for specific routes
addHeaders - to add rate liming headers to response
count - to change the cost of a route

All new settings are optional and default to original behavior.

…ay errors and to work with limitd 5.x

dschenkelman · 2017-08-16T20:59:27Z

@yonjah thanks for the PR. I'm a bit backed up at work but will try to make some time to review

dschenkelman · 2017-08-17T02:45:53Z

README.md

@@ -44,11 +44,94 @@ The object has the following schema (validated [here](./lib/index.js) using [Joi
  * `done: (err: Error, key: String)` - A function that takes an error as the first parameter and the bucket key as the second parameter.

 **Optional**
+* `enabled: Boolean default(true)` - if `true` rate limiting will be enabled you can set it to `false` on route settings to disable rate limiting


I'm not sure enabled is the best way of describing this. e.g.: enabled: false at the top level can be confusing.

"can set it to false on route settings to disable rate limiting"

Why "on route settings"? If it is done at the global level this is also disabled. What about "if true rate limiting will be enabled. Setting this false` disable this at the appropriate level (route or global)"?

I agree "enabled" can be confusing but couldn't think of a better term.
I'll change the description as you suggested

what if we call it allRoutes and it can only be set at the global plug in level not at the route level? if a route has the plugin defined, it wants to enable it so no need for the flag at the route level

That sounds good for my use case where I have many routes which are not rate limited and I only want to enable it for a few routes.
But if you want to enable it for most route and only disable it on a few routes it will be more cumbersome

hmm, I see. let's, leave as is

dschenkelman · 2017-08-17T02:47:49Z

README.md

@@ -44,11 +44,94 @@ The object has the following schema (validated [here](./lib/index.js) using [Joi
  * `done: (err: Error, key: String)` - A function that takes an error as the first parameter and the bucket key as the second parameter.

 **Optional**
+* `enabled: Boolean default(true)` - if `true` rate limiting will be enabled you can set it to `false` on route settings to disable rate limiting
+* `addHeaders: Boolean default(true)` - if `true` rate limiting headers will be sent with response


I don't think this should be an option. This should always be true. Can you expand on why you think it should be optional?

My main use case for using rate limiting is to disable brute force on login attempts and tokens.
Since I don't see a case where legitimate use will hit those limits and if he does all he need to know is to try again in a few minutes. I don't want to give attacker extra information about when exactly he can do each attempt.

ok, sounds good. what about sendResponseHeaders?

dschenkelman · 2017-08-17T02:50:43Z

README.md

@@ -44,11 +44,94 @@ The object has the following schema (validated [here](./lib/index.js) using [Joi
  * `done: (err: Error, key: String)` - A function that takes an error as the first parameter and the bucket key as the second parameter.

 **Optional**
+* `enabled: Boolean default(true)` - if `true` rate limiting will be enabled you can set it to `false` on route settings to disable rate limiting
+* `addHeaders: Boolean default(true)` - if `true` rate limiting headers will be sent with response
+* `count: Number default(1)` - how many tokens request should cost useful when you want one route to be more expensive than another but use the same bucket


can you share a bit more about this use case? I understand the concept, but this is not something that I see being very common

Just thought it might be useful as a route option.
Personally I don't have a use case for it

let's drop it for the time being then please. it adds additional logic with no usage

dschenkelman · 2017-08-17T02:51:18Z

README.md

 * `onError: (error, reply) => ()` - A function that takes the `error` that occurred when trying to get a token from the bucket and the `reply` interface.
  * `error: Error` - The error that occurred.
  * `reply: Reply` - The hapi.js [reply interface](http://hapijs.com/api#reply-interface).
  > If an error occurs and no function is provided, the request lifecycle continues normally as if there was no token bucket restriction. This is a useful default behavior in case the limitd server goes down.

+Options can be overridden in route settings.
+if plugin is disabled by default setting patova setting for route will also enable the plugin for the route.


Not sure about what this particular line means. Can you provide a bit more detail please?

dschenkelman · 2017-08-17T02:58:35Z

README.md

 * `onError: (error, reply) => ()` - A function that takes the `error` that occurred when trying to get a token from the bucket and the `reply` interface.
  * `error: Error` - The error that occurred.
  * `reply: Reply` - The hapi.js [reply interface](http://hapijs.com/api#reply-interface).
  > If an error occurs and no function is provided, the request lifecycle continues normally as if there was no token bucket restriction. This is a useful default behavior in case the limitd server goes down.

+Options can be overridden in route settings.


I would move from here until "Contributing" to a wiki article, leaving the README.md simpler.

Do you want me to remove this completely or create a wiki page and add a link ?

let's remove it. it is simple enough

dschenkelman · 2017-08-17T03:00:58Z

lib/index.js

+  const extractKeyAndTakeToken = function(options, request, reply, type) {
+    options.extractKey(request, reply, (err, key) =>{
+      const limitd = options.limitd;
+      const count = options.count;


let's discuss usage above, but I would probably remove from now

dschenkelman · 2017-08-17T03:02:03Z

lib/index.js

          if (onError) { return onError(err, reply); }
          // by default we don't fail if limitd is unavailable
          return reply.continue();
        }

-        const oldMinimumLimitResponse = request.plugins.patova && request.plugins.patova.limit
-        const newMinimumLimitResponse = getMinimumLimit(currentLimitResponse, oldMinimumLimitResponse)
+        const oldMinimumLimitResponse = request.plugins.patova && request.plugins.patova.limit;


dschenkelman · 2017-08-17T03:02:28Z

lib/index.js

-          newMinimumLimitResponse.limit,
-          newMinimumLimitResponse.remaining,
-          newMinimumLimitResponse.reset);
+        if (options.addHeaders) {


As mentioned previously, I would not make this optional unless there is a good reason to

dschenkelman · 2017-08-17T03:06:25Z

lib/index.js

+    reply.continue();
+  });
+
+  function parseRouteOptions(request) {


this logic looks good. it is not really parsing though, right? I would change the function name to getRouteOptions maybe?

dschenkelman · 2017-08-17T03:08:11Z

lib/index.js

 }

-exports.register = function (server, options, next) {
-  Joi.validate(options, schema, { abortEarly: false }, (err, processedOptions) => {
+const defaults = {


does it make sense to use Joi default: https://github.com/hapijs/joi/blob/master/API.md#anydefaultvalue-description?

yonjah · 2017-08-18T01:26:12Z

lib/index.js

+  const extractKeyAndTakeToken = function(options, request, reply, type) {
+    options.extractKey(request, reply, (err, key) =>{
+      const limitd = options.limitd;
+      const count = options.count;
      if (err) { return reply(err); }

      if (!limitd) {


BTW I don't think limitd can be missing or falsey at this stage so this can probably be removed

…rrid route settings

yonjah · 2017-08-18T07:51:08Z

Ok hope it looks better now

yonjah · 2017-08-22T04:51:52Z

lib/index.js

-    extractKey(request, reply, (err, key) =>{
+  const extractKeyAndTakeToken = function(options, request, reply, type) {
+    options.extractKey(request, reply, (err, key) =>{
+      const limitd = options.limitd;
      if (err) { return reply(err); }

      if (!limitd) {


@dschenkelman do you want me to also remove this check ?
I'm not sure why it's there but I don't think limitd can ever be falsey at this stage

no, this is fine

dschenkelman · 2017-08-23T20:25:20Z

lib/index.js

+    if (routeOptions === undefined) {
+      return pluginOptions;
+    }
+    if (routeOptions._merged) { //already merged with previous pluginOptions;


maybe this is a premature optimization? I think hapi merges per request. Storing _merged on the plugins.patova object which we don't own feels odd.

Yea sure. It just feels a bit wasteful since it can be deduced in advance.
But this is anyway kinda hacky so I don't mind removing it

yonjah · 2017-08-24T05:52:16Z

README.md

@@ -44,11 +44,14 @@ The object has the following schema (validated [here](./lib/index.js) using [Joi
  * `done: (err: Error, key: String)` - A function that takes an error as the first parameter and the bucket key as the second parameter.

 **Optional**
+* `enabled: Boolean default(true)` - if true rate limiting will be enabled for all routes. Setting this `false` will only rate limit routes which have the plugin defined


I've been thinking a bit more and we can probably change this to allRoutes as you suggested.
if it's set to true a user can still disable a specific route by setting the route settings to false instead of an object

config: { plugins: { patova: false } }

So we can accommodate both use cases this way.

Upgrade mocha and limitd dev dependencies, Fix test to properly displ…

71c5823

…ay errors and to work with limitd 5.x

yonjah force-pushed the route-options branch from 25dc160 to a016130 Compare August 15, 2017 07:10

dschenkelman requested changes Aug 17, 2017

View reviewed changes

yonjah commented Aug 18, 2017

View reviewed changes

Add settings (enabled, sendResponseHeaders), Add feature to allow ove…

645dfcf

…rrid route settings

yonjah force-pushed the route-options branch from a016130 to 645dfcf Compare August 18, 2017 07:46

yonjah commented Aug 22, 2017

View reviewed changes

dschenkelman reviewed Aug 23, 2017

View reviewed changes

yonjah added 2 commits August 24, 2017 09:34

Remove caching of route settings

06aca4c

Fix onError setting not being checked for each route settings

13a378a

yonjah commented Aug 24, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Route options #34

Route options #34

yonjah commented Aug 15, 2017

dschenkelman commented Aug 16, 2017

dschenkelman Aug 17, 2017

dschenkelman Aug 17, 2017

yonjah Aug 17, 2017

dschenkelman Aug 17, 2017

yonjah Aug 18, 2017

dschenkelman Aug 18, 2017

dschenkelman Aug 17, 2017

yonjah Aug 17, 2017

dschenkelman Aug 17, 2017

dschenkelman Aug 17, 2017

yonjah Aug 17, 2017

dschenkelman Aug 17, 2017

dschenkelman Aug 17, 2017

dschenkelman Aug 17, 2017

yonjah Aug 18, 2017

dschenkelman Aug 18, 2017

dschenkelman Aug 17, 2017

dschenkelman Aug 17, 2017

dschenkelman Aug 17, 2017

dschenkelman Aug 17, 2017

dschenkelman Aug 17, 2017

yonjah Aug 18, 2017

yonjah commented Aug 18, 2017

yonjah Aug 22, 2017

dschenkelman Aug 23, 2017

dschenkelman Aug 23, 2017

yonjah Aug 24, 2017

yonjah Aug 24, 2017

Route options #34

Are you sure you want to change the base?

Route options #34

Conversation

yonjah commented Aug 15, 2017

dschenkelman commented Aug 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yonjah commented Aug 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment