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

Fix follow limit preventing re-following of a moved account #14207

Merged
merged 1 commit into from
Dec 18, 2020

Conversation

Gargron
Copy link
Member

@Gargron Gargron commented Jul 3, 2020

ImportService already enforces follow limit.

@ClearlyClaire
Copy link
Contributor

Seems sensible to me, the tests just need to be updated, as they assume rate limiting is enabled, while it's off by default (but on in all codepath that matter)

@Gargron Gargron force-pushed the fix-follow-limit-on-unfollow-follow-worker branch from 3411896 to 07b982c Compare September 17, 2020 19:52
@ClearlyClaire
Copy link
Contributor

Actually, I missed that the Import::RelationshipWorker calls FollowService without setting the with_rate_limit parameter, thus making it trivial to bypass the follow rate limiting by going through the import functionality.

@Gargron
Copy link
Member Author

Gargron commented Sep 21, 2020

Right, I don’t think imports were ever rate limited, I’m not sure if there is a user-friendly way to do it. Like it would suck to only import 200 follows per day (or whatever) and force the user to re-submit the same file every day. We could maybe schedule individual follows into the future to align with the rate limit?

@ClearlyClaire
Copy link
Contributor

Right, I don’t think imports were ever rate limited

I think right now they are, since this PR changes it? I'm not sure we should enforce this limit, but if we don't enforce it on list import, I don't know why we should have this limit at all.

@Gargron
Copy link
Member Author

Gargron commented Dec 15, 2020

I think right now they are, since this PR changes it? I'm not sure we should enforce this limit, but if we don't enforce it on list import, I don't know why we should have this limit at all.

I realized you are mixing up rate limits and follow limits here. Imports were never rate limited, but they were (unintentionally) follow limited. This PR changes the latter, but not the former. The change in the spec is only due to the fact that the spec tests follow limits.

@Gargron Gargron force-pushed the fix-follow-limit-on-unfollow-follow-worker branch from 07b982c to fd9bfa1 Compare December 15, 2020 04:44
@ClearlyClaire
Copy link
Contributor

Alright, I did indeed mix the total follow limit and the rate limiting (but the two being tied together in this PR doesn't help). Rate limiting indeed never applied to imports, it wasn't applied before this PR, and isn't after this PR either.

However, with this PR, the total follow limit can be bypassed by using the follow import functionality, as the service will refuse importing more than the total number of accounts the person is allowed to follow at once, but calling it repeatedly would just work.

@Gargron Gargron force-pushed the fix-follow-limit-on-unfollow-follow-worker branch from fd9bfa1 to c68c449 Compare December 15, 2020 18:49
@Gargron
Copy link
Member Author

Gargron commented Dec 15, 2020

Fixed that

@ClearlyClaire
Copy link
Contributor

Technically susceptible to race conditions, I think.

@ClearlyClaire
Copy link
Contributor

There are other issues with this I did not think about:

  • if you import a list that contains more than FollowLimitValidator.limit_for_account(@account) - @account.following_count accounts, it will silently cut off the list in whatever order it was provided even if most of those accounts are people you already follow
  • in overwrite mode, it will make you unfollow everyone but not let you follow more than your limit minus your previous number of followed people

@Gargron Gargron force-pushed the fix-follow-limit-on-unfollow-follow-worker branch from c68c449 to 43a9f95 Compare December 18, 2020 05:09
@Gargron Gargron force-pushed the fix-follow-limit-on-unfollow-follow-worker branch from 43a9f95 to 4a273bf Compare December 18, 2020 05:28
Copy link
Contributor

@ClearlyClaire ClearlyClaire left a comment

Choose a reason for hiding this comment

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

I'm a bit uncomfortable with tying the total follow count limit and the rate limiting together in the Follow model validation, as well as with duplicating followers count logic, but the former is nitpicking and the latter is justified by the new Import validation, which is a clear improvement.

There's still the issue of possible race conditions that can be exploited to import more than the limit, but this can be addressed in a follow-up PR.

@Gargron Gargron merged commit eb35be0 into master Dec 18, 2020
@Gargron Gargron deleted the fix-follow-limit-on-unfollow-follow-worker branch December 18, 2020 08:18
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