FoundKeyGang/FoundKey
FoundKeyGang/FoundKey
7
19
Fork 22
Code Issues 37 Pull requests 4 Releases Activity

activitypub: process Move activities and movedTo field on actors #309

Manually merged
Johann150 merged 10 commits from account-moving into main 2023-03-23 20:48:19 +00:00
Conversation 5 Commits 10 Files changed 17 +24 -3
1 changed files with 24 additions and 3 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit 3c7b7d4279 - Show all commits

27
packages/backend/src/remote/activitypub/models/person.ts
View file

@ -39,7 +39,7 @@ const summaryLength = 2048;
* @param x Fetched object
* @param uri Fetch target URI
*/
function validateActor(x: IObject): IActor {
async function validateActor(x: IObject, resolver: Resolver): Promise<IActor> {
if (x == null) {
throw new Error('invalid Actor: object is null');
}
@ -61,6 +61,25 @@ function validateActor(x: IObject): IActor {
throw new StatusError('cannot resolve local user', 400, 'cannot resolve local user');
}
if (x.movedTo !== undefined) {
if (!(typeof x.movedTo === 'string' && x.movedTo.length > 0)) {
throw new Error('invalid Actor: wrong movedTo');
}
if (x.movedTo === uri) {
throw new Error('invalid Actor: moved to self');
}
await resolvePerson(x.movedTo, resolver)
.then(moveTarget => {
x.movedTo = moveTarget.id
})
.catch((err: Error) => {
// Can't find the move target for some reason.
// Don't treat the actor as invalid, just ignore the movedTo.
logger.warn(`cannot find move target "${x.movedTo}", error: ${err.toString()}`);
delete x.movedTo;
Johann150 marked this conversation as resolved
amybones commented 2023-03-21 22:51:28 +00:00
Review
Copy link

I understand having a warning in the log, but I'm not totally convinced that deleting the movedTo information makes sense to me?

Things that I think of: It could be that the destination isn't resolvable because it's a fedi server that possibly somehow conforms to the account moving spec, but not the lookup part, which doesn't make a ton of sense to me. Or, the destination is temporarily unreachable at the time an attempt to resolve the new account is made.

For the first case, it's unlikely that the new account is going to properly broadcast updates over activitypub, so sure. But for the second case, this means that the foundkey database is going to be inconsistent. The user is going to lose that follow silently, as well as have no indication that there's something amiss, since if they go look at the profile page for the old follow wondering where they went, it'll just show up normally, and they won't have a chance to go find the person or manually re-follow.

I understand having a warning in the log, but I'm not totally convinced that deleting the movedTo information makes sense to me? Things that I think of: It could be that the destination isn't resolvable because it's a fedi server that possibly somehow conforms to the account moving spec, but not the lookup part, which doesn't make a ton of sense to me. Or, the destination is temporarily unreachable at the time an attempt to resolve the new account is made. For the first case, it's unlikely that the new account is going to properly broadcast updates over activitypub, so sure. But for the second case, this means that the foundkey database is going to be inconsistent. The user is going to lose that follow silently, as well as have no indication that there's something amiss, since if they go look at the profile page for the old follow wondering where they went, it'll just show up normally, and they won't have a chance to go find the person or manually re-follow.
Johann150 commented 2023-03-22 20:19:49 +00:00
Review
Copy link

I agree with you for the 2nd case, it should be an atomic operation.

The correct behaviour is to throw an error there. Since there is no try/catch statement around resolvePerson in the move handler this would cause the queue handler to retry this incoming activity at a later time.

I agree with you for the 2nd case, it should be an atomic operation. The correct behaviour is to throw an error there. Since there is no try/catch statement around `resolvePerson` in the move handler this would cause the queue handler to retry this incoming activity at a later time.
amybones commented 2023-03-22 21:50:53 +00:00
Review
Copy link

Aha. Okay, I missed that bubbling up to a retry, thanks!

Aha. Okay, I missed that bubbling up to a retry, thanks!
});
}
if (!(typeof x.inbox === 'string' && x.inbox.length > 0)) {
throw new Error('invalid Actor: wrong inbox');
}
@ -137,7 +156,7 @@ export async function fetchPerson(uri: string): Promise<CacheableUser | null> {
export async function createPerson(value: string | IObject, resolver: Resolver): Promise<User> {
const object = await resolver.resolve(value) as any;
const person = validateActor(object);
const person = await validateActor(object, resolver);
apLogger.info(`Creating the Person: ${person.id}`);
@ -177,6 +196,7 @@ export async function createPerson(value: string | IObject, resolver: Resolver):
isBot,
isCat: (person as any).isCat === true,
showTimelineReplies: false,
movedToId: person.movedTo,
})) as IRemoteUser;
await transactionalEntityManager.save(new UserProfile({
@ -287,7 +307,7 @@ export async function updatePerson(value: IObject | string, resolver: Resolver):
const object = await resolver.resolve(value);
const person = validateActor(object);
const person = await validateActor(object, resolver);
apLogger.info(`Updating the Person: ${person.id}`);
@ -328,6 +348,7 @@ export async function updatePerson(value: IObject | string, resolver: Resolver):
isCat: (person as any).isCat === true,
isLocked: !!person.manuallyApprovesFollowers,
isExplorable: !!person.discoverable,
movedToId: person.movedTo,
} as Partial<User>;
if (avatar) {