Project page

https://drupal.org/sandbox/programadoresweb/2062343

Installation

git clone --branch 6.x-1.x http://git.drupal.org/sandbox/programadoresweb/2062343.git antispammer_bot

Issue summary

My main motivation to write this module was having thousands of comments and nodes published after a week off. What it is worst, a single spammer can affect your server, even force it down because of publishing some millions of comments/nodes in your site.

Said that, this is Not Just Another Spam Module which detects spam, but a bot itself which detects if the user is a possible spammer and take some actions, like:

  • block the user account
  • un-publish all the contents of this user
  • notify the site administrator

There are nodes which control the number of comments or nodes a user can publish, like:

Comments

sergei_brill’s picture

Status: Needs review » Needs work

1. antispam_bot.module, line 86. Use empty array as a default value for variable. After installation I got warning: Invalid argument supplied for foreach() in /home/sergei/web/drupal_6_test/includes/form.inc on line 1217. on bot settings page.

2. antispam_bot.module, line 53-55. You add submit callback to the form, there is no function with name 'antispammer_bot'

3. function antispammer_bot_settings_form is not used anywhere.

4. warning: Missing argument 2 for variable_get(), called in .../antispammer_bot/antispammer_bot.module on line 166. Also there is few other case where 2nd argument is missing for variable_get()

5. I limited content per non safe user by 1. I created 1 node. When I tried to edit the node, antispammer bot didn't allow me to do that. I think, it is wrong behavior. Try to use $op === 'validate' instead of $op === 'prepare'

6. I changed limit to 30 and added 2nd node. After I was able to add nodes, edit 1st node, but was not able to edit 2nd node. It showed 'Your account must be reviewed before being able to edit or create new contents. Contact the admin in case of doubt'

As I see, the module require much work and tests. Please, finish work on base features of module. Reviewing of modules with many bugs take a lot time.

alexmoreno’s picture

Hi sergei,

thanks a lot for your time in the review.

1. I have never seen the 1st issue, but basically because I have always worked in an environment with roles. Probably that was the problem, I have added a couple of lines to check what could it happen if there ara no roles in the site:

  // User roles in the site.
  $roles = user_roles();
  if(empty($roles)){
   	$roles = array();
  }

2. Deleted these lines, you are right, you do not need a submit button here.

3. Deleted too, I was thinking a couple of steps forward in the evolution of the module.

4. Added the argument in all variable_get functions.

5. It is a deliberate behaviour. The bots consumes a lot of time in your site, and resources. Removing them completely the access to your forms limits the access to sensible sections of your site, and processing time in your server (of thousand of bots trying to submit forms). They simply have no access there.

I can be wrong with the approach of course, I'll be very happy to discuss it.

6. What it has happen here is that the time threshold is too short, 1 minute I think I remember. So, you have probably submitted two nodes in the same minute, so the bot has flagged you as possible spammer. I will double check this anyway.

What I am going to do is add the possibility to select the seconds between posts in which we can flag a user as spammer bot, so the admin site will be able to change this.

I am sorry you felt angry Sergei. I have tested the module in different installations and I did not get some of your problems, but I will try to be more careful in my next submit.

Thanks a lot for your time, I really appreciate it Sergei.

sergei_brill’s picture

I have added a couple of lines to check what could it happen if there ara no roles in the site:

No, problem is in other. you need use
'#default_value' => variable_get('antispammer_bot_safe_roles', array()),
instead of
'#default_value' => variable_get('antispammer_bot_safe_roles', 0),

alexmoreno’s picture

thanks a lot for your work sergei. I will try to help reviewing your module too.

problem in #3 corrected. I have also added a "block user" functionality when the time limit is passed.

alexmoreno’s picture

alexmoreno’s picture

Issue summary: View changes

Corrected urls

pgautam’s picture

Hi,

1) Please specify file responsibility in @file comment in antispammer_bot.module.
2) Function antispammer_bot_settings() form titles should be in t() function for module antispammer_bot.module.
3) hook_permission missing form module file for ( 'access arguments' => array("administer Antispammer bot") in line no 33).

Thanks,
Paritosh Gautam

alexmoreno’s picture

Hi pgautam,

thanks a lot for your help. This are my changes:

1)

/**
 * @file
 * Antispammer bot module.
 * Executes some automatic actions to limitate massive spam publishing.
 */

2) All added
3) Sorry, what do you mean? The hook is there:

function antispammer_bot_perm() {
  return array("administer Antispammer bot");
}

The form also have the permissions and in fact they seems to work fine:

'access arguments' => array("administer Antispammer bot"),

Thanks again.

joshi.rohit100’s picture

Please fix below :-

(1) You must remove all the variables defined in system_settings_form() in hook_uninstall.
(2) Your hook_menu implementation should use a file to define the form as :-

$items['admin/settings/antispammer_bot'] = array(
'title' => 'Antispammer bot settings',
'description' => 'Antispammer bot settings.',
'page callback' => 'drupal_get_form',
'page arguments' => array('antispammer_bot_settings'),
'file' => 'antispammer_bot.admin.inc' //file to define form
'access arguments' => array("administer Antispammer bot"),
);

As it will not include the function on each page load.

Suggesstions :-

All the functions you have created in module, can be defined in separate file. Module should contain only drupal hooks or some related functionality.

Cheers

joshi.rohit100’s picture

Status: Needs review » Needs work

Sorry forgot to change the status.

joshi.rohit100’s picture

Issue tags: -PAreview: review bonus

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

alexmoreno’s picture

Hi Joshi, and thanks a lot for your help.

1) done as suggested,
2) added includes/antispammer_bot.admin.inc

Regarding the suggestions, which one is the best way to load this separate file? I have seen a lot of modules and I use to use the module_load_include like that:

module_load_include('inc', 'module', 'includes/file_to_load');

But, where this function should be loaded? At the beginning of the file or (as i guess) just when a function is going to use it?

Thanks again.

alexmoreno’s picture

Status: Needs work » Needs review

ok, everything moved to .inc (non hook of course), uninstall in .install file and everything tested and "cleaned" :-).

Thanks for your suggestions and help joshi.

alexmoreno’s picture

Issue tags: +PAreview: review bonus

I already added 3 new reviews, but anyway, I have been collaborating lastly, I have enough reviews to share :-):

Thanks.

bappa.sarkar’s picture

Manual Review

Rather than passing entire user object pass only $user->roles when you are calling _anmmer_bot_is_saftispae_role($user) in below function

function _antispammer_bot_user_permissions() {
  // Return value.
  $has_permissions = FALSE;
  // Current user.
  global $user;

  if (($user->uid == 1) || (_anmmer_bot_is_saftispae_role($user))) {
    $has_permissions = TRUE;
  }

  return $has_permissions;
}

Then you can modify the function function _antispammer_bot_is_safe_role($user) as below

function _antispammer_bot_is_safe_role($roles) {
  $is_safe = FALSE;

  // We fetch the rID's (role id's).
  $rids = array_keys($roles);

  $safe_roles = variable_get('antispammer_bot_safe_roles', array());
  
  if (!empty(array_intersect($rids, $safe_roles))) {
    $is_safe = TRUE;
  }

  return $is_safe;
}
alexmoreno’s picture

Hi bappa,

thanks for your review.

Added the a user paramter in _antispammer_bot_violates_limits, so we can reuse the same user object:

function _antispammer_bot_violates_limits($node) {
  // Responsible user.
  global $user;
  // Innocent until shown otherwise.
  $possible_spammer = FALSE;

  if (antispammer_bot_is_active($user)) {

I need to pass all the user so we also use uid, not just roles. So the _antispammer_bot_user_permissions function also changes:

function _antispammer_bot_user_permissions($user) {
  // Return value.
  $has_permissions = FALSE;

  if (($user->uid == 1) || (_antispammer_bot_is_safe_role($user->roles))) {
    $has_permissions = TRUE;
  }

  return $has_permissions;
}

and the last one, as suggested:

function _antispammer_bot_is_safe_role($roles) {
  $is_safe = FALSE;

  // We fetch the rID's (role id's).
  $rids = array_keys($roles);

  $safe_roles = variable_get('antispammer_bot_safe_roles', array());

  foreach ($rids as $role) {
    if (in_array($role, $safe_roles)) {
      $is_safe = TRUE;
    }
  }

  return $is_safe;

thanks again.

alexmoreno’s picture

I am finally not using this:

function _antispammer_bot_violates_limits($node) {
  // Responsible user.
  global $user;
  // Innocent until shown otherwise.
  $possible_spammer = FALSE;

  if (antispammer_bot_is_active($user)) {

as it was throwing a warning because a non used variable. The final code is:

function _antispammer_bot_violates_limits($node) {

  // Innocent until shown otherwise.
  $possible_spammer = FALSE;

  if (antispammer_bot_is_active()) {

and global user is loaded inside the antispammer_bot_is_active function.

bappa.sarkar’s picture

Status: Needs review » Needs work

Please change below code in function _antispammer_bot_is_safe_role($roles)

foreach ($rids as $role) {
    if (in_array($role, $safe_roles)) {
      $is_safe = TRUE;
    }
  }

Please replace with below code as the foreach is time consuming and once after $is_safe = TRUE no need to loop through till end.

if (!empty(array_intersect($rids, $safe_roles))) {
    $is_safe = TRUE;
}
alexmoreno’s picture

bappa, your ammend throws this error:

PHP Fatal error: Can't use function return value in write context in /var/www/html/bounty/sites/all/modules/antispammer_bot/includes/antispammer_bot.inc on line 113

thank you.

alexmoreno’s picture

solution:

  $intersect = array_intersect($rids, $safe_roles);
  if (!empty($intersect)) {
    if (in_array($role, $safe_roles)) {
      $is_safe = TRUE;
    }
  }
bappa.sarkar’s picture

use below

$common_roles = array_intersect($rids, $safe_roles);
if (!empty($common_roles)) {
    $is_safe = TRUE;
}
bappa.sarkar’s picture

You dont need the second if

alexmoreno’s picture

yep, the review tool throw the issue of non used variable. I forgot to remove it.

Thanks bappa, quite interesting changes, very performance focused :-).

alexmoreno’s picture

does it really worth to use intersect bappa? I mean, this array will never be bigger than 10 or 20 elements, and array_intersect is good with big arrays. Maybe a while function would be better?

bappa.sarkar’s picture

Well I think avoiding loop/ nested loop is always better if you have an option to use php primitive function.

Thanks for changing :)

alexmoreno’s picture

Status: Needs work » Needs review

needs review

joshi.rohit100’s picture

Status: Needs review » Needs work

Hi @urwen

Reviewd points :-

(1) In your _antispammer_bot_latest_post_date($uid) method, if you only required created field, then you should not use '*' in your db_query.

(2) In system settings form, you are setting default value for field 'antispammer_bot_peruser_nodes_threshold' is 2 but in method '_antispammer_bot_violates_amount_limit' , you are seting default 4 which is quite ambiguous.

look above issues.

cheers

alexmoreno’s picture

Status: Needs work » Needs review

Hi joshi.rohit100,

did as recommended and tested, thanks for your suggestions.

joshi.rohit100’s picture

Status: Needs review » Needs work

Hello @urwen.

In antispammer_bot.admin.inc file, you referred about INSTALL.txt file for admin settings but there is no INSTALL.txt file.

Also in antispammer_bot.inc, method _antispammer_bot_latest_post_date() is returning the last created node's timestamp but comment says that it will return the node.

Otherwise your module now looks fine to me but waiting for few more responses.

alexmoreno’s picture

Status: Needs work » Needs review

very good eye joshi.

Changes done, thank you very much for your help.

alexmoreno’s picture

Issue summary: View changes

Pledge added

klausi’s picture

Issue summary: View changes

removed automated reviews

klausi’s picture

Issue tags: -PAreview: review bonus

Removing review bonus tag, you have not done all manual reviews, you just posted the output of an automated review tool. Make sure to read through the source code of the other projects, as requested on the review bonus page.

I also removed the automated reviews from the issue summary.

alexmoreno’s picture

alexmoreno’s picture

please, note that some reviews start with a simple code review, but after that there are several tests in different posts, manual code reviews, recommendations, etc.

For example, yesterday:
https://drupal.org/node/2065035#comment-7859035

or that one: https://drupal.org/node/2088341#comment-7858117

Thanks for your time Klausi, and sorry for the confusion.

klausi’s picture

Assigned: Unassigned » dman
Status: Needs review » Reviewed & tested by the community
Issue tags: -PAreview: review bonus

Sorry about that, please add all your valid review comment links to the issue summary, so that we can track them.

manual review:

  1. Very similar modules exist such as https://drupal.org/project/node_limit and https://drupal.org/project/node_limitnumber and https://drupal.org/project/user_quota . Please add differences to existing modules to your project page, see also https://drupal.org/node/997024
  2. _antispammer_bot_user_permissions() and _antispammer_bot_is_safe_role() are not needed, just use user_access() to check wether the user has the permission or not. Instead saving your own antispammer_bot_safe_roles variable you could just assign the permission to those roles. Just create an additional permission "bypass antispammer bot limits".

But otherwise looks good to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Assigning to dman as he might have time to take a final look at this.

alexmoreno’s picture

Thanks a lot klausi. I'll do the reviews and rest of changes this same evening. Anyway, I've learnt a lot reviewing code in some sprints and at home. I'll continue collaborating, if I can help you somehow just let me know.

Best regards.

alexmoreno’s picture

Issue summary: View changes

fixed item

alexmoreno’s picture

Issue summary: View changes

new reviews added

alexmoreno’s picture

Issue summary: View changes

added one new review

alexmoreno’s picture

Issue tags: +PAreview: review bonus

1. done,
2. I love the change, it looks more simple and it uses the Drupal user permissions interface, thanks a lot klausi. Changes done, tested and commited.

I've just added a text message to explain how to bypass the limitations:

  $form['antispammer_bot_peruser_message'] = array(
    '#type' => 'markup',
     '#value' =>t('<p>Note: Bypass this limitations selecting "safe roles" in your user permissions</p>'),
  );

Maybe it is not 100% necessary.

New reviews added too, so bonus review tag is back with your permission :-).

Thanks a lot for your time, much appreciated.

alexmoreno’s picture

Issue summary: View changes

added similar modules and detailed explanation of differences with the Antispammer bot module.

alexmoreno’s picture

new "fresh" review added in the issue summary: https://drupal.org/node/2087025#comment-7862959

Thanks

dman’s picture

Status: Reviewed & tested by the community » Fixed

Very good collaborative work in the reviews here! Great to see.

Visual review only :
* I read the whole project page through twice, but I'm still unclear on where this differs from some of the other methods. Surely a number of existing solutions also allow you to block user accounts based on bad behavior? But still, the work here seems extensive and sounds useful to you.
* It's a bit disappointing reviewing a 'new' module for Drupal6 this month. But there is no rule against it.
* Code stucture looks fine. Internal documentation is good.
* I'm curious that you needed to do all that date parsing in _antispammer_bot_violates_time_limit(). Surely just comparing timestamps directly would have saved you 12 lines of calculation?
* I won't complain about going direct to the database like you do here in D6, but that would need improvement in D7.
* I don't see any security concerns here, and I'll expect that any further user issues will be able to be found and tested in the issue queue.
Generally, I'm content with this.

BASED ON THE REVIEWS BY OTHERS ABOVE, and the RTBC status :

-----

Thanks for your contribution, urwen!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

dman’s picture

Issue tags: -PAreview: review bonus

removing bonus tag

alexmoreno’s picture

Hi dman,

thanks a lot for your time and manual review. I will make the changes as you suggest, and I absolutely agree about D6. Some of my current projects are still under D6, but my plans once I finish next month in my current company is to migrate it to D7 and specially D8, which changes I reaaaaly love (twig, symfony, OO, ...).

Thank you everyone who has contributed with his time and effort: pgautam, sergei, joshi, bappa and of course, Klausi and dman. You all guys are the reason why Drupal is so great :-).

Warm regards.

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

Issue summary: View changes

added a new review