Skip to content

[ENG-7873] CLONE - SPAM - When Hamming a Spammed user, preprints and registrations remain private #11125

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

Merged

Conversation

antkryt
Copy link
Contributor

@antkryt antkryt commented May 2, 2025

Purpose

fix was_public state when flag spam

Changes

  • correct check if node was public when flag_spam
  • use earliest confirm/flag spam log to check if node was public instead of the latest one

  • fix TypeError when check archiving status for stuck registrations (not related to ticket ENG-7873, but it's just one line permissible_addons = set(permissible_addons), so no additional testing is required)

QA Notes

I couldn't reproduce this issue via UI, but combination confirm_spam() -> flag_spam() -> ... breaks this feature. I'm not sure if it's exactly what's happening in our case, but since flag_spam() is used with automatic spam checks during node/preprint updates, it's quite possible.

Documentation

Side Effects

Ticket

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

@antkryt antkryt changed the base branch from feature/pbs-25-08 to feature/pbs-25-09 May 2, 2025 13:38
Copy link
Collaborator

@brianjgeiger brianjgeiger left a comment

Choose a reason for hiding this comment

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

Okay, this is a tricky one. Could you add a test that:

  1. Spams a public project
  2. Hams the project to make it public
  3. Make the project private
  4. Spam the private project
  5. Ham the project

The project should be private when that's all done.

@antkryt
Copy link
Contributor Author

antkryt commented May 6, 2025

Note that logging the privacy change is crucial in this context.
For example the following test will fail:

  def test_multiple_privacy_changing(self, project):
        project.set_privacy('public')
        assert project.is_public

        project.confirm_spam()
        assert not project.is_public

        project.confirm_ham()
        assert project.is_public

        project.set_privacy('private', log=False)  # log=True is crucial!!!
        assert not project.is_public

        project.confirm_spam()
        assert not project.is_public

        project.confirm_ham()
        assert not project.is_public

There are some suspicious places where the privacy log is not created:

These seem like edge cases, so I left them untouched as I don't know whether this behavior is expected

@brianjgeiger
Copy link
Collaborator

@antkryt So the spam system doesn't set a flag when the object is spammed to say whether it was public before the spamming happened or not? It's all just relying on logs?

@antkryt
Copy link
Contributor Author

antkryt commented May 8, 2025

@brianjgeiger correct. It was proposed and implemented in this ticket to fix multiple spam scenario. Some alternatives:

  1. add was_public_at_spam (or something) field to AbstractNode and Preprint models (easy to implement , but hard to extend/modify in the future)
  2. add something like SpamContext table with OneToOne relationship (harder to implement, but easy to extend)

We can implement was_public_at_spam separately for AbstractNode and Preprint, and just check something like "if the status isn’t initial, pending, or rejected, then it was public" as you suggested in the ticket comment section. That technically works, but honestly feels a bit unreliable and messy.
A much better approach (and not just for spam) would be to have a dynamic is_public property. That way we can centralize the logic "if it’s not initial, pending, or rejected, then it’s public" and reuse it anywhere we need to know if something was or can be public

@brianjgeiger
Copy link
Collaborator

@antkryt Okay, I've been chatting with Product on the Jira ticket, and we're going to make this so that, regardless of logs or whatever, if a preprint is not in initial, pending, or rejected moderation state, then it should be public. We'll want to make sure that preprints that were never made public don't suddenly become public, but I think they'll be in the initial state, even if they aren't moderated. But please verify that.

@antkryt
Copy link
Contributor Author

antkryt commented May 8, 2025

@brianjgeiger what about registrations and projects?

@brianjgeiger
Copy link
Collaborator

@antkryt Projects we'll continue to do the way we are. We might do registrations similarly to preprints, but there are more states and it's not as urgent, so let's leave registrations for the moment and revisit that if necessary later.

@brianjgeiger brianjgeiger changed the base branch from feature/pbs-25-09 to feature/pbs-25-10 May 16, 2025 13:05
@brianjgeiger brianjgeiger merged commit cbff71f into CenterForOpenScience:feature/pbs-25-10 May 16, 2025
6 checks passed
Johnetordoff added a commit to Johnetordoff/osf.io that referenced this pull request Jun 3, 2025
…cience/osf.io into refactor-notifications

* 'feature/pbs-25-10' of https://github.com/CenterForOpenScience/osf.io:
  fix issue where trying another already confirmed email threw an uncaught exception (CenterForOpenScience#11161)
  [ENG-8148] Add ArtifactOutcome in annotations to linked nodes  (CenterForOpenScience#11158)
  [ENG-7966] Add "collected-in" relationship for Nodes (CenterForOpenScience#11140)
  fix issue where not having any external identities caused a 500
  [ENG-7965] Add v2 email token confirmation endpoints (CenterForOpenScience#11139)
  [ENG-8052] Fixed FilterMixin issue with multiple values of notification subscription field (CenterForOpenScience#11150)
  support related_counts for view_only links (CenterForOpenScience#11148)
  allow admins change registration providers (CenterForOpenScience#11145)
  [ENG-7927] Improved logging for embargo termination (CenterForOpenScience#11137)
  [ENG-7873] CLONE - SPAM - When Hamming a Spammed user, preprints and registrations remain private (CenterForOpenScience#11125)
  Update changelog and package.json
  fix TypeError when check stucked registration
  revert async email sending (CenterForOpenScience#11134)
  [ENG-7921] Add scopes for applications to full_read and full_write scopes (CenterForOpenScience#11126)

# Conflicts:
#	api_tests/nodes/views/test_node_detail.py
#	api_tests/nodes/views/test_node_linked_registrations.py
#	framework/auth/oauth_scopes.py
#	tests/test_registrations/test_retractions.py
ihorsokhanexoft pushed a commit to ihorsokhanexoft/osf.io that referenced this pull request Jun 4, 2025
…registrations remain private (CenterForOpenScience#11125)

## Purpose
fix was_public state when flag spam

## Changes
- correct check if node was public when flag_spam
- use earliest confirm/flag spam log to check if node was public instead of the latest one
---
- fix TypeError when check archiving status for stuck registrations (not related to ticket ENG-7873, but it's just one line `permissible_addons = set(permissible_addons)`, so no additional testing is required)

## QA Notes
I couldn't reproduce this issue via UI, but combination `confirm_spam()` -> `flag_spam()` -> `...` breaks this feature. I'm not sure if it's exactly what's happening in our case, but since `flag_spam()` is used with automatic spam checks during node/preprint updates, it's quite possible.

## Ticket
https://openscience.atlassian.net/browse/ENG-7873
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants