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

Add/User Delete activities #552

Open
wants to merge 39 commits into
base: master
Choose a base branch
from

Conversation

mediaformat
Copy link
Collaborator

@mediaformat mediaformat commented Nov 8, 2023

Adds a Server class for processing server activities, such as Federating a delete_user action

Fixes #566
https://gdpr.eu/right-to-be-forgotten/

Proposed changes:

  • Adds a Server class for processing server related activities
  • Adds an activitypub_send_server_activity method for Server initiated activities
  • Adds a delete_user action to Federate an actor Delete activity

Other information:

I've started this from @mattwiebe's Profile Update PR, so it makes sense for that to be merged 1st. I can rebase this later if needed

Followup PRs:

The Server class, and activitypub_send_server_activity will also be useful for Inbox forwarding activities (mainly Updates, Deletes)

  • Have you written new tests for your changes, if applicable?

Testing instructions:

  • Go to '..'

Copy link
Collaborator Author

@mediaformat mediaformat left a comment

Choose a reason for hiding this comment

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

See if rebase is needed for removing dependency on #542

@pfefferle
Copy link
Member

looks interesting

I think sooner or later we need an outbox when the number of outgoing activities steadily grows...

@Cambridgeport90

This comment has been minimized.

@mediaformat
Copy link
Collaborator Author

For now we're just deriving known Servers based on current users followers, I'll add the blog followers here as well.

But (in a subsequent PR) we'll have to think about tracking known users:

Receiving an actor Delete means we should delete any Comments they've sent. And possibly Inbox Forward the Delete Activity.

@mediaformat
Copy link
Collaborator Author

@pfefferle after testing, the activity must be signed by the actor (mastodon ignores the actor Delete signed by the application actor).

Since the activity is scheduled, it creates a race condition where even if we pass the user_id to safe_remote_post the user is likely to be deleted by the time the signature is generated.

So a workaround would be to store key pair in an option during delete_user action and delete the option during deleted_user.

Code update forthcoming

@github-actions github-actions bot added the Stale label Apr 17, 2024
@pfefferle
Copy link
Member

pfefferle commented May 7, 2024

I would not merge it "as is" yet. I think we have to update some parts (like user caps) and discuss some missing parts, like: Should we send a delete if you remove the ActivityPub capability from a user, should we ask the admin if we should do it, should we schedule it to some time in the future, so the user will be able to revert his decision, ...?

includes/class-signature.php Outdated Show resolved Hide resolved
@pfefferle
Copy link
Member

But I agree with @mattwiebe ! This is an important feature and we should work on getting it merged!

@mediaformat
Copy link
Collaborator Author

hey @mattwiebe, that is correct there isn't any option to handle the Blog User.

Should we merge this and a button to the Settings page that allows us to issue DELETE calls for the all Users (both Blog and User types) followed by deactivating the plugin?

Yes, this is what I was envisioning, with warnings about the irreversible nature of the operation.

@mattwiebe
Copy link
Contributor

Should we send a delete if you remove the ActivityPub capability from a user

Yes! Great chance to educate the user on what will happen.

Should we ask the admin if we should do it, should we schedule it to some time in the future, so the user will be able to revert his decision

I think give fair warning, at least in the case of removing the capability, that it will be sent. Adding the UI to manage the delay and cancel the deletion seems like solving a hypothetical that we can circle back later if there's demand.

@mediaformat mediaformat requested a review from pfefferle May 9, 2024 14:46
$key = self::get_private_key_for( $user->get__id() );
$key_id = $user->get_url() . '#main-key';
} else {
$temp_sig_options = get_option( 'activitypub_temp_sig_' . $user_id );
Copy link
Member

Choose a reason for hiding this comment

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

this feels a bit hacky and might break things in the future, if we maybe introduce key rotation: https://swicg.github.io/activitypub-http-signature/#key-rotation

Copy link
Member

Choose a reason for hiding this comment

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

Do we need a sig for deletes at all? The remote server is not able to verify it anyways!?!

This is very confusing https://swicg.github.io/activitypub-http-signature/#handling-deletes-of-actors

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed it is very hacky! In my previous tests Mastodon ignored actor deletes signed by the instance actor, but I will do some more tests and report back.

Copy link
Member

Choose a reason for hiding this comment

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

or maybe we do it as you mentioned it here: #552 (comment)

So a workaround would be to store key pair in an option during delete_user action and delete the option during deleted_user.

Copy link
Member

Choose a reason for hiding this comment

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

or we store the complete delete object in the schedule on the delete?!?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So a workaround would be to store key pair in an option during delete_user action and delete the option during deleted_user.

The first part is the hack I've implemented, the problem with the second part is that deleted_user will occur almost immediately, and the Delete activities will fail.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

or we store the complete delete object in the schedule on the delete?!?

Hmm, the scheduler runs before signature generation.

@mediaformat mediaformat changed the title Add/Server activities Add/User Delete activities May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to remove the fediverse-account for my closed weblog?
4 participants