Refactor exclude paths [#2063945]

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
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

kalman.hosszu CreditAttribution: kalman.hosszu commented 13 August 2013 at 08:40

Status:

Active

» Needs review

File	Size
2063945-1.patch	3.64 KB

A patch is attached. If somebody wants to review feel free to do it.

Comment #2

azinck CreditAttribution: azinck commented 13 August 2013 at 15:19

I disagree slightly with your explanation. You state:

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.

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:

Allow Shield paths to match paths that bootstrap Drupal via scripts other than index.php. This is accomplished via the patch in #1 above which looks good to me at a glance but I haven't had the chance to actually test it.
I think there's still a need for an option to make life easier for less-technical users. Perhaps a set of radio buttons like so:
How should Shield handle special bootstrapped PHP scripts (i.e. update.php, cron.php, xmlrpc.php, authorize.php, and install.php)?:
- Allow access (don't Shield)
- Prevent access (Shield all)
- Access determined by the path configuration above
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 13 August 2013 at 15:43

@@ -33,13 +33,6 @@ function shield_admin_settings($form, &$form_state) {
-  $form['general']['shield_allow_cron'] = array(
-    '#type' => 'checkbox',
-    '#title' => t('Allow browser and crontab to run <code>@basepathcron.php

', 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 commented 13 August 2013 at 16:04

The 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 commented 13 August 2013 at 16:56

Ok.

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.