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

In BulkTracking output, return the index of each invalid request. #8644

Merged
merged 5 commits into from Sep 8, 2015

Conversation

diosmosis
Copy link
Member

As title. Ready to merge, but following todo should be done either before or after:

Fixes #8296

@tsteur Can you review when you have time?

Merge Order

This PR has two other PRs in other repos that need to be merged w/ this one. The merge order should probably be:

@diosmosis diosmosis added Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Aug 25, 2015
@diosmosis diosmosis added this to the 2.15.0 milestone Aug 25, 2015
diosmosis pushed a commit to matomo-org/plugin-QueuedTracking that referenced this pull request Aug 25, 2015
diosmosis pushed a commit to matomo-org/matomo-log-analytics that referenced this pull request Aug 25, 2015
… lines of logs that were considered invalid by the Piwik tracker.
@tsteur
Copy link
Member

tsteur commented Aug 27, 2015

I reckon it's okay to merge. Wondering if one could misuse it? Eg can I send a bulk request containing 1000 requests with different siteIds and see how many are in use and how many sites this website has? Also could I try 1000 different tokens at once etc and see if maybe one is valid? I presume one gets the same information as well when sending 1000 individual requests so should not be a problem.

Another "downside" would be that we send even more data back when people are using bulk requests via piwik.js as done eg in content tracking etc but it should be rather a minor problem and can be ignored I reckon. Especially as requests should be usually valid.

@diosmosis
Copy link
Member Author

Wondering if one could misuse it? Eg can I send a bulk request containing 1000 requests with different siteIds and see how many are in use and how many sites this website has?

This sounds like a possible exploit. We could only send the indices if the request is authenticated... I'll make this change. We should perhaps make it a policy not to return information about tracking from tracker requests, since it is open to the world and is expected to be used a lot (ie, will never have rate limiting).

Also could I try 1000 different tokens at once etc and see if maybe one is valid? I presume one gets the same information as well when sending 1000 individual requests so should not be a problem.

For bulk tracking I think the token is supplied differently (ie, not for each request), and if the token auth is wrong, tracking still goes ahead (unless there is a auth required config set...).

Another "downside" would be that we send even more data back when people are using bulk requests via piwik.js as done eg in content tracking etc but it should be rather a minor problem and can be ignored I reckon.

After limiting the response to only authenticated requests, I think this will be less of an issue.

diosmosis pushed a commit to matomo-org/matomo-log-analytics that referenced this pull request Aug 27, 2015
… lines of logs that were considered invalid by the Piwik tracker.
diosmosis pushed a commit to matomo-org/matomo-log-analytics that referenced this pull request Aug 27, 2015
… lines of logs that were considered invalid by the Piwik tracker.
@diosmosis diosmosis added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Aug 27, 2015
@diosmosis
Copy link
Member Author

Modifying to require authentication is a bit non-trivial, making this a WIP for now.

Btw, I noticed that in BulkTracking, authentication is done for every request instead of just once before hand. @tsteur Do you know if this is intentional? See: https://github.com/piwik/piwik/blob/master/plugins/BulkTracking/Tracker/Requests.php#L35 and https://github.com/piwik/piwik/blob/master/plugins/BulkTracking/Tracker/Requests.php#L85. Maybe I am looking at the code wrong.

@tsteur
Copy link
Member

tsteur commented Aug 28, 2015

I think it is only if bulk_requests_require_authentication is enabled and it is disabled by default.

https://github.com/piwik/piwik/blob/master/plugins/BulkTracking/Tracker/Requests.php#L35

It shouldn't be really slow as we cache that kinda part during one request so we're not authenticating the same token twice in one request.

@diosmosis diosmosis force-pushed the 8296_bulk_track_request_index branch from 43c7ac1 to 1748ed4 Compare August 31, 2015 21:41
diosmosis pushed a commit to matomo-org/matomo-log-analytics that referenced this pull request Sep 1, 2015
… lines of logs that were considered invalid by the Piwik tracker.
@diosmosis diosmosis removed the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Sep 1, 2015
@diosmosis
Copy link
Member Author

@tsteur Ready for another review.

@diosmosis
Copy link
Member Author

@mattab @tsteur ping-ing for review

return false;
}

Piwik::postEvent('Request.initAuthenticationObject');
Copy link
Member

Choose a reason for hiding this comment

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

Have you tested this case re performance?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry just noticed this is only called once, so shouldn't be a big problem I presume

@tsteur
Copy link
Member

tsteur commented Sep 7, 2015

Looks good to me, I'm just not sure if any user with view permission should be allowed to see the result indices. Maybe we could limit it to super users or so.

@diosmosis
Copy link
Member Author

Made some changes.

diosmosis added a commit to matomo-org/matomo-log-analytics that referenced this pull request Sep 8, 2015
Refs matomo-org/matomo#8644, use bulk tracking indices to display the lines of logs that were considered invalid by the Piwik tracker.
@diosmosis
Copy link
Member Author

@tsteur Merging, let me know if you still have more comments.

diosmosis added a commit that referenced this pull request Sep 8, 2015
Fixes #8296, in BulkTracking output, return the index of each invalid request.
@diosmosis diosmosis merged commit a943485 into master Sep 8, 2015
@diosmosis diosmosis deleted the 8296_bulk_track_request_index branch September 8, 2015 19:56
diosmosis added a commit to matomo-org/plugin-QueuedTracking that referenced this pull request Sep 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. 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 not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants