Purge Spammer issues

[Destroy666x]

• [x] You can purge a super admin (similar as the recently fixed userpruning.php bug, purgespammer_show() checks only for group permissions).
• [x] You can see the purge button for your own account and purge yourself. purgespammer_show() should check the uid.
• [x] purgespammer_show() function checks only $mybb->user['usergroup'] to match the group setting. is_member() should be used instead.
• [x] All inputs on the Purge Spammer page have wrong styling because they lack default MyBB .button/.textbox class.
• [x] The Ban or Delete Spammers setting in ACP says "It will still be optional on the Purge Spammer page.". But it's not optional there.
• [x] If I allow a group to use Purge Spammer in settings and that group doesn't have access to Mod CP, the button won't show in profile (only in posts). That's because it was placed in modoptions template.
• [x] The postbit button shows up in previews and announcements. $post_type should be checked.
• [x] Since the checkbox options were removed, I think user datahandler's function with $prunecontent = 1 should be used to delete the spammer. Otherwise some newly introduced stuff is not deleted.
• [x] Purged Spammer action in mod log (more info below)

[Starpaul20]

One other thing that should be changed is the way Purged Spammer appears in the Mod Logs. Right now it looks like this:

It should be just "Purged Spammer" for the Action with the spammer's username being listed under the Information section (like it is when you edit the profile).

[JN-Jones]

Since the checkbox options were removed, I think user datahandler's function with $prunecontent = 1 should be used to delete the spammer. Otherwise some newly introduced stuff is not deleted.

Just as a note: As said in the original issue, it's still possible to reintroduce the checkboxes (simply change the input type). But I've also thought about using $prunecontent but it's also possible to remove all content and ban the user (what we do on mybb.com) where you still need to prune all content without using the datahandler. I'd leave it as it is.

[Sama34]

Why can't the delete_user() method be used here?

[JN-Jones]

We use delete_user but without $prunecontent. How would you do it when all content is pruned but the user is banned instead of deleted (that's how we do it on the community forums)? You can't call delete_user as the user shouldn't be deleted but you'd still need to delete all content.

[Sama34]

Maybe we can somehow move that to another function/method, as to avoid duplication of code.

[JN-Jones]

Feel free, but with the current code it's not possible. But if you do so, you should also remove the hidden fields as it doesn't make sense then.

[JordanMussi]

@Destroy666x???

[Destroy666x]

As always, just needed to PR it...

Works fine for me. However, we still probably need to optimise the code as said above (hidden inputs and some queries were/are redundant), but that wasn't planned by me here. Someone should open another issue/PR for 1.8.1.

[JN-Jones]

What's with the things I mentioned above? The hidden inputs are still there so it's possible to reintroduce the checkboxes. But when I select "delete user" and also deselect eg "delete posts" they're still deleted as you use the datahandler now to delete the content. Which is also unnecessary as everything is already deleted at that point (the other cases are run before).

[Destroy666x]

Well, as I said above, the hidden inputs are completely unneccessary (I don't know why would one think "Hey! I like this bot's posts/threads/PMs! Let me keep them!") and should be removed together with some other code, including several queries.

But I didn't plan to do it and I doubt I will manage to do it before 1.8 - few days ago my PC died completely after 11 or 12 years and while looking for a new one I can only share my girlfriend's laptop.

So, I created a new issue for optimising it: https://github.com/mybb/mybb/issues/1112 and please merge this if no other issues because I may be not able to fix any potential incoming conflicts.

[Sama34]

@JN-Jones so you want to be able to ban/delete an user without deleting or actually deleting selected content (posts, threads, PMs)?

I see that being useful for non-spammers, however this was meant as a tool to be used against spammers. Can't it be left as a plug-in (introducing possible necessary hooks should suffice)?

Is it really necessary?

[JN-Jones]

That was my intention first, yes. However I also have nothing against deleting the content always. But it's still not possible to use the user handler as there's no option to delete only the content (when banning the user).

[Sama34]

You can purge a super admin

This shouldn't be happening if we are using the delete_user() method. For banning similar check to those done in the banning page from the ModCP should be done.

[Sama34]

We use delete_user but without $prunecontent. How would you do it when all content is pruned but the user is banned instead of deleted (that's how we do it on the community forums)? You can't call delete_user as the user shouldn't be deleted but you'd still need to delete all content.

That was my intention first, yes. However I also have nothing against deleting the content always. But it's still not possible to use the user handler as there's no option to delete only the content (when banning the user).

I think I see what you mean, you should be able to select what content to delete. You may want to delete/ban an user but keep their threads/posts but delete PMs, etc.

I think $prunecontent should be updated to be an array.

So in inc/datahandlers/user.php:

function delete_user($delete_uids, $prunecontent=0)
{
    //..

    if($prunecontent === 1 ||  is_array($prunecontent) && isset($prunecontent['privatemessages']))
    {
        $db->delete_query('privatemessages', 'uid IN('.$this->delete_uids.')');
    }

    //..
}

And in moderation.php:

$prunecontent = array();
if($mybb->settings['purgespammer_delete_pms'] && (!$mybb->usergroup['purgespammer_can_delete_pms'] || $mybb->usergroup['purgespammer_can_delete_pms'] && $mybb->get_input('delete_pms', 1))) // administrator wants to delete pms so unless the user-group can overwrite that decision, pms are gone
{
    $prunecontent['privatemessages'] = 1;
}

$plugins->run_hooks('foo');

if($delete_user == true)
{
    $userhandler->delete_user($uid, $prunecontent);
}
else
{
    // banning code...

    $userhandler->delete_user($uid, $prunecontent);
}

Considering https://github.com/mybb/mybb/issues/1112#issuecomment-51024736 .

@JN-Jones.

[Destroy666x]

This shouldn't be happening if we are using the delete_user() method. For banning similar check to those done in the banning page from the ModCP should be done.

That's not a job for these scripts only as the button in posts/profile should be hidden in this case as well... So a super admin check should be done in purgespammer_show() as in my PR because it works everywhere.

The "Stop Forum Spam API Key" setting should be moved to the "Stop Forum Spam" setting group IMO.

This may confuse users as the API key is not needed for other settings in "Stop Forum Spam". It's only needed for Purge Spammer, so it's in a correct place in my opinion.

"Allowed Usergroups" should probably be a per-usergroup permission under the "Moderator CP" settings tab.

Agree here, but so should be other newly introduced "groupselect" settings. I don't know why we introduced that instead of adding checkboxes to groups.

As for banning, I think you're overcomplicating it. A simple conditional in moderation.php should do it:

if($mybb->settings['purgespammerbandelete'] == "delete")
{
    //use userhandler with $prunecontent = 1
}
else
{
   //use relevant queries to ban user, delete threads/posts/pms/reputations, clear only necessary fields, etc.
}

[Sama34]

That's not a job for these scripts only as the button in posts/profile should be hidden in this case as well... So a super admin check should be done in purgespammer_show() as in my PR because it works everywhere.

Ok but the banning link is still visible for superadmins to non-sueradmins in profiles and the pages are even correctly displayed until you summit it and the error() shows up, right? Similar behaibour should be done. purgespammer_show() seems really weird to me.

This may confuse users as the API key is not needed for other settings in "Stop Forum Spam". It's only needed for Purge Spammer, so it's in a correct place in my opinion.

Imagine new plugins making use of the setting. If the plugin documents to go to Purge Spammer -> Stop Forum Spam API Key wouldn't it make more sense to document to go to Purge Spammer -> Stop Forum Spam API Key?

Makes no difference really, but we have a group for "Stop Forum Spam" and all related settings to the service should go there. "Purge Spammer Settings" should have a yes/no setting to whether use SFS or not.

IMO this feature was implemented on a kinda of messy way, really. No offense to anyone. Whoever implemented this.

As for banning, I think you're overcomplicating it. A simple conditional in moderation.php should do it:

That defeats the purpose of concentrating all deletion in one place when users are deleted and/or their content.

I need a nap, now. If the user is to be deleted the datahandler should be used, where my code kinda of explains how it should be used. But you are correct for banning. My code will even delete the user always :p

Should be:

if($delete_user == true)
{
    $userhandler->delete_user($uid, $prunecontent);
}
else
{
    // banning code... similar to delete_user() but without actually deleting the user.
}

Issue being, we don't want duplicate code.

[Destroy666x]

Ok but the banning link is still visible for superadmins to non-sueradmins, right?

I don't really understand. My PR hides the button in postbit/profile if the 'spammer' (suspect) is super admin. Otherwise anyone with permissions would be able to click the button and either waste time or possibly ban the super admin. is_super_admin() check prevents that and I don't see any problem with it.

If the plugin documents to go to Purge Spammer -> Stop Forum Spam API Key wouldn't it make more sense to document to go to Purge Spammer -> Stop Forum Spam API Key?

Well, maybe you're right, but then we should make clear in this setting's description that the API key is only needed for Purge Spammer by default. Or people will think that they have to register here: http://www.stopforumspam.com/signup in order to run the SFS checking/blocking feature and then refrain from using it.

Issue being, we don't want duplicate code.

Well, it's not really a duplicate since there is no other place that bans user and removes his content.

[JN-Jones]

My suggestion to get this fixed:

Move the delete content part to a new function inside the user datahandler. Change delete_user to call that function when $prunecontent is true. Now in moderation.php:

$userhandler = [...];
if($delete_user)
    $userhandler->delete_user($uid, true);
else {
    $userhandler->delete_content();
    // Ban user
}

Remove the hidden fields and the page where it shows what will be deleted, instead say something like This will delete all users content and ban/delete the user.

Anything else what I've missed here? Note that this would be very urgent.

[Destroy666x]

^ Good idea. But please do it in the new issue: https://github.com/mybb/mybb/issues/1112 and finally merge this since everything is done here..

[JN-Jones]

What's so difficult with updating your PR with the change? As said: atm all content is deleted twice (first within moderation.php, then in the userhandler) when the user is deleted. And as delete_user runs also select queries this may (haven't tested this case yet) cause problems.

[Destroy666x]

What's difficult is that I'm mainly on tablet atm (no git here) and I don't have almost any possibility to do it as I said yesterday in of my comments above...

But I didn't plan to do it and I doubt I will manage to do it before 1.8 - few days ago my PC died completely after 11 or 12 years and while looking for a new one I can only share my girlfriend's laptop.

And she is keen on IT too.. So waiting for me to add it to this PR isn't the best idea, especially when it's "very urgent". Merging my bugfixes and someone else applying the optimisation code afterwards would probably be a better option.

[JN-Jones]

Ok, going to do that

[Destroy666x]

Thank you.

[JN-Jones]

Created PR. Please check this ASAP!

[JN-Jones]

@mybb/sqa @Destroy666x @PaulBender @Stefan-ST @WildcardSearch @Sama34