From 66a474ef0980b400bbb39948b560cecb01f059aa Mon Sep 17 00:00:00 2001 From: sun Date: Wed, 3 Apr 2013 14:18:24 +0200 Subject: [PATCH 1/5] - #1938768 by sun: Fixed CAPTCHA form element is not always hidden when it ought to be not visible. --- mollom.module | 26 +++-- mollom.pages.inc | 2 +- tests/mollom.test | 233 ++++++++++++++++++++++++++++++++++++++-- tests/mollom_test_server.module | 28 ++--- 4 files changed, 253 insertions(+), 36 deletions(-) diff --git a/mollom.module b/mollom.module index 30e91b5..a6d74a0 100644 --- a/mollom.module +++ b/mollom.module @@ -1772,19 +1772,20 @@ function mollom_process_mollom($element, $input, &$form_state, $complete_form) { if (!variable_get('mollom_testing_mode', 0)) { $element['captcha']['#attributes']['autocomplete'] = 'off'; } - // For CAPTCHA-only protected forms, retrieve and show an initial CAPTCHA. + // For CAPTCHA-only protected forms: if (!$form_state['mollom']['require_analysis'] && $form_state['mollom']['require_captcha']) { - // Unless the CAPTCHA was solved, display the CAPTCHA. - if (!$form_state['mollom']['passed_captcha']) { + // Retrieve and show an initial CAPTCHA. + if (empty($form_state['process_input'])) { // Enable Form API caching, in order to track the state of the CAPTCHA. $form_state['cache'] = TRUE; // mollom_form_add_captcha() adds the CAPTCHA and disables page caching. mollom_form_add_captcha($element, $form_state); } - // Otherwise, resemble mollom_validate_captcha(). This case is only reached - // in case the form 1) is not cached, 2) fully validated, 3) was submitted, - // and 4) is getting rebuilt; e.g., "Preview" on comment and node forms. - else { + // If the CAPTCHA was solved in a previous submission already, resemble + // mollom_validate_captcha(). This case is only reached in case the form + // 1) is not cached, 2) fully validated, 3) was submitted, and 4) is getting + // rebuilt; e.g., "Preview" on comment and node forms. + if ($form_state['mollom']['passed_captcha']) { $element['captcha']['#solved'] = TRUE; } } @@ -1984,6 +1985,7 @@ function mollom_validate_analysis(&$form, &$form_state) { switch ($result['spamClassification']) { case 'ham': $form_state['mollom']['require_captcha'] = FALSE; + $form['mollom']['captcha']['#access'] = FALSE; mollom_log(array( 'message' => 'Ham: %teaser', 'arguments' => array('%teaser' => $teaser), @@ -1992,6 +1994,7 @@ function mollom_validate_analysis(&$form, &$form_state) { case 'spam': $form_state['mollom']['require_captcha'] = FALSE; + $form['mollom']['captcha']['#access'] = FALSE; if ($form_state['mollom']['discard']) { form_set_error('mollom', t('Your submission has triggered the spam filter and will not be accepted.') . ' ' . _mollom_format_message_falsepositive($form_state, $data)); } @@ -2010,6 +2013,7 @@ function mollom_validate_analysis(&$form, &$form_state) { } else { $form_state['mollom']['require_captcha'] = TRUE; + $form['mollom']['captcha']['#access'] = TRUE; } mollom_log(array( 'message' => 'Unsure: %teaser', @@ -2026,7 +2030,7 @@ function mollom_validate_analysis(&$form, &$form_state) { mollom_log(array( 'message' => 'Unknown: %teaser', 'arguments' => array('%teaser' => $teaser), - )); + ), WATCHDOG_ERROR); break; } } @@ -2693,8 +2697,8 @@ function mollom_get_captcha(&$form_state) { 'type' => $form_state['mollom']['captcha_type'], 'ssl' => (int) (isset($_SERVER['HTTPS']) && $_SERVER['HTTPS'] == 'on'), ); - if (!empty($form_state['mollom']['response']['content']['id'])) { - $data['contentId'] = $form_state['mollom']['response']['content']['id']; + if (!empty($form_state['values']['mollom']['contentId'])) { + $data['contentId'] = $form_state['values']['mollom']['contentId']; } $result = mollom()->createCaptcha($data); @@ -2707,7 +2711,7 @@ function mollom_get_captcha(&$form_state) { if (is_array($result) && isset($result['url'])) { $url = $result['url']; $form_state['mollom']['response'][$key] = $url; - $form_state['mollom']['response']['captcha']['id'] = $result['id']; + $form_state['mollom']['response']['captcha'] = $result; } else { return ''; diff --git a/mollom.pages.inc b/mollom.pages.inc index f733f1f..e0ec300 100644 --- a/mollom.pages.inc +++ b/mollom.pages.inc @@ -41,7 +41,7 @@ function mollom_captcha_js($type, $mollom_build_id, $contentId = NULL) { else { $form_state['mollom'] = array(); if (isset($contentId)) { - $form_state['mollom']['response']['content']['id'] = $contentId; + $form_state['values']['mollom']['contentId'] = $contentId; } } $form_state['mollom']['captcha_type'] = $type; diff --git a/tests/mollom.test b/tests/mollom.test index 24364b2..0c355bd 100644 --- a/tests/mollom.test +++ b/tests/mollom.test @@ -574,8 +574,12 @@ class MollomWebTestCase extends DrupalWebTestCase { * An optional message to test does appear after submission. */ protected function postCorrectCaptcha($url, array $edit = array(), $button, $success_message = '') { + if (isset($url)) { + $this->drupalGet($url); + } + $this->assertCaptchaField(); $edit['mollom[captcha]'] = 'correct'; - $this->drupalPost($url, $edit, $button); + $this->drupalPost(NULL, $edit, $button); $this->assertNoCaptchaField(); $this->assertNoText($this->incorrect_message); if ($success_message) { @@ -596,12 +600,14 @@ class MollomWebTestCase extends DrupalWebTestCase { * An optional message to test does not appear after submission. */ protected function postIncorrectCaptcha($url, array $edit = array(), $button, $success_message = '') { + if (isset($url)) { + $this->drupalGet($url); + } + $this->assertCaptchaField(); $edit['mollom[captcha]'] = 'incorrect'; $before_url = $this->getUrl(); - $this->drupalPost($url, $edit, $button); - if ($this->getUrl() == $before_url) { - $this->assertCaptchaField(); - } + $this->drupalPost(NULL, $edit, $button); + $this->assertCaptchaField(); $this->assertText($this->incorrect_message); if ($success_message) { $this->assertNoText($success_message); @@ -625,7 +631,7 @@ class MollomWebTestCase extends DrupalWebTestCase { protected function assertSpamSubmit($url, array $spam_fields, array $edit = array(), $button, $success_message = '') { $edit += array_fill_keys($spam_fields, 'spam'); $this->drupalPost($url, $edit, $button); - $this->assertNoCaptchaField($url); + $this->assertNoCaptchaField(); $this->assertText($this->spam_message); if ($success_message) { $this->assertNoText($success_message); @@ -673,11 +679,8 @@ class MollomWebTestCase extends DrupalWebTestCase { */ protected function assertUnsureSubmit($url, array $unsure_fields, array $edit = array(), $button, $success_message = '') { $edit += array_fill_keys($unsure_fields, 'unsure'); - $before_url = $this->getUrl(); $this->drupalPost($url, $edit, $button); - if ($this->getUrl() == $before_url) { - $this->assertCaptchaField(); - } + $this->assertCaptchaField(); $this->assertText($this->unsure_message); if ($success_message) { $this->assertNoText($success_message); @@ -3710,6 +3713,10 @@ class MollomDataCRUDTestCase extends MollomWebTestCase { class MollomAnalysisTestCase extends MollomWebTestCase { protected $disableDefaultSetup = TRUE; + // Re-route Mollom communication to this testing site. + // @todo Remove this when Testing API is fixed. + protected $mollomClass = 'MollomDrupalTestLocal'; + public static function getInfo() { return array( 'name' => 'Text analysis', @@ -3727,7 +3734,211 @@ class MollomAnalysisTestCase extends MollomWebTestCase { 'access administration pages', 'administer mollom', )); - $this->web_user = $this->drupalCreateUser(array('access content')); + } + + function testUnsure() { + $methods = get_class_methods($this); + foreach ($methods as $method) { + if (substr($method, 0, 7) === 'subtest') { + //debug($method); + $this->$method(); + } + } + } + + /** + * Tests basic unsure submission flow. + */ + function subtestUnsureCorrect() { + $this->setProtection('mollom_test_form', MOLLOM_MODE_ANALYSIS); + + $this->drupalGet('mollom-test/form'); + $this->assertNoCaptchaField(); + $edit = array( + 'title' => 'unsure', + ); + $this->drupalPost(NULL, $edit, 'Submit'); + $this->assertResponseIDInForm('contentId', TRUE); + $this->assertResponseIDInForm('captchaId', TRUE); + $this->assertCaptchaField(); + + $edit = array( + 'title' => 'unsure unsure', + 'mollom[captcha]' => 'correct', + ); + $this->drupalPost(NULL, $edit, 'Submit'); + $this->assertNoCaptchaField(); + $this->assertTestSubmitData(); + } + + /** + * Tests unsure post with repetitive incorrect CAPTCHA solution. + * + * @todo CAPTCHA ID should stay the same. + * @see http://drupal.org/node/1959904 + */ + function subtestUnsureIncorrect() { + $this->setProtection('mollom_test_form', MOLLOM_MODE_ANALYSIS); + + $this->drupalGet('mollom-test/form'); + $this->assertNoCaptchaField(); + $edit = array( + 'title' => 'unsure', + ); + $this->drupalPost(NULL, $edit, 'Submit'); + $this->assertResponseIDInForm('contentId', TRUE); + $this->assertResponseIDInForm('captchaId', TRUE); + $this->assertCaptchaField(); + + $edit = array(); + $this->drupalPost(NULL, $edit, 'Submit'); + $this->assertResponseIDInForm('contentId'); +// $this->assertResponseIDInForm('captchaId'); + $this->assertCaptchaField(); + $edit = array( + 'mollom[captcha]' => 'incorrect', + ); + $this->drupalPost(NULL, $edit, 'Submit'); + $this->assertResponseIDInForm('contentId'); +// $this->assertResponseIDInForm('captchaId'); + $this->assertCaptchaField(); + + $edit = array( + 'mollom[captcha]' => 'correct', + ); + $this->drupalPost(NULL, $edit, 'Submit'); + $this->assertNoCaptchaField(); + $this->assertTestSubmitData(); + } + + /** + * Tests unsure post with other validation errors. + */ + function subtestUnsureValidation() { + $this->setProtection('mollom_test_form', MOLLOM_MODE_ANALYSIS); + + // The 'title' field is required. Omit its value to verify that the CAPTCHA + // can be solved, repetitive form validations do not show a CAPTCHA again, + // and the post can finally be submitted by providing a title. + $this->drupalGet('mollom-test/form'); + $this->assertNoCaptchaField(); + $edit = array( + 'body' => 'unsure', + ); + $this->drupalPost(NULL, $edit, 'Submit'); + $this->assertResponseIDInForm('contentId', TRUE); + $this->assertResponseIDInForm('captchaId', TRUE); + $this->assertCaptchaField(); + $edit = array( + 'body' => 'unsure unsure', + 'mollom[captcha]' => 'correct', + ); + $this->drupalPost(NULL, $edit, 'Submit'); + $this->assertResponseIDInForm('contentId'); + $this->assertResponseIDInForm('captchaId'); + $this->assertNoCaptchaField(); + $edit = array( + 'body' => 'unsure unsure unsure', + ); + $this->drupalPost(NULL, $edit, 'Submit'); + $this->assertResponseIDInForm('contentId'); + $this->assertResponseIDInForm('captchaId'); + $this->assertNoCaptchaField(); + $edit = array( + 'title' => 'unsure', + 'body' => 'unsure', + ); + $this->drupalPost(NULL, $edit, 'Submit'); + $this->assertNoCaptchaField(); + $this->assertTestSubmitData(); + } + + /** + * Tests unsure posts turning into ham. + * + * @todo CAPTCHA ID should stay the same. + * @see http://drupal.org/node/1959904 + */ + function subtestUnsureHam() { + $this->setProtection('mollom_test_form', MOLLOM_MODE_ANALYSIS); + + $this->drupalGet('mollom-test/form'); + $this->assertNoCaptchaField(); + // Posting ham as title would submit the form, so post as body instead. + $edit = array( + 'body' => 'unsure', + ); + $this->drupalPost(NULL, $edit, 'Submit'); + $this->assertResponseIDInForm('contentId', TRUE); + $this->assertResponseIDInForm('captchaId', TRUE); + $this->assertCaptchaField(); + // Turn the post into ham. + $edit = array( + 'body' => 'ham', + ); + $this->drupalPost(NULL, $edit, 'Submit'); + $this->assertResponseIDInForm('contentId'); +// $this->assertResponseIDInForm('captchaId'); + $this->assertNoCaptchaField(); + // Turn the post back into unsure. + $edit = array( + 'body' => 'unsure', + ); + $this->drupalPost(NULL, $edit, 'Submit'); + $this->assertResponseIDInForm('contentId'); +// $this->assertResponseIDInForm('captchaId'); + $this->assertCaptchaField(); + + $edit = array( + 'title' => 'irrelevant', + 'mollom[captcha]' => 'correct', + ); + $this->drupalPost(NULL, $edit, 'Submit'); + $this->assertNoCaptchaField(); + $this->assertTestSubmitData(); + } + + /** + * Tests unsure posts turning into spam. + * + * @todo CAPTCHA ID should stay the same. + * @see http://drupal.org/node/1959904 + */ + function subtestUnsureSpam() { + $this->setProtection('mollom_test_form', MOLLOM_MODE_ANALYSIS); + + $this->drupalGet('mollom-test/form'); + $this->assertNoCaptchaField(); + $edit = array( + 'title' => 'unsure', + ); + $this->drupalPost(NULL, $edit, 'Submit'); + $this->assertResponseIDInForm('contentId', TRUE); + $this->assertResponseIDInForm('captchaId', TRUE); + $this->assertCaptchaField(); + // Turn the post into spam. + $edit = array( + 'title' => 'spam', + ); + $this->drupalPost(NULL, $edit, 'Submit'); + $this->assertResponseIDInForm('contentId'); +// $this->assertResponseIDInForm('captchaId'); + $this->assertNoCaptchaField(); + // Turn the post back into unsure. + $edit = array( + 'title' => 'unsure', + ); + $this->drupalPost(NULL, $edit, 'Submit'); + $this->assertResponseIDInForm('contentId'); +// $this->assertResponseIDInForm('captchaId'); + $this->assertCaptchaField(); + + $edit = array( + 'mollom[captcha]' => 'correct', + ); + $this->drupalPost(NULL, $edit, 'Submit'); + $this->assertNoCaptchaField(); + $this->assertTestSubmitData(); } /** diff --git a/tests/mollom_test_server.module b/tests/mollom_test_server.module index de38363..5af917f 100644 --- a/tests/mollom_test_server.module +++ b/tests/mollom_test_server.module @@ -500,14 +500,15 @@ function mollom_test_server_check_content($data) { } // Lastly, take a forced 'unsure' into account. elseif (strpos($post, 'unsure') !== FALSE) { - // Differentiate between binary unsure mode. + // Enabled unsure mode. if (!isset($data['unsure']) || $data['unsure']) { $spam = TRUE; $ham = TRUE; } + // Binary mode. else { - $spam = TRUE; - $ham = FALSE; + $spam = FALSE; + $ham = TRUE; } } // Check blacklist. @@ -540,9 +541,9 @@ function mollom_test_server_check_content($data) { } // In case a previous spam check was unsure and a CAPTCHA was solved, the // result is supposed to be ham - unless the new content is spam. - if ($response['spamClassification'] != 'spam') { - $captcha_sessions = variable_get('mollom_test_server_check_captcha_sessions', array()); - if (!empty($data['captchaId']) && !empty($captcha_sessions[$data['captchaId']])) { + if (!empty($data['id']) && $response['spamClassification'] == 'unsure') { + $content_captchas = variable_get('mollom_test_server_content_captcha', array()); + if (!empty($content_captchas[$data['id']])) { $response['spamScore'] = 0.0; $response['spamClassification'] = 'ham'; } @@ -642,7 +643,7 @@ function mollom_test_server_check_content_blacklist($string, $blacklist, $reason } /** - * API callback for mollom.getImageCaptcha to fetch a CATPCHA image. + * API callback for mollom.getImageCaptcha to fetch a CAPTCHA image. */ function mollom_test_server_get_captcha($data) { $response = array(); @@ -665,8 +666,6 @@ function mollom_test_server_get_captcha($data) { /** * API callback for mollom.checkCaptcha to validate a CAPTCHA response. - * - * @todo Add support for 'redirect' and 'refresh' values. */ function mollom_test_server_check_captcha($data) { $response = array(); @@ -684,13 +683,16 @@ function mollom_test_server_check_captcha($data) { if (!isset($storage[$captchaId])) { return MENU_NOT_FOUND; } - $storage[$captchaId] = $data; + $storage[$captchaId] = array_merge($storage[$captchaId], $data); $response['id'] = $captchaId; variable_set('mollom_test_server_captcha', $storage); - $captcha_sessions = variable_get('mollom_test_server_check_captcha_sessions', array()); - $captcha_sessions[$captchaId] = $response['solved']; - variable_set('mollom_test_server_check_captcha_sessions', $captcha_sessions); + if (isset($storage[$captchaId]['contentId'])) { + $contentId = $storage[$captchaId]['contentId']; + $content_captchas = variable_get('mollom_test_server_content_captcha', array()); + $content_captchas[$contentId] = $response['solved']; + variable_set('mollom_test_server_content_captcha', $content_captchas); + } return $response; } -- 1.7.11.msysgit.1 From 59b23efb996ea1d72f9c497ed9f0bd92dfc8c972 Mon Sep 17 00:00:00 2001 From: sun Date: Thu, 4 Apr 2013 20:11:43 +0200 Subject: [PATCH 2/5] - #1938768 by sun: Fixed text analysis validation does not always (re-)trigger CAPTCHA when required. --- mollom.module | 66 ++++++--------- tests/mollom.test | 178 ++++++++++++++++++++++++++++++++++++---- tests/mollom_test_server.module | 9 +- 3 files changed, 193 insertions(+), 60 deletions(-) diff --git a/mollom.module b/mollom.module index a6d74a0..080bc40 100644 --- a/mollom.module +++ b/mollom.module @@ -1725,11 +1725,6 @@ function mollom_process_mollom($element, $input, &$form_state, $complete_form) { ); $element['captchaId'] = array( '#type' => 'hidden', - // For CAPTCHA-only protected forms, the most recent CAPTCHA ID is tracked - // in $form_state. A new CAPTCHA ID may be injected by mollom_get_captcha(). - // Furthermore, the CAPTCHA type can be switched between image and audio, - // and for each switch, there is a new CAPTCHA ID. - '#default_value' => isset($form_state['mollom']['response']['captcha']['id']) ? $form_state['mollom']['response']['captcha']['id'] : '', '#attributes' => array('class' => 'mollom-captcha-id'), ); $element['form_id'] = array( @@ -2013,6 +2008,7 @@ function mollom_validate_analysis(&$form, &$form_state) { } else { $form_state['mollom']['require_captcha'] = TRUE; + $form_state['mollom']['passed_captcha'] = FALSE; $form['mollom']['captcha']['#access'] = TRUE; } mollom_log(array( @@ -2199,6 +2195,10 @@ function mollom_pre_render_mollom($element) { // UX: Empty the CAPTCHA field value, as the user has to re-enter a new one. $element['captcha']['#value'] = ''; + // DX: Debugging helpers. +// $element['captcha']['#suffix'] = 'contentId: ' . $element['contentId']['#value']; +// $element['captcha']['#suffix'] .= '
captchaId: ' . $element['captchaId']['#value']; + return $element; } @@ -2258,19 +2258,8 @@ function mollom_form_submit($form, &$form_state) { // We only consider non-empty and non-zero values as valid entity ids. if (!empty($values['postId'])) { // Save the Mollom session data. - $response = array(); - if (isset($form_state['mollom']['response']['content'])) { - $response += $form_state['mollom']['response']['content']; - $response['contentId'] = $form_state['mollom']['response']['content']['id']; - } - if (isset($form_state['mollom']['response']['captcha'])) { - $response += $form_state['mollom']['response']['captcha']; - $response['captchaId'] = $form_state['mollom']['response']['captcha']['id']; - } - $data = (object) $response; - $data->entity = $form_state['mollom']['entity']; + $data = (object) $form_state['values']['mollom']; $data->id = $values['postId']; - $data->form_id = $form_state['mollom']['form_id']; // Set the moderation flag for forms accepting bad posts. $data->moderate = $form_state['mollom']['require_moderation']; $form_state['mollom']['data'] = mollom_data_save($data); @@ -2691,34 +2680,29 @@ function mollom_content_extra_fields($type_name) { * The markup of the CAPTCHA HTML. */ function mollom_get_captcha(&$form_state) { - $key = 'captcha_url_' . $form_state['mollom']['captcha_type']; - if (empty($form_state['mollom']['response'][$key])) { - $data = array( - 'type' => $form_state['mollom']['captcha_type'], - 'ssl' => (int) (isset($_SERVER['HTTPS']) && $_SERVER['HTTPS'] == 'on'), - ); - if (!empty($form_state['values']['mollom']['contentId'])) { - $data['contentId'] = $form_state['values']['mollom']['contentId']; - } - $result = mollom()->createCaptcha($data); + // @todo Re-use existing CAPTCHA URL when the Mollom server response for + // verifying a CAPTCHA solution returns the existing URL. + $data = array( + 'type' => $form_state['mollom']['captcha_type'], + 'ssl' => (int) (isset($_SERVER['HTTPS']) && $_SERVER['HTTPS'] == 'on'), + ); + if (!empty($form_state['values']['mollom']['contentId'])) { + $data['contentId'] = $form_state['values']['mollom']['contentId']; + } + $result = mollom()->createCaptcha($data); - // Add a log message to prevent the request log from appearing without a - // message on CAPTCHA-only protected forms. - mollom_log(array( - 'message' => 'Retrieved new CAPTCHA', - ), WATCHDOG_DEBUG); + // Add a log message to prevent the request log from appearing without a + // message on CAPTCHA-only protected forms. + mollom_log(array( + 'message' => 'Retrieved new CAPTCHA', + ), WATCHDOG_INFO); - if (is_array($result) && isset($result['url'])) { - $url = $result['url']; - $form_state['mollom']['response'][$key] = $url; - $form_state['mollom']['response']['captcha'] = $result; - } - else { - return ''; - } + if (is_array($result) && isset($result['url'])) { + $url = $result['url']; + $form_state['mollom']['response']['captcha'] = $result; } else { - $url = $form_state['mollom']['response'][$key]; + return ''; } // @todo Convert these to actual theme functions? diff --git a/tests/mollom.test b/tests/mollom.test index 0c355bd..d03d327 100644 --- a/tests/mollom.test +++ b/tests/mollom.test @@ -329,12 +329,16 @@ class MollomWebTestCase extends DrupalWebTestCase { * (optional) Whether to enforce creation of new API keys. New keys are only * created if MollomWebTestCase::$createKeys or respectively the * 'mollom_testing_create_keys' variable is set to TRUE. Defaults to FALSE. + * @param bool $once + * (optional) Whether to disable the 'mollom_testing_create_keys' variable + * after the first call (and thus omit API key verifications on every page + * request). Defaults to FALSE; i.e., API keys are verified repetitively. * * @see MollomWebTestCase::$createKeys * @see MollomDrupalTest::__construct() * @see MollomDrupalTest::createKeys() */ - protected function setKeys($force = FALSE) { + protected function setKeys($force = FALSE, $once = FALSE) { // Instantiate a Mollom client class. // Depending on MollomWebTestCase::$createKeys and ultimately the // 'mollom_testing_create_keys' variable, MollomDrupalTest::__construct() @@ -355,6 +359,9 @@ class MollomWebTestCase extends DrupalWebTestCase { // keys still exists in the test, but the configuration is gone. $this->mollom->saveKeys(); } + if ($once) { + variable_set('mollom_testing_create_keys', FALSE); + } } /** @@ -2911,14 +2918,14 @@ class MollomCommentFormTestCase extends MollomWebTestCase { // required field. $this->postIncorrectCaptcha(NULL, array(), t('Preview')); $this->assertText(t('Comment field is required.')); - $this->assertResponseIDInForm('captchaId'); + $this->assertResponseIDInForm('captchaId', TRUE); $this->assertNoPrivacyLink(); // Try to submit a correct answer for the CAPTCHA, still without required // field value. $this->postCorrectCaptcha(NULL, array(), t('Preview')); $this->assertText(t('Comment field is required.')); - $captchaId = $this->assertResponseIDInForm('captchaId'); + $captchaId = $this->assertResponseIDInForm('captchaId', TRUE); $this->assertNoPrivacyLink(); // Finally, we should be able to submit a comment. @@ -3727,13 +3734,15 @@ class MollomAnalysisTestCase extends MollomWebTestCase { function setUp() { parent::setUp(array('mollom', 'mollom_test')); - $this->setKeys(); + $this->setKeys(TRUE, TRUE); $this->assertValidKeys(); $this->admin_user = $this->drupalCreateUser(array( 'access administration pages', 'administer mollom', )); + + $this->setProtection('mollom_test_form', MOLLOM_MODE_ANALYSIS); } function testUnsure() { @@ -3750,8 +3759,6 @@ class MollomAnalysisTestCase extends MollomWebTestCase { * Tests basic unsure submission flow. */ function subtestUnsureCorrect() { - $this->setProtection('mollom_test_form', MOLLOM_MODE_ANALYSIS); - $this->drupalGet('mollom-test/form'); $this->assertNoCaptchaField(); $edit = array( @@ -3778,8 +3785,6 @@ class MollomAnalysisTestCase extends MollomWebTestCase { * @see http://drupal.org/node/1959904 */ function subtestUnsureIncorrect() { - $this->setProtection('mollom_test_form', MOLLOM_MODE_ANALYSIS); - $this->drupalGet('mollom-test/form'); $this->assertNoCaptchaField(); $edit = array( @@ -3815,8 +3820,6 @@ class MollomAnalysisTestCase extends MollomWebTestCase { * Tests unsure post with other validation errors. */ function subtestUnsureValidation() { - $this->setProtection('mollom_test_form', MOLLOM_MODE_ANALYSIS); - // The 'title' field is required. Omit its value to verify that the CAPTCHA // can be solved, repetitive form validations do not show a CAPTCHA again, // and the post can finally be submitted by providing a title. @@ -3860,8 +3863,6 @@ class MollomAnalysisTestCase extends MollomWebTestCase { * @see http://drupal.org/node/1959904 */ function subtestUnsureHam() { - $this->setProtection('mollom_test_form', MOLLOM_MODE_ANALYSIS); - $this->drupalGet('mollom-test/form'); $this->assertNoCaptchaField(); // Posting ham as title would submit the form, so post as body instead. @@ -3905,8 +3906,6 @@ class MollomAnalysisTestCase extends MollomWebTestCase { * @see http://drupal.org/node/1959904 */ function subtestUnsureSpam() { - $this->setProtection('mollom_test_form', MOLLOM_MODE_ANALYSIS); - $this->drupalGet('mollom-test/form'); $this->assertNoCaptchaField(); $edit = array( @@ -3942,6 +3941,155 @@ class MollomAnalysisTestCase extends MollomWebTestCase { } /** + * Asserts a successful mollom_test_form submission. + * + * @param $old_mid + * (optional) The existing test record id to assert. + */ + protected function assertTestSubmitData($old_mid = NULL) { + $this->assertText('Successful form submission.'); + $mid = $this->getFieldValueByName('mid'); + if (isset($old_mid)) { + $this->assertSame('Test record id', $mid, $old_mid); + } + else { + $this->assertTrue($mid > 0, t('Test record id @id found.', array('@id' => $mid))); + } + return $mid; + } +} + +/** + * Tests text analysis with page cache. + */ +class MollomAnalysisPageCacheTestCase extends MollomAnalysisTestCase { + public static function getInfo() { + return array( + 'name' => 'Text analysis (page cache)', + 'description' => 'Tests text analysis with page cache.', + 'group' => 'Mollom', + ); + } + + function setUp() { + parent::setUp(); + $this->enablePageCache(); + + // Prime the page + form cache. + $this->drupalGet('mollom-test/form'); + $this->assertText('Views: 1'); + $this->drupalGet('mollom-test/form'); + $this->assertText('Views: 1'); + $this->assertUnsureSubmit(NULL, array('title'), array(), 'Submit'); + $this->drupalGet('mollom-test/form'); + $this->assertText('Views: 1'); + } +} + +/** + * Tests text analysis with page and form cache. + */ +class MollomAnalysisPageFormCacheTestCase extends MollomAnalysisTestCase { + public static function getInfo() { + return array( + 'name' => 'Text analysis (page + form cache)', + 'description' => 'Tests text analysis with page and form cache.', + 'group' => 'Mollom', + ); + } + + function setUp() { + parent::setUp(); + $this->enablePageCache(); + variable_set('mollom_test.form.cache', TRUE); + + // Prime the page + form cache. + $this->drupalGet('mollom-test/form'); + $this->assertText('Views: 1'); + $this->drupalGet('mollom-test/form'); + $this->assertText('Views: 1'); + $this->assertUnsureSubmit(NULL, array('title'), array(), 'Submit'); + $this->drupalGet('mollom-test/form'); + $this->assertText('Views: 1'); + } +} + +/** + * Tests text analysis as authenticated user. + */ +class MollomAnalysisAuthenticatedTestCase extends MollomAnalysisTestCase { + public static function getInfo() { + return array( + 'name' => 'Text analysis (authenticated)', + 'description' => 'Tests text analysis as authenticated user.', + 'group' => 'Mollom', + ); + } + + function setUp() { + parent::setUp(); + $this->web_user = $this->drupalCreateUser(array()); + $this->drupalLogin($this->web_user); + } +} + +/** + * Tests text analysis as authenticated user with enabled page and form cache. + */ +class MollomAnalysisAuthenticatedPageFormCacheTestCase extends MollomAnalysisTestCase { + public static function getInfo() { + return array( + 'name' => 'Text analysis (authenticated + page + form cache)', + 'description' => 'Tests text analysis as authenticated user with enabled page and form cache.', + 'group' => 'Mollom', + ); + } + + function setUp() { + parent::setUp(); + $this->enablePageCache(); + variable_set('mollom_test.form.cache', TRUE); + + // Prime the page + form cache. + $this->drupalGet('mollom-test/form'); + $this->assertText('Views: 1'); + $this->drupalGet('mollom-test/form'); + $this->assertText('Views: 1'); + $this->assertUnsureSubmit(NULL, array('title'), array(), 'Submit'); + $this->drupalGet('mollom-test/form'); + $this->assertText('Views: 1'); + + $this->web_user = $this->drupalCreateUser(array()); + $this->drupalLogin($this->web_user); + } +} + +/** + * Tests text analysis functionality. + */ +class MollomAnalysisOptionsTestCase extends MollomWebTestCase { + protected $disableDefaultSetup = TRUE; + + public static function getInfo() { + return array( + 'name' => 'Text analysis options', + 'description' => 'Tests text analysis binary mode, retaining unsure/spam.', + 'group' => 'Mollom', + ); + } + + function setUp() { + parent::setUp(array('mollom', 'mollom_test')); + $this->setKeys(); + $this->assertValidKeys(); + + $this->admin_user = $this->drupalCreateUser(array( + 'access administration pages', + 'administer mollom', + )); + } + + /** * Tests binary unsure mode. */ function testUnsureBinary() { @@ -4163,7 +4311,7 @@ class MollomAnalysisTestCase extends MollomWebTestCase { /** * Tests basic text analysis functionality with enabled caching. */ -class MollomAnalysisPageCacheTestCase extends MollomWebTestCase { +class MollomAnalysisPageCachingTestCase extends MollomWebTestCase { protected $disableDefaultSetup = TRUE; diff --git a/tests/mollom_test_server.module b/tests/mollom_test_server.module index 5af917f..da13df5 100644 --- a/tests/mollom_test_server.module +++ b/tests/mollom_test_server.module @@ -646,19 +646,20 @@ function mollom_test_server_check_content_blacklist($string, $blacklist, $reason * API callback for mollom.getImageCaptcha to fetch a CAPTCHA image. */ function mollom_test_server_get_captcha($data) { - $response = array(); + $captchaId = (!empty($data['id']) ? $data['id'] : md5(mt_rand())); + $response = array( + 'id' => $captchaId, + ); // Return a HTTPS URL if 'ssl' parameter was passed. $base_url = $GLOBALS['base_url']; if (!empty($data['ssl'])) { $base_url = str_replace('http', 'https', $base_url); } - $response['url'] = $base_url . '/' . drupal_get_path('module', 'mollom') . '/images/powered-by-mollom-2.gif'; + $response['url'] = $base_url . '/' . drupal_get_path('module', 'mollom') . '/images/powered-by-mollom-2.gif?captchaId=' . $captchaId; $storage = variable_get('mollom_test_server_captcha', array()); - $captchaId = (!empty($data['id']) ? $data['id'] : md5(mt_rand())); $storage[$captchaId] = $data; - $response['id'] = $captchaId; variable_set('mollom_test_server_captcha', $storage); return $response; -- 1.7.11.msysgit.1 From 2e63ba784cbd5688afd9d24d093970f171407891 Mon Sep 17 00:00:00 2001 From: sun Date: Thu, 4 Apr 2013 22:48:04 +0200 Subject: [PATCH 3/5] - #1938768 by sun: Mimic D7's $form_state['complete form'] to allow validation handlers to alter the form structure. --- mollom.css | 2 +- mollom.module | 130 ++++++++++++++++-------------------------------------- tests/mollom.test | 9 +--- 3 files changed, 42 insertions(+), 99 deletions(-) diff --git a/mollom.css b/mollom.css index cef4eaf..0b67255 100644 --- a/mollom.css +++ b/mollom.css @@ -5,6 +5,6 @@ /* Help themes to properly display Mollom's log messages. */ .dblog-event pre, -#simpletest-result-form table td { +#simpletest-result-form table td pre { white-space: pre-wrap; } diff --git a/mollom.module b/mollom.module index 080bc40..f366fdd 100644 --- a/mollom.module +++ b/mollom.module @@ -675,6 +675,9 @@ function mollom_form_alter(&$form, &$form_state, $form_id) { '#weight' => $weight, '#tree' => TRUE, ); + // D6: Mimic D7's $form_state['complete form'] to allow validation + // handlers to access and alter the 'mollom' element. + $form_state['complete form'] = &$form; // Add Mollom form validation handlers. // Form-level validation handlers are required, since we need access to @@ -1534,7 +1537,7 @@ function _mollom_format_message_falsepositive($form_state, $data) { * new CAPTCHA, but without the previously entered value. * - In short, roughly: * - Form construction: Nothing. - * - Form processing: Nothing. + * - Form processing: Output a CAPTCHA for CAPTCHA-only protected forms. * - Form validation: Perform validation and alterations based on validation. * * This, however, is not possible due to various bugs in Drupal core. @@ -1551,18 +1554,9 @@ function _mollom_format_message_falsepositive($form_state, $data) { * - We need our own {cache_mollom} table as replacement for native form * caching, as well as our own logic to validate a submitted 'session_id' * ('form_build_id') against forms and users. - * - We need to perform form alterations during form rendering, where - * $form_state is no longer available. To make this possible, we leverage the - * fact that an element property that is a reference to a key in $form_state - * (which in itself is passed by reference) persists on to the rendering - * layer. The essential part is: - * @code - * $element['#mollom'] = &$form_state['mollom']; - * @endcode - * - Since we cannot alter elements in the form structure during form - * validation, this reference already needs to be set up during form - * processing (in a #process callback), while everything else lives in form - * validation handlers (unless it needs to add or alter the form structure). + * - We need to alter the $form structure during form validation. To make this + * possible, we mimic D7 and assign $form to $form_state['complete form'] by + * reference in mollom_form_alter(). * * @see mollom_form_alter() */ @@ -1655,19 +1649,6 @@ function mollom_process_mollom($element, $input, &$form_state, $complete_form) { // Setup initial Mollom session and form information. $form_state += array('mollom' => array()); $form_state['mollom'] += array( - // mollom_build_id works similar to form_build_id in D7; it is an ID that is - // unique for each build of a form. However, unlike form_build_id in D6, it - // does not change across POSTs, regardless of whether form storage/caching - // is enabled or not. Mollom uses the mollom_build_id as cache identifier to - // track the state of a form session/submission, which is required to - // re-inject prior knowledge about the submission (such as a "unsure" text - // analysis result). Although generated whenever this function is called, - // $form_state['mollom'] including mollom_build_id will already be - // prepopulated by mollom_process_mollom_form_state() and thus not overidden - // here. If mollom_build_id is manually removed from the form, the form's - // state is merely lost and the form submission needs to go through Mollom - // validation from scratch. - 'mollom_build_id' => md5(uniqid(mt_rand(), TRUE)), // Only TRUE if the form is protected by text analysis. 'require_analysis' => $element['#mollom_form']['mode'] == MOLLOM_MODE_ANALYSIS, // Becomes TRUE whenever a CAPTCHA needs to be solved. @@ -1699,10 +1680,24 @@ function mollom_process_mollom($element, $input, &$form_state, $complete_form) { drupal_add_js(drupal_get_path('module', 'mollom') . '/mollom.js'); // D6: - $element['mollom_build_id'] = array( - '#type' => 'hidden', - '#value' => $form_state['mollom']['mollom_build_id'], - ); + // mollom_build_id works similar to form_build_id in D7; it is an ID that is + // unique for each build of a form. However, unlike form_build_id in D6, it + // does not change across POSTs, regardless of whether form storage/caching + // is enabled or not. Mollom uses the mollom_build_id as cache identifier to + // track the state of a form session/submission, which is required to + // re-inject prior knowledge about the submission (such as a "unsure" text + // analysis result). Although generated whenever this function is called, + // $form_state['mollom'] including mollom_build_id will already be + // prepopulated by mollom_process_mollom_form_state() and thus not overidden + // here. If mollom_build_id is manually removed from the form, the form's + // state is merely lost and the form submission needs to go through Mollom + // validation from scratch. + if (!$form_state['mollom']['require_analysis']) { + $element['mollom_build_id'] = array( + '#type' => 'hidden', + '#value' => !empty($form_state['mollom']['mollom_build_id']) ? $form_state['mollom']['mollom_build_id'] : md5(uniqid(mt_rand(), TRUE)), + ); + } // Add the Mollom session data elements. // These elements resemble the {mollom} database schema. The form validation @@ -1807,12 +1802,6 @@ function mollom_process_mollom($element, $input, &$form_state, $complete_form) { ), ); - // Make Mollom form and session information available to #pre_render callback. - // This must be assigned by reference. It is the essential "communication - // layer" between form API and the rendering system. Any modifications to - // $form_state['mollom'] will be carried over to the element for rendering. - $element['#mollom'] = &$form_state['mollom']; - // Make Mollom form and session information available to entirely different // functions. // @see mollom_mail_alter() @@ -1945,7 +1934,7 @@ function mollom_validate_analysis(&$form, &$form_state) { // Ensure the latest content ID is output as value. // form_set_value() is effectless, as this is not a element-level but a // form-level validation handler. - $form['mollom']['contentId']['#value'] = $result['contentId']; + $form_state['complete form']['mollom']['contentId']['#value'] = $result['contentId']; // Prepare watchdog message teaser text. $teaser = '--'; @@ -1980,7 +1969,7 @@ function mollom_validate_analysis(&$form, &$form_state) { switch ($result['spamClassification']) { case 'ham': $form_state['mollom']['require_captcha'] = FALSE; - $form['mollom']['captcha']['#access'] = FALSE; + $form_state['complete form']['mollom']['captcha']['#access'] = FALSE; mollom_log(array( 'message' => 'Ham: %teaser', 'arguments' => array('%teaser' => $teaser), @@ -1989,7 +1978,7 @@ function mollom_validate_analysis(&$form, &$form_state) { case 'spam': $form_state['mollom']['require_captcha'] = FALSE; - $form['mollom']['captcha']['#access'] = FALSE; + $form_state['complete form']['mollom']['captcha']['#access'] = FALSE; if ($form_state['mollom']['discard']) { form_set_error('mollom', t('Your submission has triggered the spam filter and will not be accepted.') . ' ' . _mollom_format_message_falsepositive($form_state, $data)); } @@ -2009,7 +1998,7 @@ function mollom_validate_analysis(&$form, &$form_state) { else { $form_state['mollom']['require_captcha'] = TRUE; $form_state['mollom']['passed_captcha'] = FALSE; - $form['mollom']['captcha']['#access'] = TRUE; + $form_state['complete form']['mollom']['captcha']['#access'] = TRUE; } mollom_log(array( 'message' => 'Unsure: %teaser', @@ -2046,8 +2035,7 @@ function mollom_validate_captcha(&$form, &$form_state) { // If there is no CAPTCHA ID yet, retrieve one and throw an error. if (empty($form_state['values']['mollom']['captchaId'])) { - // D6: Not possible here; deferred to mollom_pre_render_mollom(). - //mollom_form_add_captcha($form['mollom'], $form_state); + mollom_form_add_captcha($form_state['complete form']['mollom'], $form_state); form_error($form['mollom']['captcha'], t('To complete this form, please complete the word verification below.')); return; } @@ -2059,8 +2047,8 @@ function mollom_validate_captcha(&$form, &$form_state) { // primary 'require_captcha' condition will not be TRUE unless needed in the // first place. if ($form_state['mollom']['passed_captcha']) { - $form['mollom']['captcha']['#access'] = FALSE; - $form['mollom']['captcha']['#solved'] = TRUE; + $form_state['complete form']['mollom']['captcha']['#access'] = FALSE; + $form_state['complete form']['mollom']['captcha']['#solved'] = TRUE; return; } @@ -2104,12 +2092,12 @@ function mollom_validate_captcha(&$form, &$form_state) { // Ensure the latest CAPTCHA ID is output as value. // form_set_value() is effectless, as this is not a element-level but a // form-level validation handler. - $form['mollom']['captchaId']['#value'] = $result['captchaId']; + $form_state['complete form']['mollom']['captchaId']['#value'] = $result['captchaId']; if (!empty($result['solved'])) { $form_state['mollom']['passed_captcha'] = TRUE; - $form['mollom']['captcha']['#access'] = FALSE; - $form['mollom']['captcha']['#solved'] = TRUE; + $form_state['complete form']['mollom']['captcha']['#access'] = FALSE; + $form_state['complete form']['mollom']['captcha']['#solved'] = TRUE; mollom_log(array( 'message' => 'Correct CAPTCHA', @@ -2118,8 +2106,7 @@ function mollom_validate_captcha(&$form, &$form_state) { else { $form_state['mollom']['passed_captcha'] = FALSE; form_set_error('mollom][captcha', t('The word verification was not completed correctly. Please complete this new word verification and try again.') . ' ' . _mollom_format_message_falsepositive($form_state, $data)); - // D6: Not possible here; deferred to mollom_pre_render_mollom(). - //mollom_form_add_captcha($form['mollom'], $form_state); + mollom_form_add_captcha($form_state['complete form']['mollom'], $form_state); mollom_log(array( 'message' => 'Incorrect CAPTCHA', @@ -2132,51 +2119,12 @@ function mollom_validate_captcha(&$form, &$form_state) { * * - Hides the CAPTCHA if it is not required or the solution was correct. * - Marks the CAPTCHA as required. - * - * This #pre_render trick is required, because form API validation does not - * allow form validation handlers to alter the actual form structure. Both the - * form constructor function and the #process callback for the 'mollom' element - * are therefore executed too early (before form validation), so the CAPTCHA - * element still contains not yet validated (default) values. - * We also cannot invoke a form validation handler during form construction or - * processing, because mollom_form_get_values() would be invoked too early - * and therefore $form_state['values'] would not contain any additions from - * form validation functions like mollom_comment_form_validate(). - * @see http://drupal.org/node/642702 */ function mollom_pre_render_mollom($element) { - $form_state['mollom'] = &$element['#mollom']; - // Request and inject a CAPTCHA after unsuccessful form validation. - if ($form_state['mollom']['require_captcha'] && !$form_state['mollom']['passed_captcha']) { - // Shortcut to responses, to make the following condition more readable. - $r = $form_state['mollom']['response']; - // However, do not request a new CAPTCHA a second time. In D7, the form - // validation handlers can adjust the form directly when needed. In D6, we - // need to check for the exact conditions in which the form validation - // handlers would have injected a new CAPTCHA normally. Thus, only if: - // 1) text analysis was unsure and there is no CAPTCHA yet. - $was_unsure = (empty($r['captcha']['id']) && isset($r['content']['spamClassification']) && $r['content']['spamClassification'] == 'unsure'); - // 2) there was a CAPTCHA, but it wasn't solved correctly. - $was_incorrect = (isset($r['captcha']['solved']) && empty($r['captcha']['solved'])); - if ($was_unsure || $was_incorrect) { - mollom_form_add_captcha($element, $form_state); - } - } - // If no CAPTCHA is required or the response was correct, hide the CAPTCHA. - elseif (!$form_state['mollom']['require_captcha'] || $form_state['mollom']['passed_captcha']) { - $element['captcha']['#access'] = FALSE; - } - // Ensure that new/current IDs are contained in the rendered form. Form - // validation handlers are not able to alter the $form structure in D6. - if (!empty($form_state['mollom']['response']['content']['id'])) { - $element['contentId']['#value'] = $form_state['mollom']['response']['content']['id']; - } - if (!empty($form_state['mollom']['response']['captcha']['id'])) { - $element['captchaId']['#value'] = $form_state['mollom']['response']['captcha']['id']; - } - // Store the Mollom form state. - cache_set($form_state['mollom']['mollom_build_id'], $form_state['mollom'], 'cache_mollom', time() + 21600); + if (!empty($form_state['mollom']['mollom_build_id'])) { + cache_set($form_state['mollom']['mollom_build_id'], $form_state['mollom'], 'cache_mollom', time() + 21600); + } // If there is no CAPTCHA ID, then there is no CAPTCHA that can be displayed. // If a CAPTCHA was solved, then the widget makes no sense either. diff --git a/tests/mollom.test b/tests/mollom.test index d03d327..97293b7 100644 --- a/tests/mollom.test +++ b/tests/mollom.test @@ -3737,11 +3737,6 @@ class MollomAnalysisTestCase extends MollomWebTestCase { $this->setKeys(TRUE, TRUE); $this->assertValidKeys(); - $this->admin_user = $this->drupalCreateUser(array( - 'access administration pages', - 'administer mollom', - )); - $this->setProtection('mollom_test_form', MOLLOM_MODE_ANALYSIS); } @@ -4028,7 +4023,7 @@ class MollomAnalysisAuthenticatedTestCase extends MollomAnalysisTestCase { function setUp() { parent::setUp(); - $this->web_user = $this->drupalCreateUser(array()); + $this->web_user = $this->drupalCreateUser(array('access content')); $this->drupalLogin($this->web_user); } } @@ -4059,7 +4054,7 @@ class MollomAnalysisAuthenticatedPageFormCacheTestCase extends MollomAnalysisTes $this->drupalGet('mollom-test/form'); $this->assertText('Views: 1'); - $this->web_user = $this->drupalCreateUser(array()); + $this->web_user = $this->drupalCreateUser(array('access content')); $this->drupalLogin($this->web_user); } } -- 1.7.11.msysgit.1 From 2f2dac0052c2da3d3e0625330511e0d5cb288d96 Mon Sep 17 00:00:00 2001 From: sun Date: Fri, 5 Apr 2013 03:29:46 +0200 Subject: [PATCH 4/5] - #1938768 by sun: Hardened test coverage for CAPTCHA-only protected forms. --- tests/mollom.test | 238 ++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 170 insertions(+), 68 deletions(-) diff --git a/tests/mollom.test b/tests/mollom.test index 97293b7..0201fe7 100644 --- a/tests/mollom.test +++ b/tests/mollom.test @@ -547,6 +547,8 @@ class MollomWebTestCase extends DrupalWebTestCase { * Assert that the CAPTCHA field is not found on the current page. */ protected function assertNoCaptchaField() { + $this->assertNoText($this->unsure_message); + $this->assertNoText($this->incorrect_message); $this->assertNoFieldByXPath('//input[@type="text"][@name="mollom[captcha]"]', '', 'CAPTCHA field not found.'); $image = $this->xpath('//img[@alt="' . t('Type the characters you see in this picture.') . '"]'); $this->assert(empty($image), 'CAPTCHA image not found.'); @@ -971,6 +973,24 @@ class MollomWebTestCase extends DrupalWebTestCase { //variable_set('cache_lifetime', 60); } + /** + * Asserts a successful mollom_test_form submission. + * + * @param $old_mid + * (optional) The existing test record id to assert. + */ + protected function assertTestSubmitData($old_mid = NULL) { + $this->assertText('Successful form submission.'); + $mid = $this->getFieldValueByName('mid'); + if (isset($old_mid)) { + $this->assertSame('Test record id', $mid, $old_mid); + } + else { + $this->assertTrue($mid > 0, t('Test record id @id found.', array('@id' => $mid))); + } + return $mid; + } + } /** @@ -3934,24 +3954,6 @@ class MollomAnalysisTestCase extends MollomWebTestCase { $this->assertNoCaptchaField(); $this->assertTestSubmitData(); } - - /** - * Asserts a successful mollom_test_form submission. - * - * @param $old_mid - * (optional) The existing test record id to assert. - */ - protected function assertTestSubmitData($old_mid = NULL) { - $this->assertText('Successful form submission.'); - $mid = $this->getFieldValueByName('mid'); - if (isset($old_mid)) { - $this->assertSame('Test record id', $mid, $old_mid); - } - else { - $this->assertTrue($mid > 0, t('Test record id @id found.', array('@id' => $mid))); - } - return $mid; - } } /** @@ -4284,23 +4286,6 @@ class MollomAnalysisOptionsTestCase extends MollomWebTestCase { } } - /** - * Asserts a successful mollom_test_form submission. - * - * @param $old_mid - * (optional) The existing test record id to assert. - */ - protected function assertTestSubmitData($old_mid = NULL) { - $this->assertText('Successful form submission.'); - $mid = $this->getFieldValueByName('mid'); - if (isset($old_mid)) { - $this->assertSame('Test record id', $mid, $old_mid); - } - else { - $this->assertTrue($mid > 0, t('Test record id @id found.', array('@id' => $mid))); - } - return $mid; - } } /** @@ -4364,11 +4349,11 @@ class MollomAnalysisPageCachingTestCase extends MollomWebTestCase { * Tests CAPTCHA functionality. */ class MollomCaptchaTestCase extends MollomWebTestCase { + protected $disableDefaultSetup = TRUE; + // Re-route Mollom communication to this testing site. protected $mollomClass = 'MollomDrupalTestLocal'; - protected $disableDefaultSetup = TRUE; - public static function getInfo() { return array( 'name' => 'CAPTCHA', @@ -4379,24 +4364,26 @@ class MollomCaptchaTestCase extends MollomWebTestCase { function setUp() { parent::setUp(array('mollom', 'mollom_test')); - $this->setKeys(); + $this->setKeys(TRUE, TRUE); $this->assertValidKeys(); - $this->admin_user = $this->drupalCreateUser(array( - 'access administration pages', - 'administer mollom', - )); - $this->web_user = $this->drupalCreateUser(array('access content')); + $this->setProtection('mollom_test_form', MOLLOM_MODE_CAPTCHA); + } - $this->drupalLogin($this->admin_user); - $this->setProtectionUI('mollom_test_form', MOLLOM_MODE_CAPTCHA); - $this->drupalLogout(); + function testCAPTCHA() { + $methods = get_class_methods($this); + foreach ($methods as $method) { + if (substr($method, 0, 7) === 'subtest') { + //debug($method); + $this->$method(); + } + } } /** * Tests #required validation of CAPTCHA form element. */ - function testRequiredValidation() { + function subtestCAPTCHARequired() { $this->drupalGet('mollom-test/form'); // Verify that CAPTCHA cannot be left empty. @@ -4417,18 +4404,28 @@ class MollomCaptchaTestCase extends MollomWebTestCase { ); $this->postIncorrectCaptcha(NULL, $edit, 'Submit', 'Successful form submission.'); - // Lastly, verify correct solution. + // Verify correct solution, but trigger other validation errors. $edit = array( + 'title' => '', 'mollom[captcha]' => 'correct', ); $this->drupalPost(NULL, $edit, 'Submit'); + $this->assertNoCaptchaField(); + $this->assertNoText('Successful form submission.'); + + // Lastly, confirm we're able to submit. + $edit = array( + 'title' => $this->randomString(), + ); + $this->drupalPost(NULL, $edit, 'Submit'); + $this->assertNoText($this->incorrect_message); $this->assertTestSubmitData(); } /** * Tests incorrect solution of CAPTCHA form element. */ - function testIncorrect() { + function subtestCAPTCHAIncorrect() { $this->drupalGet('mollom-test/form'); // Verify that incorrect solution still leaves the field required. @@ -4446,42 +4443,51 @@ class MollomCaptchaTestCase extends MollomWebTestCase { } /** - * Tests correct solution of CAPTCHA form element. + * Tests correct solution of CAPTCHA. */ - function testCorrect() { + function subtestCAPTCHACorrect() { $this->drupalGet('mollom-test/form'); - // Verify that CAPTCHA can be solved in one shot. $edit = array( - 'title' => $this->randomString(), 'mollom[captcha]' => 'correct', ); $this->drupalPost(NULL, $edit, 'Submit'); + $this->assertNoCaptchaField(); + $this->assertNoText('Successful form submission.'); + + $edit = array( + 'body' => $this->randomString(), + ); + $this->drupalPost(NULL, $edit, 'Submit'); + $this->assertNoCaptchaField(); + $this->assertNoText('Successful form submission.'); + + $edit = array( + 'title' => $this->randomString(), + ); + $this->drupalPost(NULL, $edit, 'Submit'); $this->assertTestSubmitData(); } /** - * Asserts a successful mollom_test_form submission. - * - * @param $old_mid - * (optional) The existing test record id to assert. + * Tests correct solution of CAPTCHA in a single pass. */ - protected function assertTestSubmitData($old_mid = NULL) { - $this->assertText('Successful form submission.'); - $mid = $this->getFieldValueByName('mid'); - if (isset($old_mid)) { - $this->assertSame('Test record id', $mid, $old_mid); - } - else { - $this->assertTrue($mid > 0, t('Test record id @id found.', array('@id' => $mid))); - } - return $mid; + function subtestCAPTCHACorrectSinglePass() { + $this->drupalGet('mollom-test/form'); + + // Verify that CAPTCHA can be solved in one shot. + $edit = array( + 'title' => $this->randomString(), + 'mollom[captcha]' => 'correct', + ); + $this->drupalPost(NULL, $edit, 'Submit'); + $this->assertTestSubmitData(); } /** * Tests the CAPTCHA type switch callback. */ - function testSwitchCallback() { + function subtestCAPTCHASwitchCallback() { // Verify that the CAPTCHA can be switched on a CAPTCHA-only protected form. // (without having a contentId) $this->drupalGet('mollom-test/form'); @@ -4507,6 +4513,102 @@ class MollomCaptchaTestCase extends MollomWebTestCase { } /** + * Tests CAPTCHA with page cache. + */ +class MollomCaptchaPageCacheTestCase extends MollomCaptchaTestCase { + public static function getInfo() { + return array( + 'name' => 'CAPTCHA (page cache)', + 'description' => 'Tests CAPTCHA with page cache.', + 'group' => 'Mollom', + ); + } + + function setUp() { + parent::setUp(); + $this->enablePageCache(); + + // Prime the page + form cache. + $this->drupalGet('mollom-test/form'); + $this->assertText('Views: 1'); + $this->drupalGet('mollom-test/form'); + $this->assertText('Views: 2'); + } +} + +/** + * Tests CAPTCHA with page and form cache. + */ +class MollomCaptchaPageFormCacheTestCase extends MollomCaptchaTestCase { + public static function getInfo() { + return array( + 'name' => 'CAPTCHA (page + form cache)', + 'description' => 'Tests CAPTCHA with page and form cache.', + 'group' => 'Mollom', + ); + } + + function setUp() { + parent::setUp(); + $this->enablePageCache(); + variable_set('mollom_test.form.cache', TRUE); + + // Prime the page + form cache. + $this->drupalGet('mollom-test/form'); + $this->assertText('Views: 1'); + $this->drupalGet('mollom-test/form'); + $this->assertText('Views: 2'); + } +} + +/** + * Tests CAPTCHA as authenticated user. + */ +class MollomCaptchaAuthenticatedTestCase extends MollomCaptchaTestCase { + public static function getInfo() { + return array( + 'name' => 'CAPTCHA (authenticated)', + 'description' => 'Tests CAPTCHA as authenticated user.', + 'group' => 'Mollom', + ); + } + + function setUp() { + parent::setUp(); + $this->web_user = $this->drupalCreateUser(array('access content')); + $this->drupalLogin($this->web_user); + } +} + +/** + * Tests CAPTCHA as authenticated user with enabled page and form cache. + */ +class MollomCaptchaAuthenticatedPageFormCacheTestCase extends MollomCaptchaTestCase { + public static function getInfo() { + return array( + 'name' => 'CAPTCHA (authenticated + page + form cache)', + 'description' => 'Tests CAPTCHA as authenticated user with enabled page and form cache.', + 'group' => 'Mollom', + ); + } + + function setUp() { + parent::setUp(); + $this->enablePageCache(); + variable_set('mollom_test.form.cache', TRUE); + + // Prime the page + form cache. + $this->drupalGet('mollom-test/form'); + $this->assertText('Views: 1'); + $this->drupalGet('mollom-test/form'); + $this->assertText('Views: 2'); + + $this->web_user = $this->drupalCreateUser(array('access content')); + $this->drupalLogin($this->web_user); + } +} + +/** * Tests report to Mollom functionality. */ class MollomReportTestCase extends MollomWebTestCase { -- 1.7.11.msysgit.1 From 061b0cb0e432cea54ed1b7622df6d8792198b17b Mon Sep 17 00:00:00 2001 From: sun Date: Fri, 5 Apr 2013 04:04:52 +0200 Subject: [PATCH 5/5] - #1938768 by sun: Limited mollom_build_id usage to CAPTCHA-only protected forms. --- mollom.module | 139 ++++++++++++++++++++++++++++------------------- tests/mollom.test | 4 +- tests/mollom_test.module | 2 +- 3 files changed, 86 insertions(+), 59 deletions(-) diff --git a/mollom.module b/mollom.module index f366fdd..92fe4d6 100644 --- a/mollom.module +++ b/mollom.module @@ -1630,22 +1630,20 @@ function theme_mollom($element) { * validation errors preventing the form from submitting directly, as well as * "Preview" buttons that may rebuild the entire form from scratch (if there * are no validation errors). - * - To track state, the Form API cache is enabled, which allows to store and - * retrieve the entire $form_state of a previous submission (attempt). + * - To track state, a unique and reliable mollom_build_id is generated, + * mimicking D7's form_build_id, which allows to store and retrieve the + * $form_state of a previous submission (attempt). * - Furthermore, page caching is force-disabled, so as to ensure that cached * form data is not re-used by different users/visitors. - * - In combination with the Form API cache, this is essentially equal to - * force-starting a session for all users that try to submit a CAPTCHA-only - * protected form. However, a session would persist across other pages. + * - This is essentially equal to force-starting a session for all users that + * try to submit a CAPTCHA-only protected form. However, a session would + * persist across other pages. * * @see mollom_form_alter() * @see mollom_element_info() * @see mollom_pre_render_mollom() - * - * The session ID is valid for a given $form_id only. We expire it as soon as - * the form is submitted, to avoid it being replayed. */ -function mollom_process_mollom($element, $input, &$form_state, $complete_form) { +function mollom_process_mollom($element, $input, &$form_state, &$complete_form) { // Setup initial Mollom session and form information. $form_state += array('mollom' => array()); $form_state['mollom'] += array( @@ -1679,26 +1677,6 @@ function mollom_process_mollom($element, $input, &$form_state, $complete_form) { // Add the JavaScript. drupal_add_js(drupal_get_path('module', 'mollom') . '/mollom.js'); - // D6: - // mollom_build_id works similar to form_build_id in D7; it is an ID that is - // unique for each build of a form. However, unlike form_build_id in D6, it - // does not change across POSTs, regardless of whether form storage/caching - // is enabled or not. Mollom uses the mollom_build_id as cache identifier to - // track the state of a form session/submission, which is required to - // re-inject prior knowledge about the submission (such as a "unsure" text - // analysis result). Although generated whenever this function is called, - // $form_state['mollom'] including mollom_build_id will already be - // prepopulated by mollom_process_mollom_form_state() and thus not overidden - // here. If mollom_build_id is manually removed from the form, the form's - // state is merely lost and the form submission needs to go through Mollom - // validation from scratch. - if (!$form_state['mollom']['require_analysis']) { - $element['mollom_build_id'] = array( - '#type' => 'hidden', - '#value' => !empty($form_state['mollom']['mollom_build_id']) ? $form_state['mollom']['mollom_build_id'] : md5(uniqid(mt_rand(), TRUE)), - ); - } - // Add the Mollom session data elements. // These elements resemble the {mollom} database schema. The form validation // handlers will pollute them with values returned by Mollom. For entity @@ -1762,12 +1740,41 @@ function mollom_process_mollom($element, $input, &$form_state, $complete_form) { if (!variable_get('mollom_testing_mode', 0)) { $element['captcha']['#attributes']['autocomplete'] = 'off'; } + + // D6: Comment module invokes form validation from within an #after_build + // callback during form processing; i.e., the form is not fully built yet. + // A proper submit via drupal_process_form() sets $form_state['submitted'] + // to TRUE and has a $form_state['clicked_button']. + // We cannot check for $form_state['process_input'], since that is only set + // after form building/processing, before validation handlers are invoked. + // Regular build has: submitted=FALSE, values + // Proper submit has: submitted=TRUE, clicked_button, values + // Bogus submit has: submitted=FALSE, clicked_button, buttons, values + // ~ + clicked_button][#executes_submit_callback = FALSE + $triggered_from_after_build = (!$form_state['submitted'] && isset($form_state['clicked_button'])); + // For CAPTCHA-only protected forms: if (!$form_state['mollom']['require_analysis'] && $form_state['mollom']['require_captcha']) { + // D6: Mimic D7's unique, reliable form build ID. + // Tracks the state of a CAPTCHA. Only applies to CAPTCHA-only protected + // forms, for which page caching is force-disabled. + // @see mollom_process_mollom_form_state() + // @see mollom_validate_post() + if (!isset($form_state['mollom']['mollom_build_id'])) { + $form_state['mollom']['mollom_build_id'] = md5(uniqid(mt_rand(), TRUE)); + } + $element['mollom_build_id'] = array( + '#type' => 'hidden', + '#value' => $form_state['mollom']['mollom_build_id'], + ); + // Retrieve and show an initial CAPTCHA. - if (empty($form_state['process_input'])) { - // Enable Form API caching, in order to track the state of the CAPTCHA. - $form_state['cache'] = TRUE; + // D6: As authenticated user, after previewing + incorrectly solving a + // CAPTCHA on the comment form, previewing + solving it + with other + // validation errors, and finally saving/submitting, the + // $triggered_from_after_build condition from involved #after_builds is + // still FALSE, so additionally check the 'passed_captcha' state. + if (empty($form_state['process_input']) && !$triggered_from_after_build && !$form_state['mollom']['passed_captcha']) { // mollom_form_add_captcha() adds the CAPTCHA and disables page caching. mollom_form_add_captcha($element, $form_state); } @@ -1802,6 +1809,18 @@ function mollom_process_mollom($element, $input, &$form_state, $complete_form) { ), ); + // D6: The complete form available to the #pre_render callback. + // Only applies to forms with enabled form caching (#cache = TRUE), for which + // mollom_form_alter() is not executed. D6 Form API does not pass $form or + // $complete_form by reference internally, nor to any callbacks. + // The following assignments build a cross-reference for $complete_form + // between $form_state and the element property. + // @see mollom_pre_render_mollom() + if (!isset($form_state['complete form']) || $triggered_from_after_build) { + $form_state['complete form'] = &$complete_form; + $element['#complete form'] = &$complete_form; + } + // Make Mollom form and session information available to entirely different // functions. // @see mollom_mail_alter() @@ -1821,26 +1840,18 @@ function mollom_process_mollom($element, $input, &$form_state, $complete_form) { * @see http://drupal.org/node/642702 */ function mollom_process_mollom_form_state($element, $input, &$form_state) { - // The current state can come either from the $form_state, if the form - // was just rebuilt in the same request, or from data posted by the user. In - // the latter case the state is fetched from the temporary data store. It is - // verified that the session was created for the current form and that it has - // not expired or already been used. if (empty($form_state['mollom']) && !empty($input['mollom_build_id'])) { - if (!$cache = cache_get($input['mollom_build_id'], 'cache_mollom')) { - watchdog('mollom', 'Unknown form build ID %form_build_id.', array( - '%form_build_id' => $input['mollom_build_id'], - ), WATCHDOG_WARNING); - } - elseif ($cache->data['form_id'] !== $form_state['values']['form_id']) { - watchdog('mollom', 'Invalid form ID %form_id for build ID %form_build_id (generated for %cached_form_id).', array( - '%form_build_id' => $input['mollom_build_id'], - '%cached_form_id' => $cache->data['form_id'], - '%form_id' => $form_state['values']['form_id'], - ), WATCHDOG_WARNING); - } - else { - $form_state['mollom'] = $cache->data; + if ($cache = cache_get($input['mollom_build_id'], 'cache_mollom')) { + if ($cache->data['form_id'] !== $form_state['values']['form_id']) { + watchdog('mollom', 'Invalid form ID %form_id for build ID %form_build_id (generated for %cached_form_id).', array( + '%form_build_id' => $input['mollom_build_id'], + '%cached_form_id' => $cache->data['form_id'], + '%form_id' => $form_state['values']['form_id'], + ), WATCHDOG_WARNING); + } + else { + $form_state['mollom'] = $cache->data; + } } } return $element; @@ -2121,9 +2132,13 @@ function mollom_validate_captcha(&$form, &$form_state) { * - Marks the CAPTCHA as required. */ function mollom_pre_render_mollom($element) { - // Store the Mollom form state. - if (!empty($form_state['mollom']['mollom_build_id'])) { - cache_set($form_state['mollom']['mollom_build_id'], $form_state['mollom'], 'cache_mollom', time() + 21600); + // D6: Take over any adjustments performed by form validation handlers. + // Only applies to forms with enabled form caching (#cache = TRUE). + // @see mollom_process_mollom() + if (isset($element['#complete form']['mollom'])) { + foreach (element_children($element['#complete form']['mollom']) as $key) { + $element[$key] = array_merge($element[$key], $element['#complete form']['mollom'][$key]); + } } // If there is no CAPTCHA ID, then there is no CAPTCHA that can be displayed. @@ -2144,8 +2159,11 @@ function mollom_pre_render_mollom($element) { $element['captcha']['#value'] = ''; // DX: Debugging helpers. -// $element['captcha']['#suffix'] = 'contentId: ' . $element['contentId']['#value']; -// $element['captcha']['#suffix'] .= '
captchaId: ' . $element['captchaId']['#value']; +// $element['captcha']['#suffix'] = 'contentId: ' . $element['contentId']['#value'] . '
'; +// $element['captcha']['#suffix'] .= 'captchaId: ' . $element['captchaId']['#value'] . '
'; +// if (isset($element['mollom_build_id'])) { +// $element['captcha']['#suffix'] .= 'mollom_build_id: ' . $element['mollom_build_id']['#value'] . '
'; +// } return $element; } @@ -2164,6 +2182,11 @@ function mollom_validate_post(&$form, &$form_state) { $function = $form_state['mollom']['moderation callback']; $function($form, $form_state); } + + // Update the Mollom form state. + if (!empty($form_state['mollom']['mollom_build_id'])) { + cache_set($form_state['mollom']['mollom_build_id'], $form_state['mollom'], 'cache_mollom', time() + (6 * 3600)); + } } /** @@ -2218,6 +2241,10 @@ function mollom_form_submit($form, &$form_state) { } /** + * @} End of "defgroup mollom_form_api". + */ + +/** * Instantiates a new Mollom client. * * @param $class diff --git a/tests/mollom.test b/tests/mollom.test index 0201fe7..746e841 100644 --- a/tests/mollom.test +++ b/tests/mollom.test @@ -2931,7 +2931,7 @@ class MollomCommentFormTestCase extends MollomWebTestCase { $this->drupalLogin($this->web_user); $this->drupalGet('comment/reply/' . $this->node->nid); $this->assertCaptchaField(); - $this->assertResponseIDInForm('captchaId'); + $this->assertResponseIDInForm('captchaId', TRUE); $this->assertNoPrivacyLink(); // Try to submit an incorrect answer for the CAPTCHA, without value for @@ -2945,7 +2945,7 @@ class MollomCommentFormTestCase extends MollomWebTestCase { // field value. $this->postCorrectCaptcha(NULL, array(), t('Preview')); $this->assertText(t('Comment field is required.')); - $captchaId = $this->assertResponseIDInForm('captchaId', TRUE); + $captchaId = $this->assertResponseIDInForm('captchaId'); $this->assertNoPrivacyLink(); // Finally, we should be able to submit a comment. diff --git a/tests/mollom_test.module b/tests/mollom_test.module index 6838317..3ce35e0 100644 --- a/tests/mollom_test.module +++ b/tests/mollom_test.module @@ -142,7 +142,7 @@ function mollom_test_form(&$form_state, $mid = NULL) { if (!isset($form_state['storage'])) { $form_state['storage'] = $values; } - $form_state['cache'] = TRUE; + $form['#cache'] = TRUE; } $form['#tree'] = TRUE; -- 1.7.11.msysgit.1