diff --git a/mollom.module b/mollom.module index 0113c5f..50fcb58 100644 --- a/mollom.module +++ b/mollom.module @@ -726,9 +726,6 @@ function mollom_form_alter(&$form, &$form_state, $form_id) { '#weight' => (isset($form['actions']['#weight']) ? $form['actions']['#weight'] - 1 : 99), '#tree' => TRUE, ); - // Enable caching of this form; required for our form validation and - // submit handlers. - $form_state['cache'] = TRUE; // Add Mollom form validation handlers. // Form-level validation handlers are required, since we need access to @@ -1464,7 +1461,9 @@ function _mollom_status($reset = FALSE) { * Outputs a warning message about enabled testing mode (once). */ function _mollom_testing_mode_warning() { - $warned = &drupal_static(__FUNCTION__); + // drupal_set_message() starts a session and disables page caching, which + // breaks testing. Therefore, tests set the verbose variable to TRUE. + $warned = &drupal_static(__FUNCTION__, variable_get('mollom_testing_mode_omit_warning', NULL)); if (isset($warned)) { return; } @@ -1527,6 +1526,7 @@ function mollom_element_info() { '#process' => array( 'mollom_process_mollom', ), + '#pre_render' => array('mollom_pre_render_mollom'), ), ); } @@ -1562,10 +1562,6 @@ function mollom_process_mollom($element, &$form_state, $complete_form) { 'response' => array( ), ); - // Instantiate a new Mollom class, unless there is not a cached one. - if (!isset($form_state['mollom']['class'])) { - $form_state['mollom']['class'] = mollom(); - } // Add remaining information about the registered form. $form_state['mollom'] += $element['#mollom_form']; @@ -1594,7 +1590,6 @@ function mollom_process_mollom($element, &$form_state, $complete_form) { ); $element['contentId'] = array( '#type' => 'hidden', - '#default_value' => isset($form_state['mollom']['response']['content']['id']) ? $form_state['mollom']['response']['content']['id'] : '', '#attributes' => array('class' => 'mollom-content-id'), ); $element['captchaId'] = array( @@ -1616,11 +1611,11 @@ function mollom_process_mollom($element, &$form_state, $complete_form) { ); $element['spamScore'] = $data_spec; $element['spamClassification'] = $data_spec; - $element['solved'] = $data_spec; $element['qualityScore'] = $data_spec; $element['profanityScore'] = $data_spec; - $element['reason'] = $data_spec; $element['languages'] = $data_spec; + $element['reason'] = $data_spec; + $element['solved'] = $data_spec; // Add the CAPTCHA element. $element['captcha'] = array( @@ -1635,14 +1630,16 @@ function mollom_process_mollom($element, &$form_state, $complete_form) { if (!variable_get('mollom_testing_mode', 0)) { $element['captcha']['#attributes']['autocomplete'] = 'off'; } - // Request and inject an initial CAPTCHA. - if ($form_state['mollom']['require_captcha'] && !$form_state['mollom']['passed_captcha'] && empty($form_state['mollom']['response']['captcha']['id'])) { - mollom_form_add_captcha($element, $form_state); - } - // If no CAPTCHA is required or it was solved already, hide the element. - elseif (!$form_state['mollom']['require_captcha'] || $form_state['mollom']['passed_captcha']) { - $element['captcha']['#access'] = FALSE; + if (!$form_state['mollom']['require_analysis'] && $form_state['mollom']['require_captcha']) { + if (!$form_state['mollom']['passed_captcha']) { + mollom_form_add_captcha($element, $form_state); + $form_state['cache'] = TRUE; + } + else { + // Only reached in case the form fully validated and is getting rebuilt. + $element['captcha']['#solved'] = TRUE; + } } // Add a spambot trap. Purposively use 'homepage' as field name. @@ -1685,9 +1682,6 @@ function mollom_process_mollom($element, &$form_state, $complete_form) { * The current state of the form. Passed by reference. */ function mollom_form_add_captcha(&$element, &$form_state) { - // UX: Empty the CAPTCHA field value, as the user has to re-enter a new one. - $element['captcha']['#value'] = ''; - // Prevent the page cache from storing a form containing a CAPTCHA element. drupal_page_is_cacheable(FALSE); @@ -1695,9 +1689,9 @@ function mollom_form_add_captcha(&$element, &$form_state) { // If we get a response, add the image CAPTCHA to the form element. if (!empty($captcha)) { $element['captcha']['#access'] = TRUE; - $element['captcha']['#required'] = TRUE; $element['captcha']['#field_prefix'] = $captcha; - // Assign the session ID returned by Mollom. + + // Assign the new CAPTCHA ID returned by Mollom. $element['captchaId']['#value'] = $form_state['mollom']['response']['captcha']['id']; $form_state['values']['mollom']['captchaId'] = $form_state['mollom']['response']['captcha']['id']; } @@ -1709,6 +1703,23 @@ function mollom_form_add_captcha(&$element, &$form_state) { } } +function mollom_pre_render_mollom($element) { + if (empty($element['captchaId']['#value']) || !empty($element['captcha']['#solved'])) { + $element['captcha']['#access'] = FALSE; + } + else { + // The form element cannot be marked as #required, since _form_validate() + // would throw an element validation error on an empty value otherwise, + // before the form-level validation handler is executed. + // #access cannot default to FALSE, since the $form may be cached, and + // Form API ignores user input for all elements that are not accessible. + $element['captcha']['#required'] = TRUE; + } + // UX: Empty the CAPTCHA field value, as the user has to re-enter a new one. + $element['captcha']['#value'] = ''; + return $element; +} + /** * Form validation handler to perform textual analysis of submitted form values. * @@ -1737,8 +1748,8 @@ function mollom_validate_analysis(&$form, &$form_state) { if (isset($data['postId'])) { unset($data['postId']); } - if (isset($form_state['mollom']['response']['content']['id'])) { - $data['id'] = $form_state['mollom']['response']['content']['id']; + if (!empty($form_state['values']['mollom']['contentId'])) { + $data['id'] = $form_state['values']['mollom']['contentId']; } $data['checks'] = $form_state['mollom']['checks']; $data['strictness'] = $form_state['mollom']['strictness']; @@ -1748,7 +1759,7 @@ function mollom_validate_analysis(&$form, &$form_state) { if (in_array('spam', $data['checks']) && $form_state['mollom']['unsure'] == 'binary') { $data['unsure'] = 0; } - $result = $form_state['mollom']['class']->checkContent($data); + $result = mollom()->checkContent($data); // Use all available data properties for log messages below. $data += $all_data; @@ -1760,11 +1771,12 @@ function mollom_validate_analysis(&$form, &$form_state) { // Store the response returned by Mollom. $form_state['mollom']['response']['content'] = $result; - // Set form element values accordingly. Do not overwrite the entity ID with - // the contentId, nor a possibly existing captchaId. + // Set form values accordingly. Do not overwrite the entity ID. + // @todo Rename 'id' to 'entity_id'. $result['contentId'] = $result['id']; unset($result['id']); $form_state['values']['mollom'] = array_merge($form_state['values']['mollom'], $result); + // #value has to be set manually to output it in case of a validation error, // since this is not a element-level but a form-level validation handler. $form['mollom']['contentId']['#value'] = $result['contentId']; @@ -1834,15 +1846,7 @@ function mollom_validate_analysis(&$form, &$form_state) { $form_state['mollom']['require_moderation'] = TRUE; } else { - // Only throw a validation error and retrieve a CAPTCHA, if we check - // this post for the first time. Otherwise, mollom_validate_captcha() - // issued the CAPTCHA and needs to validate it prior to throwing any - // errors. - if (!$form_state['mollom']['require_captcha']) { - $form_state['mollom']['require_captcha'] = TRUE; - form_set_error('mollom][captcha', t('To complete this form, please complete the word verification below.')); - mollom_form_add_captcha($form['mollom'], $form_state); - } + $form_state['mollom']['require_captcha'] = TRUE; } break; @@ -1869,28 +1873,22 @@ function mollom_validate_analysis(&$form, &$form_state) { * text analysis result is "unsure". */ function mollom_validate_captcha(&$form, &$form_state) { - // CAPTCHA validation may only be skipped, if we do not require it in the - // first place, or if the user already solved a CAPTCHA correctly. We need to - // validate, if $form_state['mollom']['require_captcha'] is TRUE, which is - // either set during initialization of $form_state['mollom'] in - // mollom_process_form(), or after performing text analysis. The second - // return condition, $form_state['mollom']['passed_captcha'], may only ever be - // set by this validation handler and must not be changed elsewhere. - if (!$form_state['mollom']['require_captcha'] || $form_state['mollom']['passed_captcha']) { - $form['mollom']['captcha']['#access'] = FALSE; + if (!$form_state['mollom']['require_captcha']) { return; } - // If the form is protected via text analysis and the result was "unsure", - // then this validation handler also runs when the CAPTCHA is initially - // requested by mollom_validate_analysis(), is about to be rendered, and - // before the user had a chance to solve it. In that case, there is no value - // that could be validated yet; i.e., the CAPTCHA value is empty. However, in - // all other cases, an empty value must be validated and sent to Mollom. The - // initial case has a special condition: mollom_validate_analysis() has set - // a form error on the CAPTCHA element asking the user to solve it in order to - // submit the form. - if ($form_state['values']['mollom']['captcha'] === '' && form_get_error($form['mollom']['captcha'])) { + // If there is no CAPTCHA ID yet, retrieve one and throw an error. + if (empty($form_state['values']['mollom']['captchaId'])) { + mollom_form_add_captcha($form['mollom'], $form_state); + form_error($form['mollom']['captcha'], t('To complete this form, please complete the word verification below.')); + return; + } + + // $form_state['mollom']['passed_captcha'], may only ever be set by this + // validation handler and must not be changed elsewhere. + if ($form_state['mollom']['passed_captcha']) { + $form['mollom']['captcha']['#access'] = FALSE; + $form['mollom']['captcha']['#solved'] = TRUE; return; } @@ -1904,7 +1902,7 @@ function mollom_validate_captcha(&$form, &$form_state) { return; } $data = array( - 'id' => $form_state['mollom']['response']['captcha']['id'], + 'id' => $form_state['values']['mollom']['captchaId'], 'solution' => $form_state['values']['mollom']['captcha'], 'authorIp' => $all_data['authorIp'], ); @@ -1914,7 +1912,7 @@ function mollom_validate_captcha(&$form, &$form_state) { if (isset($all_data['honeypot'])) { $data['honeypot'] = $all_data['honeypot']; } - $result = $form_state['mollom']['class']->checkCaptcha($data); + $result = mollom()->checkCaptcha($data); // Use all available data properties for log messages below. $data += $all_data; @@ -1925,11 +1923,12 @@ function mollom_validate_captcha(&$form, &$form_state) { // Store the response for #submit handlers. $form_state['mollom']['response']['captcha'] = $result; - // Set form element values accordingly. Do not overwrite the entity ID with - // the contentId, nor a possibly existing captchaId. + // Set form values accordingly. Do not overwrite the entity ID. + // @todo Rename 'id' to 'entity_id'. $result['captchaId'] = $result['id']; unset($result['id']); $form_state['values']['mollom'] = array_merge($form_state['values']['mollom'], $result); + // #value has to be set manually to output it in case of a validation error, // since this is not a element-level but a form-level validation handler. $form['mollom']['captchaId']['#value'] = $result['captchaId']; @@ -1937,6 +1936,7 @@ function mollom_validate_captcha(&$form, &$form_state) { if (!empty($result['solved'])) { $form_state['mollom']['passed_captcha'] = TRUE; $form['mollom']['captcha']['#access'] = FALSE; + $form['mollom']['captcha']['#solved'] = TRUE; mollom_log(array( 'message' => 'Correct CAPTCHA', @@ -2457,10 +2457,7 @@ function mollom_get_captcha(&$form_state) { if (!empty($form_state['mollom']['response']['content']['id'])) { $data['contentId'] = $form_state['mollom']['response']['content']['id']; } - // @todo $form_state may not contain a Mollom class when being called from - // mollom_captcha_js() until that gets converted to #ajax framework. - $mollom = (isset($form_state['mollom']['class']) ? $form_state['mollom']['class'] : mollom()); - $result = $mollom->createCaptcha($data); + $result = mollom()->createCaptcha($data); // Add a log message to prevent the request log from appearing without a // message on CAPTCHA-only protected forms. @@ -2470,6 +2467,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']; } else { diff --git a/tests/mollom.test b/tests/mollom.test index fa77fda..c10115a 100644 --- a/tests/mollom.test +++ b/tests/mollom.test @@ -125,6 +125,9 @@ class MollomWebTestCase extends DrupalWebTestCase { // automatically create testing API keys. variable_set('mollom_testing_create_keys', $this->createKeys); + // Disable testing mode warnings. + variable_set('mollom_testing_mode_omit_warning', TRUE); + // D7's new default theme Bartik is bogus in various locations, which leads // to failing tests. // @todo Remove this override. @@ -914,6 +917,81 @@ class MollomWebTestCase extends DrupalWebTestCase { } /** + * Tests protection of forms. + */ +class MollomFormProtectionTestCase extends MollomWebTestCase { + protected $disableDefaultSetup = TRUE; + + public static function getInfo() { + return array( + 'name' => 'Form protection', + 'description' => 'Tests protection of forms.', + 'group' => 'Mollom', + ); + } + + function setUp() { + parent::setUp(array('mollom', 'mollom_test')); + $this->setKeys(); + $this->assertValidKeys(); + + // Enable excessive/aggressive page caching to resemble reverse-proxies. + variable_set('cache', 1); + // A minimum cache lifetime causes cache_clear_all() to start a session. + //variable_set('cache_lifetime', 60); + variable_set('page_cache_maximum_age', 180); + } + + protected function setProtectionX($form_id, $mode = MOLLOM_MODE_ANALYSIS, $values = array()) { + if (!$mollom_form = mollom_form_load($form_id)) { + $mollom_form = mollom_form_new($form_id); + } + $mollom_form['mode'] = $mode; + if ($values) { + foreach ($values as $property => $value) { + $mollom_form[$property] = $value; + } + } + $status = mollom_form_save($mollom_form); + return $status; + } + + /** + * Tests text analysis. + */ + function testAnalysis() { + $this->setProtectionX('mollom_test_form'); + // Prime the form + page 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->assertUnsureSubmit(NULL, array('title'), array(), 'Submit'); + + $this->drupalGet('mollom-test/form'); + $this->assertText('Views: 1'); + $this->assertSpamSubmit(NULL, array('title'), array(), 'Submit'); + + $this->drupalGet('mollom-test/form'); + $this->assertText('Views: 1'); + $this->assertHamSubmit(NULL, array('title'), array(), 'Submit'); + } + + /** + * Tests text analysis with enabled form caching. + */ + function testAnalysisFormCache() { + variable_set('mollom_test.form.cache', TRUE); + $this->testAnalysis(); + } + +} + +/** * Tests testing mode functionality. */ class MollomTestingModeTestCase extends MollomWebTestCase { @@ -941,6 +1019,9 @@ class MollomTestingModeTestCase extends MollomWebTestCase { function setUp() { parent::setUp(array('mollom', 'mollom_test')); + // Enable testing mode warnings. + variable_del('mollom_testing_mode_omit_warning'); + $this->admin_user = $this->drupalCreateUser(array( 'access administration pages', 'administer mollom', @@ -2956,7 +3037,7 @@ class MollomCommentFormTestCase extends MollomWebTestCase { // required field. $this->postIncorrectCaptcha(NULL, array(), t('Preview')); $this->assertText(t('Comment field is required.')); - $this->assertResponseIDInForm('captchaId', TRUE); + $this->assertResponseIDInForm('captchaId'); $this->assertNoPrivacyLink(); // Try to submit a correct answer for the CAPTCHA, still without required diff --git a/tests/mollom_test.module b/tests/mollom_test.module index 22e34ff..0f24960 100644 --- a/tests/mollom_test.module +++ b/tests/mollom_test.module @@ -46,6 +46,11 @@ function mollom_test_menu() { 'page arguments' => array('mollom_test_delete_form', 2), 'access callback' => TRUE, ); + $items['mollom-test/form/views/reset'] = array( + 'page callback' => 'mollom_test_views_reset', + 'access callback' => TRUE, + 'type' => MENU_CALLBACK, + ); return $items; } @@ -115,6 +120,15 @@ function mollom_test_mollom_form_info($form_id) { } /** + * Page callback; Resets the page view counter. + */ +function mollom_test_views_reset() { + variable_del('mollom_test.form.views'); + cache_clear_all(); + drupal_goto(); +} + +/** * Form builder for Mollom test form. */ function mollom_test_form($form, &$form_state, $mid = NULL) { @@ -137,6 +151,19 @@ function mollom_test_form($form, &$form_state, $mid = NULL) { // Always add an empty field the user can submit. $form_state['storage']['field']['new'] = ''; + // Output a page view counter for page/form cache testing purposes. + $count = variable_get('mollom_test.form.views', 1); + $reset_link = l('Reset', 'mollom-test/form/views/reset', array('query' => drupal_get_destination())); + $form['views'] = array( + '#markup' => '

' . 'Views: ' . $count++ . ' ' . $reset_link . '

', + ); + variable_set('mollom_test.form.views', $count); + + // Conditionally enable form caching. + if (variable_get('mollom_test.form.cache', FALSE)) { + $form_state['cache'] = TRUE; + } + $form['#tree'] = TRUE; $form['mid'] = array( '#type' => 'hidden', diff --git a/tests/mollom_test_server.module b/tests/mollom_test_server.module index 7135726..8c54c27 100644 --- a/tests/mollom_test_server.module +++ b/tests/mollom_test_server.module @@ -97,6 +97,9 @@ function mollom_test_server_rest_get_auth_header() { * Delivery callback for REST API endpoints. */ function mollom_test_server_rest_deliver($page_callback_result) { + // All fake-server responses are not cached. + drupal_page_is_cacheable(FALSE); + drupal_add_http_header('Content-Type', 'application/xml; charset=utf-8'); $xml = new DOMDocument('1.0', 'utf-8');