When an authenticated user votes on a poll, it will be recorded without a timestamp and for anonymous users votes will generate a SQL "integrity constraint violation, duplicate entry" because the IP address is being saved as ''.

Before the DBTNG conversion, we had in poll_vote():

if ($user->uid) {
    db_query('INSERT INTO {poll_vote} (nid, chid, uid, timestamp) VALUES (%d, %d, %d, %d)', $node->nid, $choice, $user->uid, REQUEST_TIME);
  }
  else {
    db_query("INSERT INTO {poll_vote} (nid, chid, hostname, timestamp) VALUES (%d, %d, '%s', %d)", $node->nid, $choice, ip_address(), REQUEST_TIME);
  }

currently we have:

  db_insert('poll_vote')
    ->fields(array(
      'nid' => $node->nid,
      'chid' => $choice,
      'uid' => $user->uid,
      'hostname' => $user->uid ? ip_address() : '',
    ))
    ->execute();

The ip_address() : '' part needs to be reversed, and the timestamp field value of REQUEST_TIME is missing.

CommentFileSizeAuthor
#8 602998-grndlvl-8.patch5.42 KBgrndlvl
#6 602998-grndlvl-6.patch5.29 KBgrndlvl
poll_timestamp.patch463 bytesasimmonds

Comments

sivaji_ganesh_jojodae’s picture

sivaji_ganesh_jojodae commented

This patch looks okay and ready to mark it as RTBC.

kscheirer’s picture

kscheirer
Primary language English
Location Vallejo
commented

Is REQUEST_TIME defined here? If so, this seems like a good fix.

kscheirer’s picture

kscheirer
Primary language English
Location Vallejo
commented
Priority: Normal » Critical
Status: Needs review » Reviewed & tested by the community

patch works for me, thanks!

Bumping this up to critical since this patch also fixes : recording the IP for logged in users will actually prevent an anonymous user from the same IP from being able to vote.

(To test that, open browser A, log in and vote. Then on the same computer open a new browser B, stay anonymous and go to the poll node page. You will be shown the current vote tally, since your IP will already be recorded as having voted.)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Is there a way to add a test for this? Perhaps by force-setting $_SERVER['REMOTE_ADDR'] to something?

grndlvl’s picture

grndlvl commented
Assigned: Unassigned » grndlvl

Currently working on tests for this one.

grndlvl’s picture

grndlvl commented
Assigned: grndlvl » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 602998-grndlvl-6.patch5.29 KB

Alright I have written a test for this.

I was unable to spoof the ip for testing but instead changed the hostname of the poll votes for ones that have a host name.

Test sequence
- User 1(hostname1) Vote
- Check User 1(hostname1) cannot vote again.
- Anonymous(hostname1) Vote
- Check Anonymous(hostname1) cannot vote again.
- User 2(hostname1) Vote
- Change hostname of previous votes.(Simulate user coming from another hostname)
- Anonymous(hostname2) Vote
- Check Anonymous(hostname2) cannot vote again.
- Check User 1(hostname2) cannot vote again.

Not really sure what to call this test... would like some suggestions there.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Needs work 
+++ modules/poll/poll.test	8 Dec 2009 20:26:00 -0000
@@ -349,3 +349,111 @@ class PollJSAddChoice extends DrupalWebT
+    db_update('poll_vote')
+      ->fields(array(
+        'hostname' => '123.456.789.20',
+        ))
+        ->condition('hostname', '', '!=')
+        ->execute();

Indentation is broken. It should be always 2 spaces

+++ modules/poll/poll.test	8 Dec 2009 20:26:00 -0000
@@ -349,3 +349,111 @@ class PollJSAddChoice extends DrupalWebT
+    $this->assertText('Your vote was recorded.', t('%user vote was recorded.', array('%user' => $this->web_user1->name)));

I think we should use t() here, because the messages could be translated, too.

Powered by Dreditor.

The rest looks fine and rtbc

grndlvl’s picture

grndlvl commented
Status: Needs review » Needs work
StatusFileSize
new 602998-grndlvl-8.patch5.42 KB

- Fixing asserts to include t().
- Fixing tabbing of db_update('poll_vote')

Changes applied (#7) from http://drupal.org/node/602998#comment-2511150.

grndlvl’s picture

grndlvl commented
Status: Needs work » Needs review
grndlvl’s picture

grndlvl commented
Status: Needs work » Needs review

It is weird but the Status is "Test request sent" but if you click on the "View details" link it says pass. So it does seem that this patch does pass all the tests.

Thanks,

Jonathan

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Looks fine.

dries’s picture

dries commented 
+++ modules/poll/poll.module	25 Jan 2010 21:34:11 -0000
@@ -720,7 +720,8 @@ function poll_vote($form, &$form_state) 
-      'hostname' => $user->uid ? ip_address() : '',
+      'hostname' => $user->uid ? '' : ip_address(),

Any reason we don't always store the IP address?

JacobSingh’s picture

JacobSingh commented

Although it is not the greatest for data retrieval, we do store that info in watchdog when someone logs in.

I agree though, the inconsistency isn't needed. I'd vote for creating a new issue though so that this critical error doesn't get held up.

-J

dries’s picture

dries commented

I created #702134: Always store IP address for poll votes to track the IP address issue so we can make progress with this critical bug.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD.

Status: Fixed » Closed (fixed)
Issue tags: -Needs tests

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

Issue tags: +Needs tests

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.