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?

CommentFileSizeAuthor
#1 slot_machine.patch6.1 KBebeyrent

Comments

ebeyrent’s picture

ebeyrent commented
Status: Active » Needs review
Issue tags: +Node access
StatusFileSize
new slot_machine.patch6.1 KB

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.

mcarbone’s picture

mcarbone commented

Thanks. I'll take a look at this soon.

mcarbone’s picture

mcarbone commented
Status: Needs review » Fixed

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

ebeyrent’s picture

ebeyrent commented

Awesome - I'll start testing immediately. Thanks for doing this so quickly!

sethcohn’s picture

sethcohn commented
Status: Fixed » Needs review

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?)

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?)

mcarbone’s picture

mcarbone commented
Status: Needs review » Fixed

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.

Status: Fixed » Closed (fixed)

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

guysung’s picture

guysung commented
Title: Doesn't respect node access for queries » _sm_helper_truncate() could be fell into infinite while loop so that page execution time out happend.
Version: 6.x-1.0-beta3 » 6.x-1.0-beta4
Status: Closed (fixed) » Needs work

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.

guysung’s picture

guysung commented
Title: _sm_helper_truncate() could be fell into infinite while loop so that page execution time out happend. » Doesn't respect node access for queries
Version: 6.x-1.0-beta4 » 6.x-1.0-beta3

I posted a new bug report in wrong place. sorry for the confusion.

mcarbone’s picture

mcarbone commented
Status: Needs work » Closed (fixed)