Hello, tried to investigate this behaviour myself. Am not certain why it is happening.
The Classified Ad is still viewable on the front end (as both a non-owner and anonymous reader) which it should not be.
Owners (Authenticated users) are allowed to view/edit their own unpublished Classified Ads. An administrator unpublishes a Classified Ad (e.g. terms and conditions is not ok, Classified is disabled until the reader re-works it).
The following node permission settings are enabled for Authenticated Users:
Create new classified
Update own classified
Delete own classified
View own unpublished content
Guests and Authenticated users are allowed to read published content.
All other modules are up to date, Drupal 7.12 core is installed.
We tested with the same permissions enabled for a Page Node. Guests received a 403 page when attempting to view an unpublished page node created by an authenticated user. So it cannot be a Drupal core issue.
We have tested the module 7.x-3.x-rc2 manually fairly extensively and will deploy it in a new website later this month. This is the only security flaw we have found.
If you need more feedback, I am happy to help. Thanks!
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | hide_unpublished_ads-1432606.patch | 6.33 KB | fgm |
| #4 | ed_classified-node-access-1432606-4.patch | 1.42 KB | jjchinquist |
Comments
Comment #1
fgmThanks for this detailed error report. More details could help too:
Comment #2
jjchinquistWe are using varnish and entity cache.
It is odd however, that core node types work fine and the classifieds are not. I think the php access logic is bypassed and it is not a problem with the cache.
Comment #3
jjchinquistHello FGM,
You implement hook_node_access(). I can see one difference between your classified_node_access and node_node_access in the node.module file. In your access function, you address the View operation and return TRUE when the user has "access content". This overrides the system logic. The node.module does not even handle this behavior.
My suggestion, which we are testing, is to remove the classified_node_access() function completely. Do you have a reason for it?
Getting a patch next.
Comment #4
jjchinquistRemoved hook_node_access. Tested removing this function on our production website and a clean test site - both worked.
Comment #6
jjchinquistIn the log, it states "corrupt at line 58". I followed the EGIT/Drupal guidelines for creating a patch. Did I miss a setting? Thanks, Jeremy
Comment #7
fgmThis is probably because your file is missing a single newline at the end.
This being said, I have noticed your issue, even though I have not had time to look into it yet. There may indeed be a problem with the current node_acccess implementation, but it is here for a reason, and I do not think just removing it improves the situation. It may fix the specific problem you mention, but not others, for example. Did you run the simpletests provided with the module to make sure this broke none of the existing tests ?
Comment #8
jjchinquistHi,
All tests do check out with the change. I ran it twice to be certain.
If you look at test "test0123396And0143680", I do not think you test for that specific case though. I will try to add that in the above patch and test again.
KR,
Jeremy
Comment #9
fgmOK, I just spent some time checking more in depth, and there is indeed a problem, as this implementation is mostly the same as in D6, but the node access logic has been simplified in D7. classified_node_access() is still needed, but only for the limited 'administer ads' permission. Everything else is handled directly by node module.
I will now write a set of tests for these cases: anon, author, other user, and ads admin, to catch them.
Comment #10
fgmThe attached test catches the errors on the unpatched module and passes when the patch is applied. Do you think other test scenarii should be added to validate this ?
Comment #12
fgmPatch is for latest dev, not RC2. Also, note that one of the tests fails on qa.d.o. but always succeeds when testing locally: this is a known problem with qa.d.o. and some types of tests.
Comment #13
fgm#10: hide_unpublished_ads-1432606.patch queued for re-testing.
Comment #15
fgmOK, so with the usual qa.d.o. exception, the tests pass. Unless you suggest some more changes, I will commit this soon (early this week).
Comment #16
fgmCommitted to 7.x-3.x. To be ported to 6.x-3.x if applicable.
Comment #17
fgmFixed in D6 too and improved test committed to D7.
Comment #18
jjchinquistThanks very much! I will report again when I get a chance to test as well.
Comment #19
jjchinquistThe 7.x-3.0 upgrade that was released yesterday worked for our website. Thanks