search

club-1 / flarum-ext-cross-references

Add cross reference links when a discussion is mentioned from another one.
GNU Affero General Public License v3.0
6 stars 1 forks source link

Do not invalidate `unknown` discussion links of CommentPosts at render time #35

Closed n-peugnet closed 1 year ago

n-peugnet commented 1 year ago

This invalidation causes problems for multiple reasons, as @rob006 explained in https://github.com/club-1/flarum-ext-cross-references/pull/31#issuecomment-1435655730:

Replacing the original link with an unclickable placeholder is no-go in my case. Even if we ignore the possibility, that detection may be incorrect, you're just hiding useful information, like ID or slug, that may explain the intention of this link. There is also compatibility with other extensions or tools - some of them may operate on different permissions than consumer of output. For example, an external tool that converts discussion to PDF may not have permission to see a particular discussion, but a user that will read this PDF may have such permission, and hiding these links from him is a regression. Or extension that handles redirections - AFAIK it is not implemented yet, but it makes perfect sense to handle redirection if 2 discussions were merged, so deleted one will redirect to a target discussion. Right now this extension hides links, that may actually work. IMO this feature should be optional, and I strongly advise disabling it by default.

To summarize:

  1. The permission checking code can be wrong if $request is null or invalid, thus removing a link that should be present.
  2. Replacing a link with the unclickable [unknown discussion] makes it not only unfollow-able, it removes the information it contains: its text and destination (which are the same in this case).

Questions

Should invalidating the link still be an option? And if yes, should it be disabled by default?

@rob006 thought no, or if yes, disabled by default.

What should be the displayed text of the link?

Simply the original content of the message. So this means using the @url parameter.

What about unknown Short tags?

I think it should not produce a link, as it was not present in the original source.

Should it have Discussion* classes?

Not sure.

What about the problem of revealing the title of the discussions?

The [unknown discussion] placeholder was first added to prevent users from discovering information about hidden discussions (#10). For instance by posting a link to all the discussion ids incrementally, to get access to te title of all the discussions. The link invalidation came later (#15).

But as @rob006 pointed out, this behaviour is mostly useful for DiscussionReferencedPost:

Replacing links to potentially inaccessible discussions with placeholder should be switchable in extension settings and disabled by default. This applies only to links in post content - current behavior for DiscussionReferencedPost is correct.

By displaying the original link content instead of [unknown discussion] we do not reveal more information that what the message originally contained.

rob006 commented 1 year ago

But as @rob006 pointed out, this behaviour is mostly useful for DiscussionReferencedPost:

To be clear, I think that it would be much better to completely hide info about references in DiscussionReferencedPost, if they're not accessible. So if DiscussionReferencedPost has info about 2 discussions, but one of them is unavailable, then display info only about one available discussion. If all discussions in DiscussionReferencedPost are unavailable, then I would hide this event post completely. In this way we can avoid unnecessary noise and unintentional revealing of some information (currently you don't know title of discussion, but you still can see that specific person mentioned this discussion).

So if you decide to not have a setting to enable replacing unavailable discussion with placeholder, there will be no scenario with placeholder at all.

n-peugnet commented 1 year ago

But as @rob006 pointed out, this behaviour is mostly useful for DiscussionReferencedPost:

To be clear, I think that it would be much better to completely hide info about references in DiscussionReferencedPost, if they're not accessible. So if DiscussionReferencedPost has info about 2 discussions, but one of them is unavailable, then display info only about one available discussion. If all discussions in DiscussionReferencedPost are unavailable, then I would hide this event post completely. In this way we can avoid unnecessary noise and unintentional revealing of some information (currently you don't know title of discussion, but you still can see that specific person mentioned this discussion).

So if you decide to not have a setting to enable replacing unavailable discussion with placeholder, there will be no scenario with placeholder at all.

After thinking a little bit about it, I think I like this approach. But to implement it properly kind of requires #13 if we filter them in the frontend. Another problem is that I don't know how to completely invalidate an EventPost in the frontend. Maybe I could override the view() method, but it would probably not look good. Maybe the best way would be to filter them from the backend, but not sure how feasible it would be.