Feature/107 no auth connect#126
Conversation
mbarnach
left a comment
mbarnach
left a comment
There was a problem hiding this comment.
Thanks for that!
I think a new init could make it more obvious of what's happening.
| } | ||
| let authMethod = try getAuthMethod(authMethods: authMethods, serverOptions: serverOptions, hostname: hostname) | ||
| try login(authMethod: authMethod, email: email, password: password) | ||
| if authMethod != .none { |
There was a problem hiding this comment.
I would advocate for a dedicated init, since in that case, the password is useless and the email also. We do not have to specify the authMethods neither if I get it right in such case (must be none).
The functions getAuthMethod and login are useless in the init in the none case.
Maybe a minor refactoring of the connection part (socket and server options) into a dedicated (private) function could keep it cleared and avoid duplication.
There was a problem hiding this comment.
I see your point, and it makes sense.
There was a problem hiding this comment.
Here's a difficulty: consumers of this library won't (normally) be calling SMTPSocket(...) directly, they'll be instantiating an SMTP struct. That struct is what constructs the socket -- so, yes, having two different init methods on the socket definitely makes clear what's going on, but it means that upstream, in the SMTP struct, we'll need to gather that choice. Now, since the struct needs to support both ways of working, what do we do?
One approach would be to add a boolean such as useAuthentication and have it default to true. This would allow existing users of the library to keep their code unmodified and working correctly.
Another would be to add a second initializer to the struct, to match the two-init pattern in the socket, and then in the struct's send method we check to see which socket initializer to use.
Have you got a preference?
|
|
||
| let hostname = "mail.kitura.dev" | ||
| let myEmail: String? = nil | ||
| let myEmail: String? = "tester@local" |
| } | ||
| let authMethod = try getAuthMethod(authMethods: authMethods, serverOptions: serverOptions, hostname: hostname) | ||
| try login(authMethod: authMethod, email: email, password: password) | ||
| if authMethod != .none { |
There was a problem hiding this comment.
I see your point, and it makes sense.
mbarnach
left a comment
mbarnach
left a comment
There was a problem hiding this comment.
The API looks much nicer in my opinion like that! 🎉
👍
|
Thanks for the submission @sbeitzel and for your review @mbarnach. Cursory glance looks okay to me, I'll aim to look more closely this weekend if that's okay for you. Please don't worry if I close/open the PR, which I'll probably have to do to ensure travis is integrated correctly. It might take a little extra time since I just realized something happened to the server I was using as a docker registry and will have to rebuild it. |
|
@sbeitzel I had to make some changes to CI, would you mind rebasing? |
sbeitzel
force-pushed
the
feature/107_no-auth_connect
branch
from
1d8fa9b to
b764fab
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
|
In some cases a server can support authenticated and unauthenticated connections. Does this change support unauthenticated connections to servers which support both?
|
|
Without a test to prove it, I can only say, "Sure, should do." Note that the calling code still needs to set up the |
| } | ||
| } | ||
| throw SMTPError.noAuthMethodsOrRequiresTLS(hostname: hostname) | ||
| if requiresAuth { |
There was a problem hiding this comment.
I gave it a test on a server that supports auth, but does not require it. This check prevents sending.
It looks like you'll need to call the correct SMTPSocket.init from within SMTP.send for those using the new SMTP.init.
Not a contribution
Co-authored-by: Justin Oroz <Juice805@users.noreply.github.com>
| let socket = try SMTPSocket( | ||
| hostname: hostname, | ||
| email: email, | ||
| password: password, | ||
| port: port, | ||
| tlsMode: tlsMode, | ||
| tlsConfiguration: tlsConfiguration, | ||
| authMethods: authMethods, | ||
| domainName: domainName, | ||
| timeout: timeout |
There was a problem hiding this comment.
@sbeitzel sorry, it looks like something is going wrong with my GitHub suggestions, possibly because this code wasn't originally changed in the PR. The lines I selected for the suggestion wasn't replaced and my addition was just appended.
The old socket declaration needs to be removed.
4 participants
Fix issue #107 by only trying to authenticate if the server advertises the AUTH extended capability.
Description
During socket initialization, we will check to see if the server even supports AUTH before we attempt to perform a login.
Motivation and Context
Not every SMTP server requires authentication. If you try to use this library to connect to one such, you will have no luck. A trivial real-world use case for this would be a docker container that's running an open SMTP relay but which is only accessible to an application server running in a different container. It seems strange for an SMTP client library to enforce a server's authentication policy, and to limit itself to connecting only to servers which support an optional command.
Fixes issue #107
How Has This Been Tested?
Running an open, "dummy" SMTP server locally, I tried to connect an SMTPSocket to it. Initially, since the server did not advertise an AUTH capability, this threw an exception. After the change, the connection succeeded.
Checklist: