I’m using the passbolt_docker installation, and my smtp server requires that the EHLO/HELO message sent by passbolt is a valid fqdn. But currently there’s no easy way for me to set this without managing my own email.php file, when all that should be needed is a environment-variable. As of v1.6.5, the client variable gets defaulted to ‘localhost’ in lib/Cake/Network/Email/SmtpTransport.php, lines 151-154.
To get what i’d like, two changes are needed. First, the email.php.default would need to include a “‘client’ => null,” line, in the $default section of email.php.default, and then the docker_entrypoint.sh needs an additional sed line that replaces the default “null” with an actual value if it exists as an environment variable.
I’ve already implemented this, and can send pull requests, but as I understood the contributing.md, you’d want to have a discussion about it here first.
Is this something that you’d be willing to merge or is there another way to do this, that i’ve completely missed?
Thanks a lot for opening the dicussion here and for your interest in passbolt! What you suggest sounds perfect. Thing is the next release of the docker image will be for the v2 of passbolt which will introduce changes on the configuration files. What I suggest is that you could send the PR so I won’t forget to include your request on the new release.
I just noticed that these changes has been merged, but unfortunately I also noticed that I introduced a problem with the setting in the docker environment.
The problem is that the email.php.default sets username and password to a blank string, or rather two empty single-tics. And I failed to recognise that this is later replaced with “null” (without quotes), in docker if you don’t have EMAIL_AUTH set, this renders the sed-line for default_client match the wrong line, eg the ‘username’ => null, line will be matched instead of the intended ‘client’ => null, line.
I didn’t pick that up when i was testing it, and I’m a bit uncertain on how you’d wish for me to fix it. One ugly/simple way is just to move the entire “client”-line to be above the user-password stansa. But that feels like a cheap-fix where we just rely on timing to allow the correct setting to be modified. A similar issue already exists in the event if some user runs their Smtp server on port 30, and wish to configure a timeout to say 60. That would overwrite the port-configuration from 30 to 60, instead.
I see a few immediate ways of solving it.
Just simply move the client-line above username/password and be done with it.
Delete the username/password rows all together if they’re not used, instead of initializing them to ‘null’.
Delete and rewrite the entire $default declaration in class EmailConfig from docker-entrypoint.sh using some template-style-syntax.
Search / replace based on matching the whole row, instead of just the values from docker-entrypoint.sh instead.
1 and 2 won’t fix the Port vs Timeout cornercase though, and 2 might introduce other errors that I don’t know about since other code might rely on the key being initialized but unused maybe.
Thanks for your report! It has been a speedy run to release this last 1.6.9 version with a lot of PR merging so chances are more of this corner cases will appear.
Search and replacing is a clunky method that will go away with the 2.x series of the container. I have been checking quickly your report along with the docker-entrypoint. I think we could search for the first appearance of the ‘client’ string and perform the substitution on that line instead of search for the null pattern. I’m doing some tests right now I will let you now when the fix is ready so you can test it also.
134 if [ -n "$EMAIL_CLIENT" ] ; then
135 sed -i "0,/"'client'"/s:"$default_client":'$EMAIL_CLIENT':" $email_config
136 fi
It is not elegant at all but none of the seds are
We could make it more consistent with the previous search and replacements like setting ‘client’ as a local variable but the essential functionality is this one.
I have tested it without setting the EMAIL_CLIENT env var and setting it to a random value. Seems like it works, let me know if you test it and find some other problem.
That sed-line still suffers from the problem I described.
When EMAIL_AUTH is set to false, the username and password is set to null by docker-entrypoint.
And your sed will actually also replace those null to $EMAIL_CLIENT, since we run the regex on the address space up until the first 'client'occurance, which includes them.
The following command works in that scenario, matching up to 'client', and then replacing the full line. It also does away with all the redundant(?) double-quotes/single-quotes, since I don’t under stand why they’re there so frequently in all the other sed lines (variables expand just fine inside double-quotes, but not in single-quotes, or am I missing something?).
sed -i "0,/'client'/s:'client' => $default_client:'client' => '$EMAIL_CLIENT':" $email_config
