-
-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
Add conversation-based forwarding for limited visibility statuses through bearcaps #14666
Conversation
@@ -52,6 +52,7 @@ def preprocess_attributes! | |||
@text = @options.delete(:spoiler_text) if @text.blank? && @options[:spoiler_text].present? | |||
@visibility = @options[:visibility] || @account.user&.setting_default_privacy | |||
@visibility = :unlisted if @visibility&.to_sym == :public && @account.silenced? | |||
@visibility = :limited if @visibility&.to_sym != :direct && @in_reply_to&.limited_visibility? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, replying in unlisted/public to a followers-only toot is possible and can be useful, even if the use cases for it are rather rare.
This changes disallows replying in unlisted/public to a limited toot. I understand the reasoning, but it's new behavior, and might be especially confusing since it is done silently. Maybe erroring out would be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replying private to limited would be a privacy downgrade (more people see the reply than the original post) which is unintuitive and as I figured, undesirable.
c54afbd
to
6c5a68d
Compare
aae3e79
to
48096fd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok this PR is becoming huge and contains both important UX and protocol changes.
While I like most of the ideas, I'm pretty worried about backward compatibility, compatibility with other software, and unwelcome UX changes. Most of those remarks are made as inline comments.
UX-wise, I still have the issue that you don't know who you are replying to exactly, and can't restrict this to “those of the original audience who are also in your followers” although most of the groundwork is here for a somewhat inelegant solution to that (checking for the token and the signature).
Overall this is a large PR which is difficult to reason about. Could you split out the refactoring/cleanup as well as the ActivityPub::Dereferencer
stuff to another PR? I'm much more confident with them than with the changes in the Conversation
model or use, or the forwarding (the token creation and handling part is fine too, but not useful on its own).
48096fd
to
53beb83
Compare
4730144
to
2a7b259
Compare
2a7b259
to
32629ce
Compare
status.save! | ||
check_for_spam(status) | ||
|
||
# Silent mentions need to be delivered separately |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens when the status that started a conversation is deleted? No forwarding will be possible. But explicitly mentioned actors receive the reply in their inbox regardless.
@@ -0,0 +1,7 @@ | |||
class AddInboxUrlToConversations < ActiveRecord::Migration[5.2] | |||
def change | |||
add_column :conversations, :parent_status_id, :bigint, null: true, default: nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No foreign key or index defined on the table. Not sure if we should do that to make it consistent with other columns in the database, or not because the conversations table is huge and the index would be huge too and the foreign keys make everything much slower and we should begin moving away from them.
183b0d2
to
8a3f656
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to the inlined comments, more general remarks, most of which are related to backward compatibility and upgrade path (“older Mastodon instances” refer to any instance before this PR gets merged):
- older Mastodon instances/Mastodon instances that don't understand the bearcap URIs etc. won't be able to fetch/get replies. That is a breaking change, one we probably can't do much about (other than ship the bearcap handling code much earlier than this PR).
- of the limited toots they can see, older Mastodon instances will see see them as a mixture of “DM” and “limited” posts. For “limited” posts, they handle them exactly as followers-only with the exception of having a possibly different audience (hidden from clients). This means people replying will overwhelmingly be replying with followers-only visibility, and never with the new limited visibility.
- instances running this PR will display limited toots as followers-only toots to end-users, and yet change every reply to limited visibility regardless of the selected privacy setting. Meaning end-users won't be able to tell whether something they reply to in “followers-only” will actually be only for their followers + mentioned users or a totally different set of users they don't have control on. This is a huge change in UX, and one that I think is extremely damaging.
- an interesting/weird thing is that replies to a thread in which the root status has been deleted will silently stop being relayed, leading them to behave like DMs but with the interface showing them like regular limited/followers-only toots. This is very likely to result in people talking in essentially DM without noticing. Indeed, imagine people starting to engage in a conversation with the initial toot, then the author delete&redrafting the original toot. Offsprings of the original conversation will then behave like DMs, and the participants will likely not notice something is amiss, but people who did not participate in the conversation won't get the replies, even if they were able to see the first toot before it got deleted.
if request.authorization.present? && request.authorization.match(/^Bearer /i) | ||
raise Mastodon::NotPermittedError unless @status.capability_tokens.find_by(token: request.authorization.gsub(/^Bearer /i, '')) | ||
else | ||
authorize @status, :show? | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, Vary
should include Authorization
. Also, request.authorization
might match other headers than just Authorization
so they should be taken into account as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added Authorization
to Vary
headers.
I used request.authorization
instead of request.headers['Authorization']
because I saw that doorkeeper does that and wanted to be consistent with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get that, I'm just worried that it may possibly lead to security issues if an implementation ends up somehow using something else than the Authorization
header that request.authorization
will use. I know for sure it accepts variants of Authorization
and I wonder if Devise or something doesn't make it also support other things like the bearer_token
field in form-data POST. This would mean such an implementation would possibly enable caching of something private without an Authorization
header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code for request.authorization
is:
def authorization
get_header("HTTP_AUTHORIZATION") ||
get_header("X-HTTP_AUTHORIZATION") ||
get_header("X_HTTP_AUTHORIZATION") ||
get_header("REDIRECT_X_HTTP_AUTHORIZATION")
end
Admittedly, I don't understand exactly when X-HTTP_AUTHORIZATION
would be set vs X_HTTP_AUTHORIZATION
etc, but it definitely does not touch form-data. I am guessing that these header variants are the different ways the Authorization
header may be forwarded by Apache and other proxies but not knowing the exact reasons does make me uncomfortable.
Perhaps I should use request.headers['Authorization']
directly.
def perform(conversation_id, json) | ||
conversation = Conversation.find(conversation_id) | ||
|
||
@status = conversation.parent_status |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Conversation
's parent_status
is the root status in a thread, right? So this means that if you reply in limited visibility to a thread started with a different visibility (including a different limited
audience), replies to that toot will be distributed to the root status' audience, which could be extremely misleading.
If we’re running on the assumption that users should not be able to tell when they’re inside someone’s circle, then this is the only reasonable behavior. It makes no sense for the user to manually select “reply to circle” if the circle is not meant to be known. I also doubt that users think or care about every one of their followers being able to see a reply to a thread, only that replies are public or not. Most people are aware replies don’t pop up in home feeds unless you follow both people. |
A solution to this could be duplicating visibility data on the conversation model so it’s not dependent on the status continuing to exist, including having a “silent mentions” version of association for the conversation (or doing something more hacky like removing foreign key constraints on the mentions table, keeping that data after the status is removed, then query it using parent_status_id). Or maybe we could somehow prevent new replies in such “dead” conversations. |
By now, most people expect “followers-only” toots to only be seen by their followers and mentioned users, at most. This behavior changes that and gives the replying user no way to know about who will be able to see the toots. For instance, imagine you, with 429K followers, posting something in followers-only. Currently, I could feel somewhat comfortable replying with something somewhat private, in followers-only, I'd know only you and my ~100 followers would see it. With this change, I wouldn't be able to tell if only you and my ~100 followers could see it, or any of your 429K followers, or even a different set of people entirely. |
What do you suggest? |
I don't know, having limited replies' audience be the intersection between the original audience and the followers, maybe. |
8a3f656
to
1c01a8c
Compare
1c01a8c
to
e909d20
Compare
e909d20
to
7cd4ed7
Compare
If the Activity object is bearcaps, the string will not be LD-signed because it does not have a sign? method. If you want to allow the string to be signed
|
uri = value_or_id(@object['context']) | ||
conversation ||= ActivityPub::TagManager.instance.uri_to_resource(uri, Conversation) | ||
|
||
return conversation if (conversation.present? && conversation.uri == uri) || !uri.start_with?('https://') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The circle's origin of the parent status, Conversation.uri = nil, and local? is true.
When a reply arrives from the remote, the context uri is set and local? is false.
When Conversation.local? = true, it adds a condition not to update the conversation.
return conversation if (conversation.present? && (conversation.local? || conversation.uri == uri)) || !uri.start_with?('https://')
status.thread.mentions.includes(:account).find_each do |mention| | ||
status.mentions.create(silent: true, account: mention.account) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exclude status account from mentions and include replied status account.
status.thread.mentions.includes(:account).find_each do |mention|
status.mentions.create(silent: true, account: mention.account) unless status.account_id == mention.account_id
end
status.mentions.create(silent: true, account: status.thread.account) unless status.account_id == status.thread.account_id
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
We need to close or discuss ourselves. |
would it maybe be better to use |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Problem: When replying to a limited visibility status, you don't know who else is supposed to see your reply because you don't know who can see the parent status. Only the origin of the parent status knows that. Thus, replies to limited visibility posts invariably become DMs.
Replies to limited visibility statuses are sent directly to the origin of the conversation the reply is part of. The origin of the conversation is then responsible for forwarding that reply to everyone who was allowed to see the status that initiated the conversation. The
Create
activity of the reply uses a linked-data signature to allow forwarding while not including the actual object within the activity. Instead, the object is referenced using a bearcap. This solves the problem of being able to re-create objects after they were deleted while also not exposing the object to the public by only its URI. (While the URI of a status consisting of a snowflake ID is not enumerable/guessable and could therefore be enough to secure the reply against those who have not received the exact URI, the token adds another layer of entropy and should make accidents both in development and usage less likely).Note
objects receive a new attributecontext
which is a resolvable URI that points to aGroup
actor with an inbox that represents a conversation. This URI can be included in theto
of an object which signals that the object wants to be forwarded to all parties in the conversation. Theatom:conversation
attribute that uses OStatus-style unresolvable URIs is kept for backwards-compatibility, if both are present, old conversations are upgraded to use the new URI and inbox.