Block/Close and wipe data for all accounts on local pod in admin UI #8249

tclaus · 2021-06-07T18:37:20Z

It solves #7464 and #7463

It adds the SuperTux88 / JHass' script to close and wipe an account - even remote ones.
For this function you now can search through all persons on your pod, not only the local user.
No external ruby code to remove spammers!
It changes the order of the lock/close buttons in Admin view by its severity.
It has some code refinements (Fixed Close / Lock/Unlock on Person and User classes)
It prevents an admin to remove its own admin status accidently.

Remote / Local user in users' search view

tclaus · 2021-06-07T19:08:36Z

Uh.. "Green" with first commit!

Flaburgan · 2021-06-07T21:57:55Z

Awesome work, thank you! One suggestion: use "Block" instead of "Close" when you're talking about remote account. Also, is the pub key of the user stored somewhere or definitely replaced? If the account is blocked indefinitely (can't be unblocked), I would warn about it in the confirm popup (you kept it for remote account, did you?).

tclaus · 2021-06-08T05:58:59Z

"Block" instead of "Close" when you're talking about remote account.

It was intended to 'block' external accounts and 'close' local ones. Do you think to change button names for local and remote accounts?

tclaus · 2021-06-09T18:39:14Z

Awesome work, thank you! One suggestion: use "Block" instead of "Close" when you're talking about remote account. Also, is the pub key of the user stored somewhere or definitely replaced? If the account is blocked indefinitely (can't be unblocked), I would warn about it in the confirm popup (you kept it for remote account, did you?).

Yes, I kept the confirmation.
About to change the buttons caption.

tclaus · 2021-06-09T18:49:49Z

It is intended that the serialized_public_key is set to "BLOCKED" ( but it don't work now)

Flaburgan · 2021-06-09T20:04:43Z

Yeah I would love confirmation on this by others @jhass @SuperTux88 what do you think, should we store the pub key somewhere in case the podmin wants to restore it, or can we go like this with irreversible block?

diaspora-code-review · 2021-06-10T06:22:19Z

app/views/admins/_user_entry.haml

+          .label.label-danger
+            Closed
+        - else 
+          -if person.locked_access?


Style/IfInsideElse: Convert if nested inside else to elsif.

The - symbol should have one space separating it from code

diaspora-code-review · 2021-06-10T06:22:19Z

app/views/admins/_user_entry.haml

+        - if person.closed_account?
+          .label.label-danger
+            Closed
+        - else 


Line contains trailing whitespace

diaspora-code-review · 2021-06-10T06:22:19Z

app/views/admins/user_entry/_close_action.haml

+    admin_close_account_path(person.id),
+    method: :post, data: {confirm: t("admins.user_search.are_you_sure")},
+    class: "btn btn-danger btn-block"
+  = link_to person.owner.present? ? t("admins.user_search.wipe_and_close_account") : t("admins.user_search.wipe_and_block_account"),


Line is too long. [132/120]

Should be ignored, I don't like to split the ternary into multiple lines. Other Ideas?

diaspora-code-review · 2021-06-10T06:22:20Z

app/views/admins/user_entry/_view_profile.haml

@@ -0,0 +1,4 @@
+- if !person.closed_account?


Style/NegatedIf: Favor unless over if for negative conditions.

tclaus · 2021-06-10T06:25:15Z

Changed the buttons label to "block" on external accounts and "close" for locals as it was before.
Moved the "Account Active" - label to the header. It now shows the states "Active", "Locked" and "Closed" in green, orange and red colors.

SuperTux88 · 2021-06-10T23:47:02Z

It is intended that the serialized_public_key is set to "BLOCKED" ( but it don't work now)

Don't do that. That's a hacky workaround from a manual script for people who know what they are doing and know how to fix it again when things breaks. It isn't guaranteed that manually changing keys won't cause problem in the future, so this is not something that should be done here (a similar hacky workaround with messing with the key in the past did break things eventually).

The clean solution would be just deny federation messages from closed accounts (maybe except AccountMigration messages, so you could still mark an account as merged even after it was closed?). And that way it then also could cleanly reject relayables (if a closed account tries to comment on a local post, like if the person is blocked).

And if the button closes an account it should say "close", not "block".

The federation blocking from closed accounts doesn't necessarily need to be done in the scope of this PR, but just don't modify the key here.

Flaburgan · 2021-06-11T08:08:25Z

And if the button closes an account it should say "close", not "block".

But what does "Close" mean for a remote account?

SuperTux88 · 2021-06-11T11:35:55Z

Maybe "Close locally" or something?

The alternative would be to introduce a "blocked" state of a person and use that instead. The advantage of that would be, that it can be undone, and can be differentiated from the closed state (so people trying to visit a blocked account could get a "this account was blocked" message instead of the message that the account was closed), and we could still see when the account was closed for real (by either the user itself or the remote podmin) ... and we could even keep the "blocked" state when somebody migrates a previously blocked account to a new pod, which we probably want to block again.

tclaus · 2021-06-12T06:37:04Z

@SuperTux88
Can you please give some more detailed answers on this: ?

Don't delete public Key,right? Remove: app/workers/wipe_account.rb:46 ?
The Text on button: "Close" (as before), but "Close locally" on remotes. Is is OK to name "Wipe and close". Look into the UI: it's clear which user is local or remote, so it might be redundant to name the button "close locally". See first screenshots here.
A "Block" state may come with a model change. Shouldn't be done in this PR.
Can you please give a line to

deny federation messages from closed accounts

"Closed" accounts still spill new posts or comments into the pod. But I am still not so deep into architecture to find the 'right' point in source.

SuperTux88 · 2021-06-12T17:48:35Z

Don't delete public Key,right? Remove: app/workers/wipe_account.rb:46 ?

Yes

The Text on button: "Close" (as before), but "Close locally" on remotes. Is is OK to name "Wipe and close". Look into the UI: it's clear which user is local or remote, so it might be redundant to name the button "close locally". See first screenshots here.

To people who know how diaspora works, it should be clear that "close" of a remote account doesn't allow to close the account on other pods. But people who are new to diaspora "close locally" maybe is cleaner.

A "Block" state may come with a model change. Shouldn't be done in this PR.

If a blocked state is added later, the button could be changed to use that then (and not close the account anymore) ... the button then can be renamed to "block" and blocking can also be undone then. But I think it's fine to first do it with closing in this PR and do the separate block state later (also I think this can then be merged to 0.7.x while the blocking then can go to 0.8.x because of model change ... but if it's separated the 0.7.x people can also already profit from it)

Can you please give a line to

See my comment here: #8228 (comment) (sorry, I accidentally submitted that comment too early, but it's now complete)

diaspora-code-review · 2021-06-15T06:45:11Z

app/views/admins/user_entry/_close_action.haml

+    admin_close_account_path(person.id),
+    method: :post, data: {confirm: t("admins.user_search.are_you_sure")},
+    class: "btn btn-danger btn-block"
+  = link_to person.owner.present? ? t("admins.user_search.wipe_and_close_account") : t("admins.user_search.wipe_and_close_local_account"),


Line is too long. [138/120]

tclaus · 2021-06-15T06:49:34Z

@SuperTux88
Changed UI facing texts.
Added person.closed_account? in Diaspora_federation.

To block receiving messages is mainly implemented in #8228 and will come into this code after merging. (Added a TODO on the lines) Will clean this lines while expecting merge conflicts then.

goobertron · 2021-07-14T21:05:01Z

This looks very useful, and thanks for working on it, @tclaus. Just one question: are the user accounts in your screen-shots real accounts or did you invent them? If they're real accounts, would you mind blurring the personal details in your screenshots, or replacing them with invented ones? Just for personal privacy.

tclaus changed the title ~~Close remote accounts in admin UI~~ Close remote accounts on local pod in admin UI Jun 8, 2021

tclaus changed the title ~~Close remote accounts on local pod in admin UI~~ Block/Close and wipe data for all accounts on local pod in admin UI Jun 8, 2021

diaspora-code-review reviewed Jun 10, 2021

View reviewed changes

tclaus mentioned this pull request Jun 10, 2021

AccountDeleter does not do enough #6221

Open

tclaus mentioned this pull request Jun 12, 2021

Adds ability to block a pod in Admin-View #8228

Closed

tclaus mentioned this pull request Jun 14, 2021

Provide a Rake task to delete local/remote profile data as per GDPR requirements #7848

Open

diaspora-code-review reviewed Jun 15, 2021

View reviewed changes

tclaus force-pushed the 7464_7463_close_remote_accounts branch from e14f182 to fea288e Compare July 14, 2021 19:36

Close remote accounts in admin UI

0ed7113

tclaus force-pushed the 7464_7463_close_remote_accounts branch from fea288e to 0ed7113 Compare July 14, 2021 20:19

Fixing Tests

839120e

tclaus requested review from SuperTux88 and Flaburgan December 19, 2021 17:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Block/Close and wipe data for all accounts on local pod in admin UI #8249

Block/Close and wipe data for all accounts on local pod in admin UI #8249

tclaus commented Jun 7, 2021 •

edited

tclaus commented Jun 7, 2021

Flaburgan commented Jun 7, 2021

tclaus commented Jun 8, 2021

tclaus commented Jun 9, 2021

tclaus commented Jun 9, 2021

Flaburgan commented Jun 9, 2021

diaspora-code-review Jun 10, 2021

diaspora-code-review Jun 10, 2021

diaspora-code-review Jun 10, 2021

tclaus Jun 10, 2021

diaspora-code-review Jun 10, 2021

tclaus commented Jun 10, 2021 •

edited

SuperTux88 commented Jun 10, 2021

Flaburgan commented Jun 11, 2021 •

edited

SuperTux88 commented Jun 11, 2021

tclaus commented Jun 12, 2021

SuperTux88 commented Jun 12, 2021

diaspora-code-review Jun 15, 2021

tclaus commented Jun 15, 2021 •

edited

goobertron commented Jul 14, 2021

Block/Close and wipe data for all accounts on local pod in admin UI #8249

Are you sure you want to change the base?

Block/Close and wipe data for all accounts on local pod in admin UI #8249

Conversation

tclaus commented Jun 7, 2021 • edited

tclaus commented Jun 7, 2021

Flaburgan commented Jun 7, 2021

tclaus commented Jun 8, 2021

tclaus commented Jun 9, 2021

tclaus commented Jun 9, 2021

Flaburgan commented Jun 9, 2021

diaspora-code-review Jun 10, 2021

Choose a reason for hiding this comment

diaspora-code-review Jun 10, 2021

Choose a reason for hiding this comment

diaspora-code-review Jun 10, 2021

Choose a reason for hiding this comment

tclaus Jun 10, 2021

Choose a reason for hiding this comment

diaspora-code-review Jun 10, 2021

Choose a reason for hiding this comment

tclaus commented Jun 10, 2021 • edited

SuperTux88 commented Jun 10, 2021

Flaburgan commented Jun 11, 2021 • edited

SuperTux88 commented Jun 11, 2021

tclaus commented Jun 12, 2021

SuperTux88 commented Jun 12, 2021

diaspora-code-review Jun 15, 2021

Choose a reason for hiding this comment

tclaus commented Jun 15, 2021 • edited

goobertron commented Jul 14, 2021

tclaus commented Jun 7, 2021 •

edited

tclaus commented Jun 10, 2021 •

edited

Flaburgan commented Jun 11, 2021 •

edited

tclaus commented Jun 15, 2021 •

edited