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

Twidere mention workaround #5552

Merged
merged 2 commits into from
Nov 7, 2017

Conversation

ClearlyClaire
Copy link
Contributor

This pull request works around a bug in both Twidere (TwidereProject/Twidere-Android#1001) and Tootdon.

Tootdon and Twidere construct @user@domain handles from mentions in toots based
solely on the mention text and account URI's domain without performing any
webfinger call or retrieving account info from the Mastodon server.

As a result, when a remote user has WEB_DOMAINLOCAL_DOMAIN, Twidere and
Tootdon will construct the mention as @user@WEB_DOMAIN. Now, this will usually
resolve to the correct account (since the recommended configuration is to have
WEB_DOMAIN perform webfinger redirections to LOCAL_DOMAIN) when processing
mentions, but won't do so when displaying them (as it does not go through the
whole account resolution at that time).

While this is essentially a workaround for a bug in other software, it's a simple workaround whereas fixing Twidere would be more involved. In addition, it also fixes some inconsistency in Mastodon: it is somewhat dubious for Mastodon to manage to resolve the account in one place (ProcessMentionsService) and fail in another (status serialization).

Lastly, this PR depends on #5539 but that could be changed.

end
status.text = status.text.gsub(Account::MENTION_RE) do |match|
begin
mentioned_account = resolve_remote_account_service.call($1)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Codeclimate complains about $1, but I'm not sure there is a better way to do that in Ruby, at least I have not found one reading the documentation for gsub.

Copy link
Member

Choose a reason for hiding this comment

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

what's wrong with resolve_remote_account_service.call(match.first.to_s)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

match is the full matched string, not a list of groups :/


mentioned_account.mentions.where(status: status).first_or_create(status: status)
mentioned_account.domain.nil? ? "@#{mentioned_account.username}" : "@#{mentioned_account.username}@#{mentioned_account.domain}"
Copy link
Member

Choose a reason for hiding this comment

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

this line should just be "@#{mentioned_account.acct}"

end

status.save!
Copy link
Member

Choose a reason for hiding this comment

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

at this point have we already delivered the message to subscribers? I don't know if we're allowed to re-write the message here then—we'd have to pull this code somewhere else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No we haven't, this is right before delivering it.

Copy link
Member

Choose a reason for hiding this comment

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

How do we know that though? I don't know if this is the right thing to do from a code organization perspective. adding a transaction might create a nested transactions error, for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand your concern, and I share it to some extent. However, the serialized status sent over both the WebUI and remote servers through OStatus and ActivityPub already depends on this function, as the serialization depends on the mentioned_account.mentions created by this function.

TL;DR: I understand the concern, but it's not a new one

@nightpool
Copy link
Member

nightpool commented Oct 28, 2017

I'm not sure merging this change is a good idea. While in theory processing webfinger aliases is a useful thing to be doing when @mentioning users, this really needs to be fixed upstream as well—there's no guarantee that users with web_domain != local_domain will have webfinger aliases correctly set up. And without the bug driving this pull request i'm not sure the additional complexity doesn't outweigh the benefits of inclusion.

@ClearlyClaire
Copy link
Contributor Author

ClearlyClaire commented Oct 28, 2017

@nightpool I fixed the issues you pointed out. I somewhat agree with you, although I also see it as a consistency fix for the following behavior: Mastodon resolves the account, records a “mention” relationship and notify the remote user, but the serialized toot does not contain the html-rendered mention, which breaks further interaction as the WebUI does not pick it up on reply

Tootdon and Twidere construct @user@domain handles from mentions in toots based
solely on the mention text and account URI's domain without performing any
webfinger call or retrieving account info from the Mastodon server.

As a result, when a remote user has WEB_DOMAIN ≠ LOCAL_DOMAIN, Twidere and
Tootdon will construct the mention as @user@WEB_DOMAIN. Now, this will usually
resolve to the correct account (since the recommended configuration is to have
WEB_DOMAIN perform webfinger redirections to LOCAL_DOMAIN) when processing
mentions, but won't do so when displaying them (as it does not go through the
whole account resolution at that time).

This change rewrites mentions to the resolved account, so that displaying the
mentions will work.
Indeed, substitutions with the previous regexp would erroneously eat any
preceding whitespace, which would lead to concatenated mentions in the
previous commit.

Note that users will “lose” up to one character space per mention for their
toots, as that regexp is also used to remove the domain-part of mentioned
users for character counting purposes, and it also erroneously removed the
preceding character if it was a space.
@ClearlyClaire
Copy link
Contributor Author

Rebased on latest master, so it doesn't include the first commit anymore.

@Gargron Gargron merged commit 5d5c0f4 into mastodon:master Nov 7, 2017
@ClearlyClaire ClearlyClaire deleted the twidere-mention-workaround branch November 11, 2017 15:38
cobodo pushed a commit to cobodo/mastodon that referenced this pull request Dec 6, 2017
* Work around Twidere and Tootdon bug

Tootdon and Twidere construct @user@domain handles from mentions in toots based
solely on the mention text and account URI's domain without performing any
webfinger call or retrieving account info from the Mastodon server.

As a result, when a remote user has WEB_DOMAIN ≠ LOCAL_DOMAIN, Twidere and
Tootdon will construct the mention as @user@WEB_DOMAIN. Now, this will usually
resolve to the correct account (since the recommended configuration is to have
WEB_DOMAIN perform webfinger redirections to LOCAL_DOMAIN) when processing
mentions, but won't do so when displaying them (as it does not go through the
whole account resolution at that time).

This change rewrites mentions to the resolved account, so that displaying the
mentions will work.

* Use lookbehind instead of non-capturing group in MENTION_RE

Indeed, substitutions with the previous regexp would erroneously eat any
preceding whitespace, which would lead to concatenated mentions in the
previous commit.

Note that users will “lose” up to one character space per mention for their
toots, as that regexp is also used to remove the domain-part of mentioned
users for character counting purposes, and it also erroneously removed the
preceding character if it was a space.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants