Fix groups match in auth_ismanager() and auth_isadmin()

[annda]

Even if a user was passed to the check but no groups, current user's groups were used for the match.

This PR makes sure that the correct group membership is used.

[Klap-in]

Could you add some unit tests to cover this group behavior?

Current tests can be found here: https://github.com/splitbrain/dokuwiki/blob/master/_test/tests/inc/auth_admincheck.test.php

[annda]

Writing unit tests for this issue is more complicated than I expected. It's because the tests run against the abstract AuthPlugin class, which does not let me reproduce the actual problematic cases. More extensive mocking would be necessary.

Specifically, we need to actually set up a user in an auth backend. Only then can we fetch and test their group membership. Currently this is not possible in the minimalistic test environment.

I should be able to mock auth_plugin_authplain for the purpose relatively easily, but the test will still be potentially fragile.

[splitbrain]

Tests do run in a proper DokuWiki environment. AuthPlain should be available (maybe even in global $auth - I don't remember), there is a user in https://github.com/splitbrain/dokuwiki/blob/master/_test/conf/users.auth.php and you can add your own during the test preparation. It should be possible to test this without much hassle.

[annda]

I amended the original commit to avoid merging conflicts with the latest changes in master.

Tests are also included.

[splitbrain]

This still does not seem totally right. When there is not $USERINFO you will return an empty array instead of fetching the groups via getUserData. Admittedly this will probably never happen, but if you check for $USERINFO then the fallback should be correct.

Overall I'm not too happy about all these ifs and ternary operators. Is there a way to reformat this code in a way that has less nested conditionals?

[annda]

This is the most straightforward condition I can come up with:

    if(is_null($groups)) {
        // checking the logged in user, or another one?
        if ($USERINFO && $user === $INPUT->server->str('REMOTE_USER')) {
            $groups =  (array) $USERINFO['grps'];
        } else {
            $groups = (array) $auth->getUserData($user)['grps'];
        }
    }

Now I have to figure out what is the best way to update the old tests (AuthPlugin does not have a real implementation of getUserData() so some tests will fail).

[splitbrain]

$auth->getUserData($user)['grps']; will fail when getUserData can't find the user and returns an empty array.

Which tests are failing? Can they be rewritten to test against auth_plain instead of AuthPlugin?

[annda]

auth->getUserData($user) will return false, not an empty array, when the method is not implemented.

This results in multiple tests failing with Argument 3 passed to auth_isMember() must be of the type array, null given

When I cast the return value into an (empty) array, as in the latest commit 1525c22 , the tests pass and my PR should be fit for merging.