Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Route options #34

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Route options #34

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
71c5823
Upgrade mocha and limitd dev dependencies, Fix test to properly displ…
yonjah Aug 15, 2017
645dfcf
Add settings (enabled, sendResponseHeaders), Add feature to allow ove…
yonjah Aug 15, 2017
06aca4c
Remove caching of route settings
yonjah Aug 24, 2017
13a378a
Fix onError setting not being checked for each route settings
yonjah Aug 24, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ server.register({
event: 'onPostAuth',
type: 'users',
limitd: limitdClient,
extractKey: function(request, reply, done){
extractKey: function(request, reply, done) {
var key = request.auth.credentials.userId;
done(null, key);
}
Expand All @@ -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
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* `sendResponseHeaders: Boolean default(true)` - if `true` rate limiting headers will be sent with response
* `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.


## Contributing
Feel free to open issues with questions/bugs/features. PRs are also welcome.

Expand Down
97 changes: 59 additions & 38 deletions lib/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ const schema = Joi.object().keys({
type: [Joi.string(), Joi.func()],
limitd: Joi.object(),
onError: Joi.func(),
extractKey: Joi.func()
extractKey: Joi.func(),
enabled: Joi.boolean().default(true),
sendResponseHeaders: Joi.boolean().default(true)
}).requiredKeys('type', 'event', 'limitd', 'extractKey');

function setResponseHeader(request, header, value) {
Expand All @@ -22,41 +24,24 @@ function setResponseHeader(request, header, value) {
}
}

function setupPreResponseExt(server, options) {
server.ext('onPreResponse', (request, reply) => {
const requestLimit = request.plugins.patova && request.plugins.patova.limit;

if (requestLimit && requestLimit.conformant){
const headers = new RateLimitHeaders(
requestLimit.limit,
requestLimit.remaining,
requestLimit.reset);

Object.keys(headers).forEach(
key => setResponseHeader(request, key, headers[key]));
}
reply.continue();
});
}

function getMinimumLimit(limit1, limit2) {
if (!limit1) { return limit2; }
if (!limit2) { return limit1; }

if (limit1 && limit2.remaining > limit1.remaining) {
if (limit2.remaining > limit1.remaining) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks

return limit1;
}

return limit2;
}

function setupRateLimitEventExt(server, options) {
const event = options.event;
const extractKey = options.extractKey;
const onError = options.onError;
function setupRateLimitEventExt(server, pluginOptions) {
const event = pluginOptions.event;
const onError = pluginOptions.onError;

const extractKeyAndTakeToken = function(limitd, request, reply, type) {
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) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, this is fine

Expand All @@ -65,14 +50,14 @@ function setupRateLimitEventExt(server, options) {
}

limitd.take(type, key, (err, currentLimitResponse) => {
if (err){
if (err) {
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;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks

const newMinimumLimitResponse = getMinimumLimit(currentLimitResponse, oldMinimumLimitResponse);

request.plugins.patova = request.plugins.patova || {};
request.plugins.patova.limit = newMinimumLimitResponse;
Expand All @@ -83,17 +68,19 @@ function setupRateLimitEventExt(server, options) {
}

const error = Boom.tooManyRequests();
error.output.headers = new RateLimitHeaders(
newMinimumLimitResponse.limit,
newMinimumLimitResponse.remaining,
newMinimumLimitResponse.reset);
if (options.sendResponseHeaders) {
error.output.headers = new RateLimitHeaders(
newMinimumLimitResponse.limit,
newMinimumLimitResponse.remaining,
newMinimumLimitResponse.reset);
}

reply(error);
});
});
};

const getType = function(request, reply, callback) {
const getType = function(options, request, reply, callback) {
const type = options.type;

if (typeof type !== 'function') {
Expand All @@ -114,18 +101,52 @@ function setupRateLimitEventExt(server, options) {
};

server.ext(event, (request, reply) => {
const options = getRouteOptions(request);
if (!options.enabled) {
return reply.continue();
}
// This handler is going to be called one time per registration of patova
getType(request, reply, (err, type) => {
extractKeyAndTakeToken(options.limitd, request, reply, type);
getType(options, request, reply, (err, type) => {
extractKeyAndTakeToken(options, request, reply, type);
});
});

server.ext('onPreResponse', (request, reply) => {
const requestLimit = request.plugins.patova && request.plugins.patova.limit;

if (requestLimit && requestLimit.conformant) {
const options = getRouteOptions(request);
if (options.sendResponseHeaders) {
const headers = new RateLimitHeaders(
requestLimit.limit,
requestLimit.remaining,
requestLimit.reset);

Object.keys(headers).forEach(
key => setResponseHeader(request, key, headers[key]));
}
}
reply.continue();
});

function getRouteOptions(request) {
const routeOptions = request.route.settings.plugins.patova;
if (routeOptions === undefined) {
return pluginOptions;
}
if (routeOptions._merged) { //already merged with previous pluginOptions;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return routeOptions;
}
request.route.settings.plugins.patova = Object.assign({_merged: true}, pluginOptions, {enabled: true}, routeOptions);
return request.route.settings.plugins.patova;
}

}

exports.register = function (server, options, next) {
Joi.validate(options, schema, { abortEarly: false }, (err, processedOptions) => {
exports.register = function(server, pluginOptions, next) {
Joi.validate(pluginOptions, schema, { abortEarly: false }, (err, processedOptions) => {
if (err) { return next(err); }
setupRateLimitEventExt(server, processedOptions);
setupPreResponseExt(server, processedOptions);
next();
});
};
Expand Down
4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@
"chai": "^2.1.0",
"hapi": "^15.2.0",
"js-yaml": "^3.2.7",
"limitd": "^4.20.1",
"mocha": "^2.1.0",
"limitd": "^5.3.1",
"mocha": "^3.5.0",
"request": "^2.81.0",
"rimraf": "^2.3.1",
"xtend": "^4.0.0"
Expand Down
Loading