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

include remote users in followers collection #418

Merged
merged 8 commits into from
Jan 15, 2024

Conversation

BentiGorlich
Copy link
Member

magazines returned the wrong amount of followers via ap because remote subscribers were not included

magazines returned the wrong amount of followers via ap because remote subscribers were not included
@BentiGorlich BentiGorlich added bug Something isn't working activitypub ActivityPub related issues backend Backend related issues and pull requests labels Jan 10, 2024
@BentiGorlich BentiGorlich self-assigned this Jan 10, 2024
asdfzdfj
asdfzdfj previously approved these changes Jan 12, 2024
Copy link
Contributor

@asdfzdfj asdfzdfj left a comment

Choose a reason for hiding this comment

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

this also shows remote subscribed user when querying for detailed list, while I can't find a group to query follower lists for cross checking, it looks like querying one for user would return similar results, so I think it should be fine

@BentiGorlich
Copy link
Member Author

BentiGorlich commented Jan 12, 2024

Since this is a privacy concern I think we should get rid of listing the users, but that is another issue

@e-five256
Copy link
Member

I am slightly confused as to why

$count = $this->magazineSubscriptionRepository->findMagazineSubscribers(1, $magazine)->count();
does this rather than using the subscription_count column value of the magazine, which seems like it would be a much less expensive call. But maybe there's a difference in the count that I'm not realizing from just reading it off-hand

@BentiGorlich
Copy link
Member Author

I think there is no difference. I tried it with

SELECT name, subscriptions_count, (SELECT COUNT(*) FROM magazine_subscription WHERE magazine_id=magazine.id) as count2 FROM magazine WHERE ap_id IS NULL LIMIT 10;

And the 2 counts were the same. I'll change it to the less expensive call and remove the output of the followers, as it is not used by anything and just a data leak

@BentiGorlich BentiGorlich marked this pull request as draft January 15, 2024 12:41
@BentiGorlich BentiGorlich marked this pull request as ready for review January 15, 2024 19:30
@e-five256
Copy link
Member

e-five256 commented Jan 15, 2024

remove the output of the followers

Is this still in progress or part of other work? Just wanted to check because it looks like this PR still has the exposed subscriber list. That's interesting, I didn't realize that about kbin/Mbin. Comparing it to lemmy it looks like they don't expose that information e.g. here's lemmy.world/c/world

{
  "id": "https://lemmy.world/c/world/followers",
  "type": "Collection",
  "totalItems": 34492,
  "items": []
}

Edit: It seems ?page=1 etc is what makes it go through this; I was a bit confused on that because I was like, "Page"? Like "Article"? I see it means if it gets an AP request for pagination it returns all the information on followers

@BentiGorlich
Copy link
Member Author

BentiGorlich commented Jan 15, 2024

Yeah it still includes the subscriber list. I didn't want to put that in this PR. I am aware of the problem. I just fixed that one can pull the local subscribers to a remote magazine, cause that just didn't make sense.
Yeah yeah the subscriber list is paginated. It's an AP feature.

Copy link
Member

@e-five256 e-five256 left a comment

Choose a reason for hiding this comment

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

this looks good to me. I think there's broader privacy questions we've come up with from this that might need further looking into. definitely the less information on users we can return that isn't being used for AP actions, the better

@BentiGorlich BentiGorlich merged commit b5b140d into main Jan 15, 2024
7 checks passed
@BentiGorlich BentiGorlich deleted the ap-fix-magazines-followers-collection branch January 15, 2024 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
activitypub ActivityPub related issues backend Backend related issues and pull requests bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants