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.

[Ibochkarev]

Summary

Removes the line $context = 'mgr'; from modMediaSource::findPolicy(), which was overwriting the $context parameter regardless of the caller. This fixes the error "Principal 0 does not have permission to load object of class modContext with primary key: mgr" when an anonymous user loads a web page that triggers media source initialization (e.g. via a plugin like Lingua loading content from a custom media source).

Root Cause

The stack trace in #16212 shows:

1. Web context page load → modX::_initCulture → invokeEvent → script processing
2. modScript::process → modElement::getContent → getSourceFile (for elements in custom media sources)
3. modMediaSource::initialize → checkPolicy → findPolicy
4. findPolicy forced $context = 'mgr', then called $this->xpdo->getContext('mgr')
5. getContext('mgr') loads modContext with key mgr, which triggers modAccessibleObject::_loadInstance → checkPolicy('load') on the context object
6. Principal 0 (anonymous) has no mgr access → error logged

Assessment

The change is correct and should be merged.

1. Parameter semantics: The $context parameter exists for a reason; overwriting it unconditionally makes the parameter useless and violates the principle of least surprise.

2. Behavior after fix: When findPolicy() is called with no args (from modAccessibleObject::checkPolicy), $context remains ''. Then:

   • $context === $this->xpdo->context->get('key') → '' === 'web' → false
   • $this->xpdo->getContext('') → no context with key '' → falsy, so the elseif is skipped
   • $enabled stays true, the SQL runs with :context => '', and media source ACLs (typically context_key = 'mgr') do not match → empty policy → access allowed (no restrictions)

3. No mgr context load: We no longer call getContext('mgr') when in web context, so the error is avoided.

4. Consistency: modTemplateVar::findPolicy() uses $context = !empty($context) ? $context : $this->xpdo->context->get('key') to default to the current context. For modMediaSource, the minimal fix (removing the forced mgr) is sufficient because media source ACLs are mgr-only; in web context we simply get an empty policy.

3.x Port

PR #16845 ports this fix to 3.x with the enhanced implementation: instead of just removing the line, it uses the same defaulting pattern as modContext, modResource, and modTemplateVar:

$context = !empty($context) ? $context : $this->xpdo->context->get('key');

This ensures callers like modCacheManager that pass a context key (e.g. web) are respected, and when no context is passed, the current request context is used.

Verdict

Approve. The fix resolves the reported issue, restores correct use of the $context parameter, and avoids unnecessary mgr context loading in web context.

[CarlBohman]

I updated the fix to match PR #16845. This seems like a good fix.