If allow_authorize_operations is FALSE, print a better error about it on Update Manager routes

Here's a Drupal 8 version of the patch.

That makes a lot of sense.

Once committed to D8 would it be a good idea to backport to D7 with skwashd's patch? To me this sounds like a good fit for a D7 security update.

I have reviewed Tim's patch and I RTBC'd it. I am not sure where the confusion is from.

This seems to remove one of the most useful features we added in D7, and one whose very target audience are people who are not technical enough to edit settings.php by hand to restore its functionality.

Is there some kind of specific vulnerability that can be done with update.module? I'd rather harden it another way that doesn't destroy usability.

Fixing title.

Webchick,

I think the risk here is unsafe modules (such as PHP Filter when it moved to contrib) being installed by people who have managed to get access to the admin interface. Thankfully many of the big name hosts have a read-only codebase and force code to be committed via a version control system, but many don't.

The flip-side is people not bothering to update their modules to secure versions because it's an incredible pain in the ass. It was reps from the security team who added this feature in the first place for that reason.

Also, my normal way of using this is to run update status on localhost, then check the updated code into Git, then deploy that to prod. Don't assume that this really useful feature precludes the use of version control and other best practices.

oooh looks like have tests that depend on this being enabled.

/me goes to work out how to do a variable_set() (or whatever the new version is) in a test.

Just spent half a day figuring out why I'm getting "access denied" at /admin/modules/update as user/1, only to face that I have

$settings['allow_authorize_operations'] = FALSE;

in settings.php

So... I suppose forcing this to FALSE by default will give "access denied" at /admin/modules/update even for user/1, w/o any other checks.
And the only way to gain access back will explicitly be

$settings['allow_authorize_operations'] = TRUE;

in settings.php.

Or to make some other way of access check in ./core/modules/update/src/Access/UpdateManagerAccessCheck.php in access() method.

Confirming #23... same here! -.-

Finding out why access is denied to the update.module pages, while 'administer software updates' was given, drove me bonkers and is a situation which shouldn't be kept this way!
As a developer I had a chance to find this, but non-developer users will cry...

Current behavior (TL;DR):

If "allow_authorize_operations" is set FALSE in settings.php

$settings['allow_authorize_operations'] = FALSE;

• all routes using _access_update_manager: 'TRUE' (see below) are affected and return ACCESS DENIED
• no reason for the access denied is given
• 'administer software updates' permission doesn't affect anything
• menu items linking these ACCESS DENIED pages are still visible

So you're left alone in the dark with the ACCES DENIED message!

Expected behavior

At least show a message for users with 'administer software updates' on the affected routes which tells them that this setting is set to TRUE and results in ACCESS DENIED and link it to a documentation page.

Also add documentation to the settings.php comment which tells the administrator that these pages can't be accessed anymore if the setting is TRUE.

Relation and relevance for this issue

When changing the value to TRUE by default, as requested here, the situation will be worse as all new Drupal installations are affected!
So I'd see a fix for that as a precondition to change the value to TRUE by default.

Affected routes

These are the pages from update.routing.yml affected by UpdateManagerAccessCheck and 'allow_authorize_operations' setting:

update.report_install:
  path: '/admin/reports/updates/install'
  defaults:
    _form: '\Drupal\update\Form\UpdateManagerInstall'
    _title: 'Add new module or theme'
  requirements:
    _permission: 'administer software updates'
    _access_update_manager: 'TRUE'

update.report_update:
  path: '/admin/reports/updates/update'
  defaults:
    _form: '\Drupal\update\Form\UpdateManagerUpdate'
    _title: 'Update'
  requirements:
    _permission: 'administer software updates'
    _access_update_manager: 'TRUE'

update.module_install:
  path: '/admin/modules/install'
  defaults:
    _form: '\Drupal\update\Form\UpdateManagerInstall'
    _title: 'Add new module'
  requirements:
    _permission: 'administer software updates'
    _access_update_manager: 'TRUE'

update.module_update:
  path: '/admin/modules/update'
  defaults:
    _form: '\Drupal\update\Form\UpdateManagerUpdate'
    _title: 'Update'
  requirements:
    _permission: 'administer software updates'
    _access_update_manager: 'TRUE'

update.theme_install:
  path: '/admin/theme/install'
  defaults:
    _form: '\Drupal\update\Form\UpdateManagerInstall'
    _title: 'Add new theme'
  requirements:
    _permission: 'administer software updates'
    _access_update_manager: 'TRUE'

update.theme_update:
  path: '/admin/theme/update'
  defaults:
    _form: '\Drupal\update\Form\UpdateManagerUpdate'
    _title: 'Update'
  requirements:
    _permission: 'administer software updates'
    _access_update_manager: 'TRUE'

update.confirmation_page:
  path: '/admin/update/ready'
  defaults:
    _form: '\Drupal\update\Form\UpdateReady'
    _title: 'Ready to update'
  requirements:
    _permission: 'administer software updates'
    _access_update_manager: 'TRUE'

Should we split these issues or keep them together?

I just ran into this randomly myself. It definitely would benefit from a better warning message, at minimum.

Until the Auto Update Initiative is done, there's no way we're going to change the default for this killswitch. The target audience of the Update Manager are non-technical users who are the least likely to go hacking around in settings.php to figure this out and opt-in. The whole thing was built to help those people keep their sites more up to date. Having it default to FALSE would completely remove what value it provides by turning it off for exactly the group it was designed to serve.

However, I agree that when the setting is FALSE, the error messages could be more helpful than a generic 403. Giving this a better title/scope to reflect what could happen here.

Thanks,
-Derek