Visiting the comment approval link for a published comment causes 403

I'd expect a redirect back to where I came from + proper message "The comment is already published.".

Patch that changes the message attached.

Patch works fine on D8 too.

Minor re-roll for core/ directory change.

This works as expected.

Un-related error:

Failed: Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[HY000]: General error: 1114 The table 'simpletest841577field_revision_taxonomy_vocabulary_3_2' is full: ALTER TABLE {field_revision_taxonomy_vocabulary_3_2} ADD PRIMARY KEY (`entity_type`, `entity_id`, `revision_id`, `deleted`, `delta`, `langcode`); Array ( ) in Drupal\Core\Database\Connection->query() (line 532 of /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Connection.php).

The patch no longer applies. Because drupal 8.x folder structure has been changed now.

Re-roll for core/ directory change.

This works as expected.

Yep, last two patches are exactly equal ;-)

This is trivial, so hopefully it's OK to move it to RTBC.

BTW, this is a bug.

http://drupal.org/node/314328#assigned

Nice fix! Let's add an automated test to make sure we don't break it again. :)

Also, this is a nitpick, but

+++ b/core/modules/comment/comment.pages.inc
@@ -120,10 +120,14 @@ function comment_approve($cid) {
+      drupal_set_message(t('The comment is already published.'));
...
+      drupal_set_message(t('Comment approved.'));

Let's also shorten up that message so it's more inline with the approved one. Something like "Comment already published."

Does this need backport to D7 as well?

\Drupal\comment\Controller\CommentController::commentApprove() now

I reviewed this issue and wonder if there is anything to do?
Having approve functionality in \Drupal\comment\Controller\CommentController::commentApprove() (#14) marks published comments as "approved".
May be there are any test cases, when the issue can be reproduced?

minor bug with tests + UX could came in for 8.0

Patch rerolled!

patch is totally wrong, keep tags!

proper re-roll, still needs tests

Revisiting this issue, the problems mentioned seems to be resolved.
First, _csrf_token prevents access directly to the link.
Second, as reiterated on comment #16, there is already a check for the published status in place after #2322949: Implement generic entity link view field handlers, i.e. see CommentAccessControlHandler::checkAccess(); which should prevent the bad message on a race condition, e.g. two users approving the content at the same time.

About tests, the csrf part already has tests on CommentAdminTest::testApprovalNodeInterface(), and the other part I think is not possible to be tested unless we can have in parallel to users in different screens.

The only possible improvement I see now may be to be able to return 400 (Bad Request) http status on the race condition case instead of the current 403, but I guess that could be another issue instead.

Pending: someone to confirm the position before closing this, and maybe open a new issue for the 403 to 400 change.

The reason behind the issue is UX, having 403/400 here is wrong, because user has permission to approve.
So in this case controller should properly report status of approval operation.

    if ($operation == 'approve') {
      return AccessResult::allowedIf($comment_admin && !$entity->isPublished())

Updated IS for current state.

I have implemented the following changes:
- Re-roll the patch.
- Add a test case to validate the message.

For the test cases, I am bypassing CSRF token and other validation by adding a new route in comment_test module.
My main intention is to test the proper messages only.

I agree that 400/403 is somewhat confusing because the admin already has the permission to approve the comment.
As @andypost mentioned that it is validated in the below AccessController method.

    if ($operation == 'approve') {
      return AccessResult::allowedIf($comment_admin && !$entity->isPublished())
        ->cachePerPermissions()
        ->addCacheableDependency($entity);
    }

To get the proper message and response code, I think we can remove !$entity->isPublished() validation as we are already showing proper messages in controller method.

I haven't removed that as of now as it would be great if we have some inputs from the product owner or subsystem maintainers.

I think it would be better to add a custom exception handler, check the route name, and if its this route, display a message and send them back to the commented entity via a redirect

e.g extend from HttpExceptionSubscriberBase and add a method `on403`

Inject the route match and messenger service, and if the route is the comment approval one, display the message and return a new redirect response (to the comment canonical path)

For testing, fetch the page that has the approve link then publish the comment via the api (e.g. $comment->status = 1; $comment->save())

Then click the link and assert the message is output and the redirect occurs.

alternatively we could probably remove the published check from the route and handle it in the controller instead, with a redirect from there

might be better than having a custom exception handler in memory in the event dispatcher on every page

Implemented the following changes:
- Removed !$entity->isPublished()) from the checkAccess() in AccessController
- Update test cases to validate that accordingly.
- Remove additional changes added in previous commit.

Small fix to remove the t() calls in the test file. See: https://www.drupal.org/docs/automated-testing/phpunit-in-drupal/phpunit-browser-test-tutorial#t
Except for that, the merge request looks good.

Hiding patch files

Left a minor readability suggestion, but this looks good to me

LGTM.

There are at least two contrib projects checking the 'approve' permission http://grep.xnddx.ru/search?text=-%3Eaccess%28%27approve&filename=

Can we just confirm that there's no security impact of this change

Looking at related list I bet there's impact

@andypost @larowlan
I did a quick review of all the MR/issues mentioned in the above comment.

In our case, primarily we are changing "approve" operation access.
I think https://www.drupal.org/project/drupal/issues/2879087 might have significant conflicts.

IMO, can we revisit later after the https://www.drupal.org/project/drupal/issues/2879087 is merged?

Can we update MR to 10.1 please.

Please review.

I can't believe this issue is still open! :)

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.

Seems the D10 MR 3384 has some failures.

@andypost and @larowlan does #47 answer your comments about security?