Close #71: Allow explicit specification of SASL mechanisms

[ehuelsmann]

The current implementation uses Authen::SASL with the default list of mechanisms. With this minimal change, users of Email::Sender can select the list of allowable mechanisms by passing in a fully configured Authen::SASL instance.

This is a variant of code that I have in production myself and released as part of an open source project to an unknown number of other installs. The code can be found at https://github.com/ledgersmb/LedgerSMB/blob/master/old/lib/LedgerSMB/Mailer/TransportSMTP.pm and is used at https://github.com/ledgersmb/LedgerSMB/blob/970e0e8713ad85a2a144bf085048937d603bb1c3/old/lib/LedgerSMB/Mailer.pm#L213-L226 (instantiation of the Authen::SASL instance) and https://github.com/ledgersmb/LedgerSMB/blob/970e0e8713ad85a2a144bf085048937d603bb1c3/old/lib/LedgerSMB/Mailer.pm#L248 (instantiation of the transport).

[ehuelsmann]

@rjbs could you please indicate if this change is acceptable as it is and if not, what you think should change, or that i should forget about it? (In that last case, #71 should probably be closed too?)

Thank you for considering this change and Email::Sender in general!

[ehuelsmann]

@rjbs hi! anything i need to change on this PR?

[rjbs]

Sorry to leave you hanging.

Rather than have a username sometimes contain an Authen::SASL object, there should be a new sasl_authenticator attribute. Setting both sasl_username and sasl_authenticator should be an error at construction time.

[ehuelsmann]

No problem. I'll get that implemented.

[ehuelsmann]

@rjbs the force-push above replaces the former PR with a new one which implements a new sasl_authenticator attribute as per your request. Please let me know what changes you'd like to see, if any.

Thanks for your time!

[rjbs]

Thanks, looks good, I'll give it a bit of a spin and probably merge soon.

I will be rewriting just a tiny bit to match usual style, namely:

if ($x)
  die "..."
    unless $y
}

will become

if ($x and not $y) {
  die "..."

Thank you for the patch!

[wesQ3]

Would be useful for us as well!

[ehuelsmann]

I will be rewriting just a tiny bit to match usual style, namely:

I took the liberty to incorporate this into the PR. Hope you find some time to merge this soonish with that last bit resolved.

[dakkar]

ah, good thing I checked the PRs, I was going to write pretty much the same thing ☺

[wesQ3]

@ehuelsmann Perhaps the style change should also apply to this check:

-  if (exists $arg->{sasl_authenticator}) {
+  if (exists $arg->{sasl_authenticator} && exists $arg->{sasl_username}) {
       Carp::croak("can't pass both sasl_authenticator and sasl_username to constructor")
-          if exists $arg->{sasl_username};
  }

[ehuelsmann]

@rjbs, I've incorporated all review comments, including those by @wesQ3. Could you please indicate if there's anything I can do to expedite the PR? If you want further edits, please let me know, I'll submit those so the result for you is that you simply only need to click the "Merge" button.

[rjbs]

Thanks for the work and also your patience. I'm a bit of a foot-dragger in the best of circumstances, and lately things are a bit weirder than usual. Applied!

[rjbs]

Actually, this fails tests:

t/smtp-via-mock.t ..... 1/95 sasl_username but no sasl_password

Trace begun at /Users/rjbs/code/hub/Email-Sender/lib/Email/Sender/Transport/SMTP.pm line 259
Email::Sender::Transport::SMTP::_throw('Email::Sender::Transport::SMTP=HASH(0x126c807c8)', 'sasl_username but no sasl_password') called at /Users/rjbs/code/hub/Email-Sender/lib/Email/Sender/Transport/SMTP.pm line 53
Email::Sender::Transport::SMTP::BUILD('Email::Sender::Transport::SMTP=HASH(0x126c807c8)', 'HASH(0x126c4a370)') called at (eval 24) line 327
Email::Sender::Transport::SMTP::new('Email::Sender::Transport::SMTP', 'HASH(0x126c48778)') called at t/smtp-via-mock.t line 287
# Looks like your test exited with 255 just after 36.
t/smtp-via-mock.t ..... Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 59/95 subtests

My guess: you ran the test suite without first installing Test::MockObject. I'll give it a look.

[rjbs]

Addressed and released!

[wesQ3]

Thanks @rjbs ! 🙏

On Wed, Jan 17, 2024, 18:16 Ricardo Signes @.***> wrote:

Addressed and released!

— Reply to this email directly, view it on GitHub https://github.com/rjbs/Email-Sender/pull/77#issuecomment-1897538739, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADLEDZTXRRTTSTM3ULT373YPBSUFAVCNFSM4ZE3ZY62U5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TCOBZG42TGOBXGM4Q . You are receiving this because you were mentioned.Message ID: @.***>

[ehuelsmann]

Thank you! (Had I known the test failed, would I have resolved it before creating the PR, my apology for not noticing)

[rjbs]

No problem. 😉