* FIX: Secure upload post processing race condition
This commit fixes a couple of issues.
A little background -- when uploads are created in the composer
for posts, regardless of whether the upload will eventually be
marked secure or not, if secure_uploads is enabled we always mark
the upload secure at first. This is so the upload is by default
protected, regardless of post type (regular or PM) or category.
This was causing issues in some rare occasions though because
of the order of operations of our post creation and processing
pipeline. When creating a post, we enqueue a sidekiq job to
post-process the post which does various things including
converting images to lightboxes. We were also enqueuing a job
to update the secure status for all uploads in that post.
Sometimes the secure status job would run before the post process
job, marking uploads as _not secure_ in the background and changing
their ACL before the post processor ran, which meant the users
would see a broken image in their posts. This commit fixes that issue
by always running the upload security changes inline _within_ the
cooked_post_processor job.
The other issue was that the lightbox wrapper link for images in
the post would end up with a URL like this:
```
href="/secure-uploads/original/2X/4/4e1f00a40b6c952198bbdacae383ba77932fc542.jpeg"
```
Since we weren't actually using the `upload.url` to pass to
`UrlHelper.cook_url` here, we weren't converting this href to the CDN
URL if the post was not in a secure context (the UrlHelper does not
know how to convert a secure-uploads URL to a CDN one). Now we
always end up with the correct lightbox href. This was less of an issue
than the other one, since the secure-uploads URL works even when the
upload has become non-secure, but it was a good inconsistency to fix
anyway.
This commit removes any logic in the app and in specs around
enable_experimental_hashtag_autocomplete and deletes some
old category hashtag code that is no longer necessary.
It also adds a `slug_ref` category instance method, which
will generate a reference like `parent:child` for a category,
with an optional depth, which hashtags use. Also refactors
PostRevisor which was using CategoryHashtagDataSource directly
which is a no-no.
Deletes the old hashtag markdown rule as well.
Not all revisions involve changes to the actual post/topic content. We
may want to know if a revisions includes the topic title or post raw.
Specifically introducing these for use in the Akismet plugin to
conditionally queue checks.
* FIX: Displaying the wrong number of minimum tags in the composer
When the minimum number of tags set for the category is larger than the minimum number of tags
set in the category tag-groups, the composer was displaying the wrong value.
This commit fixes the value displayed in the composer to show the max value between the required
for the category and the tag-groups set for the category.
This bug was reported on Meta in https://meta.discourse.org/t/tags-from-multiple-tag-groups-required-only-suggest-select-at-least-one-tag/263817
* FIX: Limiting tags in categories not working as expected
When a category was restricted to a tag group A, which was set to only allow
one tag from the group per topic, selecting a tag belonging only to A returned
other tags from A that also belonged to other group/s (if any).
Example:
Tag group A: alpha, beta, gamma, epsilon, delta
Tag group B: alpha, beta, gamma
Both tag groups set to only allow one tag from the group per topic.
If Category 1 was set to only allow tags from the tag group A, and the first tag
selected was epsilon, then, because they also belonged to tag group B, the tags
alpha, beta, and gamma were still returned as valid options when they should not be.
This commit ensures that once a tag from a tag group that restricts its tags to
one per topic is selected, no other tag from this group is returned.
This bug was reported on Meta in https://meta.discourse.org/t/limiting-tags-to-categories-not-working-as-expected/263143.
* FIX: Moving topics does not prompt to add required tag for new category
When a topic moved from a category to another, the tag requirements
of the new category were not being checked.
This allowed a topic to be created and moved to a category:
- that limited the tags to a tag group, with the topic containing tags
not allowed.
- that required N tags from a tag group, with the topic not containing
the required tags.
This bug was reported on Meta in https://meta.discourse.org/t/moving-tagged-topics-does-not-prompt-to-add-required-tag-for-new-category/264138.
* FIX: Editing topics with tag groups from parents allows incorrect tagging
When there was a combination between parent tags defined in a tag group
set to allow only one tag from the group per topic, and other tag groups
relying on this restriction to combine the children tag types with the
parent tag, editing a topic could allow the user to insert an invalid
combination of these tags.
Example:
Automakers tag group: landhover, toyota
- group set to limit one tag from the group per topic
Toyota models group: land-cruiser, hilux, corolla
Landhover models group: evoque, defender, discovery
If a topic was initially set up with the tags toyota, land-cruiser it was
possible to edit it by removing the tag toyota and adding the tag landhover
and other landhover model tags like evoque for example.
In this case, the topic would end up with the tags toyota, land-cruiser,
landhover, evoque because Discourse will automatically insert the
missing parent tag toyota when it detects the tag land-cruiser.
This combination of tags would violate the restriction specified in
the Automakers tag group resulting in an invalid combination of tags.
This commit enforces that the "one tag from the group per topic"
restriction is verified before updating the topic tags and also
make sure the verification checks the compatibility of parent tags that
would be automatically inserted.
After the changes, the user will receive an error similar to:
The tags land-cruiser, landhover cannot be used simultaneously.
Please include only one of them.
- Update welcome topic copy
- Edit the welcome topic automatically when the title or description changes
- Remove “Create your Welcome Topic” banner/CTA
- Add "edit welcome topic" user tip
What is the problem?
Consider the following timeline:
1. OP starts a topic.
2. Troll responds snarkily.
3. Flagger flags the post as “inappropriate”.
4. Admin agrees and hides the post.
5. Troll ninja-edits the post within the grace period, but still snarky.
6. Flagger flags the post as inappropriate again.
The current behaviour is that the flagger is met with an error saying the post has been reviewed and can't be flagged again for the same reason.
The desired behaviour is after someone has edited a post, it should be flaggable again.
Why is this happening?
This is related to the ninja-edit feature, where within a set grace period no new revision is created, but a new revision is required to flag the same post for the same reason.
So essentially there is a window between the naughty corner cooldown where a flagged post can't be edited, and the ninja-edit grace period, where an edit can be made without a new revision. Posts that are edited within this window can't be re-flagged by the same user.
|-----------------|-------------------------------|
^ Flag accepted | ~~~~~~~~~~~~~ 🥷🏻 ~~~~~~~~~~~~ |
| ^ Editing grace period over
^ Naughty corner cooldown over
How does this fix it?
We already create a new revision when ninja-editing a post with a pending flag. The issue above happens only in the case where the flag is already accepted.
This change extends the existing behaviour so that a new revision is created when ninja-editing any flagged post, regardless of the status of the flag. (Deleted flags excluded.)
This should also help with posterity, avoiding situations where a successfully flagged post looks innocuous in the history because it was ninja-edited, and vice versa.
This adds a SiteSetting, which when enabled, creates a small_action post for tag/category changes to the topic. It uses `topic.add_moderator_post, and passes raw text in, to describe the change.
The #pluck_first freedom patch, first introduced by @danielwaterworth has served us well, and is used widely throughout both core and plugins. It seems to have been a common enough use case that Rails 6 introduced it's own method #pick with the exact same implementation. This allows us to retire the freedom patch and switch over to the built-in ActiveRecord method.
There is no replacement for #pluck_first!, but a quick search shows we are using this in a very limited capacity, and in some cases incorrectly (by assuming a nil return rather than an exception), which can quite easily be replaced with #pick plus some extra handling.
This will be used by plugins to handle the client side of their custom
post validations without having to overwrite the whole composer save
action as it was done in other plugins.
Co-authored-by: Penar Musaraj <pmusaraj@gmail.com>
When PostRevisor is called with 'skip_validations: true' it can save
the post twice and one of the calls passes the correct 'validate: false'
argument, but the other one does not.
* FIX: hide welcome topic banner as soon as the welcome topic is edited
This commit adds a message bus listener on client to hide the welcome
topic banner as soon as the welcome topic is edited.
* update test
* only subscribe when show_welcome_topic_banner is true
* Do not lookup for messageBus service if it's not required
* Remove unneeded code
* Cache result for Site.show_welcome_topic_banner
* Update tests per latest changes
* Changes per PR review
Tag edit notifications are either created by PostActionNotifier or
PostRevisor. PostActionNotifier already checks if the site setting is
enabled. but PostRevisor scheduled a NotifyTagChange job without
checking disable_tags_edit_notifications.
This commit handles the edge case where a draft is lost with no warnings if the user edits the title (or category/tags) of a topic while they're replying.to the same topic. Repro steps are as follows:
1. Start replying to a topic and type enough to get a draft saved.
2. Scroll up to the topic title and click the pencil icon next to the topic title, change the title, category and/or tags, and then save the changes.
3. Reload the page and you'll see that the draft is gone.
This happens because we only allow 1 draft per topic per user and when you edit the title of a topic that you're replying to, from the server perspective it'll look like as if you've submitted your reply so it will advance the draft sequence for the topic and delete the draft.
The fix in this commit makes `PostRevisor` skip advancing the draft sequence when a topic's title is edited using the pencil button next to the title.
Internal ticket: t60854.
Co-authored-by: Robin Ward <robin.ward@gmail.com>
Ensures that `UserStat#post_count` and `UserStat#topic_count` does not
go below 0. When it does like it did now, we tend to have bugs in our
code since we're usually coding with the assumption that the count isn't
negative.
In order to support the constraints, our post and topic fabricators in
tests will now automatically increment the count for the respective
user's `UserStat` as well. We have to do this because our fabricators
bypasss `PostCreator` which holds the responsibility of updating `UserStat#post_count` and
`UserStat#topic_count`.
* FIX: Preserve field types when updating revision
When a post was edited quickly twice by the same user, the old post
revision was updated with the newest changes. To check if the change
was reverted (i.e. rename topic A to B and then back to A) a comparison
of the initial value and last value is performed. If the check passes
then the intermediary value is dismissed and only the initial value and
the last ones are preserved. Otherwise, the modification is dismissed
because the field returned to its initial value.
This used to work well for most fields, but failed for "tags" because
the field is an array and the values were transformed to strings to
perform the comparison.
* FIX: Reset last_editor_id if revision is reverted
If a post was revised and then the same revision was reverted,
last_editor_id was still set to the ID of the user who last edited the
post. This was a problem because the same person could then edit the
same post again and because it was the same user and same post, the
system attempted to update the last one (that did not exist anymore).
This reverts a part of changes introduced by https://github.com/discourse/discourse/pull/13947
In that PR I:
1. Disallowed topic feature links for TL-0 users
2. Additionally, disallowed just putting any URL in topic titles for TL-0 users
Actually, we don't need the second part. It introduced unnecessary complexity for no good reason. In fact, it tries to do the job that anti-spam plugins (like Akismet plugin) should be doing.
This PR reverts this second change.
This disallows putting URLs in topic titles for TL0 users, which means that:
If a TL-0 user puts a link into the title, a topic featured link won't be generated (as if it was disabled in the site settings)
Server methods for creating and updating topics will be refusing featured links when they are called by TL-0 users
TL-0 users won't be able to put any link into the topic title. For example, the title "Hey, take a look at https://my-site.com" will be rejected.
Also, it improves a bit server behavior when creating or updating feature links on topics in the categories with disabled featured links. Before the server just silently ignored a featured link field that was passed to him, now it will be returning 422 response.
We renamed the site setting for this long ago, but there
were a few places left in the code base where "ninja edit"
needed to be turned into "grace period". Doing this here
to avoid combatative language.
When editing the first post for the topic we do two AJAX requests
to two separate controllers in this order:
PUT /t/topic-name
PUT /posts/2489523
This causes two post revisor calls, which end up triggering the
:post_edited DiscourseEvent twice. This is then picked up and sent
as a WebHook event twice. However we do not need to send a :post_edited
webhook event if the first post is being edited and topic_changed is
true from the :post_edited DiscourseEvent, because a second event will
shortly come through for just the post.
See https://meta.discourse.org/t/post-webhook-fires-two-times-on-post-edited-for-first-post-in-a-topic/162408
Continued on from https://github.com/discourse/discourse/pull/10590
When uploads are created from the composer (editing or creating a post),
for sites with secure uploads enabled we assume security by default and
that new upload is set to secure. When the post is created, we then
check whether the post uploads _actually_ need to be secure and adjust
accordingly.
We were not doing this when revising a post, so when a new upload was
created when editing a post in a public topic, the secure status stayed
true erroneously causing issues with image previews, among other things.
* FEATURE: Review every post using the review queue.
If the `review_every_post` setting is enabled, posts created and edited by regular uses are sent to the review queue so staff can review them. We'll skip PMs and posts created or edited by TL4 or staff users.
Staff can choose to:
- Approve the post (nothing happens)
- Approve and restore the post (if deleted)
- Approve and unhide the post (if hidden)
- Reject and delete it
- Reject and keep deleted (if deleted)
- Reject and suspend the user
- Reject and silence the user
* Update config/locales/server.en.yml
Co-authored-by: Robin Ward <robin.ward@gmail.com>
Co-authored-by: Robin Ward <robin.ward@gmail.com>
The instance of the PostRevisor is passed to the post_edited
event. It is useful to know what has happened to the topic in
this event (we already pass a boolean for topic_changed? but that
is not so helpful by itself).
The bug was mentioned on meta https://meta.discourse.org/t/users-are-seeing-handling-of-unhandled-tag-again/155367
It was related to users who are watching a specific topic. In that case, when the hidden tag was added or removed to the topic they were notified by `NotifyTagChangeJob`.
That job should take hidden tags into consideration. If all changed tags are in a hidden group, it should exclude user not belong to that group.
At the same time, if visible to anyone tag is added or removed users watching topic should be notified.
Editing a post didn't update the `post_uploads` right away. Instead it relied on the `CookedPostProcessor`. This can lead to an inconsistent state if uploads are added or removed during an edit and, for some reason, the `ProcessPost` job doesn't run (successfully). This inconsistency leads to missing uploads, because the newly added uploads appear to be unused and will be deleted by the `CleanUpUploads` job. In addition to that, uploads, which got removed during the edit, appear to be still in use and won't be deleted by the background job.
This commit ensures that the `post_uploads` are updated during the edit without relying on a background job.
This reverts commit 20780a1eee.
* SECURITY: re-adds accidentally reverted commit:
03d26cd6: ensure embed_url contains valid http(s) uri
* when the merge commit e62a85cf was reverted, git chose the 2660c2e2 parent to land on
instead of the 03d26cd6 parent (which contains security fixes)
* We now have a site setting "topic_excerpt_maxlength" that is used when the OP is created or revised to generate a topic excerpt.
* However, posts created before this setting was introduced cannot benefit from this change unless they are revised, and if the topic excerpt length setting is changed that situation is also not covererd.
* This PR makes a change to rebake! to update the topic excerpt IF the post is the OP.
* PERF: Dematerialize topic_reply_count
It's only ever used for trust level promotions that run daily, or compared to 0. We don't need to track it on every post creation.
* UX: Add symbol in TL3 report if topic reply count is capped
* DEV: Drop user_stats.topic_reply_count column
Recently, we added feature that we are sending `/muted` to users who muted specific topic just before `/latest` so the client knows to ignore those messages - https://github.com/discourse/discourse/pull/9482
Same `/muted` message should be included when the post is edited
* DEV: Use `render_json_error` (Adds specs for Admin::GroupsController)
* DEV: Use a specific error on blank category slug (Fixes a `render_json_error` warning)
* DEV: Use a specific error on reviewable claim conflict (Fixes a `render_json_error` warning)
* DEV: Use specific errors in Admin::UsersController (Fixes `render_json_error` warnings)
* FIX: PublishedPages error responses
* FIX: TopicsController error responses (There was an issue of two separate `Topic` instances for the same record. This makes sure there's only one up-to-date instance.)
* Do not grant badges for posts with no user
* Ensure instructions are correct in Change Owner modal
* Hide user-dependent actions from posts with no user
* Make PostRevisor work with posts with no user
* Ensure posts with no user can be deleted
* discourse-narrative-bot should ignore posts with no user
* Skip TopicLink creation for posts with no user
Martin Brennan
Selase Krakani
Sérgio Saquetim
Bianca Nenciu
Ted Johansson
Mark VanLandingham
David Taylor
Alan Guo Xiang Tan
Rafael dos Santos Silva
Arpit Jalan
Osama Sayegh
Dan Ungureanu
Andrei Prigorshnev
Jean
Martin Brennan
mintsaxon
Roman Rizzi
Mark VanLandingham
Krzysztof Kotlarek
Gerhard Schlager
jbrw
Michael Brown
Jeff Atwood
Kane York
Jarek Radosz
Vinoth Kannan