@flamisz opened this Pull Request on May 4th 2021 Contributor

Description:

fixes #17335

Review

  • [ ] Functional review done
  • [ ] Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • [ ] Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • [ ] Security review done see checklist
  • [ ] Code review done
  • [ ] Tests were added if useful/possible
  • [ ] Reviewed for breaking changes
  • [ ] Developer changelog updated if needed
  • [ ] Documentation added if needed
  • [ ] Existing documentation updated if needed
@flamisz commented on May 4th 2021 Contributor

Hi @tsteur, I updated the PR, not resetting the super user access. During this update I've seen this comment in the code:

https://github.com/matomo-org/matomo/blob/104135aebddd112567f5ec17b8f11e757c7e57f2/core/FrontController.php#L426-L433

But I modified the reloadAccess method and now it works with widgets as well. Do you think it was delibaretly did this way? Could it be any side effect somewhere?
https://github.com/matomo-org/matomo/blob/104135aebddd112567f5ec17b8f11e757c7e57f2/core/Access.php#L137

@tsteur commented on May 4th 2021 Member

@flamisz I think we can undo the changes in Access.php and apply this change:

--- a/core/FrontController.php
+++ b/core/FrontController.php
@@ -675,7 +675,8 @@ class FrontController extends Singleton
             return null;
         }

-        if (Common::getRequestVar('token_auth', '', 'string') !== '') {
+        if (Common::getRequestVar('token_auth', '', 'string') !== ''
+            && !Common::getRequestVar('force_api_session')) {
             return null;
         }

It basically means if there's a force_api_session=1 parameter then we still load the session and authenticate the session first which is needed for the session token to work. This way the dashboard etc will work and all other requests should be working too. It be only an issue if users falsely have force_api_session=1 set in the request and don't use a session token but a real token auth. Then it won't work but then it generally shouldn't work anyway AFAIK.

It be still good for someone to review this though additionally.

@flamisz commented on May 4th 2021 Contributor

@flamisz I think we can undo the changes in Access.php and apply this change:

I just realized the way it was is not working and tried to figure out how it should 😄. Thanks, I was going to check the force_api_session but wasn't sure where yet.

I'll test it how it works this way.

This Pull Request was closed on May 11th 2021
Powered by GitHub Issue Mirror