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
Introducing a new role "write" and possibility to define capabilities #13163
Conversation
@mattab should we may lower permission needed for recording certain tracking requests in past to "write"? This can increase security as if token leaks for some reason or whoever gets this token cannot give new users access to a site etc... |
{ | ||
if (!isset($roleProvider)) { |
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.
was needed as some system tests otherwise fail... it would try to use DI before the environment is set up etc.
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.
Left a couple comments, still need to test locally.
* | ||
* @return bool | ||
* @api | ||
*/ |
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 think the docs here are for a different method?
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.
👍 will fix it.
core/Plugin/Controller.php
Outdated
@@ -698,6 +699,18 @@ protected function setBasicVariablesView($view) | |||
$view->emailSuperUser = implode(',', Piwik::getAllSuperUserAccessEmailAddresses()); | |||
} | |||
|
|||
$capabilities = array(); | |||
if ($this->idSite && $this->site) { | |||
$capabilityProvider = StaticContainer::get('Piwik\Access\CapabilitiesProvider'); |
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.
Could use CapabilitiesProvider::class
here
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.
👍 will do, for some reason I never do it but definitely should.
plugins/Goals/Widgets/AddNewGoal.php
Outdated
@@ -31,7 +31,7 @@ public static function configure(WidgetConfig $config) | |||
|
|||
$goals = API::getInstance()->getGoals($idSite); | |||
|
|||
if (Piwik::isUserHasAdminAccess($idSite)) { | |||
if (Piwik::isUserHasWriteAccess($idSite)) { | |||
$config->setName('Goals_AddNewGoal'); | |||
} else { | |||
$config->setName('Goals_CreateNewGoal'); |
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.
Unrelated to this PR, but I can't find the Goals_CreateNewGoal
. Is this if
even necessary?
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.
That's really funny... same is in EditGoals.php
. In this example it doesn't even make too much sense to either "add or create". I'll remove both IFs
and go only with the initial wording.
plugins/UsersManager/API.php
Outdated
$sitesIdWithCapability[(int) $site['site']] = array(); | ||
} | ||
$sitesIdWithCapability[(int) $site['site']][] = $site['access']; | ||
} | ||
} |
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 think this chunk of code could probably be refactored into a private method.
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 was thinking about that... but it wouldn't really be re-used anywhere and I would kind of need to split it into two private methods and go over the for loop in each method as they work on different variables (unless one private method returned an array of two variables). Didn't see much benefit of it then to put it into a private method but could do it.
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 figured you could have one method like:
private function getRolesAndCapabilitiesFor($userLogin)
{
$sites = $this->model->getSitesAccessFromUser($userLogin);
$roleIds = $this->roleProvider->getAllRoleIds();
$sitesIdWithRole = array();
$sitesIdWithCapability = array();
foreach ($sites as $site) {
if (in_array($site['access'], $roleIds, true)) {
$sitesIdWithRole[(int) $site['site']] = $site['access'];
} else {
if (!isset($sitesIdWithCapability[(int) $site['site']])) {
$sitesIdWithCapability[(int) $site['site']] = array();
}
$sitesIdWithCapability[(int) $site['site']][] = $site['access'];
}
}
return [$sitesIdWithRole, $sitesIdWithCapability];
}
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.
Yeah I'm not a big fan of [$value1, $value2]
but will apply the change.
plugins/UsersManager/lang/en.json
Outdated
"PrivNone": "No access", | ||
"PrivView": "View", | ||
"PrivViewDescription": "A user with this role can you view all reports.", |
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.
Typo here I think.
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.
Yes, will fix it 👍
// plugins/Live/tests/System/ApiCounterTest.php the environment is not set up yet | ||
$role = new Access\RolesProvider(); | ||
$capabilities = new Access\CapabilitiesProvider(); | ||
parent::__construct($role, $capabilities); |
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.
Think this is because it's used in provideContainerConfigBeforeClass()
. Might work to set it as \DI\object(FakeAccess::class)
, but I suppose devs would expect to be able to create a new FakeAccess()
w/o much trouble.
Absolutely, this would be a welcome security improvement 👍 |
@diosmosis made the review changes |
…matomo-org#13163) * started working on some ACL concept * acl implementation * add category * small tweaks * more tweaks * more api methods and fixes * cache capabilities * various enhancements, fixes, tweaks * more tweaks * added more tests and fixed some bugs * fix parameter * make sure to be BC * make sure to be BC * fix some tests * more apis, translations, changelog entry, ... * update db * correct error message * fix capabilities were not detected in tests * directly access provider * fix and add test * JS api to check capabilities, better structure for capabilities in tests * add ability to inject permissions * apply review changes * fix test
write
permission which has write permissions to eg goals, custom dimensions, funnels, a/b tests, heatmaps, ... but not any manage user or website.Neither
write
role nor thecapabilities
are shown in the UI yet as we are changing the user management in #13158 anyway. We still want to prepare the core for this already so we can start implementing those features in plugins.I'd suggest to adjust custom dimensions, and other plugins after merging this into core and change them to check for write permission instead of admin permission.