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

Jump to bottom

[Bug] Cant not Copy User Dashboard to another user #18796

Closed
wants to merge 1 commit into from
Closed

[Bug] Cant not Copy User Dashboard to another user #18796

wants to merge 1 commit into from

Conversation

peterhashair
Copy link
Contributor

@peterhashair peterhashair commented Feb 15, 2022 •
edited

Description:

Fixes: #18692

Cant not Copy User Dashboard for different sites with different access.

My guess was the getUser API endpoints, do have related param site Id, when site selector changes, getUser still return default site sitting maybe.

Question: do we allow users to copy the dashboard to themselves?

Review

update api endpoint
d1c1628 
update api endpoint
@peterhashair peterhashair changed the title [Permission] Copy User Dashboard for different site and different access. [Bug] Copy User Dashboard for different site and different access. Feb 15, 2022
@peterhashair peterhashair changed the title [Bug] Copy User Dashboard for different site and different access. [Bug] Cant not Copy User Dashboard to another user Feb 15, 2022
@peterhashair
Copy link
Contributor Author

peterhashair commented Feb 15, 2022 •
edited

Notes: This is a draft, not sure I am on the right track if that's not the case. I will look at it here

matomo/plugins/UsersManager/UserAccessFilter.php

Lines 168 to 182 in 8121427

private function hasAccessToSameSite($login)
{
// users is allowed to see other users having view or admin access to these sites
if (!isset($this->idSitesWithAdmin)) {
$this->idSitesWithAdmin = $this->access->getSitesIdWithAdminAccess();
$this->usersWithAdminAccess = $this->model->getUsersSitesFromAccess('admin');
$this->usersWithViewAccess = $this->model->getUsersSitesFromAccess('view');
}
return (
(isset($this->usersWithViewAccess[$login]) && array_intersect($this->idSitesWithAdmin, $this->usersWithViewAccess[$login]))
||
(isset($this->usersWithAdminAccess[$login]) && array_intersect($this->idSitesWithAdmin, $this->usersWithAdminAccess[$login]))
);
}

@peterhashair peterhashair added this to the 4.8.0 milestone Feb 15, 2022
@sgiehl sgiehl added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Feb 15, 2022
sgiehl
sgiehl requested changes Feb 15, 2022
View reviewed changes
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

@peterhashair If there are issues with users not being displayed, it might be worth checking the tests first. Maybe there are test cases for UsersManager.getUsers that are currently uncovered. If so it might be better to start with writing tests for such cases in order to find the problem. Your solution looks like a wild guess, without any evidence it actually solves something.

plugins/UsersManager/Model.php
@@ -132,7 +132,21 @@ public function getUsersLoginWithSiteAccess($idSite, $access)
{
$db = $this->getDb();
$users = $db->fetchAll("SELECT login FROM " . Common::prefixTable("access")
. " WHERE idsite = ? AND access = ?", array($idSite, $access));
. " WHERE idsite = ? AND access in (?)", array($idSite, $access));
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 changing that line? Can't see any benefits in using an IN for a singe value only.

plugins/Dashboard/javascripts/dashboard.js
@@ -103,16 +103,13 @@ function copyDashboardToUser() {
var ajaxRequest = new ajaxHelper();
ajaxRequest.addParams({
module: 'API',
method: 'UsersManager.getUsers',
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't getUser already filter the returned list for users having access to the same site? If that doesn't work here, than the api method to copy a dashboard would have the same issue:

matomo/plugins/Dashboard/API.php

Lines 114 to 126 in 1155273

public function copyDashboardToUser($idDashboard, $copyToUser, $dashboardName = '')
{
Piwik::checkUserHasSomeAdminAccess();
// get users only returns users of sites the current user has at least admin access to
$users = Request::processRequest('UsersManager.getUsers', ['filter_limit' => -1]);
$userFound = false;
foreach ($users as $user) {
if ($user['login'] === $copyToUser) {
$userFound = true;
break;
}
}

@peterhashair
Copy link
Contributor Author

@sgiehl that makes sense, will write tests for that, see if getUsers() covers different site ID permission.

@justinvelluppillai justinvelluppillai modified the milestones: 4.8.0, 4.9.0 Mar 1, 2022
@sgiehl sgiehl marked this pull request as draft March 8, 2022 09:34
@github-actions
Copy link
Contributor

If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Mar 23, 2022
@peterhashair peterhashair removed the Stale The label used by the Close Stale Issues action label Mar 23, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Apr 7, 2022

If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Apr 7, 2022
@justinvelluppillai justinvelluppillai mentioned this pull request Apr 12, 2022
Closed
@sgiehl sgiehl deleted the m-18692-copy-dashboard-user branch April 5, 2023 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgiehl sgiehl sgiehl requested changes

Assignees
No one assigned
Labels
not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Stale The label used by the Close Stale Issues action
Projects
None yet
Milestone
4.9.0
Development

Successfully merging this pull request may close these issues.

Fixes problem where dashboards can't be copied to users in certain circumstances
3 participants
@peterhashair @sgiehl @justinvelluppillai