-
Notifications
You must be signed in to change notification settings - Fork 268
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
fix: add rate limiting logic to autoRenew #469
Conversation
Codecov Report
@@ Coverage Diff @@
## dev-4.1 #469 +/- ##
===========================================
+ Coverage 90.31% 90.38% +0.06%
===========================================
Files 31 31
Lines 1549 1560 +11
Branches 340 343 +3
===========================================
+ Hits 1399 1410 +11
Misses 150 150
Continue to review full report at Codecov.
|
test/spec/tokenManager.js
Outdated
client.tokenManager.on('error', handler); | ||
client.tokenManager.renew = jest.fn().mockImplementation(() => Promise.resolve()); | ||
for (let i = 0; i <= 10; i++) { | ||
client.emitter.emit('expired'); |
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 it will emit an error on the 10th request. But what happens on the 11th?
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.
it will keep emitting the error. Maybe should add logic to re-evaluate the flag and also make sure only one error is sent out if the evaluation is the same as the previous one.
lib/TokenManager.ts
Outdated
const onTokenExpiredHandler = (key) => { | ||
if (options.autoRenew) { | ||
this.renew(key).catch(() => {}); // Renew errors will emit an "error" event | ||
// detect if there are too many renew requests |
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 comment doesn't really line up with what follows
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.
can you explain more on how it's not lined up? it's logic to assign value to the tooManyRenews
flag, which is the rate-limiting detection here.
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.
It looks like it's describing the 'if' statement, but the if condition is actually "not yet too many".
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'll extract it to an isolated function, should be more declarative.
lib/TokenManager.ts
Outdated
} | ||
} | ||
|
||
if (tooManyRenews) { |
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.
So if there are 10+ in the queue, we will absolutely remove the first, but we may not decide there are too many. (and we aren't putting "now" into the queue for that case)
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.
So here it's maintaining a queue with the max length of 10. When items in q reach 10, it compares if these 10 requests happen in a too short time window.
The flag here is just to prevent running the renew method again. When the next expired event comes, it just emit the error again, I think that what Aaron is concerned.
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.
But the queue is getting shorter (because of the shift), so the next time through tooManyRenews will be false (because it never gets evaluated, because there were only 9 items on the queue)
so in an infinite loop situation it would be:
- Queue up first 10 items
- 11th item emits error, but queue is reduced to 9
- Queue up the 12 item as the 10th in the queue
- 13 item emits error, and queue is again reduced to 9
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.
the tooManyRenews is initialized in the constructor, which only be assigned once no matter how many expired events emitted. In the current implementation, the value won't be reassigned after set to true. I was initially thought that would be enough to prevent the infinite renew calls, but it does introduce side effect like keep emitting errors.
@@ -192,6 +193,18 @@ function clear(tokenMgmtRef, storage) { | |||
storage.clearStorage(); | |||
} | |||
|
|||
function isTooManyRenews(tokenMgmtRef, renewTimeQueue) { | |||
let tooManyRenews = false; | |||
renewTimeQueue.push(Date.now()); |
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 would prefer that this function (which promises to answer a question true/false) could be called multiple times without side-effect and give the same answer. It could be broken into 3 functions (track, evaluate, cleanup),
but it would be simpler to rename to something like 'allowRenewRequest' or throttleRenew
with a comment explaining there is some time-based and stateful flood prevention logic here to prevent accidental DDoS and/or infinite loops
CHANGELOG.md
Outdated
|
||
- [#469](https://github.com/okta/okta-auth-js/pull/469) | ||
- Adds "rate limiting" logic to token autoRenew process to prevent too many requests be sent out which may cause application rate limit issue. | ||
- Adds [tokenManager.tooManyRenewsSecondsWindow](#toomanyrenewssecondswindow) option. |
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'm not sure we need to expose this option to users. The default value is so large it should never cause any problems, I can't think of a reason anyone should need to modify this value. Only case I can think of is to make it very small to make testing of this feature easier.
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.
minor notes on naming, documentation
@shuowu @aarongranick-okta @swiftone Just wanted to check, what is the status of this fix? I note that it was merged into a I come asking because okta/okta-oidc-js#894 continues to trigger rate limit warnings for us, and in fact triggered a violation over the weekend. |
@nick-jones - Thanks for your patience on this - the 4.1 release (which is currently in the master branch) is due out (hopefully) this week. |
related issue: okta/okta-oidc-js#894