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?
Comment | File | Size | Author |
---|---|---|---|
#48 | node-access-table-1204658-48-patch-only.patch | 4.51 KB | Berdir |
#48 | node-access-table-1204658-48.patch | 5.9 KB | Berdir |
#48 | node-access-table-1204658-48-interdiff.txt | 1.16 KB | Berdir |
#44 | node-access-table-1204658-44.patch | 4.74 KB | Berdir |
#44 | node-access-table-1204658-44-tests-only.patch | 2.67 KB | Berdir |
Comments
Comment #2
catchThe 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.
Comment #3
webchickOk 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.
Comment #4
catch@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?
Comment #5
pillarsdotnet CreditAttribution: pillarsdotnet commentedNote 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
Comment #6
xjmThis issue is only 5 comments long but I'm still confused. :) As far as I understand:
Comment #7
BerdirYes, 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.
Comment #9
xjmComment #10
BerdirOk, 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.
Comment #11
webchickRemoving this tag since it doesn't make sense to backport the non-BC-compatible change to D7.
Comment #12
xjmThis 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).
Comment #13
Berdir@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.
Comment #14
webchickAh, well that's fair enough.
Comment #15
BerdirThere 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.
Comment #16
kbasarab CreditAttribution: kbasarab commentedAlright 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.
Comment #18
kbasarab CreditAttribution: kbasarab commentedThink I had to change to DrupalWebTestCase over to WebTestBase. Trying just the tracker test for now before rerolling the fail/pass patches.
Comment #19
kbasarab CreditAttribution: kbasarab commentedForgot to change status.
Comment #20
kbasarab CreditAttribution: kbasarab commentedSo 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.
Comment #21
BerdirGreat, logic looks good to me, just some coding style stuff to clean up. See details below.
I guess this one doesn't have test coverage yet...
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.".
entity, text and field_sql_storage modules should be installed by default, you shouldn't need to specify them explicitly.
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.
Comments should always end with a ".". Also the second one seems to have an "i" that doesn't belong there :)
Assertion messages don't need t() anymore.
Comment #22
kbasarab CreditAttribution: kbasarab commentedWe'll try this one for the styling issues.
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?
Comment #23
BerdirThe 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?
See the link above, this should be a docblock comment (/** ... */).
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.
Comment #24
kbasarab CreditAttribution: kbasarab commentedLets 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.
Comment #25
tim.plunkettCoding style review:
Remove the blank line
These commas need spaces after them, and this can just be combined with the setUp
Add a space between these two lines, and decapitalize Active
Missing space after the comma
That full stop should be a comma.
Missing a space after the comma
That full stop should be a comma.
Remove the t() from the assertion message (right side)
Remove the t() from the assertion message (right side)
Missing a blank line between php and the comment, and the comment is indented incorrectly
Namespace should be namespace
Same as above (commas, combine)
Reword so this fits under 80 character, and watch the trailing whitespace
Remove this extra blank line
Comment #26
kbasarab CreditAttribution: kbasarab commentedThanks for review Tim. Silly mistakes. Rerolled and updated:
Comment #27
kbasarab CreditAttribution: kbasarab commentedFound two additional comment style changes inside ForumTest.php.
Comment #28
tim.plunkettPart of this was added in http://drupalcode.org/project/drupal.git/commit/352645e, and the rest was moved to PSR-0. Reroll.
Comment #29
tim.plunkettStill applies.
Comment #30
chx CreditAttribution: chx commentedI think this is good to go. As a reminder:
Comment #31
Dries CreditAttribution: Dries commentedCommitted to 8.x. Glad that magic code is gone. Thanks all.
Comment #32
webchickNot sure if we can backport this, but moving to 7.x for discussion.
Comment #33
BerdirWhat 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.
Comment #35
BerdirHm, nobody actually changed the version back to 7.x. Might work better then.
Also, does this need a change notice for 8.x?
Comment #36
Berdir#33: node-access-metadata-tests-1204658-33.patch queued for re-testing.
Comment #38
BerdirFixed the base class name and also made sure that the tests actually pass...
Comment #39
YesCT CreditAttribution: YesCT commented#38: node-access-metadata-tests-1204658-38.patch queued for re-testing.
Comment #40
xjmI 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.
Comment #41
xjmComment #42
David_Rothstein CreditAttribution: David_Rothstein commentedThis 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):
These variables in the forum test (but also similar ones in the tracker tests) don't look like they are ever used.
Hm, I guess this works (though with an extra slash in the test URLs) - seems like it should just be $this->drupalGet('') though.
Spacing issues on both 'private' lines.
Comment #43
David_Rothstein CreditAttribution: David_Rothstein commentedThe 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.
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...
Comment #44
BerdirSorry 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
Comment #46
BerdirPassed/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.Comment #47
xjmI'd tentatively say that throwing an exception sounds like a much better idea than failing silently.
Comment #48
BerdirOk.
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.
Comment #49
benjifisher#48: node-access-table-1204658-48.patch queued for re-testing.
Comment #50
druderman CreditAttribution: druderman commentedLooking at issue via #drupalcon sprintno longer looking at this.Comment #51
swentel CreditAttribution: swentel commentedSweet!
Comment #52
catchCommitted/pushed to 8.x, moving to 7.x for backport.
Comment #53
BerdirGreat, back to normal, as this is just a non-critical cleanup there
Comment #53.0
Berdiradded remaining tasks