It would create a security whole if gcr was a command in private group. It isn't though, it's in extpgmsg. Due to the way access control is implemented there are some problems with trying to set rights for commands that aren't there.
So I removed the access control check for extpgmsg (and only that) completly. So modules that use the channel have to implement some kind of access control itself. If modules with more then just one command start to use extpgmsg too it will be time to think about a solution for this issue, right now I feel just the checks in the extpgmsg() function are enough.
But here's the "problem":
<?php
if (!empty($this -> commands["extpgmsg"]))
{
$comms = array_keys($this -> commands["extpgmsg"]);
for ($i = 0; $i < count($comms); $i++)
{
if ($this -> is_command($comms[$i], $args[2])
&& $this -> accesscontrol -> check_rights($user, $comms[$i], "pgmsg"))
{
$this -> commands["extpgmsg"][$comms[$i]] -> extpgmsg ($pgname, $user, $args[2]);
$i = count($comms);
$found = true;
}
}
}
?>
Now, if I understand this correctly we consider extpgmsg part of pgmsg for security purposes. So checking the access level makes sense in that situation.
I personally think that the checks we do outside of security make it quite good (especially since my relay bot is completely locked down). But it's less of a security measure and more of a functionality measure.
Hmm, to give an example.
Org A wants to relay to B. Org C wants to relay to D. Org E wants to see everything.
Using the same relay channel, Org A gives access to Org B (and vice versa). Org C gives access to D (and vice versa). Org E gives access to everyone.
Now Org A/B can chat with each other without C/D seeing anything. C/D can chat without A/B seeing anything. E can see everything, but can't reply (since A/B/C/D didn't give them access).
All using a single relay channel.
Anyway, I think it gives it more flexabilty and doesn't really hurt anything.
There are reasons for both ways. If no exact check is implemented anyone with the access rights can spam org chat in tells.
With it you can't abuse it for other things.
Guess I'll just add another setting to enable and disable the check as wished.
Edit: check added, will commit as soon as I can reach svn again.
It still seems redundant to me, we're basically checking that we allow them to use !gcr in tells (security), then checking that we allow them to use !gcr in tells (setting in bot).
If they shouldn't be using !gcr then they shouldn't have access to it in the first place.
Content
Total Members: 1246
