Removed forced mgr context from findPolicy()

[CarlBohman]

What does it do?

Removes the forced mgr context for the findPolicy() function.

Why is it needed?

See bug #16212

How to test

1. Turn on info level logging.
2. Turn off the access_resource_group_enabled setting.
3. Access a page in the web context as an anonymous user.
4. No log messages about the mgr context should show up.

Related issue(s)/PR(s)

Resolves #16212

[Mark-H]

I'm not sure this is a great idea - don't we only have media source policies assigned to the mgr context?

[CarlBohman]

I don't know. All I do know is that this function is definitely getting triggered when I try to load a page in the web context and it generates a lot of extraneous error messages in those cases.

If it only applies to mgr context, why is it being called at all in web context? Alternatively, if it should only work in mgr context, then shouldn't it return null (or similar) immediately in any other context rather than creating a query, etc.? At the very least, it is causing additional queries to run right now.

[Mark-H]

I didn't mean it should only run in the mgr context, but because we only assign media source ACLs to the mgr context this code looks like it should definitely stay in place. We don't give a web context separate media source permissions, so it shouldn't try to look them up for web - it should look them up for mgr.

In your issue #16212 you haven't posted the exact error messages you were getting, so I think we need to really start there and figure out why what happens in a media source is affected by the access_resource_group_enabled setting at all, before trying to fix a problem that isn't fully understood.

[CarlBohman]

There are the error messages I was seeing:

[2022-05-11 20:00:03] (INFO @ /var/www/core/model/modx/modaccessibleobject.class.php : 40) Principal 0 does not have permission to load object of class modContext with primary key: mgr

I traced it back to this line of code.

[CarlBohman]

Based on the code that follows the line I removed, it looks like there are already sufficient controls based on context without forcing to mgr context (which renders the function parameter obsolete).

[JoshuaLuckers]

which renders the function parameter obsolete.

What's your thoughts about this @Mark-H ? I didn't look into the code but based on the comments here I think @CarlBohman is right.

[theboxer]

@Mark-H you have more thoughts on this one?

@CarlBohman Do you have steps for reproducing the issue? I tried disabling access_resource_group_enabled and setting log_level to 3 in 2.x and 3.x Revo, but didn't manage to trigger the error.

[CarlBohman]

@Mark-H you have more thoughts on this one?

@CarlBohman Do you have steps for reproducing the issue? I tried disabling access_resource_group_enabled and setting log_level to 3 in 2.x and 3.x Revo, but didn't manage to trigger the error.

I was able to get it to work consistently on my server. Maybe there is another contributing factor somewhere.

However, even if there is no error produced (which there is on my system), why is this line even here? The function definition allows for $context to be supplied, but then completely overwrites it on the line I am proposing to remove. The remainder of the code depends on the context but it will be incorrect on any context except for the mgr context.

[CarlBohman]

It may be worth noting that the error message shows that it is produced when Principal 0 (an anonymous user, importantly with no access to the mgr context) tries to access a page in the web context.

[CarlBohman]

Would it be fine if (in addition to the removal of the line), this line was added to the top of the function?
if ($context != 'mgr') return [];

That seems like it might address the concerns raised by ensuring that it only runs for the mgr context and is always empty otherwise.

[theboxer]

tbh. I do not know why the mgr context is forced there and all proposed solutions are solving just that without actually looking into why it's getting called in the first place.

without a proper steps to reproduce (I didn't manage to trigger the error, even for anonymous user), there's not much I can do about it.

[CarlBohman]

I just checked a local instance of ModX (2.8.4) and am still seeing the issue. This is what shows up in the error log (many times):
[2024-02-05 14:18:28] (INFO @ C:\MAMP\mysite\core\model\modx\modaccessibleobject.class.php : 40) Principal 0 does not have permission to load object of class modContext with primary key: mgr

I edited that modaccessibleobject.class.php file and added this line after line 36 (which is the line in _loadInstance that calls checkPolicy):
echo '<pre>'; var_dump(debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS)); die();

This is the output:

  [0]=>
  array(5) {
    ["file"]=>
    string(70) "C:\MAMP\mysite\core\model\modx\modaccessibleobject.class.php"
    ["line"]=>
    int(109)
    ["function"]=>
    string(13) "_loadInstance"
    ["class"]=>
    string(19) "modAccessibleObject"
    ["type"]=>
    string(2) "::"
  }
  [1]=>
  array(5) {
    ["file"]=>
    string(49) "C:\MAMP\mysite\core\xpdo\xpdo.class.php"
    ["line"]=>
    int(758)
    ["function"]=>
    string(4) "load"
    ["class"]=>
    string(19) "modAccessibleObject"
    ["type"]=>
    string(2) "::"
  }
  [2]=>
  array(5) {
    ["file"]=>
    string(49) "C:\MAMP\mysite\core\xpdo\xpdo.class.php"
    ["line"]=>
    int(845)
    ["function"]=>
    string(4) "call"
    ["class"]=>
    string(4) "xPDO"
    ["type"]=>
    string(2) "->"
  }
  [3]=>
  array(5) {
    ["file"]=>
    string(55) "C:\MAMP\mysite\core\model\modx\modx.class.php"
    ["line"]=>
    int(2072)
    ["function"]=>
    string(9) "getObject"
    ["class"]=>
    string(4) "xPDO"
    ["type"]=>
    string(2) "->"
  }
  [4]=>
  array(5) {
    ["file"]=>
    string(73) "C:\MAMP\mysite\core\model\modx\sources\modmediasource.class.php"
    ["line"]=>
    int(647)
    ["function"]=>
    string(10) "getContext"
    ["class"]=>
    string(4) "modX"
    ["type"]=>
    string(2) "->"
  }
  [5]=>
  array(5) {
    ["file"]=>
    string(70) "C:\MAMP\mysite\core\model\modx\modaccessibleobject.class.php"
    ["line"]=>
    int(221)
    ["function"]=>
    string(10) "findPolicy"
    ["class"]=>
    string(14) "modMediaSource"
    ["type"]=>
    string(2) "->"
  }
  [6]=>
  array(5) {
    ["file"]=>
    string(73) "C:\MAMP\mysite\core\model\modx\sources\modmediasource.class.php"
    ["line"]=>
    int(696)
    ["function"]=>
    string(11) "checkPolicy"
    ["class"]=>
    string(19) "modAccessibleObject"
    ["type"]=>
    string(2) "->"
  }
  [7]=>
  array(5) {
    ["file"]=>
    string(73) "C:\MAMP\mysite\core\model\modx\sources\modmediasource.class.php"
    ["line"]=>
    int(274)
    ["function"]=>
    string(11) "checkPolicy"
    ["class"]=>
    string(14) "modMediaSource"
    ["type"]=>
    string(2) "->"
  }
  [8]=>
  array(5) {
    ["file"]=>
    string(77) "C:\MAMP\mysite\core\model\modx\sources\modfilemediasource.class.php"
    ["line"]=>
    int(27)
    ["function"]=>
    string(10) "initialize"
    ["class"]=>
    string(14) "modMediaSource"
    ["type"]=>
    string(2) "->"
  }
  [9]=>
  array(5) {
    ["file"]=>
    string(61) "C:\MAMP\mysite\core\model\modx\modelement.class.php"
    ["line"]=>
    int(493)
    ["function"]=>
    string(10) "initialize"
    ["class"]=>
    string(18) "modFileMediaSource"
    ["type"]=>
    string(2) "->"
  }
  [10]=>
  array(5) {
    ["file"]=>
    string(61) "C:\MAMP\mysite\core\model\modx\modelement.class.php"
    ["line"]=>
    int(532)
    ["function"]=>
    string(13) "getSourceFile"
    ["class"]=>
    string(10) "modElement"
    ["type"]=>
    string(2) "->"
  }
  [11]=>
  array(5) {
    ["file"]=>
    string(60) "C:\MAMP\mysite\core\model\modx\modscript.class.php"
    ["line"]=>
    int(178)
    ["function"]=>
    string(14) "getFileContent"
    ["class"]=>
    string(10) "modElement"
    ["type"]=>
    string(2) "->"
  }
  [12]=>
  array(5) {
    ["file"]=>
    string(61) "C:\MAMP\mysite\core\model\modx\modelement.class.php"
    ["line"]=>
    int(445)
    ["function"]=>
    string(14) "getFileContent"
    ["class"]=>
    string(9) "modScript"
    ["type"]=>
    string(2) "->"
  }
  [13]=>
  array(5) {
    ["file"]=>
    string(61) "C:\MAMP\mysite\core\model\modx\modelement.class.php"
    ["line"]=>
    int(305)
    ["function"]=>
    string(10) "getContent"
    ["class"]=>
    string(10) "modElement"
    ["type"]=>
    string(2) "->"
  }
  [14]=>
  array(5) {
    ["file"]=>
    string(60) "C:\MAMP\mysite\core\model\modx\modscript.class.php"
    ["line"]=>
    int(65)
    ["function"]=>
    string(7) "process"
    ["class"]=>
    string(10) "modElement"
    ["type"]=>
    string(2) "->"
  }
  [15]=>
  array(5) {
    ["file"]=>
    string(55) "C:\MAMP\mysite\core\model\modx\modx.class.php"
    ["line"]=>
    int(1674)
    ["function"]=>
    string(7) "process"
    ["class"]=>
    string(9) "modScript"
    ["type"]=>
    string(2) "->"
  }
  [16]=>
  array(5) {
    ["file"]=>
    string(55) "C:\MAMP\mysite\core\model\modx\modx.class.php"
    ["line"]=>
    int(2517)
    ["function"]=>
    string(11) "invokeEvent"
    ["class"]=>
    string(4) "modX"
    ["type"]=>
    string(2) "->"
  }
  [17]=>
  array(5) {
    ["file"]=>
    string(55) "C:\MAMP\mysite\core\model\modx\modx.class.php"
    ["line"]=>
    int(562)
    ["function"]=>
    string(12) "_initCulture"
    ["class"]=>
    string(4) "modX"
    ["type"]=>
    string(2) "->"
  }
  [18]=>
  array(5) {
    ["file"]=>
    string(41) "C:\MAMP\mysite\htdocs\index.php"
    ["line"]=>
    int(50)
    ["function"]=>
    string(10) "initialize"
    ["class"]=>
    string(4) "modX"
    ["type"]=>
    string(2) "->"
  }
}

I can add debugging where needed to try to narrow the issue down.

If I make the change in this pull request or if I make the other change I suggested (returning an empty array if the context is not mgr), then the log messages go away.

[CarlBohman]

A bit more context based on my own tracing through the stack trace:

The specific plugin that is being initialized is Lingua. It is a static plugin located in a custom media source (not "Filesystem") that is defined on my system (a separate directory). In modelement, it calls getSourceFile at one point that has special code for media sources. This eventually calls checkPolicy in modmediasource which in turn calls findPolicy in modaccessibleobject. This eventually loads the modmediasource instance (in _loadInstance in modaccessibleobject) which calls checkpolicy('load') on the modmediasource object. This ends up calling checkPolicy with a hard-coded context of mgr even though the request has nothing to do with the mgr context.

Because anonymous users do not have "load" (or any other) permissions on modmediasource instances (again, in the mgr context), it complains. However, if the correct context is used, there is no complaint. If the context is required to be mgr in order for checkPolicy to return anything, there is also no complaint.

I still see this as an issue that needs to be resolved.