Executive summary

JAccess::checkGroup() does not work when the optional third argument $asset is not defined. This is in stark contrast with the docblock which states that it "Defaults to the global asset node."

Technical explanation

The root caused is a botched copy-pasted if-block spanning lines 144-147. It was copied from lines 101-104 except the last line $asset = $rootId was never copied. As a result, whenever $asset is empty we look into the #__assets table, get the root asset ID... and never use it. This causes null to always be returned, even if the given group does have the requested global privilege enabled.

The solution is to merely change line 144 from $rootId = $assets->getRootId(); to $asset = $assets->getRootId(); as it was intended.

Testing

Unless you are using the affected method in your own custom code you will not be able to reproduce this issue. Apparently everyone was avoiding to use checkGroup because it's buggy but nobody figured out why.

2411f29 5 Aug 2014

JAccess::checkGroup() doesn't work without an $asset

nikosdion - open - 5 Aug 2014

jissues-bot - change - 5 Aug 2014

Labels

Added: ? ?

zero-24 - comment - 5 Aug 2014

Thanks @nikosdion this makes sense.

I have test this successful with a quick Dry run (called in german: Schreibtischtest) http://en.wikipedia.org/wiki/Dry_run_%28testing%29

I don't know if we can count it as a successful test as this is only theoretical and not the real code but it is successful and the change by @nikosdion makes absolute sense.

wilsonge - comment - 5 Aug 2014

@test works as described. Get null as an asset with no 3rd param before. Get the root id after as expected.

RTC.

Bakual - comment - 5 Aug 2014

Would it make sense to change the line 103 as well?

            $rootId = $assets->getRootId();
            $asset = $rootId;

doesn't make much sense to me.
$asset = $assets->getRootId(); looks much cleaner and would make consistent code.
@nikosdion what do you think?

nikosdion - comment - 5 Aug 2014

@Bakual This is a code quality issue, not a bug, so it's out of scope of this PR. I'd rather make a new PR once this one is merged. In fact, I think this entire if-block should be moved into a private static method of its own, e.g. verifyAsset($asset).

Bakual - change - 6 Aug 2014

Status	New	⇒	Closed
Closed_Date	0000-00-00 00:00:00	⇒	2014-08-06 05:39:10

Bakual - close - 6 Aug 2014

Bakual - comment - 6 Aug 2014

Thanks!