[ENG-10340] Check for flag by bodintsov · Pull Request #11792 · CenterForOpenScience/osf.io

bodintsov · 2026-07-01T10:01:02Z

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 · 2026-07-02T15:17:42Z

+        return False
+
+    text = name_content.lower()
+    if any(suffix in text for suffix in ['m.sc.', 'msc.', 'phd.', 'ph.d.', 'msc.pt', 'pt.', 'prof.', 'dr.', 'md.', 'jd.', 'esq.']):


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 · 2026-07-06T14:18:01Z

+    if candidate.startswith('www.'):
+        candidate = f'https://{candidate}'


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.

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

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 · 2026-07-06T14:34:20Z

+            'given_name': 'Judith',
+            'family_name': 'Preuss',
+            'middle_names': 'https://www.google.com',
+            'suffix': 'M.Sc.',


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 · 2026-07-06T14:41:10Z

        ret = super().save(*args, **kwargs)  # must save BEFORE spam check, as user needs guid.

-        if has_domain and not was_creating:
+        if has_domain and self.is_hammy:


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.

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 · 2026-07-08T14:24:22Z

+    def test_validate_domain_in_field_for_existing_not_ham(self):
+        self.user.fullname = 'google.com'
+        self.user.save()
+        assert self.user.is_spammy is True
+        assert self.user.is_hammy is False
+
+        self.user.unspam(save=True)
+        self.user.fullname = 'not a url'
+        self.user.save()


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.

brianjgeiger

One thing to change from the comments.

Also, could you make a test that if a user tries to make an account with a non-notable domain in its name but it's not an unregistered user, that the account creation succeeds? So if a regular user like Brian google.com Geiger tries to register normally it succeeds? This should only target unregistered contributors as things stand.

I think that will do it.

brianjgeiger · 2026-07-09T19:18:37Z

+    def test_validate_domain_fields_notable_domain_in_field_unregistered_contributor(self):
+        NotableDomain.objects.get_or_create(domain='google.com', note=NotableDomain.Note.IGNORED)
+        user = OSFUser.create_unregistered(fullname='google.com')
+        user.save()
+        assert user.is_spammy is False
+        assert user.is_hammy is True


I'm not sure this should be true.

brianjgeiger · 2026-07-09T19:19:49Z

+            has_domain, status = has_domain_in_user_fields_for_names(self)
+            should_mark_ham = True if status == SpamStatus.HAM else False


I'm not convinced about this logic. I don't think we want to mark the user as ham if they have an ignored domain in their user parts. I think we just want to ignore domains that are presumed ham or are ignored.

brianjgeiger

I think one last test change and we're good. Should be simple.

brianjgeiger · 2026-07-10T13:21:40Z

+            setattr(self.user, field, 'Brian')
+            self.user.save()
+            self.user.refresh_from_db()
+            assert self.user.is_spammy is False


This doesn't look like it's testing what the test name is suggesting. This looks like it's taking an existing user and setting all of the name fields to "Brian" and saving the object and making sure that doesn't spam the user, which I wouldn't expect it to since you're not setting a domain on any of those fields.

So I see a test above does the testing of the user that already exists. Can we make this a new user creation? Or is that already happening that way because AuthUserFactory() doesn't finish creating the new user until the save happens? If it is already happening that way, then we probably don't need that test. If it's not, then we probably want to do something like user = AuthUserFactory(full_name='google.com') or similar to make sure we're catching the proper case. Might be safest just to create the user in the test with the full_name as part of the factory and ensure it doesn't spam.

brianjgeiger · 2026-07-10T13:36:33Z

+        if was_creating:
+            if self.is_hammy and has_domain:
+                self.flag_spam()
+            if not self.is_hammy and has_domain:
+                self.confirm_spam()


Is this even necessary with L1074-L1075 below?

brianjgeiger · 2026-07-10T13:41:45Z

@@ -1059,17 +1059,20 @@ def save(self, *args, **kwargs):
        if not self.is_spammy:


Hmm. I wonder if this should be:

Suggested change

if not self.is_spammy:

if was_creating:

brianjgeiger · 2026-07-10T13:45:46Z

+                self.confirm_spam()
+
+        if has_domain and was_creating:
+            raise ValidationError('Invalid personal information.')


Okay, one last thing. Since the user gets created on L1066, we'll need to add a spam status to the user object. Should say "Domain in name fields". Unless we move this ValidationError above L1066, in which case the user will never be saved.

brianjgeiger · 2026-07-10T14:10:07Z

+        if not self.is_hammy:
+            self.deactivate_account()


I think you can revert this change out with the other changes we're making.

bodintsov force-pushed the fix/use-account-get-disabled branch 2 times, most recently from 26e58cf to 4c6cf27 Compare July 1, 2026 14:13

brianjgeiger reviewed Jul 2, 2026

View reviewed changes

sh-andriy requested changes Jul 3, 2026

View reviewed changes

cslzchen force-pushed the feature/pbs-26-13 branch from a29364e to 3308e4e Compare July 5, 2026 15:33

bodintsov force-pushed the fix/use-account-get-disabled branch from f825b0d to 8ca3c5c Compare July 6, 2026 09:19

bodintsov added 4 commits July 6, 2026 12:32

fix REGEX

d0c735e

revert REGEX, add tests, improve validator

243dcda

Flake8

9d2a552

revert regex change, add tests, add urlsplit

baa5c5f

bodintsov force-pushed the fix/use-account-get-disabled branch from 8ca3c5c to baa5c5f Compare July 6, 2026 09:32

brianjgeiger reviewed Jul 6, 2026

View reviewed changes

bodintsov added 3 commits July 8, 2026 16:57

apply new logic, add check for notable domains, add tests

8445fc7

flake8

35ee1be

consistent naming

088b98c

brianjgeiger reviewed Jul 8, 2026

View reviewed changes

bodintsov added 2 commits July 8, 2026 17:54

logic improve, fix tests

f90791a

flake8

10b3ec3

brianjgeiger reviewed Jul 9, 2026

View reviewed changes

bodintsov added 2 commits July 10, 2026 11:48

fix logic, tests

3097814

flake8

a6b12ad

brianjgeiger requested changes Jul 10, 2026

View reviewed changes

brianjgeiger reviewed Jul 10, 2026

View reviewed changes

		if candidate.startswith('www.'):
		candidate = f'https://{candidate}'

		has_domain, status = has_domain_in_user_fields_for_names(self)
		should_mark_ham = True if status == SpamStatus.HAM else False

		@@ -1059,17 +1059,20 @@ def save(self, args, *kwargs):
		if not self.is_spammy:

Uh oh!

Conversation

bodintsov commented Jul 1, 2026

Ticket

Purpose

Changes

Side Effects

QE Notes

CE Notes

Documentation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sh-andriy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brianjgeiger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brianjgeiger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brianjgeiger Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

brianjgeiger Jul 10, 2026 •

edited

Loading