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

Potential BC break: controller requests with token_auth no longer use session auth #17596

Closed
diosmosis opened this issue May 19, 2021 · 5 comments
Assignees
Labels
answered For when a question was asked and we referred to forum or answered it. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Milestone

Comments

@diosmosis
Copy link
Member

After #17520 we avoid using session auth if token_auth is in the URL and force_api_session is not set to 1. This is fine for API requests, but in some places (like the GoogleAnalyticsImporter), we make ajax requests to controller methods w/ the token_auth in the URL. These now are not authenticated when they should be.

Expected Behavior

When requesting a controller method w/ token_auth in the URL, allow use of SessionAuth.

Current Behavior

The session is not used, even if the API is not being requested, if token_auth is in the URL.

Possible Solution

Two quick fixes would be:

  • add to the check in core $module == 'API'
  • in GoogleAnalyticsImporter add force_api_session=1 to the calls (though this would not fix any other uses in the wild or in our code, so the BC break would still be there)

Steps to Reproduce (for Bugs)

Can be reproduced by trying to start an import in the GoogleAnalyticsImporter API.

FYI @tsteur

@diosmosis diosmosis added the Regression Indicates a feature used to work in a certain way but it no longer does even though it should. label May 19, 2021
@diosmosis diosmosis added this to the 4.3.1 milestone May 19, 2021
@tsteur
Copy link
Member

tsteur commented May 19, 2021

@diosmosis the solution in this case is likely similar to #17587 to send the correct force_api_session=1 request in GA. It basically wasn't really supposed to work before. Or alternatively remove the token_auth from the request. Can you point me to the code maybe in GA Importer where the problem is?

Generally we'd want to have this check not just for API, but also widgets and because any action can be embedded using token we kind of have to keep this new behaviour for things to work correctly as expected and it might be better to fix the code in the plugins etc. Be good to let me know though where this happens

@tsteur
Copy link
Member

tsteur commented May 19, 2021

Just seeing the code in https://github.com/matomo-org/plugin-GoogleAnalyticsImporter/blob/4.x-dev/angularjs/import-scheduler/import-scheduler.controller.js#L57 . I think in that case we shouldn't set the token hard coded but use the method withTokenInUrl in the API angular service which then will behave correct automatically

@tsteur
Copy link
Member

tsteur commented May 19, 2021

Looked through the code and think the only other place that also needs updating is https://github.com/matomo-org/matomo/blob/4.3.0/plugins/UserCountryMap/javascripts/visitor-map.js#L1189

@flamisz
Copy link
Contributor

flamisz commented May 19, 2021

I'd prefer fixing the code as @tsteur suggested. This way will have a clear way of how it should and how it works.

@tsteur tsteur self-assigned this May 20, 2021
tsteur added a commit that referenced this issue May 20, 2021
@tsteur
Copy link
Member

tsteur commented May 20, 2021

I just tested and visitor-map is already doing it correctly. @diosmosis I think you updated the GA importer so we can close this issue? Let me know if that's not the case and I will reopen.

@tsteur tsteur closed this as completed May 20, 2021
@tsteur tsteur added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. answered For when a question was asked and we referred to forum or answered it. and removed Regression Indicates a feature used to work in a certain way but it no longer does even though it should. labels May 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
answered For when a question was asked and we referred to forum or answered it. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

No branches or pull requests

3 participants