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

fix set access #8718

Closed
wants to merge 1 commit into from
Closed

fix set access #8718

wants to merge 1 commit into from

Conversation

haseebeqx
Copy link
Contributor

fix for #8697 do not allow to set access to non existing websites
this is a minor security issue

fix for matomo-org#8697 do not allow to set access to non existing websites
"login" => $userLogin,
"access" => $access)
);
}
Copy link
Member

Choose a reason for hiding this comment

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

Our models are supposed to be very stupid, not having any logic. It's not really like a model more like a DAO. Also the users manager should not know about the sites manager or the data structure of sites (in the case it should not having to know that there is a site table etc).

Ideally the logic would be implemented in the UsersManager\API method setUserAccess(). There you could do something like

foreach ($idSites as $idsite) {
    $site = Piwik\API\Request::processRequest('SitesManager.getSiteFromId', array('idSite' => $idSite));
    if (!empty($site)) {
         $this->model->addUserAccess($userLogin, $access, $idSite);
   }
}

I have not tested the code but something like that should work. I know it's a bit annoying as one would have to do it also around deleteUserAccess() etc. Maybe the code could be simplified eg $idSites = $this->getOnlyActuallyExistingIdSites($idSites) at the beginning of setUserAccess API method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i don't understand what you are saying

Copy link
Member

Choose a reason for hiding this comment

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

What I meant is there should be no "SELECT count(*) FROM " . Common::prefixTable("site"). " WHERE idsite = ?",$idsite in this method. This method that you changed should stay unchanged.

Instead, wherever addUserAccess() is called, one should ask the SitesManager API (Piwik\API\Request::processRequest('SitesManager.getSiteFromId', array('idSite' => $idSite));) whether the site actually exists. For example:

foreach ($idSites as $idsite) {
    $site = Piwik\API\Request::processRequest('SitesManager.getSiteFromId', array('idSite' => $idSite));
    if (!empty($site)) {
         $this->model->addUserAccess($userLogin, $access, $idSite);
   }
}

Copy link
Member

Choose a reason for hiding this comment

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

Hey @haseebeqx are you still working on the PR? Can I maybe help you somehow or explain it in a different way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not.

On Tue, Nov 10, 2015, 12:42 AM Thomas Steur notifications@github.com
wrote:

In plugins/UsersManager/Model.php
#8718 (comment):

@@ -245,11 +245,14 @@ public function userEmailExists($userEmail)
public function addUserAccess($userLogin, $access, $idSites)
{
foreach ($idSites as $idsite) {

  •        $this->getDb()->insert(Common::prefixTable("access"),
    
  •            array("idsite" => $idsite,
    
  •                  "login"  => $userLogin,
    
  •                  "access" => $access)
    
  •        );
    
  •        $count = $this->getDb()->fetchOne("SELECT count(*) FROM " . Common::prefixTable("site"). " WHERE idsite = ?",$idsite);
    
  •        if($count != 0){
    
  •             $this->getDb()->insert(Common::prefixTable("access"),
    
  •                array("idsite" => $idsite,
    
  •                     "login"  => $userLogin,
    
  •                     "access" => $access)
    
  •            );
    
  •        }
    

Hey @haseebeqx https://github.com/haseebeqx are you still working on
the PR? Can I maybe help you somehow or explain it in a different way?


Reply to this email directly or view it on GitHub
https://github.com/piwik/piwik/pull/8718/files#r44316670.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for letting us know. I will close the PR for now. Cheers for starting it, we will try to apply the other mentioned changes some time and fix it.

@tsteur
Copy link
Member

tsteur commented Nov 10, 2015

PR is incomplete and user is not working on it anymore. This PR is a good start and we only need to apply a few changes to fix this issue.

@tsteur tsteur closed this Nov 10, 2015
@tsteur tsteur added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Nov 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants