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

tsteur
Copy link
Member

@tsteur tsteur 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 tsteur changed the title Acl Introducing a new role "write" and possibility to define capabilities Jul 12, 2018
@tsteur tsteur added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Jul 12, 2018
@tsteur tsteur added this to the 3.7.0 milestone Jul 12, 2018
@tsteur
Copy link
Member Author

tsteur 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
{
if (!isset($roleProvider)) {
Copy link
Member Author

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.

@tsteur tsteur added the Needs Review PRs that need a code review label Jul 13, 2018
@tsteur tsteur removed the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Jul 13, 2018
Copy link
Member

@diosmosis diosmosis left a 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
*/
Copy link
Member

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 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');
Copy link
Member

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

Copy link
Member Author

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.

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

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?

Copy link
Member Author

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.

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

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.

Copy link
Member Author

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.

Copy link
Member

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

Copy link
Member Author

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.

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

Choose a reason for hiding this comment

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

Typo here I think.

Copy link
Member Author

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);
Copy link
Member

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.

@mattab
Copy link
Member

mattab 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
Copy link
Member Author

tsteur 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
@diosmosis diosmosis deleted the acl branch July 18, 2018 04:47
@mattab mattab added Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. labels Aug 28, 2018
InfinityVoid pushed a commit to InfinityVoid/matomo that referenced this pull request Oct 11, 2018
…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
Labels
Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants