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

Use token auth when widgetize #17520

Merged
merged 7 commits into from May 11, 2021
Merged

Use token auth when widgetize #17520

merged 7 commits into from May 11, 2021

Conversation

flamisz
Copy link
Contributor

@flamisz flamisz commented May 4, 2021

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 flamisz self-assigned this May 4, 2021
@flamisz flamisz added 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. labels May 4, 2021
@flamisz flamisz added this to the 4.3.0 milestone May 4, 2021
@@ -428,8 +428,13 @@ public function init()
if (Common::getRequestVar('token_auth', '', 'string') !== ''
&& Request::shouldReloadAuthUsingTokenAuth(null)
) {
if (!Common::isPhpCliMode()) {
Copy link
Member

Choose a reason for hiding this comment

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

@flamisz might be worth trying not to use the session authenticator when a token_auth is used in the query. Eg return null in makeSessionAuthenticator() if a session is set? Unsetting super user access etc may have side effects not sure

@flamisz
Copy link
Contributor Author

flamisz commented May 4, 2021

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

// Force the auth to use the token_auth if specified, so that embed dashboard
// and all other non widgetized controller methods works fine
if (Common::getRequestVar('token_auth', '', 'string') !== ''
&& Request::shouldReloadAuthUsingTokenAuth(null)
) {
Request::reloadAuthUsingTokenAuth();
Request::checkTokenAuthIsNotLimited($module, $action);
}

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?

public function reloadAccess(Auth $auth = null)

@flamisz flamisz removed the Needs Review PRs that need a code review label May 4, 2021
@flamisz flamisz marked this pull request as draft May 4, 2021 21:57
@tsteur
Copy link
Member

tsteur commented May 4, 2021

@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
Copy link
Contributor Author

flamisz commented May 4, 2021

@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.

@flamisz flamisz marked this pull request as ready for review May 4, 2021 22:40
@flamisz flamisz added the Needs Review PRs that need a code review label May 4, 2021
@@ -430,6 +430,7 @@ public function init()
) {
Request::reloadAuthUsingTokenAuth();
Request::checkTokenAuthIsNotLimited($module, $action);
Session::close();
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for closing the session at this point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

During the process somewhere we start it, but actually we don't need it as we handle this request as stateless, so I thought we can close it (the original idea was coming from @tsteur).

Copy link
Member

Choose a reason for hiding this comment

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

In theory that might make sense, but not sure if that could cause any regressions if someone tries to perform a controller action with a token_auth, that actually tries to change the session. But maybe we can simply change it and wait if any regressions are reported.

Copy link
Member

Choose a reason for hiding this comment

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

It should be fine to close the session if force_api_session is not present in the URL AFAIK. There could be a regression but it would be hard to know. In theory though in these cases the session shouldn't be started anyway.

Copy link
Member

@diosmosis diosmosis May 6, 2021

Choose a reason for hiding this comment

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

Would this be an issue for the "embed all of matomo" use case (one big user uses this, can't remember their name starts w/ an 'M')? Ie, the [General] enable_framed_pages [General] enable_framed_settings config. I would think the session might be written to there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done 👍

Copy link
Member

Choose a reason for hiding this comment

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

I realise using new SessionNamespace will actually lounge a session. Eg in redirectDashboardToWelcomePage method it might start a session and that might even happen when it's an API call or when the action is embedded as an iframe. Closing it now could break some things that were working before (whether they should have been working before or not is a different question). I didn't debug but likely the close shouldn't do anything because ideally it doesn't start the session in the first place so I'm tempted to simply remove the close call. And eg if force_api_session=1 then the session might be still needed in order to write session changes

Copy link
Member

Choose a reason for hiding this comment

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

If the request is an API call then we for sure don't want to use any session but widgetized requests may use a session I would say

Copy link
Member

Choose a reason for hiding this comment

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

@flamisz I think ^ means my idea wouldn't be enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I tried a couple of things, tested what I could, and looks like the force_api_session does the job here. If that's false, we don't have a session, so don't need to close.

I couldn't find a case when we really need to close it.

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.

Widgetize request with token_auth param fails if superuser session exists
4 participants