Unexpected requirements of /admin/structure/taxonomy/manage/{taxonomy_vocabulary}/reset route with Drupal 10.1+

Created on 16 February 2023, over 1 year ago
Updated 16 October 2023, about 1 year ago

Problem/Motivation

With Drupal 9.5.3, this module fails the verification of the requirements for the taxonomy reset route. I'm not sure if this is the case with any earlier versions of Drupal 9.

The module expects these requirements:

'_permission' => 'administer taxonomy'

However, upon reviewing the core taxonomy module, the route has this requirement:

'_entity_access' => 'taxonomy_vocabulary.reset all weights'

I'm not quite sure why there's a need to verify the requirements unless it's trying to avoid conflicts with other modules that may alter the requirements of this same route.

Steps to reproduce

Install the current beta version of the module in your Drupal 9.5.3 site. After it's installed, try to rebuild the cache and run the database updates and it will bail out with the error.

Proposed resolution

Change the expected requirements to match the current version of the core taxonomy module.

Don't throw an exception and bail out if the verification fails. Instead, simply do not set the new requirements and instead log and/or display a notice about it.

Remaining tasks

Release notes snippet

The operation name the vocabulary access control handler uses to check for permissions to reset a vocabulary has been renamed from reset to reset all weights to be in line with changes to be released in Drupal Core 10.1.0. This will only affect users, who use the vocabulary access control handler programmatically, e.g. in a custom module.

🐛 Bug report
Status

Fixed

Version

4.0

Component

Code

Created by

🇨🇦Canada teknocat

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @teknocat
  • 🇨🇦Canada teknocat

    The permissions provided by this module should also account for the fact that the core taxonomy module now provides a permission to reset any. As such, maybe this should actually be removed altogether from this module?

  • 🇩🇪Germany FeyP

    Thank you very much for filing this issue, it's really appreciated.

    Are you sure that you get this error on an unpatched core 9.5.3? The core issue for that change was recently committed to 10.1.x, but I can't find the requirement change in 9.5.x and 10.0.x and I can't reproduce the error on various real world installations of 9.5.x and 10.0.x. Are you maybe using a patch from that issue in your 9.5.3 (see the related issue I just added)? Or maybe you are using another contributed or custom module that modifies this route? Any candidates?

    Of course we still need to fix this since it will be released in core 10.1.x and I will look into preparing a patch, if no one else beats me to it, that you could then probably also use on 9.5.x, if you're currently using a patch from that core issue, but given that 10.1.x has not been released yet, I don't consider it that urgent.

    W/r/t to the part of the proposed resolution where your suggestion is to not throw an exception when the verification fails: If the requirements don't match our expectations, we can't guarantee that our access control handler returns correct access results. If we just ignore this, it may result in an access bypass. That's why we throw the exception so that it is immediately clear that the module might not work as expected. So this is a safe guard against potential security vulnerabilities introduced through unexpected changes in core or use of other incompatible contrib or custom code. This has actually happened to this module in the past (before I joined as a co-maintainer), so it's not theoretical. Just logging a notice will not work to prevent a security advisory. That's why I don't want to remove this exception. In my opinion, the correct way to deal with it is just to file an issue for any incompatibility encountered and if it's in core or contrib, we'll fix the code to work with the new requirements. Having a user file an issue, like you did, was exactly what I hoped for in such a case, so in my opinion the exception worked exactly as intended.

  • 🇨🇦Canada teknocat

    Ah yes, indeed I was using the patch from that related core issue.

    If I remove that patch, then I can use the latest version of this module without issue and that provides the reset permissions and it all works.

    So options for anyone until the release of 10.1.x and the appropriate update to this module to go along with that are either:

    a) Use the core patch and use a workaround to continue using version 3.3 of this module, or
    b) Don't use the core patch and use the latest version of this module

  • 🇯🇴Jordan Rajab Natshah Jordan

    Facing this issue with Drupal 10.1.0-alpha1

  • Status changed to Needs review over 1 year ago
  • Open in Jenkins → Open on Drupal.org →
    Core: 10.0.7 + Environment: PHP 8.1 & MySQL 8
    last update over 1 year ago
    11 pass
  • 🇩🇪Germany FeyP

    Attached is a patch against 4.x-dev.

  • Open in Jenkins → Open on Drupal.org →
    Core: 10.1.x + Environment: PHP 8.1 & MySQL 8
    last update over 1 year ago
    11 pass
  • Open in Jenkins → Open on Drupal.org →
    Core: 9.5.x + Environment: PHP 7.4 & MySQL 8
    last update over 1 year ago
    11 pass
  • 🇯🇴Jordan Rajab Natshah Jordan

    Thank you, Patrick

    The quick fix patch from #6 is working with 4.0.0-beta1 and Drupal 10.1.0-alpha1

    • FeyP committed 9919d44a on 4.x
      Issue #3342219 by FeyP, teknocat, Rajab Natshah: Unexpected requirements...
  • Status changed to Fixed over 1 year ago
  • 🇩🇪Germany FeyP

    Committed. Thanks all!

  • Automatically closed - issue fixed for 2 weeks with no activity.

  • Status changed to Fixed about 1 year ago
  • 🇮🇳India rajesh.vishwakarma

    Facing issue with Drupal 10.1.4
    I recently updated my core 9.5.8 to -> 10.1.4
    During the update, I received this as a fatal error and stopped the update. So I disabled taxonomy_access_fix module and finished the update. After the successful update, I enabled taxonomy_access_fix module but got the same issue. It is also showing with drush cr

  • 🇩🇪Germany Lutfiyeturan Stuttgart

    I also still have this issue with Drupal 10.1.5
    Appears when i do drush cr

    Unexpected requirements of /admin/structure/taxonomy/manage/{taxonomy_vocabulary}/reset route. This might be due to an unexpected change in Drupal Core or due to a conflict with another contributed or custom module in use.

  • 🇩🇪Germany axroth Stuttgart

    also still having this issue on 9.5.11 when updating the module to ^4.

  • 🇩🇪Germany FeyP

    Thanks for letting us know. Looking at the code in core I don't see any recent changes that we didn't address and I haven't had any issues myself on a bunch of systems.

    Most likely this is either a core patch that you're using that modifies the route, another contributed module that alters this route or some custom code you use. Could you please check your systems for any candidates? If it's a contrib module, we might be able to accommodate with a patch.

  • 🇩🇪Germany axroth Stuttgart

    I could fix this only by deleting the cache tables in db. drush cr, did not solve it.

    > another contributed module that alters this route

    that route is not mentioned at all in custom or contrib code, that's why I researched and found this ticket where others report the same issue. I don't see an obvious issue in the modules code as well.

  • 🇩🇪Germany FeyP

    I could fix this only by deleting the cache tables in db. drush cr, did not solve it.

    That's interesting. So it's not related to the code at all, but some kind of caching issue? I'll see if I can find something in that direction. Might be a few days until I can take a look though.

Production build 0.71.5 2024