Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SMTP: avoid loading templates if not enabled #1548

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

crabique
Copy link

@crabique crabique commented Feb 26, 2024

Hi!

This small fix aligns SMTP with the current httpd behaviour: at the moment, even if smtp.host is '', it still tries to load the templates before actually disabling SMTP capabilities.

This change allows to serve without the templates/bundle if both httpd and smtp are explicitly disabled.

Checklist for Pull Requests


@crabique crabique requested a review from drakkan as a code owner February 26, 2024 15:26
@CLAassistant
Copy link

CLAassistant commented Apr 12, 2024

CLA assistant check
All committers have signed the CLA.

@drakkan
Copy link
Owner

drakkan commented Apr 12, 2024

Thanks for this PR and sorry for the delay. We have been working on our contribution guidelines and have delayed PR reviews for this reason. We now require a CLA. More details here

@crabique
Copy link
Author

CLA signed, thanks for the heads-up!

@drakkan
Copy link
Owner

drakkan commented Apr 13, 2024

CLA signed, thanks for the heads-up!

Thanks for signing. I see you have used an email address like users.noreply.github.com. I think this is fine because we just need to make sure that you are entitled to provide a contribution, anyway I asked our lawyers to be sure (this is something I need to know in any case if it happens again in the future). They may be a little slow to respond, please be patient.

Meanwhile I'll start taking a look at your PR. It looks good at first glance. Thank you!

@drakkan
Copy link
Owner

drakkan commented Apr 13, 2024

If you configure SMTP settings from the WebAdmin and try to reset a password SFTPGo will crash after this patch because templates are never loaded in this case.

SMTP can be enabled from WebAdmin, so even if you start without configuration, email templates can be requested at runtime.
To add this functionality we would have to track the loading of the templates and load them when necessary, this would complicate the code a bit and so it will increase the maintenance burden.

Please explain your use case. If you want to execute SFTPGo as a single binary you build it embedding the templates and static resources.

@crabique
Copy link
Author

Hello! Sorry for the delay, didn't have much time for personal stuff.

Thanks for checking this, I did not know it was possible to enable the feature at runtime from WebAdmin as I never actually used WebAdmin.

My use case was that I just extracted a built binary from a tarball and wanted to spin up a minimal basic SFTP server without any bells and whistles, as I simply don't need anything besides the core SFTP file server. But my minimal config kept failing due to the missing templates, and even having the bundled templates feels wrong if they are not going to be ever used.

A possible way to handle this would be to also check if WebAdmin is enabled, and if it's not, then there will be no way to re-enable SMTP at runtime, so the template loading could be safely skipped.

But I understand it's not the cleanest way and my Go skill / SFTPGo architecture understanding is not good enough to offer a better fix at this time, so I'm completely fine if you close this PR, I tried and I failed 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants