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.

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

Merged
merged 4 commits into from
Sep 17, 2020

Conversation

shuowu
Copy link
Contributor

@shuowu shuowu commented Sep 11, 2020

related issue: okta/okta-oidc-js#894

@codecov-commenter
Copy link

Codecov Report

Merging #469 into dev-4.1 will increase coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             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              
Impacted Files Coverage Δ
lib/TokenManager.ts 97.00% <100.00%> (+0.21%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3208b7...69139e0. Read the comment docs.

client.tokenManager.on('error', handler);
client.tokenManager.renew = jest.fn().mockImplementation(() => Promise.resolve());
for (let i = 0; i <= 10; i++) {
client.emitter.emit('expired');
Copy link
Contributor

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?

Copy link
Contributor Author

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.

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
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

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".

Copy link
Contributor Author

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.

}
}

if (tooManyRenews) {
Copy link
Contributor

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)

Copy link
Contributor Author

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.

Copy link
Contributor

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:

  1. Queue up first 10 items
  2. 11th item emits error, but queue is reduced to 9
  3. Queue up the 12 item as the 10th in the queue
  4. 13 item emits error, and queue is again reduced to 9

Copy link
Contributor Author

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());
Copy link
Contributor

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.
Copy link
Contributor

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.

Copy link
Contributor

@aarongranick-okta aarongranick-okta left a 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

@nick-jones
Copy link
Contributor

@shuowu @aarongranick-okta @swiftone

Just wanted to check, what is the status of this fix? I note that it was merged into a dev-4.1 branch which has since been deleted. No 4.1 release was ever created. Is the intention to release this change in 5.0?

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.

@swiftone
Copy link
Contributor

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants