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

Improve role/capability handling in usermanager #18750

Closed
sgiehl opened this issue Feb 7, 2022 · 1 comment · Fixed by #19388
Closed

Improve role/capability handling in usermanager #18750

sgiehl opened this issue Feb 7, 2022 · 1 comment · Fixed by #19388
Assignees
@sgiehl
Labels
Bug For errors / faults / flaws / inconsistencies etc. c: Usability For issues that let users achieve a defined goal more effectively or efficiently.
Milestone
4.12.0

Comments

@sgiehl
Copy link
Member

sgiehl commented Feb 7, 2022

When editing the permissions for a user, it is currently always possible in the UI to add capabilities.
But if the users doesn't have any access to a certain site, the API request for adding the capability does nothing, but still returns success.

This is somehow unexpected, as the API should throw an exception if adding a capability isn't possible. And also the UI shouldn't show the capability selection in that case.

Throwing an exception can easily be added here:

matomo/plugins/UsersManager/API.php

Lines 1206 to 1226 in d17b842

foreach ($capabilities as $entry) {
$cap = $this->capabilityProvider->getCapability($entry);
foreach ($idSites as $idSite) {
$hasRole = array_key_exists($idSite, $sitesIdWithRole);
$hasCapabilityAlready = array_key_exists($idSite, $sitesIdWithCapability) && in_array($entry, $sitesIdWithCapability[$idSite], true);
// so far we are adding the capability only to people that also have a role...
// to be defined how to handle this... eg we are not throwing an exception currently
// as it might be used as part of bulk action etc.
if ($hasRole && !$hasCapabilityAlready) {
$theRole = $sitesIdWithRole[$idSite];
if ($cap->hasRoleCapability($theRole)) {
// todo this behaviour needs to be defined...
// when the role already supports this capability we do not add it again
continue;
}
$this->model->addUserAccess($userLogin, $entry, array($idSite));
}
}

Hiding the selection box can be achieved by adding something like v-if="userRole !== 'noaccess'" here:

matomo/plugins/UsersManager/vue/src/CapabilitiesEdit/CapabilitiesEdit.vue

Lines 37 to 46 in 1dd8569

<Field
:model-value="capabilityToAddId"
@update:model-value="capabilityToAddId = $event; onToggleCapability(true)"
:disabled="isBusy"
uicontrol="expandable-select"
name="add_capability"
:full-width="true"
:options="availableCapabilitiesGrouped"
>
</Field>

@tsteur this one should be quite easy to fix. Let me know if I should quickly set up a PR to fix that.
@sgiehl sgiehl added Bug For errors / faults / flaws / inconsistencies etc. c: Usability For issues that let users achieve a defined goal more effectively or efficiently. labels Feb 7, 2022
@tsteur tsteur added this to the 4.9.0 milestone Feb 7, 2022
@tsteur
Copy link
Member

tsteur commented Feb 7, 2022

Makes sense, put it in 4.9 as it's quick to do. If super quick feel free to do it in 4.8

@tsteur tsteur modified the milestones: 4.9.0, 4.10.0 Mar 2, 2022
@sgiehl sgiehl self-assigned this Jun 21, 2022
@sgiehl sgiehl mentioned this issue Jun 22, 2022
Merged
11 tasks
@justinvelluppillai justinvelluppillai changed the title Adding capabilities for users without permissions doesn't work correctly Improve role/capability handling in usermanager Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sgiehl sgiehl

Labels
Bug For errors / faults / flaws / inconsistencies etc. c: Usability For issues that let users achieve a defined goal more effectively or efficiently.
Projects
None yet
Milestone
4.12.0
Development

Successfully merging a pull request may close this issue.

Improve role/capability handling in usermanager matomo-org/matomo
3 participants
@tsteur @justinvelluppillai @sgiehl