introduce a "cron" user to fix various cron related issues

Sounds like a good idea to me. It's one of many things I'd like to change if I get time to work on it.

Couln't we just write this in cron.php:

 $cronuser = variable_get('cron_cronuser','nobody');
if ($cronuser = 'nobody'){
  $user = user_load(array()); //I believe this results in Anonymous??
}else{
  $user = user_load(array('name' => $cronuser));
}

We can set the variable cron_cronuser in a page like admin/cron. If the site administrator doesn't set a value, cron uses Anonymous for everything (not some user that happens to be logged on). If a valid user name is set, the site admin can give the cron user special privileges.

Setting the user like this in cron.php sends a cookie. (How? Where? Can we prevent this?) So everybody can get "cron" privileges by loading cron.php once. We could fix this by making cron.php accessible only from localhost (in .htaccess).

Any ideas?

This patch lets the admin select a UID for cron in the admin/settings page. The variable name in which this is stored is 'cron_uid'. When cron.php is loaded, it sets a variable $cron_is_running. If this variable is set, no session will be opened (and no cookies will be sent). The cron page will use the Anonymous user (uid==0) if cron_uid is not set.
Note that the site admin should create a "cron" user before setting the UID.

Previous patch still applies cleanly to HEAD and code algorith looks good on inspection. However, there are some style problems and one ugly bit using StdClass(), so I have re-rolled the patch.

I like the idea, some comments, though:

+if (!$cron_is_running) {
+ session_start();
+}

Is this neccessary?

'User ID '.$cron_uid.' does not exist' <== not translatable.

'Cron user identifier (uid)' <== not very clear.

'User ID '.$cron_uid.' does not exist' < == needs to be made translatable.

Setting to active.

Ooops. Sorry I missed that text that needed translating. New patch attached with that bit corrected.

Yes, I think the if (!$cron_is_running) code is needed to keep the cron user from creating many unneeded sessions.

I am all for the idea of introducing a special id for running the cronjobs, but the approach you took here is far from perfect imo..
After looking at your patch, there are several questions and remarks that came up to my mind:

I agree with Gerhard that the $cron_is_running is probably unneeded, I think it's better todo:

if (!variable_get('cron_busy', true)) {
  session_start();
}

I think that this is much to technical. Severa newbies will probable ask what a user id is, or what cron jobs are.. From both usability and userfriendlyness this patch is a big -1 from me..

I would suggest todo the following:
- Make a new user in the db called 'Cron' with a user id -1, or something like that.. That way you don't have to add extra options to the administration, and the way of implementing should be much cleaner. Also does it improves usability...

What do you think?

"Make a new user in the db called 'Cron' with a user id -1"

Just on a technical note, the database type for uids is "unsigned" and -1 won't work.

@Chris: I am pretty sue that this patch will not be usefull if the cron user does not get a session. Drupal relies on the session to authenticate the user. If you are worried about too many sessions, you should make sure the session is clossed at the end of the run. his could be done in cron.php.

@Stefan, Ber: I think we should introduce just a standard user and assign the "authneticated user" role to him. The set the variable to that uid so we can check which user it is. The problem is that we cannot create the user from database.mysql beacuse the next uid would be No. 1 which is reserved. So we would need to create the user on the first visit to the admin/settings page. We could also use drupal_set_message to inform the admin of this. We should also extent the relevant help text.

While we are at it we could make the anon user use the db tabe for its name too and not a variable.

Looks like we are sort of in a corner. Really the cron user should be in the database at install time, but we have to reserve uid 1 for the first user. Maybe there is another way to do that without having the first entry to admin create the cron user? That seems hackish to me.

I agree that the anonymous user should also be completely coded into the database, and not a variable. The current methods causes anomolous behaviors. I, in fact, wrote several patches to do that a long time ago (http://drupal.org/node/5639). I looked at updating that patch a couple of months ago, but the new unregistered user comment capability where the user can enter his name made the code a lot more complicated. Maybe I will get motivated to try it again. If only my patch had been applied the first time! :-)

How about creating a bunch of "reserved" users (like most linux distributions do)?
uid 1 = root
uid 2 = cron
uid 3 - 8 = reserved for later use
uid 9 = anonymous

And then let the admin log in as root the first time. We still let him/her change the username of any of these users, so it should not cause to many problems for new installations. Older ones would either just have to live with it, or do a lot of 'UPDATE module_table SET uid=X WHERE uid=2'-queries.

What about following the linux thing, and start counting at 500 for normal users? That way people will recognise it, and numbers do not matter anyway. I mean, there is really no difference between 4 and 500, other than thats its a different number. 500 makes just as much sense as 10, or 2 or 4.

Bèr

The reason why I created the "ugly" user interface, and let the site admin choose a cron-user ID in the settings page is because of backward compatibility with existing installations. (guess what: my site IS an existing installation)

Perhaps another option is to give special users a user ID close to 2147483647 (MAXINT). That way it won't interfere with existing users.

At the risk of being publicly humiliated, and in the spirit of "there's no such thing as a stupid question" ... what exactly is the (security?) issue with adding:

$user = user_load(array('uid' => 1));

in cron.php? I have two sites with node_privacy_by_role (NPBR) enabled, and cron does not index all the pages as a consequence (except when it is given a user id with full access like above). It took me some puzzling to figure out why the sites seemed stuck at "11% of the site has been indexed" or something similar.

Of course, now a non-authenticated user can see excerpts of "private" pages through the search, which could be a big deal.

Anyway, being able to choose a search user would solve the problem with NPBR as well it seems.

Why do we need a user besides uid=0 for cron? Just because the project module writes anonymous on certain closed issues? If cron runs with uid=0 most of the time anyway, why don't we just hardcode that in instead of creating a new type of user?

@robertDouglass: Why do we need a user besides uid=0 for cron?

There are genuine use cases for cron running with greater permissions than those provided by uid=0, and having a specific cron user allows the administrator to determine exactly which permissions the cron user gets.

For instance I just spent half a day figuring out why my simplenews installation was mailing multiple empty messages to subscribers plus 1 working message. My simplenews mail messages are only accessible by users with the 'authenticated user' role via taxonomy_access, my simplenews install sends for 20 seconds on message submit and then continues sending later via cron. When sending via cron the message body couldn't be accessed by the anonymous user and so an empty message got sent. It was only when a logged in user initiated a send run that a proper message was sent. Without the ability to assign permissions to a cron user, I have to allow access to my newsletters to anonymous users.

So, that's my +1 for this functionality.

@killes: . So we would need to create the user on the first visit to the admin/settings page.

I agree that this variable ('cron_uid') should be set automatically without requiring any admin intervention. Can't we create this user in update.php? The variable defaults to 0 which is the current behaviour in any case.

Finally, I think that the logic to not create a session for the cron user should be removed from this patch. Instead we should destroy the session at the end of the cron run.

Cheers,
Eric

Eric, that makes sense. Thanks for the explanation.

Based on comments.

Robin
I ♥ Bugz

If you are using the Webserver_auth module, the problem with adding $user = user_load(array('uid' => 1)); is if you run cron.php manually via your webbrowser, you become that user. I verified this on new computer with a brand new firefox profile to verify the problem wasn't with a cached info.

How about this approach: We use the anonymous user, but just add more roles to its user object while cron is running. This patch adds a select element on the cron configuration page that lets you select multiple roles. These roles get added to the anonymous user while cron is running (any other user that runs cron is unchanged). Then the session is destroyed (for anonymous users) when cron is finished.

While perhaps not the ideal solution of having each user with its own uid, this is a big step forward to solving a bunch of cron-related problems. Code looks good. +1

How is a user expected to be able to decide which roles should be checked for cron? Any permissions issues seem like something that is the responsibility for modules to fix themselves since the modules should know what they need. Core might want to provide convenience functions for getting user permissions during cron.

Drumm, so in other words, you feel that a module should load and switch users (or add roles) during the cron hook itself? And that perhaps core should offer a mechanism for adding a role to the current user for the length of the request?

Whatever works, if adding a role is what code needs to do, then it should do that.

An example of a recent change that went well was search index updating wasn't working since node_load() does some checking. Those checks in node_load() were removed since they were decided to be redundant.

The code should know what permission it needs or what API function is too strict in checking, so the code should handle it without bothering the user to make a choice.

Neil, can you point me to the patch you mention in #28? That's exactly the issue I'm dealing with now.

Regarding the patch in #24, I find that sess_destroy() is not actually getting rid of the row in the sessions table, or its somehow being written again. I worry with that approach that a user could gain extra permission by running cron.php from a browser, even if it is for the brief duration of the cron call.

found it myself: http://drupal.org/node/64857

Thanks.

If you rely on cron to destroy the cron user's session, if cron fails to complete (not uncommon), the session is not destroyed, opening up the possibility that if someone manually runs cron with their browser, they would get the cron user's privileges.

So you would have to do something hackish like make sure the cronuser's status is permanently set to "blocked" (which does not interfere with the cron user's permissions for some reason, and still allows it to execute the cron hook...)

It's by design that there's no secure way to run cron with sufficient permissions on a site that uses a node access control module?

See http://drupal.org/node/64857