We need to get the SA-CORE-2011-002 changes into D8.

Here's the patch from chx.

To summarize a great deal of discussion, basically taxonomy listings were not respecting node access because the queries did not specify a $query->addMetaData('base_table', 'taxonomy_index');. This new requirement has been posted at http://drupal.org/node/1204572. However, to care for existing contrib modules, a fallback mechanism was put in place which attempts to guess the base table if it's not specified.

I don't think the fallback mechanism makes any sense in D8. Not sure if we should just commit this and keep iterating on it, or if we want to come up with a new patch. Let's discuss!

Remaining Tasks

  • #44 Address: Sounds like we should throw an exception if a table is specified that doesn't exist? Right now it seems that we ignore it silently in that case?
Files: 
CommentFileSizeAuthor
#48 node-access-table-1204658-48-patch-only.patch4.51 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 54,250 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#48 node-access-table-1204658-48.patch5.9 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 55,911 pass(es).
[ View ]
#48 node-access-table-1204658-48-interdiff.txt1.16 KBBerdir
#44 node-access-table-1204658-44.patch4.74 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 54,048 pass(es).
[ View ]
#44 node-access-table-1204658-44-tests-only.patch2.67 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 53,985 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#38 node-access-metadata-tests-1204658-38.patch9.36 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 40,060 pass(es).
[ View ]
#33 node-access-metadata-tests-1204658-33.patch9.36 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#28 drupal-1204658-28.patch12.19 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 37,118 pass(es).
[ View ]
#27 NodeAccessFallback-1204658-27.patch14.96 KBkbasarab
PASSED: [[SimpleTest]]: [MySQL] 36,882 pass(es).
[ View ]
#27 interdiff.txt1.27 KBkbasarab
#26 NodeAccessFallback-1204658-26.patch13.96 KBkbasarab
PASSED: [[SimpleTest]]: [MySQL] 36,881 pass(es).
[ View ]
#26 interdiff.txt5.74 KBkbasarab
#24 NodeAccessFallback-1204658-24.patch12.14 KBkbasarab
PASSED: [[SimpleTest]]: [MySQL] 36,673 pass(es).
[ View ]
#22 TrackerNodeAccessTest-1204658-21.patch2.3 KBkbasarab
PASSED: [[SimpleTest]]: [MySQL] 36,612 pass(es).
[ View ]
#20 TrackerNodeAccessTest-1204658-18-Fail.patch8.16 KBkbasarab
FAILED: [[SimpleTest]]: [MySQL] 36,612 pass(es), 3 fail(s), and 2 exception(s).
[ View ]
#20 TrackerNodeAccessTest-1204658-18-Pass.patch8.15 KBkbasarab
PASSED: [[SimpleTest]]: [MySQL] 36,622 pass(es).
[ View ]
#19 TrackerNodeAccessTest-1204658-18.patch2.25 KBkbasarab
PASSED: [[SimpleTest]]: [MySQL] 36,614 pass(es).
[ View ]
#18 TrackerNodeAccessTest-1204658-18.patch2.25 KBkbasarab
PASSED: [[SimpleTest]]: [MySQL] 36,607 pass(es).
[ View ]
#16 TrackerNodeAccess-Fail.patch8.15 KBkbasarab
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#16 TrackerNodeAccess-Pass.patch8.15 KBkbasarab
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#16 TrackerNodeAccessTest.patch2.25 KBkbasarab
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#10 add_meta_data.patch5.89 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 35,352 pass(es).
[ View ]
#7 remove_fallback-1204658.patch2.54 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 35,335 pass(es), 24 fail(s), and 12 exception(s).
[ View ]
sdo_43599_26.patch15.61 KBwebchick
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch sdo_43599_26.patch.
[ View ]

Comments

Status:Needs review» Needs work

The last submitted patch, sdo_43599_26.patch, failed testing.

Issue tags:+needs backport to D7

The patch is failing to apply due to #1136130: Regression: Reinstate WATCHDOG_* constants and document why they are necessary., which happens to be RTBC. Not sure if there are other conflicts after that but seems pointless to re-roll unless something specific is holding that other patch up.

Even with the addMetaData() the node access system still would not have correctly rewritten the queries if I understand correctly. Also those addMetaData calls are not added here, I would think we need those for at least taxonomy and forum, maybe tracker too. Those could be backported to Drupal 7 for clarity, for some reason I thought they were in the original patch. Adding Needs backport to D7 tag so that doesn't get lost - if it ends up happening in a followup issue we can untag this one.

My preference would be to commit the patch as it went into Drupal 7, then post a major followup for removing the fallback and ensuring everything works, doing it the other way around tended to leave SA followups lingering in the queue for months during D7.

I'm also wondering now if the fallback added here will have changed behaviour of EntityFieldQuery for queries involving node reference fields with an nid column.

Category:bug» task
Priority:Critical» Major

Ok great, #1136130: Regression: Reinstate WATCHDOG_* constants and document why they are necessary. was committed, so I committed and pushed this patch to 8.x in order to get the two in sync again.

Reducing from a critical bug to a major task to figure out the optimal solution to this for D8, now that we have some breathing room.

@todos (may well be more than this):

Add the addMetaData() to at least taxonomy, forum and tracker queries.

Remove the fallback mechanism and throw a big error if the query is tagged node_access and does not add metadata.

Not sure what we should do if the base table is node - add metadata anyway or not?

Note that the tests added in this issue wrongly assume that the "Read more" link will always be present for teaser nodes.

This is blocking: #823380: Read More link is always visible on teaser.

See also: #1300568: Fix tests that wrongly assume all teasers have a Read more link

This issue is only 5 comments long but I'm still confused. :) As far as I understand:

  • The SA forward-port has been resolved--so should we retitle this issue?
  • #4 seems to describe the current goal/scope? Kinda? Can we clarify?
  • #5 is not actually related to this issue.

Status:Needs work» Needs review
StatusFileSize
new2.54 KB
FAILED: [[SimpleTest]]: [MySQL] 35,335 pass(es), 24 fail(s), and 12 exception(s).
[ View ]

Yes, I think there are two tasks here:

- Remove the magic fallback handling.
- Fix all instances which should provide a base_table.

The patch does the first thing, still keeps node as the default. Everything that does miss the base table should now fall on it's nose and throw an exception.

I guess the base_table definitions could then be backported to 7.x so that core doesn't have to rely on it's own fallback handling.

Let's see how many exception this gives us.

Status:Needs review» Needs work

The last submitted patch, remove_fallback-1204658.patch, failed testing.

Title:SA-CORE-2011-002 - Drupal core - Access bypassRemove node access base table fallback

Status:Needs work» Needs review
Issue tags:+Needs tests
StatusFileSize
new5.89 KB
PASSED: [[SimpleTest]]: [MySQL] 35,352 pass(es).
[ View ]

Ok, looks like we have test coverage for comments, taxonomy and forum (maybe not all, but at least some queries there).

Leaves tracker.module...

The attached patch fixes all missing base_tables I think.

Issue tags:-needs backport to D7

Removing this tag since it doesn't make sense to backport the non-BC-compatible change to D7.

This looks correct to me. I might like to add the metadata immediately after the node access table and add a comment in each case indicating why it matters, since it's rather opaque otherwise.

Looks like we need to add test coverage for the forum, taxonomy, and tracker cases (because no failures for those are exposed when the fallback is ripped out without the metadata in #7).

@webchick:

While the removal of the fallback is obviously not backportable, the actual changes in forum/comment/taxonomy/tracker can and imho should be backported. Core should implement it's own API's correctly and be an example for contrib and not rely on it's own security fallbacks. Same for the additional tests that are going to be added here.

Issue tags:+needs backport to D7

Ah, well that's fair enough.

There are already tests for taxonomy and forum in node.test (which should probably be moved, but that's another story). So that means we're left with tests for tracker.module. That one should be similar to e.g. NodeAccessBaseTableTestCase.

- Enable the tracker and node_access_test modules
- call variable_set('node_access_test_private') and node_access_rebuild(). Just like the test class mentioned above.
- Create two users, one with the permission "'node test view'" and one without.
- Create a few nodes
- Make sure the user with the node test view permission can see the nodes, the other can't.

StatusFileSize
new2.25 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
new8.15 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
new8.15 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]

Alright forgive me if this isn't quite right but I think I have this working at least basic functionality.

Three patches attached --
TrackerNodeAccessTest.patch - Simply the test cases for tracker module as documented in #15

TrackerNodeAccess-Pass.patch - This one rolls #10 in and adds the test case. Shows test case passing with patch from #10 untouched.

TrackerNodeAccess-Fail.patch - This one rolls #10 in but removes ->addMetaData in tracker/tracker.pages.inc though to show that the SimpleTest fails when that is removed from Tracker.

I know there are probably some more coding standards issues I probably missed and other ideas to make it better so have at it and let me know what should change. Trying to start helping out with core and this is the first core patch submission. Yay.

Status:Needs review» Needs work

The last submitted patch, TrackerNodeAccessTest.patch, failed testing.

StatusFileSize
new2.25 KB
PASSED: [[SimpleTest]]: [MySQL] 36,607 pass(es).
[ View ]

Think I had to change to DrupalWebTestCase over to WebTestBase. Trying just the tracker test for now before rerolling the fail/pass patches.

Status:Needs work» Needs review
StatusFileSize
new2.25 KB
PASSED: [[SimpleTest]]: [MySQL] 36,614 pass(es).
[ View ]

Forgot to change status.

StatusFileSize
new8.15 KB
PASSED: [[SimpleTest]]: [MySQL] 36,622 pass(es).
[ View ]
new8.16 KB
FAILED: [[SimpleTest]]: [MySQL] 36,612 pass(es), 3 fail(s), and 2 exception(s).
[ View ]

So patch in #19 is just the patch for the test. Attached two are applying patch from #10 and one is my test against #10 while the fail patch is my patch against #10 but with the addMetaData on Tracker removed.

Status:Needs review» Needs work

Great, logic looks good to me, just some coding style stuff to clean up. See details below.

+++ b/core/modules/forum/forum.moduleundefined
+++ b/core/modules/forum/forum.moduleundefined
@@ -677,7 +677,8 @@ function forum_block_save($delta = '', $edit = array()) {
@@ -677,7 +677,8 @@ function forum_block_save($delta = '', $edit = array()) {
function forum_block_view($delta = '') {
   $query = db_select('forum_index', 'f')
     ->fields('f')
-    ->addTag('node_access');
+    ->addTag('node_access')
+    ->addMetaData('base_table', 'forum_index');

I guess this one doesn't have test coverage yet...

+++ b/core/modules/tracker/lib/Drupal/tracker/Tests/TrackerNodeAccessTest.phpundefined
@@ -0,0 +1,63 @@
+/**
+* @file
+* Node Access tests
+*

No need to reference to the issue unless there really is something very important in there.

The standard for PSR-0 files is simply "Definition of Fully\Qualified\Classname.".

+++ b/core/modules/tracker/lib/Drupal/tracker/Tests/TrackerNodeAccessTest.phpundefined
@@ -0,0 +1,63 @@
+    $modules = array('node','comment','entity','text','field_sql_storage','tracker','node_access_test');

entity, text and field_sql_storage modules should be installed by default, you shouldn't need to specify them explicitly.

+++ b/core/modules/tracker/lib/Drupal/tracker/Tests/TrackerNodeAccessTest.phpundefined
@@ -0,0 +1,63 @@
+  function testTrackerNodeAccess() {
+    // Create user with node test view permission

setUp() and getInfo() don't need them, but each test*() mthod should have a simple one-line docblock that explains what is being tested here.

+++ b/core/modules/tracker/lib/Drupal/tracker/Tests/TrackerNodeAccessTest.phpundefined
@@ -0,0 +1,63 @@
+    // Create user without node test view permission
+    $no_access_user = $this->drupalCreateuser();
+
+    $this->drupalLogin($access_user);
+
+    // Create some nodesi
+    $private_node = $this->drupalCreateNode(array(

Comments should always end with a ".". Also the second one seems to have an "i" that doesn't belong there :)

+++ b/core/modules/tracker/lib/Drupal/tracker/Tests/TrackerNodeAccessTest.phpundefined
@@ -0,0 +1,63 @@
+    $this->drupalGet('tracker');
+    $this->assertText($private_node->title, t('Private node is visible to user with private access.'));
+    $this->assertText($public_node->title, t('Public node is visible to user with private access.'));
+
+    // User without access should not see private node
+    $this->drupalLogin($no_access_user);
+    $this->drupalGet('tracker');
+    $this->assertNoText($private_node->title, t('Private node is not visible to user without private access.'));
+    $this->assertText($public_node->title, t('Public node is visible to user without private access.'));

Assertion messages don't need t() anymore.

StatusFileSize
new2.3 KB
PASSED: [[SimpleTest]]: [MySQL] 36,612 pass(es).
[ View ]

We'll try this one for the styling issues.

+++ b/core/modules/forum/forum.moduleundefined
+++ b/core/modules/forum/forum.moduleundefined
@@ -677,7 +677,8 @@ function forum_block_save($delta = '', $edit = array()) {
@@ -677,7 +677,8 @@ function forum_block_save($delta = '', $edit = array()) {
function forum_block_view($delta = '') {
   $query = db_select('forum_index', 'f')
     ->fields('f')
-    ->addTag('node_access');
+    ->addTag('node_access')
+    ->addMetaData('base_table', 'forum_index');

I guess this one doesn't have test coverage yet...

I was just testing in Tracker. To test for that just to see if I'm right here:

1. Add tests to forum.test since it hasn't been converted yet.
2. Create a private forum topic
3. Place the forum block on a page
4. See if private node access user can see and non private node access user cannot
5. Try same thing for a public forum topic?

+++ b/core/modules/tracker/lib/Drupal/tracker/Tests/TrackerNodeAccessTest.phpundefined
@@ -0,0 +1,65 @@
+
+class TrackerNodeAccessTest extends WebTestBase {

The class should have a comment as well, see http://drupal.org/node/1354#classes for guidelines.

Also, according to our guidelines, it *might* not be necessary to keep using the Tracker prefix here. However, we *are* testing node access for the tracker pages, so I'm not sure that NodeAccess.php makes sense.

Thoughts, anyone?

+++ b/core/modules/tracker/lib/Drupal/tracker/Tests/TrackerNodeAccessTest.phpundefined
@@ -0,0 +1,65 @@
+  // Ensures private nodes are only visible to users ¶
+  // with permission on /tracker.

See the link above, this should be a docblock comment (/** ... */).

+++ b/core/modules/tracker/lib/Drupal/tracker/Tests/TrackerNodeAccessTest.phpundefined
@@ -0,0 +1,65 @@
+    $this->assertNoText($private_node->title, 'Private node is not visible to user without private access.');
+    $this->assertText($public_node->title, 'Public node is visible to user without private access.');
+  }
+}
+?>

There shouldn't be a closing php tag but make sure to keep the newline at the end, git diff doesn't like it if it's not there.

Status:Needs work» Needs review
StatusFileSize
new12.14 KB
PASSED: [[SimpleTest]]: [MySQL] 36,673 pass(es).
[ View ]

Lets try this one. This adds in forum fallback testing as well. Full patch attached.

Made the styling modifications you mentioned berdir also. Just gotta get all of them stuck into my head.

Status:Needs review» Needs work

Coding style review:

+++ b/core/modules/forum/forum.testundefined
@@ -653,3 +653,95 @@ class ForumIndexTestCase extends WebTestBase {
+ */
+
+class ForumNodeAccessTestCase extends WebTestBase {

Remove the blank line

+++ b/core/modules/forum/forum.testundefined
@@ -653,3 +653,95 @@ class ForumIndexTestCase extends WebTestBase {
+    $modules = array('node','comment','forum','taxonomy','tracker','node_access_test','block');
+    parent::setUp($modules);

These commas need spaces after them, and this can just be combined with the setUp

+++ b/core/modules/forum/forum.testundefined
@@ -653,3 +653,95 @@ class ForumIndexTestCase extends WebTestBase {
+   * Creates some users and creates a public node and a private node.
+   * Adds both Active forum topics and new forum topics blocks to the sidebar.

Add a space between these two lines, and decapitalize Active

+++ b/core/modules/forum/forum.testundefined
@@ -653,3 +653,95 @@ class ForumIndexTestCase extends WebTestBase {
+    $this->drupalPost('node/add/forum/1',$edit, t('Save'));

Missing space after the comma

+++ b/core/modules/forum/forum.testundefined
@@ -653,3 +653,95 @@ class ForumIndexTestCase extends WebTestBase {
+    $this->assertTrue(!empty($private_node). 'New private forum node found in database.');

That full stop should be a comma.

+++ b/core/modules/forum/forum.testundefined
@@ -653,3 +653,95 @@ class ForumIndexTestCase extends WebTestBase {
+    $this->drupalPost('node/add/forum/1',$edit, t('Save'));

Missing a space after the comma

+++ b/core/modules/forum/forum.testundefined
@@ -653,3 +653,95 @@ class ForumIndexTestCase extends WebTestBase {
+    $this->assertTrue(!empty($public_node). 'New public forum node found in database.');

That full stop should be a comma.

+++ b/core/modules/forum/forum.testundefined
@@ -653,3 +653,95 @@ class ForumIndexTestCase extends WebTestBase {
+    $this->assertText(t('The block settings have been updated.'), t('Active forum topics forum block was enabled'));

Remove the t() from the assertion message (right side)

+++ b/core/modules/forum/forum.testundefined
@@ -653,3 +653,95 @@ class ForumIndexTestCase extends WebTestBase {
+    $this->assertText(t('The block settings have been updated.'), t('[New forum topics] Forum block was enabled'));

Remove the t() from the assertion message (right side)

+++ b/core/modules/tracker/lib/Drupal/tracker/Tests/TrackerNodeAccessTest.phpundefined
@@ -0,0 +1,70 @@
+<?php
+/**
+* @file
+* Definition of Drupal\tracker\Tests\TrackerNodeAccessTest.
+*
+*/

Missing a blank line between php and the comment, and the comment is indented incorrectly

+++ b/core/modules/tracker/lib/Drupal/tracker/Tests/TrackerNodeAccessTest.phpundefined
@@ -0,0 +1,70 @@
+Namespace Drupal\tracker\Tests;

Namespace should be namespace

+++ b/core/modules/tracker/lib/Drupal/tracker/Tests/TrackerNodeAccessTest.phpundefined
@@ -0,0 +1,70 @@
+    $modules = array('node','comment','tracker','node_access_test');
+    parent::setUp($modules);

Same as above (commas, combine)

+++ b/core/modules/tracker/lib/Drupal/tracker/Tests/TrackerNodeAccessTest.phpundefined
@@ -0,0 +1,70 @@
+   * Ensures private nodes are only visible to users ¶
+   * with permission on /tracker.

Reword so this fits under 80 character, and watch the trailing whitespace

+++ b/core/modules/tracker/lib/Drupal/tracker/Tests/TrackerNodeAccessTest.phpundefined
@@ -0,0 +1,70 @@
+}
+
diff --git a/core/modules/tracker/tracker.pages.inc b/core/modules/tracker/tracker.pages.inc

Remove this extra blank line

Status:Needs work» Needs review
StatusFileSize
new5.74 KB
new13.96 KB
PASSED: [[SimpleTest]]: [MySQL] 36,881 pass(es).
[ View ]

Thanks for review Tim. Silly mistakes. Rerolled and updated:

StatusFileSize
new1.27 KB
new14.96 KB
PASSED: [[SimpleTest]]: [MySQL] 36,882 pass(es).
[ View ]

Found two additional comment style changes inside ForumTest.php.

StatusFileSize
new12.19 KB
PASSED: [[SimpleTest]]: [MySQL] 37,118 pass(es).
[ View ]

Part of this was added in http://drupalcode.org/project/drupal.git/commit/352645e, and the rest was moved to PSR-0. Reroll.

Issue tags:-Needs tests

Still applies.

Status:Needs review» Reviewed & tested by the community

I think this is good to go. As a reminder:

Also those addMetaData calls are not added here, I would think we need those for at least taxonomy and forum, maybe tracker too. Those could be backported to Drupal 7 for clarity, for some reason I thought they were in the original patch. Adding Needs backport to D7 tag so that doesn't get lost - if it ends up happening in a followup issue we can untag this one.

Status:Reviewed & tested by the community» Fixed

Committed to 8.x. Glad that magic code is gone. Thanks all.

Status:Fixed» Patch (to be ported)

Not sure if we can backport this, but moving to 7.x for discussion.

Status:Patch (to be ported)» Needs review
StatusFileSize
new9.36 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]

What we can backport are the explicit MedataData additions, so that core doesn't rely on it's own table fallback mechanism magic and the tests.

Here's a patch for that, haven't run the tests, no idea if that will work.

Status:Needs review» Needs work

The last submitted patch, node-access-metadata-tests-1204658-33.patch, failed testing.

Version:8.x-dev» 7.x-dev
Status:Needs work» Needs review

Hm, nobody actually changed the version back to 7.x. Might work better then.

Also, does this need a change notice for 8.x?

Status:Needs review» Needs work
Issue tags:+Needs issue summary update, +Security Advisory follow-up, +needs backport to D7

The last submitted patch, node-access-metadata-tests-1204658-33.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new9.36 KB
PASSED: [[SimpleTest]]: [MySQL] 40,060 pass(es).
[ View ]

Fixed the base class name and also made sure that the tests actually pass...

Title:Remove node access base table fallbackUse query metadata to specify the node access base table
Priority:Major» Normal
Status:Needs review» Reviewed & tested by the community
Issue tags:+Novice

I think this is just a normal task for D7, since we are essentially just deprecating the base table fallback in #38 rather than removing it.

I also filed #1885420: Document the use of metadata to indicate a base table for node access queries.

Title:Use query metadata to specify the node access base tableAlways use query metadata to specify the node access base table

Status:Reviewed & tested by the community» Needs work

@@ -32,6 +32,7 @@ function tracker_page($account = NULL, $set_title = FALSE) {
   // while keeping the correct order.
   $nodes = $query
     ->addTag('node_access')
+    ->addMetaData('base_table', 'tracker_node')

This looks wrong - the query is actually sometimes against the 'tracker_user' table instead. I am not positive, but I think this would introduce a security issue?

---

Other issues (all very minor, but while I was reviewing):

  1. +  protected $access_user;
    +  protected $admin_user;
    +  protected $no_access_user;

    These variables in the forum test (but also similar ones in the tracker tests) don't look like they are ever used.

  2. There are some spacing/newline issues throughout the tests.
  3. +    $this->drupalGet('/');

    Hm, I guess this works (though with an extra slash in the test URLs) - seems like it should just be $this->drupalGet('') though.

  4. +    // Create some nodes.
    +    $private_node = $this->drupalCreateNode(array(
    +      'title' => t('Private node test'),
    +      'private'=> TRUE,
    +    ));
    +    $public_node = $this->drupalCreateNode(array(
    +      'title' => t('Public node test'),
    +      'private'=>FALSE,
    +    ));

    Spacing issues on both 'private' lines.

Version:7.x-dev» 8.x-dev
Priority:Normal» Critical
Issue tags:+Needs change record

The issue with the tracker module seems to be present in Drupal 8 as well. Since it looks like a security issue, I'm bumping to critical (I don't see any tests that would have been related to node access on the per-user tracker page, so I'm still not positive it has security implications but I think it does). For what it's worth, most of the minor issues identified above look to be in Drupal 8 too.

Also, does this need a change notice for 8.x?

Yikes, I would definitely think so.

I don't see an existing one after a quick search, and the Drupal 8 patch in this issue changed the API in a way that would have security implications for contrib modules so it definitely needs one. Another reason to bump this to critical...

Status:Needs work» Needs review
Issue tags:-Novice
StatusFileSize
new2.67 KB
FAILED: [[SimpleTest]]: [MySQL] 53,985 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new4.74 KB
PASSED: [[SimpleTest]]: [MySQL] 54,048 pass(es).
[ View ]

Sorry for not getting back to this earlier.

Yes, you were of course right, that is a security problem. Sounds like we should throw an exception if a table is specified that doesn't exist? Right now it seems that we ignore it silently in that case?

Fixed what I could find, not everything existed in 8.x

Status:Needs review» Needs work

The last submitted patch, node-access-table-1204658-44-tests-only.patch, failed testing.

Status:Needs work» Needs review

Passed/Failed as expected, just in the wrong order :) Back to needs review.

Who wants to RTBC? :) Edit: Forgot my own question in #44, probably needs to be answered first.

I'd tentatively say that throwing an exception sounds like a much better idea than failing silently.

StatusFileSize
new1.16 KB
new5.9 KB
PASSED: [[SimpleTest]]: [MySQL] 55,911 pass(es).
[ View ]
new4.51 KB
FAILED: [[SimpleTest]]: [MySQL] 54,250 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Ok.

We still need the additional test coverage to actually trigger this bug but now it explodes with a meaningful error message (I think) and doesn't just silently not work.

#48: node-access-table-1204658-48.patch queued for re-testing.

Looking at issue via #drupalcon sprint no longer looking at this.

Status:Needs review» Reviewed & tested by the community

Sweet!

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Committed/pushed to 8.x, moving to 7.x for backport.

Priority:Critical» Normal

Great, back to normal, as this is just a non-critical cleanup there

Issue summary:View changes

added remaining tasks