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

Page Overlay ignores token_auth in URL when opened from a Widget #17640

Closed
Starker3 opened this issue May 31, 2021 · 24 comments · Fixed by #17851
Closed

Page Overlay ignores token_auth in URL when opened from a Widget #17640

Starker3 opened this issue May 31, 2021 · 24 comments · Fixed by #17851
Assignees
Labels
Bug For errors / faults / flaws / inconsistencies etc. Help wanted Beginner friendly issues or issues where we'd highly appreciate community's help and involvement.

Comments

@Starker3
Copy link
Contributor

Expected Behavior

When embedding Matomo widgets in an iFrame, it is expected that all links in the widget will work when using a token_auth with the correct permissions.

Current Behavior

When embedding Matomo widgets that contain links to view the Page Overlay (For example the Pages or Page URL reports) the Page Overlay links open in a new tab and force the user to log in instead of using the token_auth present in the URL.

This causes any users that are already logged in to Matomo but don't have access to the site to see You do not have access in the Page Overlay UI.
Users that are not logged in will see the error message Your session has expired due to inactivity. Please log in to continue.

Steps to Reproduce (for Bugs)

  1. Embed the Page URL report as a widget in a page using a token_auth of a user that has view access to the report
  2. Open the page that contains the iFramed widget in a private browsing session and click on a link to to a Page Overlay
  3. You should be presented with the following error message if you are not logged in to Matomo:
    image

Your Environment

  • Matomo Version: 4.3.1
@Starker3 Starker3 added the Potential Bug Something that might be a bug, but needs validation and confirmation it can be reproduced. label May 31, 2021
@Starker3 Starker3 changed the title Page Overlay ignores token_auth in URL Page Overlay ignores token_auth in URL when opened from a Widget May 31, 2021
@sgiehl
Copy link
Member

sgiehl commented May 31, 2021

I'm not sure if the Page Overlay is meant to work in an iframe. ping @tsteur @mattab
But I guess the "problem" is that the overlay appends force_api_session=1 to various urls.

@tsteur
Copy link
Member

tsteur commented May 31, 2021

@Starker3 @sgiehl I just tested it using 4.2.1 and it worked there from what I can see so I'm assuming this might be a regression

@tsteur tsteur added Regression Indicates a feature used to work in a certain way but it no longer does even though it should. and removed Potential Bug Something that might be a bug, but needs validation and confirmation it can be reproduced. labels May 31, 2021
@tsteur tsteur added this to the 4.4.0 milestone May 31, 2021
@flamisz
Copy link
Contributor

flamisz commented May 31, 2021

Probably related to this PR: #17520

@flamisz flamisz self-assigned this May 31, 2021
@Starker3
Copy link
Contributor Author

@tsteur I tested this on 4.2.1 and had the exact same error. I tried appending the token_auth to the end of the URL (Since the link from the widget adds a # with additional information at the end of the URL) as well and it returned the error Error: You must be logged in to access this functionality.

@tsteur
Copy link
Member

tsteur commented May 31, 2021

Weird, it worked for me there using

https://mylocaldomain/index.php?module=Widgetize&action=iframe&disableLink=0&widget=1&moduleToWidgetize=Actions&actionToWidgetize=getPageUrls&idSite=1&period=year&date=yesterday&disableLink=1&widget=1&token_auth=4a45307d01810d2307be558f9596e51f#

and then opening the page overlay. Tested this in a private window where you aren't logged in.

@flamisz
Copy link
Contributor

flamisz commented May 31, 2021

@tsteur and @Starker3 could you please check if the URL of the overlay page contains the force_api_session=1 and try it with or without it to reload the page?

@tsteur
Copy link
Member

tsteur commented May 31, 2021

actually 4.2 and 4.3 works for me nicely with only the token.
Also works when adding force_api_session=1 (because I don't have a session active anyway)

@tsteur
Copy link
Member

tsteur commented May 31, 2021

I can't reproduce the issue actually

@Starker3
Copy link
Contributor Author

@flamisz removing force_api_session=1 from the URL didn't make any difference when testing (On 4.2.1).
I got the same error from the URL with and without the force_api_session: Error: Your session has expired due to inactivity. Please log in to continue.
Tested using a private window (To ensure that there is no open session)

The link that is generated for the overview on both my testing instances is the following:
https://localhost/index.php?module=Overlay&period=month&date=today&idSite=1&force_api_session=1&token_auth=f1d1fd6c253df6d6aa3840aca258a046#?l=http$3A$2F$2Flocalhost$2Ftest$20page1.html

@flamisz
Copy link
Contributor

flamisz commented May 31, 2021

@tsteur the same for me, it works on the latest version with a view token even when the force_session_api=1 is in the url.

@Starker3
Copy link
Contributor Author

@flamisz If it helps narrow it down, I'm testing using localhost for the iFrame widget with an externally hosted Matomo install (Accessible from the internet).

@flamisz
Copy link
Contributor

flamisz commented May 31, 2021

OK I was able to reproduce it, had to turn off anom user (obviously) on my local environment. @tsteur @Starker3

@tsteur
Copy link
Member

tsteur commented May 31, 2021

great find @flamisz could you also git checkout 4.2.1 to see if it happens there too?

@flamisz
Copy link
Contributor

flamisz commented Jun 1, 2021

@tsteur it happens there as well. it's not a regression in my opinion.

@tsteur tsteur added Bug For errors / faults / flaws / inconsistencies etc. and removed Regression Indicates a feature used to work in a certain way but it no longer does even though it should. labels Jun 1, 2021
@tsteur tsteur modified the milestones: 4.4.0, 4.6.0 Jun 1, 2021
@tsteur tsteur added the Help wanted Beginner friendly issues or issues where we'd highly appreciate community's help and involvement. label Jun 1, 2021
@tsteur
Copy link
Member

tsteur commented Jun 1, 2021

Great, thanks @flamisz I moved it out of the milestone for now.

@flamisz flamisz removed their assignment Jun 1, 2021
@justinvelluppillai justinvelluppillai self-assigned this Jun 29, 2021
@justinvelluppillai justinvelluppillai removed their assignment Jul 13, 2021
@geekdenz
Copy link
Contributor

geekdenz commented Aug 1, 2021

I think the token_auth should be appended to all URLs in Widgets that have it in the link or iframe widget. I think it should not create a new session as this could be confusing. If it starts a session, the view user would be logged in and potentially logged out of another account the person is using or be logged in when they were not.

We could use the force_api_session URL parameter to only allow this also. But then this parameter is required also.

If we append the token_auth to every URL, maybe it could be made part of a core JS library. However, this might prove tricky to do. Maybe an event that bubbles up the DOM could be caught and inject it so to speak, but this could be complicated to do and have other downsides later that we don't consider now.

The easy thing to do would be to just ask plugin developers to append any token_auth parameters to the URLs used in their plugins. We could add a function to the framework that does this automatically.

For example, a plugin dev could call something like:

var url = '/index.php?param1=value1&' + tokenAuth();

If we later have more URL parameters that need to be added, we could also generically do something like:

function appendAdditionalURLParameters(url) {
  return url + '&token_auth=...&other_matomo_url_parameters=values';
}

// usage
var url = appendAdditionalURLParameters('/index.php?something=else');

Of course the function name could be shortened to something like:

function aaup(url) {
  // ...
}

@tsteur
Copy link
Member

tsteur commented Aug 2, 2021

@geekdenz I'm not entirely sure but I think there are three issues:

issue 1

When you embed the widget eg like this:

index.php?module=Widgetize&action=iframe&disableLink=0&widget=1&moduleToWidgetize=Actions&actionToWidgetize=getPageUrls&idSite=1&period=year&date=yesterday&disableLink=1&widget=1&token_auth=7f4c29dfe7aea5d1400d561237b43446

then you click on overlay then the URL includes the force_api_session URL parameter which it shouldn't see below (it's not really a problem but it shouldn't contain this parameter in this case when it's not set in the original URL)

/index.php?module=Overlay&period=month&date=today&idSite=1&force_api_session=1&token_auth=7f4c29dfe7aea5d1400d561237b43446#?l=https$3A$2F$2Ffoobar$2Ftest.html

There is a method piwik.broadcast.isWidgetizeRequestWithoutSession() that we usually use to decide if we need to append force_api_session or not see example.

issue 2

The next problem is that it seems to call the startOverlaySession URL without any token in the URL

Request URL: /index.php?module=Overlay&action=startOverlaySession&idSite=1&period=month&date=today&segment=

This triggers an exception and the login screen to be shown (as it has the iframe buster on). Meaning plugins/Overlay/templates/index.twig should add the token_auth to the URL but it doesn't.

issue 3

Same applied to where it tries to render the sidebar in https://github.com/matomo-org/matomo/blob/4.4.1-rc1/plugins/Overlay/javascripts/Piwik_Overlay.js#L40-L53 --> a ajaxRequest.withTokenInUrl(); call should fix this likely.

It's probably not really to do with plugins but really with the implementation in overlay.js

Hope this helps

@geekdenz
Copy link
Contributor

geekdenz commented Aug 2, 2021

Thanks @tsteur . It does.

I found this setting:

[General]
enable_framed_pages = true

here:

matomo/core/View.php

Lines 458 to 462 in a35070b

private function shouldPropagateTokenAuthInAjaxRequests()
{
$generalConfig = Config::getInstance()->General;
return Common::getRequestVar('module', false) == 'Widgetize' || $generalConfig['enable_framed_pages'] == '1';
}

Should this be required for this to work?

I also found it in the Overlay code here:

if (token_auth.length && piwik.shouldPropagateTokenAuth) {
url += '&force_api_session=1&token_auth=' + encodeURIComponent(token_auth);

Otherwise we could add this parameter I assume:

module=Widgetize

@geekdenz
Copy link
Contributor

geekdenz commented Aug 2, 2021

Sorry, no adding the module parameter probably does not work as it is the Overlay module already.

@geekdenz
Copy link
Contributor

geekdenz commented Aug 2, 2021

Maybe we could just open it in the current window/tab if it is a widget? Would that make sense from a user's perspective?

@tsteur
Copy link
Member

tsteur commented Aug 2, 2021

While testing I changed the target to test instead of _blank then it was a bit easier to debug as you could then open the browser developer tools and see what is going on. Not sure if you meant that? I simply changed it for the "row action" element or maybe it works also to change the target here: https://github.com/matomo-org/matomo/blob/4.4.1/plugins/Overlay/javascripts/rowaction.js#L64

enable_framed_pages = true is not needed indeed.

The overlay helper you are referencing might need to be changed indeed 👍

@geekdenz
Copy link
Contributor

geekdenz commented Aug 2, 2021

@tsteur Please consider checking out my branch above. I have not created a PR yet, as there are no unit tests and I have not done extensive testing. You could check it out though to see if we're on the right track.

@tsteur
Copy link
Member

tsteur commented Aug 3, 2021

@geekdenz generally I think only a few changes in the Overlay plugin should be needed. All the other changes can be likely reverted. I'll also leave a comment in the code

@tsteur
Copy link
Member

tsteur commented Aug 3, 2021

actually be good to create a PR already so I can leave comments. You can then mark it as a "draft". It will then also run the overlay UI tests etc.

geekdenz pushed a commit that referenced this issue Aug 5, 2021
geekdenz pushed a commit that referenced this issue Aug 5, 2021
geekdenz pushed a commit that referenced this issue Aug 6, 2021
…client side while validating token_auth in View::shouldPropagateTokenAuthInAjaxRequests() #17640
geekdenz pushed a commit that referenced this issue Aug 6, 2021
geekdenz pushed a commit that referenced this issue Aug 9, 2021
geekdenz pushed a commit that referenced this issue Aug 10, 2021
@sgiehl sgiehl linked a pull request Aug 11, 2021 that will close this issue
10 tasks
sgiehl added a commit that referenced this issue Aug 13, 2021
* add token_auth to overlay requests where necessary #17640

* ensure all links on overlay page work as expected both, with token_auth and when logged in #17640

* DRY force_api_session=1 and token_auth parameters in broadcast.js and correct in other code for convenience #17640

* polish logic for overlay with token_auth and change minimal logic in client side while validating token_auth in View::shouldPropagateTokenAuthInAjaxRequests() #17640

* use 'string' as string parameter #17640

* simplify token_auth check #17640

* revert git submodule to 4.x-dev version #17640

* return $tokenAuth string (truthy) only, simplify condition, ensure & is prepended to token_auth url param #17640

* revert submodule change

* Update core/View.php

Co-authored-by: Stefan Giehl <stefan@matomo.org>

Co-authored-by: sgiehl <stefan@matomo.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For errors / faults / flaws / inconsistencies etc. Help wanted Beginner friendly issues or issues where we'd highly appreciate community's help and involvement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants