Change REST API to return empty data for suspended accounts #14765

Gargron · 2020-09-09T21:10:53Z

Instead of HTTP 410, return empty account with { ... "suspended": true ... } in REST API
Exclude suspended accounts from queries
Return empty results instead of account's data

ClearlyClaire

It looks mostly fine to me, except:

I don't think suspended users should be hidden from someone's own followers/followed accounts, muted etc.: let people manage their relationship with people who have been suspended
I'd like additional tests, especially around the default URL thing
this whole thing is definitely acceptable as a first state, but i wouldn't accept it in a release that doesn't handle federating the fact that an account is suspended, otherwise this may lead to confusion as you wouldn't know a remote account is indeed suspended

app/controllers/activitypub/outboxes_controller.rb

ClearlyClaire · 2020-09-10T13:14:27Z

app/controllers/api/v1/accounts/relationships_controller.rb

-    accounts = Account.where(id: account_ids).select('id')
+    accounts = Account.without_suspended.where(id: account_ids).select('id')


Hm, maybe this should keep suspended accounts, so one can unfollow/block/etc. suspended accounts while those are suspended, in case they get reinstated.

I'm going for consistency with previous behaviour. Previous behaviour where account data was deleted immediately meant that all following/muting/blocking/etc relationships were erased the moment the account was suspended. And unless the suspension is cancelled within 30 days, all of it will still be deleted in the end. So it makes sense for me to keep it that way. If an account is unsuspended, then other users may worry about following/unfollowing/muting/unmuting etc.

Yes, but we're precisely changing the behavior here, to have suspension not be necessarily definitive. Regardless of the 30 days thing (which isn't implemented yet, and, depending on how federation is handled, might not be handled the same way by remote instances).

No, it is implemented in the other PR.

If another server does not handle suspension the same way, then the account will not look suspended there and their users will be able to continue to unfollow, mute, and block without issue. Then after 30 days or earlier, the Delete will arrive there.

I meant, if another implementation federates the suspension info but doesn't implement the 30 days-to-delete thing (either having a longer delay or no automatic deletion at all), there is no bound between the time an account is suspended and the time an account is reinstated. This can be very surprising if you have a “suspended” follower not listed anymore and they reappear months ago from seemingly nowhere.

I fear that by including suspended users on followers lists and allowing following/unfollowing them it takes away from the seriousness of a suspension in the eyes of the users. Imagine "Why is @Hitler8814 still in my followers, I have to block them myself?! This platform does nothing!"

You bring up a good point, what if another (malicious) server marks an account as suspended but allows that account to operate as normal, in that case the lacking controls would become a liability. But I propose that we move that discussion to federation of suspensions. While suspensions do not federate, a suspended account is merely a normal account on other servers up until it is deleted as usual.

I guess following shouldn't be allowed, but muting, blocking, unfollowing should.

Good point about an obviously offending account still appearing in followers. I was more viewing it from the perspective ofs someone with a locked account, and considering an account that isn't obviously offensive but has been suspended for reasons I may or may not be aware of. I am unsure what the exact solution for this should be, but I'm still uncomfortable with the following relationship being hidden from me, the person being followed or following them.

Following a suspended account is already not possible but I didn't change anything about MuteService, BlockService or UnfollowService so interacting with suspended accounts directly that way is possible.

ClearlyClaire · 2020-09-10T13:15:39Z

app/controllers/api/v1/blocks_controller.rb

+                               .joins(:target_account)
+                               .merge(Account.without_suspended)


This is probably fine in the blocks controller, but as above, I would prefer if an un-suspended user could still manage their relationships with a suspended user.

ClearlyClaire · 2020-09-10T13:17:52Z

app/controllers/api/v1/lists/accounts_controller.rb

-      @list.accounts.includes(:account_stat).all
+      @list.accounts.without_suspended.includes(:account_stat).all
    else
-      @list.accounts.includes(:account_stat).paginate_by_max_id(limit_param(DEFAULT_ACCOUNTS_LIMIT), params[:max_id], params[:since_id])
+      @list.accounts.without_suspended.includes(:account_stat).paginate_by_max_id(limit_param(DEFAULT_ACCOUNTS_LIMIT), params[:max_id], params[:since_id])


As above, I think this falls within “letting users manage relationships with suspended users”.

ClearlyClaire · 2020-09-10T13:18:02Z

app/controllers/api/v1/mutes_controller.rb

+                             .joins(:target_account)
+                             .merge(Account.without_suspended)


Same as for mutes.

ClearlyClaire · 2020-09-10T13:21:25Z

app/serializers/rest/account_serializer.rb

+  def locked
+    object.suspended? ? false : object.locked
+  end
+
+  def bot
+    object.suspended? ? false : object.bot
+  end


Not sure about those two. I understand the “blank account” thing but I'm not sure about reverting those. Also, if overwriting those, “locked” may make a bit more sense as true.

Again, mostly consistency with old behaviour. These values were reset during suspension so it makes sense for me to emulate it here.

Gargron · 2020-09-10T13:42:07Z

this whole thing is definitely acceptable as a first state, but i wouldn't accept it in a release that doesn't handle federating the fact that an account is suspended, otherwise this may lead to confusion as you wouldn't know a remote account is indeed suspended

I think this would be an appropriate critique for the PR this was extracted from but not this one. I am splitting the work in smaller chunks to make reviewing easier, so of course a single chunk will not be release-ready.

ClearlyClaire · 2020-09-10T13:55:43Z

I think this would be an appropriate critique for the PR this was extracted from but not this one. I am splitting the work in smaller chunks to make reviewing easier, so of course a single chunk will not be release-ready.

I completely agree, and I just wanted to make that clear. Also, I'm somewhat worried about that seemingly being tied to the current suspended_at boolean (thus not being able to distinguish between a remote suspension and a local one) but I expect this will be addressed by the AP-related PR.

…#14765)

Change REST API to return empty data for suspended accounts

18a4d9b

Gargron added the api REST API, Streaming API, Web Push API label Sep 9, 2020

Gargron mentioned this pull request Sep 9, 2020

Change account suspensions to be reversible by default #14726

Merged

ClearlyClaire self-requested a review September 10, 2020 13:07

ClearlyClaire requested changes Sep 10, 2020

View reviewed changes

krainboltgreene approved these changes Sep 10, 2020

View reviewed changes

Gargron merged commit e6b272e into master Sep 11, 2020

Gargron deleted the feature-suspended-api branch September 11, 2020 13:16

ClearlyClaire mentioned this pull request Sep 11, 2020

Improve handling of suspended accounts #14775

Open

thenameisnigel-old pushed a commit to ChatterlyOSE/Chatterly that referenced this pull request Sep 18, 2020

Change REST API to return empty data for suspended accounts (mastodon…

71b8794

…#14765)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change REST API to return empty data for suspended accounts #14765

Change REST API to return empty data for suspended accounts #14765

Gargron commented Sep 9, 2020 •

edited

Loading

ClearlyClaire left a comment

ClearlyClaire Sep 10, 2020

Gargron Sep 10, 2020

ClearlyClaire Sep 10, 2020

Gargron Sep 10, 2020

ClearlyClaire Sep 10, 2020

Gargron Sep 10, 2020

ClearlyClaire Sep 10, 2020

Gargron Sep 10, 2020

ClearlyClaire Sep 10, 2020

ClearlyClaire Sep 10, 2020

ClearlyClaire Sep 10, 2020

ClearlyClaire Sep 10, 2020

Gargron Sep 10, 2020

Gargron commented Sep 10, 2020

ClearlyClaire commented Sep 10, 2020

		accounts = Account.where(id: account_ids).select('id')
		accounts = Account.without_suspended.where(id: account_ids).select('id')

Change REST API to return empty data for suspended accounts #14765

Change REST API to return empty data for suspended accounts #14765

Conversation

Gargron commented Sep 9, 2020 • edited Loading

ClearlyClaire left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gargron commented Sep 10, 2020

ClearlyClaire commented Sep 10, 2020

Gargron commented Sep 9, 2020 •

edited

Loading