auto closer broken when anon can't create issue comments

I had to work hard to break this one, but I found a way. Two ways, actually.

1. If the site administrator never sets project_issue_auto_close_user, the original flaw still exists, because the auto_close_user is still anonymous.
2. If a user (including anonymous) has sufficient permissions when the admin sets them as the auto-close user, but then those rights are revoked, we again have the original problem and the issue gets corrupted.

I tried to rewrite a new patch for this, but wasn't quite sure how to do it. I think all it really needs is another call to user_access('access project issues', $user) as part of the safety check in the project_issue_auto_close function, probably with an appropriate watchdog error on failure.

@micah: thanks for testing this. i think your proposal to do a safety check in project_issue_auto_close() sounds reasonable. feel free to take a stab at coding it, since that seems like the right approach to addressing the concerns you raised. thanks!

oh, and while you're re-rolling this, the following is wrong: t('The anonymous user'). There's a setting for this -- that should be: variable_get('anonymous', t('Anonymous')).

otherwise, a quick visual skim of the patch seems ok. i'll study more closely once it's been rerolled to handle this extra safety check.

thanks!
-derek

I updated the patch and tested as many possible scenarios I could think of. Please review.

Upon slightly closer inspection, this looks pretty good. A few concerns:

A) This is a bad idea inside a call to t(): <em>&lt;none&gt;</em>. If we're hard-coding <none> in the rest of this patch, untranslated, we don't want to expose this to translators or they're likely to try to translate it, which would lead to much confusion and trouble. So, we want to force this little chunk to be untranslatable by putting it in a placeholder, outside of the t() string itself. Furthermore, we want to use theme('placeholder') (by means of % in the t() string) to ensure things are consistently themed on sites that choose to use something other than <em> to indicate placeholders in help text. So, this line should be:

'#description' => t('Enter the user which will auto-close fixed issues -- leave empty to use the anonymous user or set to %none to disable auto-closing.', array('%none' => '<none>')),

B) I must admit it's a little counter intuitive that the value of this setting is either empty for anonymous, <none> for disabled, and a regular name for a regular user. Why not just use empty for none, variable_get('anonymous', t('Anonymous')) for anonymous, and a regular name for regular users? Then, we don't even need to mess with this <none> business from (A). Maybe hunmonk has a good reason? Is it just that variable_get('anonymous', t('Anonymous')) isn't foolproof, and someone really could register a user called "anonymous"?

C) In your new safety code, if the call to user_load() fails, you still need to set session_save_session(TRUE) before you return or all hell could break loose for anything else happening in hook_cron().

Oh, yes. I just realized that the code to user_load for the anonymous user is probably unnecessary,

+  else {
+    $user = user_load(array('uid' => 0));
+  }

Because $user would already be set to the anonymous user, or it could even be set to a regular user who fired cron manually. So clearly those three lines should come back out. I muddled this up a bit once I realized why my test for user_access was failing when it was inside the if($name) conditional.

Although using <none> is somewhat consistent with other things in core, (such as using <front> to indicate the front page,) dww raises some good usability points in A and B. It would also make things to that auto close is disabled unless the administrator chooses to configure it. It's tempting to go so far as to say that this needs to be set to a real user to be enabled, even if uid=0 has sufficient privileges to do this.

When I first started using project*, I didn't know about the auto closer. The first time I had a "fixed" issue switch to "closed" by anonymous was very disconcerting, because I don't even let anonymous access content - let alone projects - on that site. I like the idea that this value has to be configured to be enabled, if only because it forces the site admin to be aware of it happening.

OTOH, if it's more practical to allow anonymous to do this for sites that want it, then if some user also registers "anonymous" it's unlikely that the admin would specify this user as the auto close user. In that case, something like this would probably work:

  if (! $name = variable_get('project_issue_auto_close_user', '')) {
    // Skip auto-closing.
    return;
  }
  if (! $name == variable_get('anonymous',  t('Anonymous'))) {
    //do the user_load on $name and bail out on failure
  }
  if(!user_access...

Otherwise, the second test could just exit if $name == variable_get('anonymous', t('Anonymous')).

What do you think?

There's still a small problem with blank vs anonymous user that I couldn't quite sort out.

When the Anonymous user is selected (i.e.: project_issue_auto_close_user in variables is set to s:1:"0";), then the prompting field in admin/project/project-issue-settings appears blank, and I'm pretty sure the cron job is treating this as unset, as well.

I tried tweaking the patch, but only managed to get it stuck in the other direction, with it using the Anonymous user, even when it should have been blank to not run.

Oops. Faulty test procedures on my side.

Yes, everything now works as expected.

Visual inspection looks good. micah and hunmonk say it works. RTBC. ;)

I'll deal with deployment.

oh, whoops. this was deployed a while ago, i just forgot about closing this issue...