This is a follow up of 5fcb7c262d
It was missing the case where secure uploads is enabled, which creates a copy of the upload no matter what.
So this checks for the original_sha1 of the uploads as well when checking for duplicates.
For e-mails, secure uploads redacts all secure images, and later uses the access control post to re-attached allowed ones. We pass the ID of this post through the X-Discourse-Post-Id header. As the name suggests, this assumes there's only ever one access control post. This is not true for activity summary e-mails, as they summarize across posts.
This adds a new header, X-Discourse-Post-Ids, which is used the same way as the old header, but also works for the case where an e-mail is associated with multiple posts.
## What?
Depending on the email software used, when you reply to an email that has some attachments, they will be sent along, since they're part of the embedded (replied to) email.
When Discourse processes the reply as an incoming email, it will automatically add all the (valid) attachments at the end of the post. Including those that were sent as part of the "embedded reply".
This generates posts in Discourse with duplicate attachments 🙁
## How?
When processing attachments of an incoming email, before we add it to the bottom of the post, we check it against all the previous uploads in the same topic. If there already is an `Upload` record, it means that it's a duplicate and it is _therefore_ skipped.
All the inline attachments are left untouched since they're more likely new attachments added by the sender.
For performance reasons we don't automatically add fabricated users to trust level auto-groups. However, when explicitly passing a trust level to the fabricator, in 99% of cases it means that trust level is relevant for the test, and we need the groups.
This change makes it so that when a trust level is explicitly passed to the fabricator, the auto-groups are refreshed. There's no longer a need to also pass refresh_auto_groups: true, which means clearer tests, fewer mistakes, and less confusion.
We have all these calls to Group.refresh_automatic_groups! littered throughout the tests. Including tests that are seemingly unrelated to groups. This is because automatic group memberships aren't fabricated when making a vanilla user. There are two places where you'd want to use this:
You have fabricated a user that needs a certain trust level (which is now based on group membership.)
You need the system user to have a certain trust level.
In the first case, we can pass refresh_auto_groups: true to the fabricator instead. This is a more lightweight operation that only considers a single user, instead of all users in all groups.
The second case is no longer a thing after #25400.
This change converts the min_trust_to_create_topic site setting to
create_topic_allowed_groups.
See: https://meta.discourse.org/t/283408
- Hides the old setting
- Adds the new site setting
- Add a deprecation warning
- Updates to use the new setting
- Adds a migration to fill in the new setting if the old setting was
changed
- Adds an entry to the site_setting.keywords section
- Updates tests to account for the new change
- After a couple of months, we will remove the min_trust_to_create_topicsetting entirely.
Internal ref: /t/117248
This change refactors the check `user.groups.any?` and instead uses
`user.staged?` to check if the user is staged or not.
Also fixes several tests to ensure the users have their auto trust level
groups created.
Follow up to:
- 8a45f84277
- 447d9b2105
- c89edd9e86
This change converts the `email_in_min_trust` site setting to
`email_in_allowed_groups`.
See: https://meta.discourse.org/t/283408
- Hides the old setting
- Adds the new site setting
- Add a deprecation warning
- Updates to use the new setting
- Adds a migration to fill in the new setting if the old setting was
changed
- Adds an entry to the site_setting.keywords section
- Updates tests to account for the new change
After a couple of months we will remove the
`email_in_min_trust` setting entirely.
Internal ref: /t/115696
This change converts the `approve_unless_trust_level` site setting to
`approve_unless_allowed_groups`.
See: https://meta.discourse.org/t/283408
- Adds the new site setting
- Adds a deprecation warning
- Updates core to use the new settings.
- Adds a migration to fill in the new setting of the old setting was
changed
- Adds an entry to the site_setting.keywords section
- Updates many tests to account for the new change
After a couple of months we will remove the `approve_unless_trust_level`
setting entirely.
Internal ref: /t/115696
The most common thing that we do with fab! is:
fab!(:thing) { Fabricate(:thing) }
This commit adds a shorthand for this which is just simply:
fab!(:thing)
i.e. If you omit the block, then, by default, you'll get a `Fabricate`d object using the fabricator of the same name.
* 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.
There are cases where a user can copy image markdown from a public
post (such as via the discourse-templates plugin) into a PM which
is then sent via an email. Since a PM is a secure context (via the
.with_secure_uploads? check on Post), the image will get a secure
URL in the PM post even though the backing upload is not secure.
This fixes the bug in that case where the image would be stripped
from the email (since it had a /secure-uploads/ URL) but not re-attached
further down the line using the secure_uploads_allow_embed_images_in_emails
setting because the upload itself was not secure.
The flow in Email::Sender for doing this is still not ideal, but
there are chicken and egg problems around when to strip the images,
how to fit in with other attachments and email size limits, and
when to apply the images inline via Email::Styles. It's convoluted,
but at least this fixes the Template use case for now.
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.
This commit adds an aria-label attribute to cooked hashtags using
the post/chat message decorateCooked functionality. I have just used
the inner content of the hashtag (the tag/category/channel name) for
the label -- we can reexamine at some point if we want something
different like "Link to dev category" or something, but from what I
can tell things like Twitter don't even have aria-labels for hashtags
so the text would be read out directly.
This commit also refactors any ruby specs checking the HTML of hashtags
to use rspec-html-matchers which is far clearer than having to maintain
the HTML structure in a HEREDOC for comparison, and gives better spec
failures.
c.f. https://meta.discourse.org/t/hashtags-are-getting-a-makeover/248866/23?u=martin
This commit makes some fundamental changes to how hashtag cooking and
icon generation works in the new experimental hashtag autocomplete mode.
Previously we cooked the appropriate SVG icon with the cooked hashtag,
though this has proved inflexible especially for theming purposes.
Instead, we now cook a data-ID attribute with the hashtag and add a new
span as an icon placeholder. This is replaced on the client side with an
icon (or a square span in the case of categories) on the client side via
the decorateCooked API for posts and chat messages.
This client side logic uses the generated hashtag, category, and channel
CSS classes added in a previous commit.
This is missing changes to the sidebar to use the new generated CSS
classes and also colors and the split square for categories in the
hashtag autocomplete menu -- I will tackle this in a separate PR so it
is clearer.
https://meta.discourse.org/t/improving-mailman-email-parsing/253041
When mirroring a public mailling list which uses mailman, there were some cases where the incoming email was not associated to the proper user.
As it happens, for various (undertermined) reasons, the email from the sender is often not in the `From` header but can be in any of the following headers: `Reply-To`, `CC`, `X-Original-From`, `X-MailFrom`.
It might be in other headers as well, but those were the ones we found the most reliable.
Currently processing emails that are blank or have a nil value for the mail will cause several errors.
This update allows emails with blank body or missing sender to log the blank email error to the mail logs rather than throwing an error.
This header is used by Microsoft Exchange to indicate when certain types of
autoresponses should not be generated for an email.
It triggers our "is this mail autogenerated?" detection, but should not be used
for this purpose.
This commit does a couple of things:
1. Changes the limit of tags to include a subject for a
notification email to the `max_tags_per_topic` setting
instead of the arbitrary 3 limit
2. Adds both an X-Discourse-Tags and X-Discourse-Category
custom header to outbound emails containing the tags
and category from the subject, so people on mail clients
that allow advanced filtering (i.e. not Gmail) can filter
mail by tags and category, which is useful for mailing
list mode users
c.f. https://meta.discourse.org/t/headers-for-email-notifications-so-that-gmail-users-can-filter-on-tags/249982/17
* DEV: Remove enable_whispers site setting
Whispers are enabled as long as there is at least one group allowed to
whisper, see whispers_allowed_groups site setting.
* DEV: Always enable whispers for admins if at least one group is allowed.
In some cases (e.g. user notification emails) we
are passing an excerpted/stripped version of the
post HTML to Email::Styles, at which point the
<span> elements surrounding the hashtag text have
been stripped. This caused an error when trying to
remove that element to replace the text.
Instead we can just remove all elements inside
a.hashtag-cooked and replace with the raw #hashtag
text which will work in more cases.
This commit fleshes out and adds functionality for the new `#hashtag` search and
lookup system, still hidden behind the `enable_experimental_hashtag_autocomplete`
feature flag.
**Serverside**
We have two plugin API registration methods that are used to define data sources
(`register_hashtag_data_source`) and hashtag result type priorities depending on
the context (`register_hashtag_type_in_context`). Reading the comments in plugin.rb
should make it clear what these are doing. Reading the `HashtagAutocompleteService`
in full will likely help a lot as well.
Each data source is responsible for providing its own **lookup** and **search**
method that returns hashtag results based on the arguments provided. For example,
the category hashtag data source has to take into account parent categories and
how they relate, and each data source has to define their own icon to use for the
hashtag, and so on.
The `Site` serializer has two new attributes that source data from `HashtagAutocompleteService`.
There is `hashtag_icons` that is just a simple array of all the different icons that
can be used for allowlisting in our markdown pipeline, and there is `hashtag_context_configurations`
that is used to store the type priority orders for each registered context.
When sending emails, we cannot render the SVG icons for hashtags, so
we need to change the HTML hashtags to the normal `#hashtag` text.
**Markdown**
The `hashtag-autocomplete.js` file is where I have added the new `hashtag-autocomplete`
markdown rule, and like all of our rules this is used to cook the raw text on both the clientside
and on the serverside using MiniRacer. Only on the server side do we actually reach out to
the database with the `hashtagLookup` function, on the clientside we just render a plainer
version of the hashtag HTML. Only in the composer preview do we do further lookups based
on this.
This rule is the first one (that I can find) that uses the `currentUser` based on a passed
in `user_id` for guardian checks in markdown rendering code. This is the `last_editor_id`
for both the post and chat message. In some cases we need to cook without a user present,
so the `Discourse.system_user` is used in this case.
**Chat Channels**
This also contains the changes required for chat so that chat channels can be used
as a data source for hashtag searches and lookups. This data source will only be
used when `enable_experimental_hashtag_autocomplete` is `true`, so we don't have
to worry about channel results suddenly turning up.
------
**Known Rough Edges**
- Onebox excerpts will not render the icon svg/use tags, I plan to address that in a follow up PR
- Selecting a hashtag + pressing the Quote button will result in weird behaviour, I plan to address that in a follow up PR
- Mixed hashtag contexts for hashtags without a type suffix will not work correctly, e.g. #ux which is both a category and a channel slug will resolve to a category when used inside a post or within a [chat] transcript in that post. Users can get around this manually by adding the correct suffix, for example ::channel. We may get to this at some point in future
- Icons will not show for the hashtags in emails since SVG support is so terrible in email (this is not likely to be resolved, but still noting for posterity)
- Additional refinements and review fixes wil
This commit renames all secure_media related settings to secure_uploads_* along with the associated functionality.
This is being done because "media" does not really cover it, we aren't just doing this for images and videos etc. but for all uploads in the site.
Additionally, in future we want to secure more types of uploads, and enable a kind of "mixed mode" where some uploads are secure and some are not, so keeping media in the name is just confusing.
This also keeps compatibility with the `secure-media-uploads` path, and changes new
secure URLs to be `secure-uploads`.
Deprecated settings:
* secure_media -> secure_uploads
* secure_media_allow_embed_images_in_emails -> secure_uploads_allow_embed_images_in_emails
* secure_media_max_email_embed_image_size_kb -> secure_uploads_max_email_embed_image_size_kb
See https://meta.discourse.org/t/discourse-email-messages-are-incorrectly-threaded/233499
for thorough reasoning.
This commit changes how we generate Message-IDs and do email
threading for emails sent from Discourse. The main changes are
as follows:
* Introduce an outbound_message_id column on Post that
is either a) filled with a Discourse-generated Message-ID
the first time that post is used for an outbound email
or b) filled with an original Message-ID from an external
mail client or service if the post was created from an
incoming email.
* Change Discourse-generated Message-IDs to be more consistent
and static, in the format `discourse/post/:post_id@:host`
* Do not send References or In-Reply-To headers for emails sent
for the OP of topics.
* Make sure that In-Reply-To is filled with either a) the OP's
Message-ID if the post is not a direct reply or b) the parent
post's Message-ID
* Make sure that In-Reply-To has all referenced post's Message-IDs
* Make sure that References is filled with a chain of Message-IDs
from the OP down to the parent post of the new post.
We also are keeping X-Discourse-Post-Id and X-Discourse-Topic-Id,
headers that we previously removed, for easier visual debugging
of outbound emails.
Finally, we backfill the `outbound_message_id` for posts that have
a linked `IncomingEmail` record, using the `message_id` of that record.
We do not need to do that for posts that don't have an incoming email
since they are backfilled at runtime if `outbound_message_id` is missing.
The maximum_staged_users_per_email site setting controls how many
staged users will be invited to the topic created from an incoming
email. Previously, it counted only the new staged users.
When sending emails with delivery_method_options -> return_response
set to true, the SMTP sending code inside Mail will return the SMTP
response when calling deliver! for mail within the app. This commit
ensures that Email::Sender captures this response if it is returned
and stores it against the EmailLog created for the sent email.
A follow up PR will make this visible within the admin email UI.
* FIX: Email Send post has already been taken error
Adding a failing test first before coming up with a good solution.
Related: 357011eb3b
The above commit changed
```
PostReplyKey.find_or_create_by_safe!
```
to
```
PostReplyKey.create_or_find_by!
```
But I don't think it is working as a 1-1 replacement because of the
`Validation failed: Post has already been taken` error we are receiving
with this change. Also we need to make sure we don't re-introduce any
concurrency issues.
Reported: https://meta.discourse.org/t/224706/13
* Remove rails unique constraint and rely on db index
I believe this is what is causing `create_or_find_by!` to fail. Because
we have a unique constraint in the db I think we can remove this rails
unique constraint?
* clean up spec wording
`TestLogger` was responsible for some flaky specs runs:
```
Error during failsafe response: undefined method `debug' for #<TestLogger:0x0000556c4b942cf0 @warnings=1>
Did you mean? debugger
```
This commit also cleans up other uses of `FakeLogger`
This PR enables custom email dark mode styles by default that were added here.
There is currently poor support for dark mode queries in mail clients. The main beneficiary of these changes will be Apple Mail and Outlook.
Enjoy the darkness 🕶️
Under some conditions, replacing an `<img` with `![]()` can break rendering, and make the image disappear.
Context at https://meta.discourse.org/t/152801
It's very easy to forget to add `require 'rails_helper'` at the top of every core/plugin spec file, and omissions can cause some very confusing/sporadic errors.
By setting this flag in `.rspec`, we can remove the need for `require 'rails_helper'` entirely.
This allows text editors to use correct syntax coloring for the heredoc sections.
Heredoc tag names we use:
languages: SQL, JS, RUBY, LUA, HTML, CSS, SCSS, SH, HBS, XML, YAML/YML, MF, ICS
other: MD, TEXT/TXT, RAW, EMAIL
