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

Adds proxy method to call API using session #10201

Closed
wants to merge 4 commits into from
Closed

Adds proxy method to call API using session #10201

wants to merge 4 commits into from

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented May 31, 2016

New proxy method is used for download links in email reports and datatables, to avoid exposing the token_auth in direct API urls.

fixes #10185
fixes #10147

@sgiehl sgiehl added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label May 31, 2016
@sgiehl sgiehl added this to the 2.16.2 milestone May 31, 2016
@sgiehl sgiehl added 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 and removed Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels Jun 1, 2016
@@ -111,7 +111,7 @@ public function showInContext()
$controllerName = Common::getRequestVar('moduleToLoad');
$actionName = Common::getRequestVar('actionToLoad', 'index');

if($controllerName == 'API') {
if($controllerName == 'API' || ($controllerName == 'Proxy' && $actionName == 'callApi')) {

Choose a reason for hiding this comment

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

Could you move whole this condition to separate method eg. isSupportedRequest or isAllowedRequest ?

@andrzejewsky
Copy link

@sgiehl 👍 (I left a few comments)

@sgiehl
Copy link
Member Author

sgiehl commented Jun 7, 2016

Thx for the comments. I'll check those the comming days

@sgiehl
Copy link
Member Author

sgiehl commented Jun 13, 2016

@andrzejewsky I've adjusted most of the stuff according to your feedback.

@tsteur @mattab anyone time for a quick look at the PR, so we can merge it?

@tsteur
Copy link
Member

tsteur commented Jun 13, 2016

I'm quite busy :( don't think I'll have time for it soon as it will take some time re security etc

@tsteur
Copy link
Member

tsteur commented Jun 13, 2016

Maybe some tests could be added? Or is tested via the export UI tests already maybe?

@mattab
Copy link
Member

mattab commented Jul 8, 2016

Review

  • Add tests so the whole functionnality is tested

@mattab mattab modified the milestones: 2.16.x (LTS), 2.16.2 Jul 8, 2016
@mattab mattab added PP and removed PP labels Jul 8, 2016
@mattab mattab closed this Jul 18, 2016
@mattab
Copy link
Member

mattab commented Jul 20, 2016

fyi reasons for not merging were:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

4 participants