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
Conversation
core/FrontController.php
Outdated
@@ -428,8 +428,13 @@ public function init() | |||
if (Common::getRequestVar('token_auth', '', 'string') !== '' | |||
&& Request::shouldReloadAuthUsingTokenAuth(null) | |||
) { | |||
if (!Common::isPhpCliMode()) { |
There was a problem hiding this comment.
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
Hi @tsteur, I updated the PR, not resetting the super user access. During this update I've seen this comment in the code: matomo/core/FrontController.php Lines 426 to 433 in 104135a
But I modified the Line 137 in 104135a
|
@flamisz I think we can undo the changes in --- 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 It be still good for someone to review this though additionally. |
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 I'll test it how it works this way. |
core/FrontController.php
Outdated
@@ -430,6 +430,7 @@ public function init() | |||
) { | |||
Request::reloadAuthUsingTokenAuth(); | |||
Request::checkTokenAuthIsNotLimited($module, $action); | |||
Session::close(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done 👍
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Description:
fixes #17335
Review