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

Profile redirect notes #5746

Merged
merged 5 commits into from
Nov 18, 2017
Merged

Profile redirect notes #5746

merged 5 commits into from
Nov 18, 2017

Conversation

Gargron
Copy link
Member

@Gargron Gargron commented Nov 18, 2017

See also #177 (partial)

image

image

  • Database structure to store information about where account moved
  • Serialize this information in REST API and ActivityPub
  • Deserialize this information from ActivityPub
  • Display note on public profiles
  • Display note on profiles in web UI

Please mind that this PR does not include the automatic followers unfollow/re-follow of new account, nor does it include a way to actually set this "move" information, yet. Therefore, it's only one step towards solving #177

@Gargron Gargron added activitypub Protocol-related changes, federation work in progress Not to be merged, currently being worked on labels Nov 18, 2017
@@ -9,6 +9,7 @@ class ActivityPub::Adapter < ActiveModelSerializers::Adapter::Base
{
'manuallyApprovesFollowers' => 'as:manuallyApprovesFollowers',
'sensitive' => 'as:sensitive',
'movedTo' => 'as:movedTo',
Copy link
Contributor

Choose a reason for hiding this comment

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

Should you really use ActivityStreams' namespace for that? I couldn't find any documentation for “movedTo”

Copy link
Member Author

Choose a reason for hiding this comment

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

I made it up since it doesn't exist, but probably should exist. @cwebber is okay with it

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, that's fine with me.

@ClearlyClaire
Copy link
Contributor

I'm wondering if it would make sense to redirect webfinger queries, too.

@Gargron
Copy link
Member Author

Gargron commented Nov 18, 2017

@ThibG

Gargron - Yesterday at 9:56 PM
@nightpool i just implemented a webfinger redirect for accounts and getting second thoughts. maybe the only redirect should be the public profile page. because a webfinger redirect would brick any look-up of all old content... plus it's a rabbit hole, there's a ton of ActivityPub endpoints, it's unclear whether you should be able to redirect them and them still be considered valid, and there's no way to tell if the new server has the same ActivityPub URI structure, etc. and so that would also brick stuff. redirecting public profile page and adding a "redirects" to the REST API so apps can show a special note, maybe that's all
nightpool - Yesterday at 10:01 PM
hm.
I don't see how redirecting the webfinger will break lookup of old content?
Gargron - Yesterday at 10:04 PM
it's all very confusing but say I am looking up "Hello world" by Gargron@mastodon.social, but Gargron@mastodon.social redirects to gargron@gonext.gg. Let's say we modified our webfinger code to even follow redirects in the first place. Now it creates "Hello world" as created by gargron@gonext.gg. You favourite it. That activity is sent to gargron@gonext.gg... but that post does not exist there, so it blackholes
That's just one example
nightpool - Yesterday at 10:04 PM
How are you looking up that post, by URI?
Gargron - Yesterday at 10:05 PM
Webfinger code not supporting redirects in all versions is even bigger. It'll just mean the content will not be processed in the first place.
nightpool - Yesterday at 10:05 PM
It should still have mastodon.social/users/gargron as the post actor
yeah the redirect is maybe a bigger concern.....
Gargron - Yesterday at 10:05 PM
It doesn't matter how you look up the post, when it checks the author it does webfinger.
nightpool - Yesterday at 10:07 PM
I don't think that's true
FetchRemoteStatusService uses attributedTo.
Gargron - Yesterday at 10:10 PM
when mastodon looks up an activitypub actor for the first time, i.e. there's no existing db record, it performs a webfinger verification
which leads to the scenario i described
there's also the question of... what if the record exists, but then webfinger cache is stale so we webfinger again, and it redirects. wtf do then
nightpool - Yesterday at 10:11 PM
Sure, okay
Gargron - Yesterday at 10:12 PM
thinking about all the ways in which this sort of thing can fuck up things is making my brain hurt

@ClearlyClaire
Copy link
Contributor

@Gargron yeah, indeed, that's what I was thinking. Since the content is still there, it kind of makes sense to not stop it from federating. The advantage is that it would be completely transparent to whichever software, OStatus or ActivityPub, supporting “movedTo” or not. The downside is that it would stop the content of the old profile from federating anymore, although it's still there… it's probably not worth it, I guess.

@ClearlyClaire
Copy link
Contributor

Another thing I'm wondering about and you haven't brought up in this PR is make every follower of the old account automatically send a follow request to the new one (and potentially unfollow the old one) once it's discovered to have moved.

@Gargron
Copy link
Member Author

Gargron commented Nov 18, 2017

@ThibG No that's not at all included in this PR.

@ClearlyClaire
Copy link
Contributor

ClearlyClaire commented Nov 18, 2017 via email

@Gargron Gargron removed the work in progress Not to be merged, currently being worked on label Nov 18, 2017
Copy link
Contributor

@ClearlyClaire ClearlyClaire left a comment

Choose a reason for hiding this comment

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

I'm not very familiar with React.js, but everything else looks fine to me.

<div className='account__moved-note'>
<div className='account__moved-note__message'>
<div className='account__moved-note__icon-wrapper'><i className='fa fa-fw fa-suitcase account__moved-note__icon' /></div>
<FormattedMessage id='account.moved_to' defaultMessage='{name} has moved to:' values={{ name: <strong dangerouslySetInnerHTML={displayNameHtml} /> }} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it better if the name was a link to the new account?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's the old name

@Gargron Gargron merged commit 58cede4 into master Nov 18, 2017
Gargron pushed a commit that referenced this pull request Nov 23, 2017
@ykzts ykzts mentioned this pull request Nov 25, 2017
Gargron pushed a commit that referenced this pull request Nov 25, 2017
* yarn manage:translations

* Add Japanese translation for #5087

* Add Japanese translation for #5616

* Add Japanese translation for #5746

* Add Japanese translation for #5750
ykzts pushed a commit that referenced this pull request Nov 25, 2017
cobodo pushed a commit to cobodo/mastodon that referenced this pull request Dec 6, 2017
* Serialize moved accounts into REST and ActivityPub APIs

* Parse federated moved accounts from ActivityPub

* Add note about moved accounts to public profiles

* Add moved account message to web UI

* Fix code style issues
cobodo pushed a commit to cobodo/mastodon that referenced this pull request Dec 6, 2017
cobodo pushed a commit to cobodo/mastodon that referenced this pull request Dec 6, 2017
* yarn manage:translations

* Add Japanese translation for mastodon#5087

* Add Japanese translation for mastodon#5616

* Add Japanese translation for mastodon#5746

* Add Japanese translation for mastodon#5750
cobodo pushed a commit to cobodo/mastodon that referenced this pull request Dec 6, 2017
@Cam-B
Copy link

Cam-B commented Dec 14, 2017

One step further. Awesome!

@gjorando gjorando mentioned this pull request Dec 15, 2017
2 tasks
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.

3 participants