Skip to content
This repository was archived by the owner on Feb 6, 2023. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion app/serializers/chat_message_serializer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -128,9 +128,14 @@ def include_user_flag_status?

def available_flags
return [] if !scope.can_flag_chat_message?(object)
return [] if reviewable_id.present?
return [] if reviewable_id.present? && user_flag_status == ReviewableScore.statuses[:pending]

PostActionType.flag_types.map do |sym, id|
if @options[:chat_channel].direct_message_channel? &&
%i[notify_moderators notify_user].include?(sym)
next
end

if sym == :notify_user &&
(
scope.current_user == user || user.bot? ||
Expand Down
1 change: 1 addition & 0 deletions app/serializers/chat_view_serializer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ def chat_messages
each_serializer: ChatMessageSerializer,
reviewable_ids: object.reviewable_ids,
user_flag_statuses: object.user_flag_statuses,
chat_channel: object.chat_channel,
scope: scope,
)
end
Expand Down
2 changes: 2 additions & 0 deletions app/serializers/reviewable_chat_message_serializer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ class ReviewableChatMessageSerializer < ReviewableSerializer
has_one :chat_message, serializer: ChatMessageSerializer, root: false, embed: :objects
has_one :chat_channel, serializer: ChatChannelSerializer, root: false, embed: :objects

payload_attributes :transcript_topic_id, :message_cooked

def chat_channel
object.chat_message.chat_channel
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,17 @@
}}

<div class="post-body">
{{html-safe reviewable.chat_message.cooked}}
{{html-safe (or reviewable.payload.message_cooked reviewable.chat_message.cooked)}}
</div>

{{#if this.reviewable.payload.transcript_topic_id}}
<div class="transcript">
<LinkTo @route="topic" @models={{array "-" this.reviewable.payload.transcript_topic_id}} class="btn btn-small">
{{i18n "review.transcript.view"}}
</LinkTo>
</div>
{{/if}}

{{yield}}
</div>
</div>
5 changes: 5 additions & 0 deletions assets/stylesheets/common/reviewable-chat-message.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
.reviewable-chat-message {
.transcript {
margin: 0 0 1em 0;
}
}
2 changes: 2 additions & 0 deletions config/locales/client.en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,8 @@ en:
label: Sender
description: Defaults to system
review:
transcript:
view: "View previous messages transcript"
types:
reviewable_chat_message:
title: "Flagged Chat Message"
Expand Down
3 changes: 3 additions & 0 deletions config/locales/server.en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,9 @@ en:
title: "Disagree"
ignore:
title: "Ignore"
direct_messages:
transcript_title: "Transcript of previous messages in %{channel_name}"
transcript_body: "To give you more context, we included a transcript of the previous messages in this conversation (up to ten):\n\n%{transcript}"
channel:
statuses:
read_only: "Read Only"
Expand Down
59 changes: 57 additions & 2 deletions lib/chat_review_queue.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,12 @@ class DiscourseChat::ChatReviewQueue
def flag_message(chat_message, guardian, flag_type_id, opts = {})
result = { success: false, errors: [] }

is_notify_type =
ReviewableScore.types.slice(:notify_user, :notify_moderators).values.include?(flag_type_id)
is_dm = chat_message.chat_channel.direct_message_channel?

raise Discourse::InvalidParameters.new(:flag_type) if is_dm && is_notify_type

guardian.ensure_can_flag_chat_message!(chat_message)
guardian.ensure_can_flag_message_as!(chat_message, flag_type_id, opts)

Expand All @@ -22,15 +28,19 @@ def flag_message(chat_message, guardian, flag_type_id, opts = {})
return result
end

if opts[:message].present? &&
ReviewableScore.types.slice(:notify_user, :notify_moderators).values.include?(flag_type_id)
payload = { message_cooked: chat_message.cooked }

if opts[:message].present? && !is_dm && is_notify_type
creator = companion_pm_creator(chat_message, guardian.user, flag_type_id, opts)
post = creator.create

if creator.errors.present?
creator.errors.full_messages.each { |msg| result[:errors] << msg }
return result
end
elsif is_dm
transcript = find_or_create_transcript(chat_message, guardian.user, existing_reviewable)
payload[:transcript_topic_id] = transcript.topic_id if transcript
end

queued_for_review = !!ActiveRecord::Type::Boolean.new.deserialize(opts[:queue_for_review])
Expand All @@ -41,6 +51,7 @@ def flag_message(chat_message, guardian, flag_type_id, opts = {})
target: chat_message,
reviewable_by_moderator: true,
potential_spam: flag_type_id == ReviewableScore.types[:spam],
payload: payload,
)
reviewable.update(target_created_by: chat_message.user)
score =
Expand Down Expand Up @@ -125,6 +136,50 @@ def companion_pm_creator(chat_message, flagger, flag_type_id, opts)
PostCreator.new(flagger, create_args)
end

def find_or_create_transcript(chat_message, flagger, existing_reviewable)
previous_message_ids =
Copy link
Contributor

Choose a reason for hiding this comment

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

What kind of check do we have to ensure transcript will never surface messages to someone who shouldn't see them?

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO any access access control we add for this should be outside the ChatTranscriptService, that should be kept as a dumb thing that just spits out the markdown for the provided IDs. In say ing that it does at least make sure the messages all belong to the same channel so you can't just give it arbitrary IDs. When using the quote in the UI we run an access check on the channel before quoting the messages so that is safe.

In this case, we are sending the context to the moderator group, so it should always be safe here too right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes Im not suggesting we add anything to transcript service. Im mostly asking Roman to take a step back and carefuly think about this.

Its potentially super easy to pass down ids to transcript service, so we should double check it and ensures we have tests for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep no problem at all, it's good to have these discussions 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

The transcript is a PM between the system user and the moderator's group, so all PM restrictions should apply. I'll add some tests to assert they're the only users with access to the PM and prevent possible regressions, but not sure if there's anything else to do here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Im fine with just extra tests 👍

ChatMessage
.where(chat_channel: chat_message.chat_channel)
.where("id < ?", chat_message.id)
.order("created_at DESC")
.limit(10)
.pluck(:id)
.reverse

return if previous_message_ids.empty?

service =
ChatTranscriptService.new(
chat_message.chat_channel,
Discourse.system_user,
messages_or_ids: previous_message_ids,
)

title =
I18n.t(
"chat.reviewables.direct_messages.transcript_title",
channel_name: chat_message.chat_channel.title(flagger),
locale: SiteSetting.default_locale,
)

body =
I18n.t(
"chat.reviewables.direct_messages.transcript_body",
transcript: service.generate_markdown,
locale: SiteSetting.default_locale,
)

create_args = {
archetype: Archetype.private_message,
title: title.truncate(SiteSetting.max_topic_title_length, separator: /\s/),
raw: body,
subtype: TopicSubtype.notify_moderators,
target_group_names: [Group[:moderators].name],
}

PostCreator.new(Discourse.system_user, create_args).create
end

def can_flag_again?(reviewable, message, flagger, flag_type_id)
return true if reviewable.blank?

Expand Down
3 changes: 2 additions & 1 deletion lib/guardian_extensions.rb
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,8 @@ def can_flag_chat_messages?

def can_flag_in_chat_channel?(chat_channel)
return false if !can_modify_channel_message?(chat_channel)
!chat_channel.direct_message_channel? && can_see_chat_channel?(chat_channel)

can_see_chat_channel?(chat_channel)
end

def can_flag_chat_message?(chat_message)
Expand Down
1 change: 1 addition & 0 deletions plugin.rb
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
register_asset "stylesheets/common/chat-onebox.scss"
register_asset "stylesheets/common/chat-skeleton.scss"
register_asset "stylesheets/colors.scss", :color_definitions
register_asset "stylesheets/common/reviewable-chat-message.scss"

register_svg_icon "comments"
register_svg_icon "comment-slash"
Expand Down
77 changes: 77 additions & 0 deletions spec/lib/chat_review_queue_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,14 @@
)
end

it "stores the message cooked content inside the reviewable" do
queue.flag_message(message, guardian, ReviewableScore.types[:off_topic])

reviewable = ReviewableChatMessage.last

expect(reviewable.payload["message_cooked"]).to eq(message.cooked)
end

context "when the user already flagged the post" do
let(:second_flag_result) do
queue.flag_message(message, guardian, ReviewableScore.types[:off_topic])
Expand Down Expand Up @@ -359,5 +367,74 @@
expect(message_poster.reload.silenced?).to eq(false)
end
end

context "when flagging a DM" do
fab!(:dm_channel) do
Fabricate(:direct_message_chat_channel, users: [message_poster, flagger])
end

12.times do |i|
fab!("dm_message_#{i + 1}") do
Fabricate(
:chat_message,
user: message_poster,
chat_channel: dm_channel,
message: "This is my message number #{i + 1}. Hello chat!",
)
end
end

it "raises an exception when using the notify_moderators flag type" do
expect {
queue.flag_message(dm_message_1, guardian, ReviewableScore.types[:notify_moderators])
}.to raise_error(Discourse::InvalidParameters)
end

it "raises an exception when using the notify_user flag type" do
expect {
queue.flag_message(dm_message_1, guardian, ReviewableScore.types[:notify_user])
}.to raise_error(Discourse::InvalidParameters)
end

it "includes a transcript of the previous 10 message for the rest of the flags" do
queue.flag_message(dm_message_12, guardian, ReviewableScore.types[:off_topic])

reviewable = ReviewableChatMessage.last
expect(reviewable.target).to eq(dm_message_12)
transcript_post = Post.find_by(topic_id: reviewable.payload["transcript_topic_id"])

expect(transcript_post.cooked).to include(dm_message_2.message)
expect(transcript_post.cooked).to include(dm_message_5.message)
expect(transcript_post.cooked).not_to include(dm_message_1.message)
end

it "doesn't include a transcript if there a no previous messages" do
queue.flag_message(dm_message_1, guardian, ReviewableScore.types[:off_topic])

reviewable = ReviewableChatMessage.last

expect(reviewable.payload["transcript_topic_id"]).to be_nil
end

it "the transcript is only available to moderators and the system user" do
moderator = Fabricate(:moderator)
admin = Fabricate(:admin)
leader = Fabricate(:leader)
tl4 = Fabricate(:trust_level_4)

queue.flag_message(dm_message_12, guardian, ReviewableScore.types[:off_topic])

reviewable = ReviewableChatMessage.last
transcript_topic = Topic.find(reviewable.payload["transcript_topic_id"])

expect(guardian.can_see_topic?(transcript_topic)).to eq(false)
expect(Guardian.new(leader).can_see_topic?(transcript_topic)).to eq(false)
expect(Guardian.new(tl4).can_see_topic?(transcript_topic)).to eq(false)
expect(Guardian.new(dm_message_12.user).can_see_topic?(transcript_topic)).to eq(false)
expect(Guardian.new(moderator).can_see_topic?(transcript_topic)).to eq(true)
expect(Guardian.new(admin).can_see_topic?(transcript_topic)).to eq(true)
expect(Guardian.new(Discourse.system_user).can_see_topic?(transcript_topic)).to eq(true)
end
end
end
end
13 changes: 2 additions & 11 deletions spec/requests/chat_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,9 @@ def flag_message(message, flagger, flag_type: ReviewableScore.types[:off_topic])
expect(response.parsed_body["meta"]["can_flag"]).to be true
end

it "returns `can_flag=false` for DM channels" do
it "returns `can_flag=true` for DM channels" do
get "/chat/#{dm_chat_channel.id}/messages.json", params: { page_size: page_size }
expect(response.parsed_body["meta"]["can_flag"]).to be false
expect(response.parsed_body["meta"]["can_flag"]).to be true
end

it "returns `can_moderate=true` based on whether the user can moderate the chatable" do
Expand Down Expand Up @@ -1220,15 +1220,6 @@ def create_notification_and_mention_for(user, sender, msg)
expect(response.status).to eq(403)
end

it "doesn't allow flagging direct messages" do
put "/chat/flag.json",
params: {
chat_message_id: admin_dm_message.id,
flag_type_id: ReviewableScore.types[:off_topic],
}
expect(response.status).to eq(403)
end

it "returns a 429 when the user attempts to flag more than 4 messages in 1 minute" do
RateLimiter.enable

Expand Down
Loading