Admin impersonation of user broken when permission check fixed

[Deltik]

Bug Description

https://github.com/e107inc/e107/commit/fbcef7a3c6e8ba6a42d6aa9692127ff843cede17 fixed getperms() incorrectly identifying an unset ADMIN constant as the literal string "ADMIN", but this had the unintended side effect of preventing a e_user::loadAs() (impersonation) at this stack (using revision 5ff319cd5c55e0a5a90c33af5d713b02037e585d):

class2.php:1321, getperms()
user_model.php:655, e_user_model->checkAdminPerms()
user_model.php:523, e_user_model->isMainAdmin()
user_model.php:2094, e_user->loadAs()
user_model.php:2065, e_user->load()
user_model.php:1790, e_user->__construct()
e107_class.php:1198, e107::getObject()
e107_class.php:2112, e107::getUser()
class2.php:1569, init_session()
class2.php:672, require_once()
page.php:11, {main}()

The ADMIN constant would not be set until later in init_session():

class2.php:1645, init_session()
class2.php:672, require_once()
page.php:11, {main}()

How to Reproduce

As the main admin:

1. Navigate to /e107_admin/users.php?mode=main&action=list
2. Create a regular user, if there's not already one.
3. Under the user's options, choose "Login as …"
4. Browse to the home page and notice that you are still the main admin, not that user.

Expected Behavior

I'm not confident on the best way to solve this, but it is clear to me that we can't use getperms() until the ADMIN has been determined.

We know from a note left by @myovchev in e_user_model::checkAdminPerms() that it was intended not to use getperms(). The fix for this issue probably involves rewriting getperms() in e_user_model::checkAdminPerms().

[tgtje]

just comment; using a 2.3.3 version dated earlier this year, running php 8.2; maybe related as this function throws error while admin says logged in as.. > clicking home page (eg leave admin) etc... no go. error from wamp

Error: Undefined constant "ADMIN" in locationdrive:\xxxx\xx\e107\class2.php on line 1321

[CaMer0n]

@Deltik I have committed a draft fix, but it messes up some tests. In order for the changes to work the CLI mode needs to add values to the user-model object.

Anyway, to test user impersonation with the new code, just add define('USE_NEW_GETPERMS',true); to e107_config.php

[Deltik]

The modified method signature of e_user_model::checkAdminPerms() defeats the encapsulation offered by moving the global getperms() to e_user_model::checkAdminPerms() per user because $ap would override the object's internal state for the purposes of simulating permissions.

To remedy this, I suggest that we extract getperms() into a third static method that doesn't depend on state―perhaps called e_userperms::checkAdminPerms()―and have both getperms() and e_user_model::checkAdminPerms() call it to check the admin permissions. This way, we can have both backwards compatibility and encapsulation for the two different styles we support.

---

I have another concern, which is the extra logic for plugins. I would move this into a new method called e_user_model::checkPluginAdminPerms($plugin_name) which then calls e_user_model::checkAdminPerms() once it has figured out the $perm_str.

[Deltik]

@CaMer0n: See https://github.com/e107inc/e107/pull/5070 for my proposed fix for the broken encapsulation (plus lots of documentation!)