Problem/Motivation

\Drupal\taxonomy\TermAccessControlHandler::checkAccess doesn't handle the 'view all revisions'

The accesscheck for Terms doesn't use the new 'view all revisions' permission yet. This should be implemented. (see #3043321: Use generic access API for node and media revision UI)

We notices this in: #3031271: Support version negotiation for any entity type (currently only Node & Media are supported)

I think this work might be done here: #2350939: Implement a generic revision UI, but i think a split might be good, since getting UI into core is a lot harder than this fix.

Steps to reproduce

Proposed resolution

Implement 'view all revisisons', operation check in TermAccessControlHandler by adding a new 'view term revisions' permission

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#11 interdiff_09_11.txt4.06 KBsandeepsingh199
#9 view-all-revisions-permission-3269029-9.patch9.11 KBsamitk
#11 3269029-11.patch5.42 KBsandeepsingh199
#13 view-all-revisions-permission-3269029-13.patch9.08 KBsamitk

Issue fork drupal-3269029

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

bbrala created an issue. See original summary.

bbrala’s picture

bbrala
Location Netherlands
commented
Issue summary: View changes
Issue tags: -\Drupal\taxonomy\TermAccessControlHandler::checkAccess doesn't handle the 'view all revisions'
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Issue tags: +Needs subsystem maintainer review

I don't see any taxonomy changes in #2350939: Implement a generic revision UI

Looking at TermAccessControlHandler, its not clear what we'd do for this operation.

I suspect rather than defer to either 'administer taxonomy' we'd probably need to add new permissions. Tagging for 'needs subsystem maintainer review' for their input on that.

The basis that we need a discussion on how to handle this is one reason not to hold up #3031271: Support version negotiation for any entity type (currently only Node & Media are supported). But it begs the question, is #3031271: Support version negotiation for any entity type (currently only Node & Media are supported) useful at all for terms if you need to grant the 'administer taxonomy' permission? I don't think it is.

catch’s picture

catch
he/him
Primary language English
commented

Adding a new permission to make taxonomy more in line with other modules makes sense to me.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Issue summary: View changes
Issue tags: -Needs subsystem maintainer review +Novice

Thanks, tagging for Novice

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

samitk’s picture

samitk commented
Assigned: Unassigned » samitk
samitk’s picture

samitk commented
Status: Active » Needs review
StatusFileSize
new view-all-revisions-permission-3269029-9.patch9.11 KB

Patch for:

  1. Adding permission for view all taxonomy revisions.
  2. Adding Tests for revisionability of taxonomy_term entities.

Status: Needs review » Needs work

The last submitted patch, 9: view-all-revisions-permission-3269029-9.patch, failed testing. View results

sandeepsingh199’s picture

sandeepsingh199 commented
Status: Needs work » Needs review
StatusFileSize
new 3269029-11.patch5.42 KB
new interdiff_09_11.txt4.06 KB

Fixed #9 issue. Kindly check & review.

bbrala’s picture

bbrala
Location Netherlands
commented
Status: Needs review » Needs work

Hi, thanks for the change?

I did see you remove the tests, shouldnt those stay and be fixed?

samitk’s picture

samitk commented
Status: Needs work » Needs review
StatusFileSize
new view-all-revisions-permission-3269029-13.patch9.08 KB

Fixing issue of #9

samitk’s picture

samitk commented
Assigned: samitk » Unassigned
samitk’s picture

samitk commented

In #13 I fixed following.

  1. Fixed #9 Build issues.
  2. Added Automate Test cases as suggested by #12

Thanks @sandeepSingh199 for your input

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative, +Needs change record

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Think has this is adding a new ability it will need a change record.

Ran the tests locally without the fix and get a valid failure

Behat\Mink\Exception\ExpectationException : Current response status code is 404, but 200 expected.
 /var/www/html/web/vendor/behat/mink/src/WebAssert.php:794
 /var/www/html/web/vendor/behat/mink/src/WebAssert.php:130
 /var/www/html/web/core/modules/taxonomy/tests/src/Functional/TaxonomyTermRevisionTest.php:46
 /var/www/html/web/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

fromme made their first commit to this issue’s fork.

silverham made their first commit to this issue’s fork.

silverham opened merge request !10360

silverham’s picture

silverham commented

Add related issue where even if revision can be fetched via JSON:API, the first one is cached. #3149854: Add revision ID to "jsonapi_normalizations" cache keys

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.