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
Bugfix: BruteForceDetection check for whitelist ip before blacklist #16348
Conversation
@markoboy @Findus23 I wonder if you are maybe more after the login whitelist feature? https://matomo.org/faq/how-to/faq_25543/ For the brute force blacklist it was meant to have a priority say some IP gets blocked from time to time and you want to block that specific IP forever. |
@tsteur what about this page https://matomo.org/faq/troubleshooting/faq_32758/ It says 'Never block these IPs from logging in', that's how I was not able to log in... I added the above on the configs although I was still unable to log in... As it is implied that these ips will never get blocked shouldn't they be checked first? |
I see. We might need to change the wording. I guess the blacklist generally wasn't meant as a "Block all" kind of feature. For this case the login whitelist feature be better suited. I'm basically saying this to avoid later someone coming like "I have whitelisted a range of IPs but want to blacklist specific IPs and the blacklist should be checked first". |
@mattab any thoughts on this? |
@mattab you are saying sounds good to merge but then giving a thumbs up on my indication not to merge. Note that changing this would also break existing configurations and in above described use case clearly the wrong feature is used as mentioned. The correct feature to be used is definitely https://matomo.org/faq/how-to/faq_25543/ so I'd be rather keen to close the PR since it allows more flexibility overall I would say. |
@tsteur I see what you mean now, I agree it's better not to merge and instead update wording. So would we just need to change the wording in the settings from 'Never block these IPs from logging in' to the actual meaning like 'Only allow these IPs to login' or so? @markoboy could you use this Login Whitelist IPs feature instead? |
Thanks for creating the PR @markoboy as mentioned in the comments we generally prefer not changing the behaviour and rather explaining it better see #16411 We generally prefer to rather block an IP than to allow it. And changing the meaning of it would break the meaning for existing users and it is maybe a more safe approach to rather block the IP when we have an extra feature where it's possible to only allow specific IPs. Thanks for this again! |
A fix for the BruteForceDetection module on Login/Security plugin. When the configs are set to disable login from all ips but never block some ips, the user gets blocked from being able to login because the first check is done on the blacklist instead of the whitelist to allow the user to login from the whitelist ips.
Check this post for your reference:
https://forum.matomo.org/t/cannot-login-because-of-blocked-ip/35446/5