Example: CCK and Content Permissions modules enabled; some fields restricted to privileged users. Search index is automatically updated via normal invocation of cron.php (running as anonymous user).
Suppose I create/amend a node and manually invoke cron (/admin/status/reports/run-cron). The contents of the restricted fields will get into the search index. 2 problems with this:
1. If an unrestricted user searches for words in the restricted fields then the relevant nodes will be returned in the search results (though happily the content of the restricted fields won't be shown)
2. Some nodes have their restricted fields indexed, while others don't.
A simple fix would be to force $user to http://api.drupal.org/api/function/drupal_anonymous_user/6. However there may be situations in which cron needs to run with additional privileges - see comment #19 in #5380: introduce a "cron" user to fix various cron related issues. Though maybe such situations would be best handled via a contrib module.
Comment | File | Size | Author |
---|---|---|---|
#23 | 431776-cron.patch | 1.83 KB | agentrickard |
#20 | 431776-cron.patch | 2.29 KB | agentrickard |
#13 | cron.patch | 1.13 KB | Damien Tournoud |
#11 | cron.patch | 1.23 KB | catch |
#9 | cron.patch | 798 bytes | catch |
Comments
Comment #1
gpk CreditAttribution: gpk commentedClarification/improvement to suggestion: the "simple fix" is only needed when cron is run using the run-cron path (i.e. in http://api.drupal.org/api/function/system_run_cron/6). This avoids interfering with cron.php in any way.
Arguably content permissions module should implement the fix (e.g. by overriding http://api.drupal.org/api/function/theme_status_report/6), but IMO it would be better to fix it this in core, otherwise any module that modifies content depending on user roles/permissions could interfere with cron when invoked via the run-cron path and would therefore need to override theme_status_report().
Comment #2
gpk CreditAttribution: gpk commentedChanging title. This seems the simplest approach, contrib cron modules or special case sites can do what they want with regard to the user that "runs" cron.
The problem is potentially wider than content permissions module; having cron run by default as different users depending on whether invoked by cron.php or the run-cron link on the status report page hardly helps troubleshooting.
Comment #3
jonathan1055 CreditAttribution: jonathan1055 commentedsubscribing
Comment #4
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis is obviously a bug in Drupal core, but seems like a serious design issue in content permission, too ;)
Comment #5
aufumy CreditAttribution: aufumy commentedIssue mentioned in content_permissions
http://drupal.org/node/510744
Comment #6
catchHere's a patch, sets $GLOBALS['user'] = drupal_anonymous_user() in drupal_cron_run() and prevents session being saved.
Critical because this blocks #523894: Comment indexing does not include the fully rendered comment which affects indexing fields on comments, which in turn blocks comment body as field (and etc.).
Comment #7
catchSpoke to DamZ in irc, inverted the two calls.
Comment #9
catchHEAD was a couple of hours stale.
Comment #11
catchThat's a real bug - if run via the ui, system_run_cron() does a drupal_set_message() to report success/fail. So, we now restore the user after all jobs have run, and don't prevent session information being saved. The other option is to remove those drupal_set_message() calls, but having to look at the status table to see if it ran properly or not isn't ideal, so would rather keep them in.
Comment #12
Damien Tournoud CreditAttribution: Damien Tournoud commentedIt's ok to restore the user at the end of the process, but you still have to prevent the session from being saved during the time the other is changed.
Comment #13
Damien Tournoud CreditAttribution: Damien Tournoud commentedHere.
Comment #15
aufumy CreditAttribution: aufumy commentedI guess I have a different perspective. Search should have access to index all the content that it is supposed to.
So whether cron is run as anonymous or as an authenticated user with privileges should be irrelevant.
Search results on the other hand should respect the access modules in place. Usually in the form of a contrib module, such as apachesolr_nodeaccess (in http://drupal.org/project/apachesolr) that will modify the query of search results to exclude those nodes the user does not have access to.
Because search results may show restricted content does not mean that that restricted to anonymous content should not get indexed at all, and therefore not be available to those that have access, to view them in search.
For apache solr field access control, I will start http://drupal.org/project/apachesolr_fieldaccess
Comment #16
Dave ReidMoving to new cron system component.
Comment #17
Dave ReidWe should probably require #287292: Add functionality to impersonate a user to land in core first.
Comment #18
sun.core CreditAttribution: sun.core commentedToo late for D7. :(
Comment #19
sun.core CreditAttribution: sun.core commentedhrm. Actually, reading the OP once more, I wonder why this is a public security issue. Moving back.
Comment #20
agentrickard@aufumy
You concern is off-topic here. Cron normally runs as anonymous (from the command0line), so we need to ensure uniform behavior.
Here's a patch that passes, but I had to alter the verifyCron() test for dbLog to make it do so.
Comment #21
agentrickardComment #22
mr.baileys... while cron is running.
Usually, this is called right after reverting the user. Is there a specific reason for delaying this by putting it in
drupal_cron_cleanup()
?drupal_cron_cleanup()
should not return any value.I don't think we can just get rid of the "Cron ran successfully" message when running cron manually through the interface. Users expect this kind of message when performing actions through the interface. Since the message is set in
system_cron()
after callingdrupal_run_cron()
, the message should be persisted if the user is reverted anddrupal_save_session(TRUE)
called at the end of drupal_run_cron(), right?Powered by Dreditor.
Comment #23
agentrickardCould be. Let's try it.
Comment #24
Dries CreditAttribution: Dries commented1. Is it 'the cron' or just 'cron'?
2. If we forced the user to the anonymous user, do we still care about sessions? I'm not sure the phpDoc provides enough context to understand why sessions need to be disabled (for the anonymous user).
Comment #25
mr.baileys@Dries: IMO, yes, we care about the session: if we do not disable session saving during impersonation and the cron run is aborted half-way through, the user will logged out.
Thinking about this does bring up an interesting point though. What if an individual hook_cron implementation does impersonation itself. When reverting, it calls session_save_session(TRUE); and switches session saving back on...
Comment #26
Dries CreditAttribution: Dries commentedmr.baileys: that sounds like it would be a bug in the module then. I think this is RTBC. I'll fix the comment and commit it.
Comment #27
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD.