[ENG-10340] Check for flag

[bodintsov]

Ticket

https://openscience.atlassian.net/browse/ENG-10340

Purpose

Fix the user's account getting disabled

Changes

Added a check for the frag in the method

Side Effects

TBD

QE Notes

TBD

CE Notes

TBD

Documentation

TBD

[brianjgeiger]

This is far too specific and only handles the cases we know of, not the potential future cases. I think we need to adjust the DOMAIN_REGEX to be better (it should grab complete URLs), and also maybe if the domain regex finds something, pass the suspected domain through urllib.parse.urlsplit to see if it has a scheme and a netloc. If it doesn't have both of those, we could probably return false, since it won't link in an email to a location.

[sh-andriy]

@bodintsov I agree with @brianjgeiger, the suffix list is just going to rot as new name formats show up. The urlsplit idea (only count it if it parses to a scheme + netloc) is the cleaner, future-proof version.

One thing though: even a perfect regex only fixes the trigger, not the disable itself. Accounts still get disabled with SPAM_ACCOUNT_SUSPENSION_ENABLED off because user.confirm_spam() calls deactivate_account() directly (user.py:1485), skipping both the suspension flag and the is_hammy check that suspend_spam_user has. So the save() → confirm_spam() path can still nuke an assumed-ham user. To actually hit the AC (assumed-ham never auto-disabled, manual only, excluded domain → flagged) we need to guard there too, flag instead of confirm_spam for hammy users.

[brianjgeiger]

There's no sense checking for parsed.scheme if you're going to add the scheme here. I'm guessing you've added that to handle the case of

        {
            'fullname': 'Judith Sarah',
            'given_name': 'Judith',
            'family_name': 'Preuss',
            'middle_names': 'www.google.com',
            'suffix': 'M.Sc.',
        },

If you're going to do that, we should probably a) not bother checking for parsed.scheme; and b) handle the case of google.com as well as www.google.com.

[bodintsov]

google.com and N.Sathesh are both the same for regular expression and urlsplit. It seems not to be possible to at the same time allow for names like that and prohibit URLs without protocol and www

[brianjgeiger]

Harrumph. I was really hoping urlparse would check for valid top level domains. Let's see if we can come up with something better than urlparse for this then. I'll give it some thought.

[brianjgeiger]

So this is problematic. M.Sc. is a valid domain name. I'm checking with Product on how we want to try to handle it.

[brianjgeiger]

If the user has been marked as ham or maaaybe presumed ham, we need to make sure that they are not marked as spam. We should definitely mark them as spam if their spam situation is unknown.

What we may want to do is only use the has_domain on presumed ham folks if they are being created as unregistered contributors, and not of they are already in the database. That may be covered by was_creating and maybe it should eb if (has_domain and was_creating) or (has_domain and not self.is_hammy) or similar. I'm not super-familiar with the spam system, so I don't know the ideal conditional to use here, but give some thought based on what I've mentioned above, and see if you can make some tests to cover each of the scenarios.

[brianjgeiger]

I talked with Product, and I think if we also add in something that doesn't spam a user if the domain it has is in the notable domains as ignored, that would prevent a lot of the problems we're having as well, and would explicitly handle the M.Sc domain problem below.

[brianjgeiger]

What we discussed on Slack was having the domains in the username field only flag as spam if you are creating a user for an unregistered contributor. That is the spam vector that it is explicitly trying to prevent. Right now, there are no situations where a user who exists already should be flagged as spam for including a domain in their user name parts. Eliminating that check will reduce the opportunity for this to fail in a way that is causing problems for real users now.