Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

new page /admin/recent_accounts #2824

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

kareila
Copy link
Member

@kareila kareila commented Aug 6, 2020

Right now when I'm checking recently created accounts for spammers, it requires a lot of time and resources on my part. So I brainstormed with @rahaeli about a way to crowdsource reviewing recently created accounts with trusted volunteers in a way that made it easy to participate on mobile devices for a few minutes here and there and distribute the workload. This is what came out of that conversation.

  • Newly created personal accounts have a new userprop, not_approved, set to 1 at account creation time.

  • Any accounts with not_approved set are excluded from /search, /latest, and /random. (New user method $u->is_approved faciliates this.)

  • After an hour, the new accounts will begin to appear on /admin/recent_accounts for review by users with the existing priv siteadmin:spamreports or the new priv siteadmin:approvenew.

  • One of these volunteers will review the journal and flag it as spam or not. If it's not, not_approved is cleared and the account is made available in the site features where it was previously excluded. If it is judged to be spam, not_approved is incremented from 1 to 2 and it goes into another queue to be suspended.

  • Another new priv, suspend:recent, is available for volunteers to suspend only accounts flagged from the /admin/recent_accounts page. Suspending or unsuspending an account will also clear the userprop.

All of this is switched off if LJ::is_enabled('approvenew') returns false. I left it turned off in the example config, but it should be active in production unless we explicitly disable it.

Screenshot of journal review form (yes it's an iframe):

Screen_Shot_2020-08-02_at_4 56 58_PM

kareila added 8 commits August 2, 2020 11:17
Values: 0/unset if OK, 1 if unexamined, 2 if flagged as spam

Supporting cast:

- user method $u->is_approved

- LJ::is_enabled('approvenew') - turned off in example config
Also: set_unsuspended wasn't actually being used, so let's do that.
Still work in progress - suspension not added yet.
The way I've set this up, the userprop has a value of 1 if the account needs to be reviewed, and a value of 2 if it has been reviewed and determined to contain spam. So anything with a value of 2 will go into a second queue for account suspension. Once the account is suspended, the userprop needs to be cleared to remove it from the suspension queue.
Shows up to 10 accounts at a time and lets the viewer uncheck any accounts they don't think should be suspended.
@azurelunatic
Copy link
Contributor

I like!!

unless $r->did_post;

my $dbr = LJ::get_db_reader();
my $sth = $dbr->prepare( "SELECT userid FROM userproplite2 "
Copy link
Member

@zorkian zorkian Aug 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are going to use the clustered property table (userproplite2), then you need to use cluster readers. Something like,

for my $cid ( @LJ::CLUSTERS ) {
    my $dbcr = LJ::get_cluster_reader( $cid ) or die;
    # look for users
}

The other option is to not cluster this property, and use the userproplite table, but to do that you need to set cldversion: 0 and indexed: 0 in the property settings.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, for the way I'm using it, I think it makes sense for it not to be clustered, but I thought it should be indexed if I was going to be searching the column for matches as opposed to fetching single user rows? Can you clarify that for me? The issue of clustering didn't come up in my testing because my dreamhack doesn't use clusters. (I thought the multihomed property designated whether it was clustered or not.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The naming on these properties is definitely wild.

cldversion means 'clustered dversion' i.e. if this field is non-zero, the property is clustered and not stored on the global database.

indexed has no effect on clustered properties. It applies to global properties (i.e. cldversion: 0).

multihomed however makes a clustered property also store a copy on the global db. But it also looks crufty and I'm not sure we should ever use it. It used to be used for properties that would be shown on the profile page (such as icq) but we stopped setting it and things work, so maybe we delete this feature? heh. 🔪

So, basically, what you probably want is cldversion: 0 and indexed: 1, that will allow you to do lookups like "where propid = ? and value = 1" and get back everybody who hasn't been approved yet while hitting an index.

FWIW, I think the indexing probably doesn't add much value since the cardinality of values is 2 and the size of the set is O(new users that haven't been reviewed). I would expect that we'd actually spend more time on the index management than we do on filtering out things anyway.

$count++;
}
else {
$u->set_prop( not_approved => 0 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot, does zero delete the property? I would rather not have the database fill up with zeros.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, zero deletes. (Line 1157 of cgi-bin/LJ/User/Permissions.pm)

$remote->has_priv( "suspend", "recent" ) && !$remote->has_priv( "suspend", "*" );

return $self->error("You can only suspend users using /admin/recent_accounts.")
if $recent_only;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs to handle the case where someone can suspend both recent and open ID, but does not have Global suspend.

Copy link
Member

@zorkian zorkian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small things

@kareila kareila marked this pull request as draft July 18, 2023 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants