-
Notifications
You must be signed in to change notification settings - Fork 325
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
Limit login retries #830
Limit login retries #830
Conversation
(I'll make a PR to wire-server-deploy soon, but since the flag is optional, that can be done independently.) |
heh :) I guess i missed one after all:
|
14ad11a
to
e666d0b
Compare
services/brig/brig.integration.yaml
Outdated
@@ -137,6 +137,9 @@ optSettings: | |||
setUserCookieThrottle: | |||
stdDev: 5 | |||
retryAfter: 1 | |||
limitFailedLogins: |
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.
sry, can u make it limitFailedLogins
-> setLimitFailedLogins
for consistency's sake?
This is slightly less accurate, but works against UTCTime, and is consistent with what we use in other places.
2195d02
to
2a53fd4
Compare
(this will pass CI once the resp. PRs there have been deployed.) |
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.
Just please double check https://github.com/wireapp/wire-server/pull/830/files#r315212362
Implements https://github.com/wearezeta/backend-issues/issues/841#issuecomment-518033685 .
Open questions:
forM_
instead ofpooledForConcurrentlyN_
here. The latter will fail becauseBridge.Budget
is not concurrency-proof: if two threads / services request a budget coin concurrently, only one of them will decrement the budget value in cassandra. We could use counters instead ofint
in the schema, but it is unclear what that will mean for the other use case. (cases? i think there is only one.)Settings
can be changed at run-time, in which caseLoginRetryOpts
shouldn't go there (I think).