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

Introducing a new role "write" and possibility to define capabilities #13163

Merged
merged 25 commits into from Jul 18, 2018

Conversation

Projects
None yet
3 participants
@tsteur
Copy link
Member

commented Jul 12, 2018

  • Adds new write permission which has write permissions to eg goals, custom dimensions, funnels, a/b tests, heatmaps, ... but not any manage user or website.
  • Adds new feature so plugins can define "capabilities".

Neither write role nor the capabilities 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.

tsteur added some commits Jul 5, 2018

@tsteur tsteur changed the title Acl Introducing a new role "write" and possibility to define capabilities Jul 12, 2018

@tsteur tsteur added this to the 3.7.0 milestone Jul 12, 2018

tsteur added some commits Jul 12, 2018

@tsteur

This comment has been minimized.

Copy link
Member Author

commented Jul 12, 2018

@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...

@tsteur tsteur modified the milestones: 3.7.0, 3.6.0 Jul 12, 2018

tsteur added some commits Jul 13, 2018

{
if (!isset($roleProvider)) {

This comment has been minimized.

Copy link
@tsteur

tsteur Jul 13, 2018

Author Member

was needed as some system tests otherwise fail... it would try to use DI before the environment is set up etc.

@tsteur tsteur added the Needs Review label Jul 13, 2018

@diosmosis
Copy link
Member

left a comment

Left a couple comments, still need to test locally.

*
* @return bool
* @api
*/

This comment has been minimized.

Copy link
@diosmosis

diosmosis Jul 17, 2018

Member

I think the docs here are for a different method?

This comment has been minimized.

Copy link
@tsteur

tsteur Jul 17, 2018

Author Member

👍 will fix it.

@@ -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');

This comment has been minimized.

Copy link
@diosmosis

diosmosis Jul 17, 2018

Member

Could use CapabilitiesProvider::class here

This comment has been minimized.

Copy link
@tsteur

tsteur Jul 17, 2018

Author Member

👍 will do, for some reason I never do it but definitely should.

@@ -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');

This comment has been minimized.

Copy link
@diosmosis

diosmosis Jul 17, 2018

Member

Unrelated to this PR, but I can't find the Goals_CreateNewGoal. Is this if even necessary?

This comment has been minimized.

Copy link
@tsteur

tsteur Jul 17, 2018

Author Member

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.

$sitesIdWithCapability[(int) $site['site']] = array();
}
$sitesIdWithCapability[(int) $site['site']][] = $site['access'];
}
}

This comment has been minimized.

Copy link
@diosmosis

diosmosis Jul 17, 2018

Member

I think this chunk of code could probably be refactored into a private method.

This comment has been minimized.

Copy link
@tsteur

tsteur Jul 17, 2018

Author Member

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.

This comment has been minimized.

Copy link
@diosmosis

diosmosis Jul 17, 2018

Member

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];
}

This comment has been minimized.

Copy link
@tsteur

tsteur Jul 17, 2018

Author Member

Yeah I'm not a big fan of [$value1, $value2] but will apply the change.

"PrivNone": "No access",
"PrivView": "View",
"PrivViewDescription": "A user with this role can you view all reports.",

This comment has been minimized.

Copy link
@diosmosis

diosmosis Jul 17, 2018

Member

Typo here I think.

This comment has been minimized.

Copy link
@tsteur

tsteur Jul 17, 2018

Author Member

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);

This comment has been minimized.

Copy link
@diosmosis

diosmosis Jul 17, 2018

Member

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.

@mattab

This comment has been minimized.

Copy link
Member

commented Jul 17, 2018

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...

Absolutely, this would be a welcome security improvement 👍

@tsteur

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2018

@diosmosis made the review changes
@mattab the write access is now enough for tracking requests... we will later need to edit the FAQs then

@diosmosis diosmosis merged commit 105e007 into 3.x-dev Jul 18, 2018

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@diosmosis diosmosis deleted the acl branch Jul 18, 2018

InfinityVoid added a commit to InfinityVoid/matomo that referenced this pull request Oct 11, 2018

Introducing a new role "write" and possibility to define capabilities (
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.