Skip to content
This repository has been archived by the owner on Sep 10, 2024. It is now read-only.

Add configuration for rate-limiting of logins, replacing hardcoded limits #3090

Merged
merged 3 commits into from
Aug 7, 2024

Conversation

reivilibre
Copy link
Contributor

@reivilibre reivilibre commented Aug 7, 2024

Follow up to #3013 to make it configurable.

@reivilibre reivilibre force-pushed the rei/rate_limiting_configuration branch from b2e24f3 to 11abd7a Compare August 7, 2024 12:56
Copy link

cloudflare-workers-and-pages bot commented Aug 7, 2024

Deploying matrix-authentication-service-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: 6100645
Status: ✅  Deploy successful!
Preview URL: https://6ca11e57.matrix-authentication-service-docs.pages.dev
Branch Preview URL: https://rei-rate-limiting-configurat.matrix-authentication-service-docs.pages.dev

View logs

@reivilibre reivilibre marked this pull request as ready for review August 7, 2024 12:56
@reivilibre reivilibre requested a review from sandhose August 7, 2024 12:58
Copy link
Member

@sandhose sandhose left a comment

Choose a reason for hiding this comment

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

Generally looks good, thanks!

crates/config/src/sections/rate_limiting.rs Outdated Show resolved Hide resolved
crates/config/src/sections/rate_limiting.rs Outdated Show resolved Hide resolved
// period must be at least 1 nanosecond according to the governor library
if recip < 1.0e-9 || !recip.is_finite() {
return Some(figment::error::Error::custom(
"`per_second` must be a number that is more than zero and less than 1_000_000_000 (1e9)",
Copy link
Member

Choose a reason for hiding this comment

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

hmm that error message doesn't look right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how so?

Copy link
Member

Choose a reason for hiding this comment

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

Aah, recip < 1.0e-9 is 'more than zero' and !recip.is_finite() is 'less than 1e9'?

Copy link
Contributor Author

@reivilibre reivilibre Aug 7, 2024

Choose a reason for hiding this comment

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

heheh, not quite, recip < 1.0e-9 means that per_second > 1e9, and is_finite guards against infinities and NaNs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe I should apply the validation on the input, I just didn't want to misthink the divisions and have it fail to create a quota with the numbers.

Copy link
Member

Choose a reason for hiding this comment

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

actually I think what you did is fine, since it's closer to what governor does internally, I just missed the .recip()

@reivilibre reivilibre force-pushed the rei/rate_limiting_configuration branch from df967f6 to 8df3ec4 Compare August 7, 2024 14:20
@reivilibre reivilibre requested a review from sandhose August 7, 2024 14:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants