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

Makes report export overlay compatible with app specific tokens #16066

Merged
merged 9 commits into from Jun 18, 2020

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Jun 14, 2020

The export popover will now allow to choose between session auth or using a custom app specific token:

image

fixes #16043

@sgiehl sgiehl added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Jun 14, 2020
@sgiehl sgiehl added this to the 4.0.0 milestone Jun 14, 2020
@tsteur
Copy link
Member

tsteur commented Jun 14, 2020

image
image

Should the place holder and description be maybe "app specific token auth"?

Could maybe also have a link to manage the tokens so user can create them easily?
image

Not really sure though if this is actually needed? Not sure what the value be of being able to enter the token auth? It might again rather encourage to share the token?

Code works though 👍

BTW could we add here: https://github.com/matomo-org/matomo/blob/4.x-dev/core/Access.php#L177 a Session::close()? Only seeing this now... makes sure we close the session again immediately which increases performance. Would do it maybe only if authentication using that token was successful.

@sgiehl
Copy link
Member Author

sgiehl commented Jun 15, 2020

Should the place holder and description be maybe "app specific token auth"?

Was thinking about that as well. But actually we are naming the tokens that way nowhere. In security settings they are simply called "auth tokens".

Not really sure though if this is actually needed? Not sure what the value be of being able to enter the token auth? It might again rather encourage to share the token?

That's correct. But actually someone needs to enter an auth token and so he is doing that kind of on purpose...

@sgiehl
Copy link
Member Author

sgiehl commented Jun 15, 2020

Could maybe also have a link to manage the tokens so user can create them easily?

Tried that, but seems not that easy as we are using an angular radio field there, and HTML seems not to work in the option titles

@tsteur
Copy link
Member

tsteur commented Jun 15, 2020

All good about the other comments 👍

That's correct. But actually someone needs to enter an auth token and so he is doing that kind of on purpose...

I'm still not sure it's really needed that users can configure their actual token though? Not sure what the purpose is and when they would use it?

@sgiehl
Copy link
Member Author

sgiehl commented Jun 16, 2020

I'm still not sure it's really needed that users can configure their actual token though? Not sure what the purpose is and when they would use it?

If someone wants to use the generated url for anything outside of Matomo or wants to bookmark it for later usage for example. The session url wouldn't work for this...

@tsteur
Copy link
Member

tsteur commented Jun 16, 2020

@sgiehl we rather not have this option for now to keep the UI easy and simple. I reckon only very few users would actually use it (like less than 5%) that way and then they could still simply replace the token in the URL manually. If few people ask for it later then we could still add it.

@sgiehl
Copy link
Member Author

sgiehl commented Jun 17, 2020

@tsteur Should we add some kind of note then, that the generated URL will only work in the current session? Might otherwise be confusing for people why it's not possible anymore to use the link somewhere else.

@tsteur
Copy link
Member

tsteur commented Jun 17, 2020

I reckon it's not needed. It's like any other link doesn't work somewhere else either. Of course this one used to work. Not sure if you have anything specific in mind?

@sgiehl
Copy link
Member Author

sgiehl commented Jun 17, 2020

@tsteur We could maybe show something like:
Note: The generated export URL will only work in the current browser session. If you want to use it somewhere else you need to use an app specific token.

We could show the note maybe only when someone clicks show export url

@tsteur
Copy link
Member

tsteur commented Jun 17, 2020

@sgiehl let's maybe show it as tooltip when they hover the export button, hide export URL or the textarea that is showing the URL.

Maybe add a sentence:

Note: The generated export URL will only work in the current browser session. If you want to use it somewhere else you need to use an app specific token. You can configure these tokens in Admin -> Security -> Token Auths.

@sgiehl
Copy link
Member Author

sgiehl commented Jun 18, 2020

@tsteur applied the changes.

@tsteur tsteur merged commit 27842d7 into 4.x-dev Jun 18, 2020
@tsteur tsteur deleted the exportauth branch June 18, 2020 21:09
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.

Export of reports no longer working in Matomo 4
2 participants