@tsteur opened this Pull Request on November 1st 2017 Owner

No need to mention in developer changelog I would say as it is only new user config settings.

IPs can now be whitelisted like this:

; When configured, only users from a configured IP can log into your Piwik. You can define one or multiple
; IPv4, IPv6, and IP ranges. This whitelist also affects API requests unless you disabled it via the setting
; "login_whitelist_apply_to_reporting_api_requests" below. Note that neither this setting, nor the
; "login_whitelist_apply_to_reporting_api_requests" restricts authenticated tracking requests (tracking requests
; with a "token_auth" URL parameter).
;
; Examples:
; login_whitelist_ip[] = 204.93.240.*
; login_whitelist_ip[] = 204.93.177.0/24
; login_whitelist_ip[] = 199.27.128.0/21
; login_whitelist_ip[] = 2001:db8::/48

; By default, if a whitelisted IP address is specified via "login_whitelist_ip[]", the reporting user interface as
; well as HTTP Reporting API requests will only work for these whitelisted IPs.
; Set this setting to "0" to allow HTTP Reporting API requests from any IP address.
login_whitelist_apply_to_reporting_api_requests = 1

When whistlisted IPs are configured and you try to access it but your IP is not matching, you will see an error like this:

image

For API the same message is used but doesn't 100% match:
{"result":"error","message":"You cannot log in as your IP is not whitelisted"} maybe we can find a more generic error message?

I decided to implement it within the Auth class since this class is not API and the authentication is happening here so we can make sure it is applied everywhere. Also here we know whether we are dealing with a token authenticated request (API) or a regular UI request (login/password or login/cookie).

I noticed LoginHttpAuth is overwriting the Auth class even though Auth is not API so it might not 100% work there.

Also I noticed authenticate() is actually not really supposed to throw an exception but to return a AuthResult(AuthResult::FAILURE). Problem with that is the user may enter the correct login / password, but still sees the error message Wrong Username and password combination.. I wouldn't really like to introduce a new AuthResult::FAILURE_NOT_ALLOWED or so as this could introduce regressions and makes it harder to identify correct vs failed logins. Any thoughts?

@tsteur commented on November 1st 2017 Owner

Auth is actually API, sorry. The class itself is not API but the interface. This means plugins might overwrite this method in their own auth adapter and it might not work for them. I am thinking to move it outside authenticate() but then we need to define the code several times under circumstances. I will check later.

@mattab commented on November 1st 2017 Owner

:+1:

  • I haven't yet looked in detail, but it would be great to display the IP address of current request in the error message such as You cannot log in as your IP address 1.2.3.4 is not whitelisted.
  • +1 to mention in developer changelog the new setting as it does not hurt
@tsteur commented on November 1st 2017 Owner

As mentioned in previous comment it is not really a good solution and will try to find a better solution. And I won't mention it in developer changelog, this does definitely not qualify for developer change.

@tsteur commented on November 1st 2017 Owner

I will need to log at tests tomorrow and probs fix ui tests due to translation change. It should work better now and more as expected and even if login plugin is disabled or using different login alternative etc.

@tsteur commented on November 1st 2017 Owner

UI tests should now be fixed (other failures not due to this change) and am all happy now with the implementation. In config UI it doesn't really show a field for login_whitelist_ip but should be still fine.

Powered by GitHub Issue Mirror