Problem/Motivation

The edit link in the breadcrumb region of the secondary toolbar shows even when the user does not have permission to edit the current page.

Steps to reproduce

  • Install gin and gin_toolbar
  • Create a user with permission to access the admin toolbar but without the permission to edit nodes
  • Create a node and view it with the aforementioned user

Proposed resolution

Don't show the link when the user doesn't have the permission to edit the current page.

Remaining tasks

Perform proper access check in gin_toolbar_preprocess_toolbar before adding the link.

User interface changes

API changes

Data model changes

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:

Comments

seutje created an issue. See original summary.

seutje’s picture

Looks like the fallback link "Back to Administration" also doesn't do a proper access check.
Here's a partial patch.

seutje’s picture

Added patch also check if user has permission to the admin_content route.

seutje’s picture

Forgot use Drupal\Core\Url;.

seutje’s picture

Status: Active » Needs review

Forgot to update status.

adstokoe’s picture

#4 Works for me! Thanks.

adstokoe’s picture

Status: Needs review » Reviewed & tested by the community
jurgenhaas’s picture

Version: 8.x-1.0-beta22 » 8.x-1.x-dev

Unfortunately, the approach with PathValidator::isValid doesn't really achieve what's needed. The permission check only looks for the permission link to any page, but that doesn't necessarily mean that the current user is allowed to edit the current entity. The rest of the patch looks ok, but the permission check needs a different approach.

jurgenhaas’s picture

Status: Reviewed & tested by the community » Needs review

I have started an MR which uses the simple entity access controller to see if the user has edit permissions.

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

elgandoz’s picture

Issue summary: View changes

#11: the merge was blocked by a conflict and I (unsuccessfully) tried to rebase from upstream, sorry about the mess.

#14: this is a new branch with the cherry picked commit by @jurgenhaas, with the merge conflict resolved.
I had to change the access check since that I tested the patch with different roles and with one of them (implementer) I wasn't able to see the breadcrumb despite having the "[CT]: Edit its own content" permission.

See the differences between a moderator and a implementer here:

The issue was basically the same as #3204423: Entity edit-form routes check "update" access but not "view" access, caused by the confusion of the names between "edit" and "update" permissions for the entity access check.
Before: $entity->access('edit')
After: $entity->access('update')

Now it works nicely as expected, please review.

jurgenhaas’s picture

Thanks @elgandoz for updating the MR. Unfortunately, it only applies to 1.0.x-dev but not to 1.0.0-RC3 because of this commit: https://git.drupalcode.org/project/gin_toolbar/-/commit/4e66cd7c20b249be...

But there isn't much we can do about that.

elgandoz’s picture

Hey @jurgenhaas, I removed the commits past your one and amended it, in order to have the 'update' access check, in the original 3319445-edit-link-in branch you created, and force-pushed.
So now the diff from MR!25 should apply on 1.0.0-RC3, while MR!30 goes on 1.0.x-dev in order to get merged.
Please check since I didn't test it.

jurgenhaas’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @elgandoz that's a nice solution as well. So MR !25 is for the latest RC3 and MR !30 is for the dev release. This is working great.

Liam Morland made their first commit to this issue’s fork.

liam morland’s picture

Rebased

jurgenhaas’s picture

MR!30 didn't apply any longer and I've done a rebase. However, one part of the fix needs to be handled in Gin now, since the part of the template where this is required has moved over there.

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

hanoii changed the visibility of the branch 3319445-edit-link-in to hidden.

hanoii changed the visibility of the branch 3319445-edit-link-rebased to hidden.

hanoii’s picture

The rebased one was missing some twig changes, I happened to worked on fixing this before having found this issue :facepalm:

I a very similar thing and went ahead and push it, it makes it cleaner in the the twig and uses the proper drupal services.

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

dpi’s picture

Overall proposal looks fine, however I've modified the MR to be a little more correct.

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

jurgenhaas’s picture

Assigned: Unassigned » jurgenhaas
jurgenhaas’s picture

Issue tags: +Barcelona2024
anil.sharma’s picture

Version: 8.x-1.x-dev » 2.0.0
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new1.98 KB

Updated patch for version 2.0.0

anil.sharma’s picture

Version: 2.0.0 » 8.x-1.x-dev
StatusFileSize
new1.98 KB

Added entity access check to version 8x.

er.garg.karan’s picture

Version: 8.x-1.x-dev » 2.0.0

er.garg.karan changed the visibility of the branch 3319445-edit-link-rebased to active.

liam morland’s picture

Version: 2.0.0 » 2.0.x-dev

The merge requests should be updated to target branch 2.0.x.

  • jurgenhaas committed 5aac9cb2 on 8.x-1.x authored by hanoii
    Issue #3319445 by sourabhsisodia_, tirupati_singh, mvonfrie, : Edit link...

jurgenhaas’s picture

Status: Needs review » Fixed

  • jurgenhaas committed 6467fd60 on 2.0.x
    Issue #3319445 by sourabhsisodia_, tirupati_singh, mvonfrie, : Edit link...

  • jurgenhaas committed 97c2f543 on 3.0.x
    Issue #3319445 by sourabhsisodia_, tirupati_singh, mvonfrie, : Edit link...
dpi’s picture

Thanks..

damienmckenna’s picture

FYI while this probably would have fallen under PSA-2023-07-12, this should have been reported privately as a security issue and then discussed with the Drupal Security Team. Please try to keep this in mind if similar issues show up again. Thank you.

Status: Fixed » Closed (fixed)

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