| Project: | Slot Machine |
| Version: | 6.x-1.0-beta3 |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | Node access |
Issue Summary
In slot_machine.utils.inc, in slot_machine_display_content there are queries which return nodes without checking for any sort of access control (usually db_rewrite_sql for listings, but since most of these are single nodes, even a basic node_access would work here potentially, and then return nothing for that topic, while an sql access_check might return a different node for the slot.. which behavior is correct depends on your use case, I think. If featured in the slot is Node A, but it isn't going to be visible to the user, do you want no Node, or Node B which is visible to them?)
I haven't yet audited closely for other places in the code, but a quick grep didn't find any db_rewrite_sql or node_access calls at all.
I have a project that will need access control to work and not show slot content to unauthorized users, will you take a patch that adds this?
Comments
#1
Here's a rough patch. It's gone through almost zero testing, and really needs a bunch of people to take a look at it.
#2
Thanks. I'll take a look at this soon.
#3
OK, I incorporated your changes (there were a few aliases that were slightly off that needed fixing) and added a few more. While I was at it, I cleaned up some more queries, removing SELECT *'s and changing JOINs to INNER JOINs. I tested all of the major functionality and things seem to be working fine. It should appear in the dev releases soon.
I'll leave it in the dev release for a little while before putting out another beta release. In the meantime, if you use views with the current beta release, you will get the node access checks you need.
Here's the Drupal 6 version commit:
http://drupal.org/cvs?commit=235510
#4
Awesome - I'll start testing immediately. Thanks for doing this so quickly!
#5
Reviewed this, and it looks very good overall, thanks for the overall and speedy update to dev!
I see one small problem:
Removed from new dev (incorrectly?)
<?php
function _sm_get_nodetopics($nid) {
$node_topics = array();
if ($nid) {
// removed
if (module_exists('node_comments')) {
$comment_nid = db_result(db_query(db_rewrite_sql("SELECT nid FROM {node_comments} WHERE cid = %d"), $nid));
}
// end removed
$terms = db_query(db_rewrite_sql("SELECT td.name FROM {node} n INNER JOIN {term_node} tn ON n.nid = tn.nid INNER JOIN {term_data} td ON tn.tid = td.tid INNER JOIN {vocabulary} v ON td.vid = v.vid WHERE v.name = 'Topics' AND n.nid = %d", 'td', 'tid'), ($comment_nid ? $comment_nid : $nid));
//without the removed code, $comment_nid is never set.
while ($term = db_fetch_object($terms)) {
$node_topics[] = $term->name;
}
}
return $node_topics;
}
?>
I suspect the removed code still needs tweaked, to be joined to the original node via node table, to help db_rewrite_sql do the right thing (Should you be able to see a comment node when you cannot see the node it's commenting about?)
#6
Thanks for catching that. The node_comment stuff is an artifact from the Fast Company version that I tried to remove entirely, but obviously failed in doing so. Node comments technically already work, but this did some handling of it so that it worked for their specific needs. So I removed all references to comment_nid and committed.
#7
Automatically closed -- issue fixed for 2 weeks with no activity.
#8
Hi, I would say thanks so much for this great module first.
The minor issue I found is that _sm_helper_truncate() could get into infinite while loop.
I know the page execution time out due to the infinite while loop may be barely happened in real world because there should be a space in the body at least. But I've experienced this infinite loop with my dummy body text which doesn't have any space within first 100 characters.
Cheers.
#9
I posted a new bug report in wrong place. sorry for the confusion.
#10