Inconsistent code / requirements for PM tagging

Bug
1

The code for handling tagging of personal messages is inconsistent.

In some parts of the code base, being in pm_tags_allowed_for_groups is sufficient for being able to tag a personal message.

In other parts of the code base, one needs to be in both pm_tags_allowed_for_groups and tag_topic_allowed_groups.

This is especially restrictive when there is a need for users to be able to tag their own personal messages while you don’t want them to tag public topics (e.g. this Private template permissions to override allowed PM tagging groups ), i.e. when pm_tags_allowed_for_groups is not a proper subset of tag_topic_allowed_groups.

Analysis

In TagGuardian these are distinct settings. You only need to be in pm_tags_allowed_for_groups in order to be able to tag a PM.

  def can_tag_topics?
    SiteSetting.tagging_enabled && @user.in_any_groups?(SiteSetting.tag_topic_allowed_groups_map)
  end
  
  def can_tag_pms?
    return false if !SiteSetting.tagging_enabled
    return false if @user.blank?
    return true if @user == Discourse.system_user

    group_ids = SiteSetting.pm_tags_allowed_for_groups_map
    group_ids.include?(Group::AUTO_GROUPS[:everyone]) ||
      @user.group_users.exists?(group_id: group_ids)
  end

The same goes for Guardian.can_tag? The settings are distinct.

  def can_tag?(topic)
    return false if topic.blank?

    topic.private_message? ? can_tag_pms? : can_tag_topics?
  end

ListController is happy with only can_tag_pms? as well

    when :private_messages_tag
      raise Discourse::NotFound if target_user.id != current_user.id
      raise Discourse::NotFound if !guardian.can_tag_pms?

However, TopicGuardian requires a user to be in both tag_topic_allowed_groups and pm_tags_allowed_for_groups if they want to be able to tag a PM.

  def can_edit_tags?(topic)
    return false unless can_tag_topics?
    return false if topic.private_message? && !can_tag_pms?
    return true if can_edit_topic?(topic)

    if topic&.first_post&.wiki &&
         @user.in_any_groups?(SiteSetting.edit_wiki_post_allowed_groups_map)
      return can_create_post?(topic)
    end

    false
  end

Client side, the same restriction is in Composer

  @discourseComputed("model.canEditTitle", "model.creatingPrivateMessage")
  canEditTags(canEditTitle, creatingPrivateMessage) {
    const isPrivateMessage =
      creatingPrivateMessage || this.get("model.topic.isPrivateMessage");
    return (
      canEditTitle &&
      this.site.can_tag_topics &&
      (!isPrivateMessage || this.site.can_tag_pms)
    );
  }

and TopicController

  @discourseComputed("model.isPrivateMessage")
  canEditTags(isPrivateMessage) {
    return (
      this.site.get("can_tag_topics") &&
      (!isPrivateMessage || this.site.get("can_tag_pms"))
    );
  }

But the Move to Topic modal does not have this restriction:

  get canTagMessages() {
    return this.site.can_tag_pms;
  }

and neither does ChatToTopicSelector

@alias("site.can_tag_pms") canTagMessages;

and neither does UserPrivateMessagesController

@readOnly("site.can_tag_pms") pmTaggingEnabled;

    if (this.pmTaggingEnabled) {
      content.push({
        id: this.router.urlFor("userPrivateMessages.tags", usernameLower),
        name: i18n("user.messages.tags"),
        icon: "tags",
      });
    }

Summary

Class C/S Behavior
TagGuardian server pm_tags_allowed_for_groups only
Guardian server pm_tags_allowed_for_groups only
ListController server pm_tags_allowed_for_groups only
TopicGuardian server both pm_tags_allowed_for_groups and tag_topic_allowed_groups
Composer client both pm_tags_allowed_for_groups and tag_topic_allowed_groups
TopicController client both pm_tags_allowed_for_groups and tag_topic_allowed_groups
Move to Topic modal client pm_tags_allowed_for_groups only
ChatToTopicSelector client pm_tags_allowed_for_groups only

Additional issues

I found two more additional impediments:

PostRevisor uses tc.guardian.can_tag_topics? instead of tc.guardian.can_tag?(tc.topic)

and in DiscourseTagging

tag_topic_by_names does the right thing

  def self.tag_topic_by_names(topic, guardian, tag_names_arg, append: false)
    if guardian.can_tag?(topic)

but then tags_for_saving does not


   def self.tags_for_saving(tags_arg, guardian, opts = {})
    return [] unless guardian.can_tag_topics? && tags_arg.present?

This last one is especially nasty since the function is not aware of whether it is working on a PM.

4 Likes