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
fix set access #8718
Conversation
fix for matomo-org#8697 do not allow to set access to non existing websites
"login" => $userLogin, | ||
"access" => $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.
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.
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 don't understand what you are saying
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.
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);
}
}
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.
Hey @haseebeqx are you still working on the PR? Can I maybe help you somehow or explain it in a different way?
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 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.
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.
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.
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. |
fix for #8697 do not allow to set access to non existing websites
this is a minor security issue