Spin-off from #1020072: Rethink/simplify UI to protect/configure a form

To aid in fighting user registration spam and other edge-case scenarios, we have two new configurable parameters:

  1. rate_limit (seconds): A configurable time interval (in seconds) that needs to elapse since the last form submission attempt that has been checked by Mollom across all sites in the Mollom network, before the same author can submit the form.

    For example, it is unlikely that someone registers more than one user account on different sites within, say, an hour. Thus, you could configure a one hour treshold here, to prevent a spambot from registering on your site, in case that bot attempted to register user accounts on other sites in the Mollom network within the past hour.

  2. strictness (high, medium [default], low): Allows to tune the strictness of the different checks that Mollom is performing (eg. the reputation of the author, the URLs, ...) For trusted users we could e.g. use the “low” value.
CommentFileSizeAuthor
#12 mollom.strictness.12.patch4.29 KBsun
#9 mollom.strictness.9.patch4.4 KBsun
#8 mollom.strictness.8.patch3.27 KBsun
#4 mollom.ratelimit.4.patch5.4 KBsun
#4 mollom-ratelimit-4.png29.89 KBsun
#3 mollom-ratelimit-3.png50.48 KBsun
#2 mollom.ratelimit.2.patch4.68 KBsun
mollom.ratelimit.0.patch4.79 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, mollom.ratelimit.0.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
mollom.ratelimit.2.patch4.68 KB

I'm told that testbot doesn't like --no-prefix anymore; trying again.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
mollom-ratelimit-3.png50.48 KB

UI review:

mollom-ratelimit-3.png

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
mollom-ratelimit-4.png29.89 KB
mollom.ratelimit.4.patch5.4 KB

New patch, new screenshot:

(Note, I've tweaked the other text analysis form labels, too -- it visually helps to "visually group" them by repeating at least the term "Text" [the elements are still shown for text analysis only])

mollom-ratelimit-4.png

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Needs review » Needs work 
+++ b/mollom.admin.inc
@@ -293,8 +293,35 @@ function mollom_admin_configure_form($form, &$form_state, $mollom_form = NULL) {
+          '#options' => array(
+            'high' => t('Strict'),
+            'medium' => t('Normal'),
+            'low' => t('Relaxed'),

Would be great if the variable names where more consistent with the UI strings?

+++ b/mollom.admin.inc
@@ -293,8 +293,35 @@ function mollom_admin_configure_form($form, &$form_state, $mollom_form = NULL) {
+        '#title' => t('Minimum interval between posts of same author across all sites in the Mollom network'),

Maybe call this 'Maximum submission interval' and just add a description?

If we call this 'Submission interval' in the UI, we might want to rename the variable too?

+++ b/mollom.admin.inc
@@ -360,6 +387,10 @@ function mollom_admin_configure_form_submit($form, &$form_state) {
+  if ($mollom_form['rate_limit'] === '') {
+    $mollom_form['rate_limit'] = NULL;
+  }

That feels like a bit of a hack?

We're still missing tests.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review

Would be great if the variable names where more consistent with the UI strings?

The current internal names directly map 1:1 to Mollom API parameters, which I think is a good idea, because it means less (or actually no) translation of stored Drupal values into Mollom API values.

Maybe call this 'Maximum submission interval' and just add a description?

Actually I spent a little time with Wiktionary to double-check and finally find the most concise term with "period". The latest patch basically moves the old label into the description, and uses a precise label.

That feels like a bit of a hack?

Yeah, could probably use a code comment. The idea/intention was to explicitly differ between "Use Mollom's rate_limit default whatever that is", and an explicit "0", which may or may not have some meaning in the future (we don't know yet), so as to be able to safely update it in the future. Therefore, the database column for the rate_limit option allows to store NULL, and the conversion from non-selected option to NULL is what we see in this snippet.

We're still missing tests.

Hm - as mentioned elsewhere already, these two parameters cannot really be tested with the current capabilities of Mollom's testing mode, since they solely affect the behavior of the Mollom backend -- which is something the Drupal module is not able to (and probably should not attempt to) test.

The only part that we can test basically is that we're sending the parameters (or not), I think.

Dries’s picture

Dries CreditAttribution: Dries commented

The current internal names directly map 1:1 to Mollom API parameters, which I think is a good idea, because it means less (or actually no) translation of stored Drupal values into Mollom API values.

Understood. Let's talk about it with the rest of the Mollom team -- it might make sense to change the Mollom API parameters? Just a thought.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
mollom.strictness.8.patch3.27 KB

The strictness parameter values have been changed to strict/normal/relaxed in the Mollom API.

However, we are not going to add the rate_limit option to the current XML-RPC-based Drupal module. The reason for that is that we are unable to tell the user why his correctly entered CAPTCHA solution is not correct. That's unacceptable, and therefore, rate_limit needs to wait for the REST API implementation. (Further details may be found in Mollom's internal issue tracker.)

Attached patch adds the strictness option only.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
mollom.strictness.9.patch4.4 KB

Added assertion to the Data test case to verify that strictness parameter is sent.

Dries’s picture

Dries CreditAttribution: Dries commented

The patch in #9 looks good to me! :)

Dries’s picture

Dries CreditAttribution: Dries commented
Version: 7.x-1.x-dev » 6.x-1.x-dev
Status: Needs review » Patch (to be ported)

Reviewed again and still looks good.

Committed to 'master' (7.x) and '7.x-rest'. Will need to be backported to Drupal 6.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Patch (to be ported) » Needs review
FileSize
mollom.strictness.12.patch4.29 KB

Backported to D6.

Discovered a small glitch in the D7 module update; will fix that afterwards.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Fixed

Apparently, I'm not sure what happened in #11, but neither the master nor 7.x-rest branch contained this change. Perhaps you committed, but didn't push?

Anyway, in this case even beneficial, as this allowed me to fix the small glitch in the module update.

Committed to master (+ merged into 7.x-rest), and also committed to 6.x-1.x now.

Status: Fixed » Closed (fixed)

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

  • Commit 3d055fb on master, fai6, 8.x-2.x, fbajs, actions by sun: 
    Issue #1111378 by sun: Added text analysis strictness option.

  • Commit 3d055fb on master, fai6, 8.x-2.x, fbajs, actions by sun: 
    Issue #1111378 by sun: Added text analysis strictness option.