Unpublished forum accessible to public

The PHPUnit test to reproduce this issue.

The test should fail as expected.

1. +++ b/core/modules/forum/tests/src/Functional/ForumTermAccessTest.php
   @@ -0,0 +1,115 @@
   +    // Create a unpublished forum.
   

   nit: a => an

2. +++ b/core/modules/forum/tests/src/Functional/ForumTermAccessTest.php
   @@ -0,0 +1,115 @@
   +    $unpublished_forum_name = $this->randomMachineName(8);
   +    $edit = [
   +      'name[0][value]' => $unpublished_forum_name,
   +      'description[0][value]' => $this->randomMachineName(200),
   +      'status[value]' => 0,
   +    ];
   +    // Save a new unpublished forum.
   +    $this->drupalGet('admin/structure/forum/add/forum');
   +    $this->submitForm($edit, 'Save');
   +    $assert_session->pageTextContains('Created new forum ' . $unpublished_forum_name);
   +
   +    // Create a new published forum.
   +    $published_forum_name = $this->randomMachineName(8);
   +    $edit = [
   +      'name[0][value]' => $published_forum_name,
   +      'description[0][value]' => $this->randomMachineName(200),
   +      'status[value]' => 1,
   +    ];
   +    // Save the new published forum.
   +    $this->drupalGet('admin/structure/forum/add/forum');
   +    $this->submitForm($edit, 'Save');
   +    $assert_session->pageTextContains('Created new forum ' . $published_forum_name);
   

   Let's create these using the api, Term::create()->save()

   We're not testing the add forum UI here, we're testing access so we don't need to use the form.

3. +++ b/core/modules/forum/tests/src/Functional/ForumTermAccessTest.php
   @@ -0,0 +1,115 @@
   +    $assert_session->statusCodeNotEquals(200);
   

   let's be specific here and use statusCodeEquals(403)

   We could have a 500 error here and the test would pass 🙃

4. +++ b/core/modules/forum/tests/src/Functional/ForumTermAccessTest.php
   @@ -0,0 +1,115 @@
   +  private function reloadForumByName(string $name): TermInterface {
   

   we won't need this if we use the API instead of the form to create the terms

Nice work on the tests, now we just need the fix 💪

Thanks @Rowlands for review the test patch #2.

1. nit: a => an

   Corrected.

2. Let's create these using the api, Term::create()->save()

   Updated.

3. let's be specific here and use statusCodeEquals(403)

   I thought the same. But when I checked the test from Taxonomy module, I saw this line https://git.drupalcode.org/project/drupal/-/blob/10.1.x/core/modules/tax...
   Ideally, it should return 403 instead of 404. let's use 403 for now to see if the coming fix can pass the test.

4. we won't need this if we use the API instead of the form to create the terms.

   Removed.

Here is the updated patch.

Thanks again.

Test looks good, now we just need the fix, which will likely involve adding

_entity_access: 'taxonomy_term.view'

To the requirements section in the routing entry for forum.page and an access check to \Drupal\forum\Controller\ForumController::build that checks ->access('view') on each term, collects cacheability metadata from each return (return as object TRUE) and adds that to the render array.

Will take a quick swing at this.

Sorry i looked at the cacheability metadata and turned my head inside out. Every example i looked at in core seemed not to line up in some way. The usual approach is to check access right in the query, the closest we have to that since frighteningly loadTree() does not have an access check parameter is to do it in ForumManager's getChildren with is used both for the index and forum pages, anywhere forums are listed— but again no idea how to get that into cache metadata over in build the right way.

Appears to have failures in 11.x which it would need to be merged into first.

FYI patches are being phased out and it's encouraged to try gitlab. It's much faster and easier for reviews.

Thanks!

Create a MR.

Let's go from there.

MR looks fine no failures moving to NR

Hi
Tested MR !6607 on Drupal version 11.x
Applied MR Successfully...

Followed steps from the summary for reproducing the issue.

Test Result:
Removed the unpublished forums from the forum landing page for anonymous users as per the proposed solution

Attaching screenshots for references
RTBC++

Keeping in "Need review" for more reviews

Forum is approved for removal. See #1898812: [policy] Deprecate forum module for removal in Drupal 11

This is now Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project and the Extensions approved for removal policies.

It will be moved to the contributed extension once the Drupal 11 branch is open.

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. For more information, see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

Thanks @smustgrave - because of the information disclosure here I think we should keep working on this just in case we miss removing forum again

Hiding patches for clarity

Applied a simple suggestion to the test method

Ran test-only feature

There was 1 error:
1) Drupal\Tests\forum\Functional\ForumTermAccessTest::testForumTermAccess
Behat\Mink\Exception\ResponseTextException: The text "z41709kq" appears in the text of this page, but it should not.
/builds/issue/drupal-3387172/vendor/behat/mink/src/WebAssert.php:907
/builds/issue/drupal-3387172/vendor/behat/mink/src/WebAssert.php:312
/builds/issue/drupal-3387172/core/modules/forum/tests/src/Functional/ForumTermAccessTest.php:85
/builds/issue/drupal-3387172/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
ERRORS!

Code change of checking forum access makese sense.

LGTM

This bug was introduced by #2981887: Add a publishing status to taxonomy terms → . the fix seems reasonable. I do wonder if status should be accounted for in \Drupal\taxonomy\TermStorage::loadTree() as that is adding the tag 'taxonomy_term_access' to it's query but afaik this is no different to nodes.

Committed and pushed f660bb4c1e to 11.x and 9397852246 to 10.2.x. Thanks!

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