Mailgun api key check backward (no API key should be required if bounces handled by mail receiver)

AI-generated summary

pfaffman identified a logic error in Discourse’s problem checks regarding Mailgun API keys. The current implementation incorrectly flags sites as having a problem when an API key is missing if email replies are enabled. However, when using the mail-receiver container to handle bounces directly, the API key and associated webhooks are unnecessary. pfaffman noted that the code logic appears backward, as it warns users who do not need webhooks while remaining silent on configurations that might actually require them. tobiaseigen agreed, stating the warning is confusing for users with mail-receiver enabled and suggested improving the UI by linking directly to the relevant settings and providing better documentation descriptions. In response to these findings, zogstrip refactored the code to create a global “email bounce handling” problem check applicable to all mail providers, rather than just Mailgun, and submitted a pull request to address the issue here.

I’ve noticed a bunch of sites complaining that no Mailgun API key is defined, but (at least most of) those sites are receiving replies via the mail-receiver container and it handles those bounces so the API key (and corresponding webhook on Mailgun) do not need to be required. It looks like the logic in the specs (and hence, the code) is backward:

The missing_mailgun_api_key_spec

Shows this:

    context "when using Mailgun without an API key" do
      let(:replies_enabled) { true }
      let(:mailgun_address) { "smtp.mailgun.org" }
      let(:api_key) { nil }

      it do
        expect(check).to have_a_problem.with_priority("low").with_message(
          "The server is configured to send emails via Mailgun but you haven't provided an API key used to verify the webhook messages.",
        )
      end

complains if we have replies enabled, but in that case, we don’t need the webhook and

    context "when replies are disabled" do
      let(:replies_enabled) { false }
      let(:mailgun_address) { anything }
      let(:api_key) { anything }

      it { expect(check).to be_chill_about_it }
    end

Here we are NOT processing bounces with the mail receiver and we are being chill about it.

Am I confused or is this backward?

Well, maybe I am confused. It sure seems like the mail receiver isn’t telling Discourse about bounces. A busy site has no recent bounced messages.

But:

And:

So this documentation suggests that if you have a working mail receiver it should be handling this. And code that I once wrote in some of my internal tooling that set up these webhooks says “# don’t need webhooks when we have a mail receiver”

3 Likes

I just came across this myself. If you have mail-receiver enabled, this warning makes no sense. I just dismissed the warning given what I read here (thanks Jay!), but can’t help thinking this could be handled better. At least provide a link to the setting so you don’t have to hunt for it (spoiler: it’s not in the email settings), and on the setting provide a better description with more detail and maybe a link to a documentation topic on meta?

1 Like

While investigating this, I noticed that we only had a problem check for Mailgun. So I did a little bit of refactoring to add a global “email bounce handling” problem check that will work for all mail providers we have configured.

https://github.com/discourse/discourse/pull/38028

2 Likes

This topic was automatically closed after 5 days. New replies are no longer allowed.