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.

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

DDPRateLimiter should allow a callback for going over the limit #5541

Closed
shaylevi2 opened this Issue Oct 26, 2015 · 12 comments

Comments

Projects
None yet
7 participants
@shaylevi2

shaylevi2 commented Oct 26, 2015

Assuming a user is constantly going over the set limit - I would like to automatically ban this user.

Given a method that ran everytime a user has gone over the specified limit, would allow me to do it.

@shaylevi2 shaylevi2 changed the title from DDPRateLimiter should allow a callback to DDPRateLimiter should allow a callback for going over the limit Oct 26, 2015

@stubailo

This comment has been minimized.

Contributor

stubailo commented Oct 26, 2015

I think this shouldn't be hard to add in the ddp-rate-limiter package. In fact, you could probably do it now by overriding the getErrorMessage function, which is basically a callback that just happens to also return the error message.

Here's the relevant code: https://github.com/meteor/meteor/blob/devel/packages/ddp-rate-limiter/ddp-rate-limiter.js

@shaylevi2

This comment has been minimized.

shaylevi2 commented Oct 26, 2015

The problem with getErrorMessage is that it's a single function for all the possible limits someone might wanna use, maybe for some you wanna ban and for some you don't.

@stubailo

This comment has been minimized.

Contributor

stubailo commented Oct 26, 2015

Yeah but you could put an if statement in there since you get all of the data in the callback argument. I'm not saying it's perfect, but it is a viable workaround for now.

@shaylevi2

This comment has been minimized.

shaylevi2 commented Nov 9, 2015

You get
{ allowed: false, timeToReset: 269, numInvocationsLeft: 0 }

No idea what function and you can't even access the connection data (i.e this.connection.id is undefined)

@stubailo

This comment has been minimized.

Contributor

stubailo commented Nov 9, 2015

Oh weird, I would have thought that should give you the name of the DDP message.

@zol zol removed the desired label Feb 23, 2016

@nlhuykhang

This comment has been minimized.

Contributor

nlhuykhang commented Jan 1, 2017

I think we could add a callback as the fourth argument of .addRule, then execute it inside check if the limit has been exceeded. Is this ok?

@abernix

This comment has been minimized.

Member

abernix commented Jan 2, 2017

@nlhuykhang Something like that. Perhaps it would be worth having the callback's signature be callback(denied, allowed) and have it called in both cases? This would allow some additional functionality (success audit logs, etc.), right?

@sebakerckhof

This comment has been minimized.

Contributor

sebakerckhof commented Jan 12, 2017

I think it would also make sense to modify https://github.com/meteor/meteor/blob/devel/packages/rate-limit/rate-limit.js#L131 so the input is added to the rate limit result. It seems to make sense to be able to see what caused the error in the error message (seeing that @stubailo thought this was already in there).

@hwillson

This comment has been minimized.

Member

hwillson commented Jan 12, 2017

Great ideas guys! Time to make this a reality then? I'd be happy to work on this if we agree this approach makes sense.

@nlhuykhang

This comment has been minimized.

Contributor

nlhuykhang commented Jan 13, 2017

@hwillson I already created a PR here #8237

@hwillson

This comment has been minimized.

Member

hwillson commented Jan 13, 2017

Sorry @nlhuykhang - I've been reading too many GH issues lately, I skipped right past your reference!

@abernix abernix added this to the Release 1.4.3.x milestone Mar 9, 2017

@abernix

This comment has been minimized.

Member

abernix commented Mar 9, 2017

This should be released in Meteor 1.4.3.2. You can try the latest 1.4.3.2 beta and help confirm/test by running:

meteor update --release 1.4.3.2-beta.0

Please report back if you encounter any problems. Thanks for suggesting this originally, @shaylevi2, and thanks very much for submitting a PR to make it happen, @nlhuykhang.

@abernix abernix closed this Mar 9, 2017

@abernix abernix modified the milestones: Release 1.4.3.x, Release 1.4.3.2 Mar 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment