Add additional checks for banned instances + various code improvements along the way

[melroy89]

Even after the previous PR: #1254. I still see some requests failing and being make to banned instances. This is a follow-up PR.

• There are several places like LikeHandler, activityPubManager and UpdateActorHandler that is calling methods directly or indirectly methods from ActivityPubManager.php (methods like: findActorOrCreate or updateActor for example).
• I added additional checks for these methods in ActivityPubManager now as well. And just return null, which just ignore the request. Without the need for additional try/catch blabla in the callee methods.
• While I was in ApHttpClient.php (but eventually decided not to add the isBannedInstance check there). I did documented some code and introduced a dedicated getCollectinCacheKey method as well, just like getActorCacheKey
• I made several method private. If methods can be private, please yes. Our code is already enough spaghetti.
• Rename a wrong log message from updateUser to logRequestException`
• Finally, I made the activityPubManager variable consistent through all out the inbox, outbox activitypub files as well as services and messenger handler files. This will make grepping easier. And give me less headache.

Fixes 25th: #1175 (comment)

[BentiGorlich]

The most important thing is the check in findActorOrCreate

[BentiGorlich]

This check should be pushed down to the line just before $actor = $this->apHttpClient->getActorObject($actorUrl);

[melroy89]

Are you sure? This might still do dispatchUpdateActor.

[BentiGorlich]

dispatch update actor checks for banned instances as well so this is not an issue i think

[melroy89]

Yes indeed, it checks it:

mbin/src/Service/ActivityPubManager.php

Line 231 in c62b450

if ($this->settingsManager->isBannedInstance($actorUrl)) {

. But not dispatching in the first place is more efficient, which I like. And reduce unnecessary CPU cycles etc.

[BentiGorlich]

Well it does the check before actually dispatching the update:

mbin/src/Service/ActivityPubManager.php

Lines 233 to 237 in 0868a7f

	public function dispatchUpdateActor(string $actorUrl)
	{
	if ($this->settingsManager->isBannedInstance($actorUrl)) {
	return;
	}

[melroy89]

I pushed the isBannedInstance check down to line 176 now.

The question now is whether you really want to return users or magazines from a banned instance..?

[BentiGorlich]

dispatch update actor checks for banned instances as well so this is not an issue i think

misclick

[garrettw]

See comment left on previous thread

[BentiGorlich]

I am really not a fan of a new contributor approving changes on highly important code. Nothing against @garrettw , but you are a new contributor.
Additionally @melroy89 you marked a conversation as completed just because I didn't bave the time to answer, which I also not appreciate.

If I have remarks in the future I will check the 'request changes option ' if I have such comments to prevent this, but please be careful with everything activity pub related

[garrettw]

No offense taken.
I only reviewed this because I was requested to, and after looking it over, if I had thought the changes were beyond my current understanding of the code, I would have declined to review it — but I thought it looked fairly straightforward.

[BentiGorlich]

The code changes are easy, the implications of the changes however are not.

[garrettw]

I guess I can see how that’s true for a couple of the files involved.

[melroy89]

I am really not a fan of a new contributor approving changes on highly important code. Nothing against @garrettw , but you are a new contributor. Additionally @melroy89 you marked a conversation as completed just because I didn't bave the time to answer, which I also not appreciate.

If I have remarks in the future I will check the 'request changes option ' if I have such comments to prevent this, but please be careful with everything activity pub related

Happy new year!