Skip to content
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

Jump to bottom

Add possibility to restrict piwik access by ip #12242

Merged
merged 8 commits into from Nov 30, 2017
Merged

Add possibility to restrict piwik access by ip #12242

merged 8 commits into from Nov 30, 2017

Conversation

tsteur
Copy link
Member

@tsteur tsteur commented Nov 1, 2017 •
edited

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 tsteur added the Needs Review PRs that need a code review label Nov 1, 2017
@tsteur
Copy link
Member Author

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
Copy link
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
Copy link
Member Author

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
Copy link
Member Author

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 Add possibility to restrict piwik login by ip Add possibility to restrict piwik access by ip Nov 1, 2017
@tsteur
Copy link
Member Author

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.

tsteur
tsteur commented Nov 1, 2017
View reviewed changes
config/global.php
@@ -75,6 +75,18 @@

'Piwik\EventDispatcher' => DI\object()->constructorParameter('observers', DI\get('observers.global')),

'login.whitelist.ips' => function (ContainerInterface $c) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

sgiehl commented Nov 22, 2017

This might fix #4577

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

mattab commented Nov 30, 2017 •
edited

Excellent new security feature! 🎉

Added new FAQ: How do I restrict viewing the analytics reports to one or more whitelisted IP addresses or IP ranges?

@mattab mattab merged commit 9020dac into 3.x-dev Nov 30, 2017
@mattab mattab deleted the whitelistip branch November 30, 2017 22:38
@mattab mattab mentioned this pull request Nov 30, 2017
Closed
@matomo-org matomo-org deleted a comment from MatomoForumBot Dec 4, 2017
@mattab mattab added the Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. label Dec 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. Needs Review PRs that need a code review
Projects
None yet
Milestone
3.2.1
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@tsteur @mattab @sgiehl