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

ActivityPub migration procedure #4617

Merged
merged 3 commits into from
Aug 20, 2017
Merged

Conversation

Gargron
Copy link
Member

@Gargron Gargron commented Aug 16, 2017

Once one account is detected as going from OStatus to ActivityPub,
unsubscribe from old PuSH subscription, invalidate WebFinger cache
for other accounts from the same domain, and queue up a re-resolve
for all of them.

@Gargron Gargron added the activitypub Protocol-related changes, federation label Aug 16, 2017
@Gargron
Copy link
Member Author

Gargron commented Aug 18, 2017

I am a bit worried if this is the best possible code to solve this problem.

cc @unarist @ThibG

@ClearlyClaire
Copy link
Contributor

I only have read the current (I've been told there have been changes to the way followers worked since) spec yet, and not your implementation. So, this is not my final opinion about this change, but so far it looks good to me, although I'm not sure remote accounts should be scheduled for re-resolving: as long as they are invalidated, they will be resolved when needed, right?

@unarist
Copy link
Contributor

unarist commented Aug 19, 2017

Some thoughts:

  • Even we recognize they're activitypub_ready, they may not recognize we're activitypub_ready and they don't send ActivityPub activities to us. We should wait receiving activity from them before unsubscribe.
  • FetchRemoteAccountService and ProcessAccountService may also be called directly. These cases wouldn't be handled by this patch.

Once one account is detected as going from OStatus to ActivityPub,
invalidate WebFinger cache for other accounts from the same domain
@Gargron Gargron force-pushed the feature-activitypub-migration branch from e68585e to 011d081 Compare August 19, 2017 22:20
@Gargron
Copy link
Member Author

Gargron commented Aug 19, 2017

  • Addressed @ThibG's concerns ("I'm not sure remote accounts should be scheduled for re-resolving: as long as they are invalidated, they will be resolved when needed"), this simplifies the code a fair bit
  • Addressed @unarist's concerns: Moved the migration detection to ActivityPub::ProcessAccountService, which is called both from ResolveRemoteAccountService and other places; only unsubscribe from PuSH when an ActivityPub payload arrives in the inbox

@unarist
Copy link
Contributor

unarist commented Aug 20, 2017

I've forgotten about subscription expires. PuSH subscription may expire just after Account#protocol update, but we won't resubscribe them due to protocol: activitypub. It means we can't receive updates from that until first AP payload arriving.

My ideas:

  • A) Update SubscribeScheduler to allow resubscribe for accounts which AP ready but not yet unsubscribed. In other words, resubscribe! if (ostatus? || !subscription_expires_at.nil?).
    (note: current master doesn't unsubscribe for upgraded accounts, so those may be fixed manually)
  • B) In the upgrade process, resubscribe subscription immediately (unless it's called from AP receive handler) and send Update profile activity. This ensures enough grace period, then encourages them to upgrade protocol for our account.

BTW, Account#protocol is used as what protocol should we use to interact with them. Also many interaction services doesn't update profile, only reads Account#protocol. So delaying its update means may bit delays protocol migration. Although I don't see serious issue for that.

@Gargron
Copy link
Member Author

Gargron commented Aug 20, 2017

@unarist Addressed first point - do not really understand the last one, but I think we're probably good to go here?

@Gargron Gargron merged commit 6e9eda5 into master Aug 20, 2017
@Gargron Gargron deleted the feature-activitypub-migration branch August 20, 2017 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
activitypub Protocol-related changes, federation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants