There are two issues: #2030017: Allow cron to run and #1898216: Choose shield method = protect all, exclude or include.
These could create a logical bug if you select "Shield no paths except the following (include)" and if you don't check the checkbox to enable cron running, the shield popup should shown in cron.php but it won't. This cause because cron.php does not included in the list, and list checks only paths which hits index.php.
I think the best solution is to remove enable cron checkbox and extend excluded/included paths to handle all bootstrapped paths (so it will handle cron.php update.php etc).
Comment | File | Size | Author |
---|---|---|---|
#1 | 2063945-1.patch | 3.64 KB | kalman.hosszu |
Comments
Comment #1
kalman.hosszu CreditAttribution: kalman.hosszu commentedA patch is attached. If somebody wants to review feel free to do it.
Comment #2
azinck CreditAttribution: azinck commentedI disagree slightly with your explanation. You state:
In this scenario the module is behaving as I'd expect: cron.php should not be shielded unless cron.php has been explicitly added to the list of paths to be shielded. The option is "Shield no paths except the following (include)". Indeed, if you've chosen that option, then the checkbox to enable cron running is fairly pointless, since cron would not actually be stopped from running unless you explicitly blocked it by adding it to the list of Shielded paths.
Regardless, there is a bug here, and I'd describe it like so:
Included/excluded path comparisons are currently only valid for regular Drupal paths and don't work for other paths that do still bootstrap Drupal. Most notably these include update.php, cron.php, xmlrpc.php, authorize.php, and install.php. And it's possible that some contributed modules might provide similar scripts. It should be possible to explicitly Shield or permit access to such paths.
I suggest doing 2 things:
Most users would probably be happy to choose "allow access" or "prevent access" and then wouldn't have to worry about configuring the paths explicitly.
What do you think about that? I'll see if I can roll a patch later today.
Comment #3
kalman.hosszu CreditAttribution: kalman.hosszu commented', array('@basepath' => base_path())),
- '#description' => t('You will not be able to run cron from the command line or crontab without this option selected.'),
- '#default_value' => variable_get('shield_allow_cron', 1),
- );
-
Check the description. This tells the cron won't work without this checkbox, but I can create a condition to work without this checkbox. I think this was the main problem. In the typical usage the include/export option should use index.php paths but because of this I have to extend the path check algorithm with all bootstrapped files.
For your second suggestion: the ides is good and I love the great user interfaces. Otherwise Shield is a typical development module for dev and staging sites so I think we shouldn't make non-technical users life easier with this module :) ... less is more, the technical users will be able to configure correctly.
Comment #4
azinck CreditAttribution: azinck commentedThe work done in #2030017: Allow cron to run didn't anticipate the addition of the "include paths" option, so it's description text isn't too accurate now. In my mind, when the "include paths" option is selected then only the identified paths should be affected, regardless of whether or not the "allow cron" checkbox is checked. You're suggesting that the "allow cron" checkbox should be construed to mean "always block cron when un-checked, and always allow cron when checked", which is not how I think it should behave.
Anyway, you want to get rid of the checkbox altogether which removes much of this confusion.
I think I still lean towards wanting the radio buttons as I described in #2 (despite the fact that it adds an extra form element) but I understand the choice, either way.
Comment #5
kalman.hosszu CreditAttribution: kalman.hosszu commentedOk.
Could you test the patch in #1? I will ask other developers about the radio buttons and if others also think this is necessary I will agree your opinion and append the patch with this.
Comment #6
kalman.hosszu CreditAttribution: kalman.hosszu commentedI commited this patch