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

Add possibility to restrict piwik access by ip #12242

Merged
merged 8 commits into from Nov 30, 2017

Conversation

Projects
None yet
3 participants
@tsteur
Member

tsteur commented Nov 1, 2017

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 added some commits Oct 29, 2017

@tsteur tsteur added the Needs Review label Nov 1, 2017

@tsteur

This comment has been minimized.

Show comment
Hide comment
@tsteur

tsteur Nov 1, 2017

Member

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.

Member

tsteur commented Nov 1, 2017

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

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Nov 1, 2017

Member

👍

  • 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
Member

mattab commented Nov 1, 2017

👍

  • 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

This comment has been minimized.

Show comment
Hide comment
@tsteur

tsteur Nov 1, 2017

Member

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.

Member

tsteur commented Nov 1, 2017

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

This comment has been minimized.

Show comment
Hide comment
@tsteur

tsteur Nov 1, 2017

Member

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.

Member

tsteur commented Nov 1, 2017

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 tsteur changed the title from Add possibility to restrict piwik login by ip to Add possibility to restrict piwik access by ip Nov 1, 2017

@tsteur

This comment has been minimized.

Show comment
Hide comment
@tsteur

tsteur Nov 1, 2017

Member

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.

Member

tsteur commented Nov 1, 2017

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.

@@ -75,6 +75,18 @@
'Piwik\EventDispatcher' => DI\object()->constructorParameter('observers', DI\get('observers.global')),
'login.whitelist.ips' => function (ContainerInterface $c) {

This comment has been minimized.

@tsteur

tsteur Nov 1, 2017

Member

Config is wrapped in DI so other plugins can add further IPs dynamically without the risk of changing the config when manipulating the config directly in DI

@tsteur

tsteur Nov 1, 2017

Member

Config is wrapped in DI so other plugins can add further IPs dynamically without the risk of changing the config when manipulating the config directly in DI

@sgiehl

This comment has been minimized.

Show comment
Hide comment
@sgiehl

sgiehl Nov 22, 2017

Member

This might fix #4577

Member

sgiehl commented Nov 22, 2017

This might fix #4577

@mattab mattab added this to the 3.2.1 milestone Nov 22, 2017

@mattab

This comment has been minimized.

Show comment
Hide comment
@mattab
Member

mattab commented Nov 30, 2017

@mattab mattab merged commit 9020dac into 3.x-dev Nov 30, 2017

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details

@mattab mattab deleted the whitelistip branch Nov 30, 2017

@matomo-org matomo-org deleted a comment from MatomoForumBot Dec 4, 2017

@mattab mattab added the Enhancement label Dec 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment