-
Notifications
You must be signed in to change notification settings - Fork 11
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
add: session uid ratelimit #136
Conversation
311db44
to
a5fc577
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you've done a very good job on this, but I have a few remarks
I'm also wondering if we should match multiple rules or not
if we do, we should .map
on rules
and each one should return an async
function (therefore a promise) and we should then await Promise.all
; basically do the rule checking in parallel
@@ -19,7 +20,10 @@ function getMongo() { | |||
} | |||
|
|||
function getRedis() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice touch
services/sentry/lib/access.js
Outdated
: [ | ||
{ uids: [channel.creator] }, | ||
{ uids: null, rateLimit: cfg.IP_RATE_LIMIT }, | ||
{ uids: null, rateLimit: cfg.SID_RATE_LIMIT } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should not be default
test/index.js
Outdated
const events = genEvents(2, 'working') | ||
const allowOnlyOneEvent = await checkAccess(channel, {}, events) | ||
t.equal(allowOnlyOneEvent.success, false, 'should not process request') | ||
t.equal(allowOnlyOneEvent.statusCode, 429, 'shoudl ahbe err') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix the typos please
test/index.js
Outdated
|
||
// redis connection preventing test from closing | ||
// hence the quit | ||
tape.onFinish(() => db.getRedis().quit()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be a separate test file, since it's more similar to an integration test then a unit test because it requires redis and is not a pure function
fixes #132