@markoboy opened this Pull Request on August 26th 2020

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

@tsteur commented on August 26th 2020 Member

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

@markoboy commented on August 26th 2020

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

@tsteur commented on August 26th 2020 Member

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

@tsteur commented on September 3rd 2020 Member

@mattab any thoughts on this?

@mattab commented on September 8th 2020 Member

@tsteur Sounds good to me to merge

Thanks for creating the PR @markoboy

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

:+1:

@tsteur commented on September 8th 2020 Member

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

@mattab commented on September 8th 2020 Member

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

@tsteur commented on September 9th 2020 Member

Thanks for creating the PR @markoboy as mentioned in the comments we generally prefer not changing the behaviour and rather explaining it better see https://github.com/matomo-org/matomo/pull/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!

This Pull Request was closed on September 9th 2020
Powered by GitHub Issue Mirror