Opened 8 years ago

Closed 7 years ago

#26487 closed Cleanup/optimization (needsinfo)

Use EHLO after smtplib.SMTP_SSL too.

Reported by: kchmiela Owned by: nobody
Component: Core (Mail) Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

In file django/core/mail/backends/smtp.py after smtplib.SMTP_SSL EHLO should be used.

Attachments (1)

patch.diff (718 bytes ) - added by kchmiela 8 years ago.
Patch

Download all attachments as: .zip

Change History (9)

by kchmiela, 8 years ago

Attachment: patch.diff added

Patch

comment:1 by Tim Graham, 8 years ago

I haven't looked at the history behind this yet, but in reading the Python docs:

Unless you wish to use has_extn() before sending mail, it should not be necessary to call this method explicitly. It will be implicitly called by sendmail() when necessary.

I don't see a has_extn() call in Django, so maybe the call can be removed?

comment:2 by Emett Speer, 8 years ago

Well you are correct in all currently supported versions of Python it does state that its not needed unless using has_extn().

comment:3 by Tim Graham, 8 years ago

Needs tests: set
Triage Stage: UnreviewedAccepted

The ticket is missing the rationale of what problem is being solved. Is it to detect communication failures in open() rather than wait until sending a message? Maybe ehlo_or_helo_if_needed() should be used instead?

  • django/core/mail/backends/smtp.py

    diff --git a/django/core/mail/backends/smtp.py b/django/core/mail/backends/smtp.py
    index 432f3a6..40778eb 100644
    a b class EmailBackend(BaseEmailBackend):  
    5656            })
    5757        try:
    5858            self.connection = connection_class(self.host, self.port, **connection_params)
     59            self.connection.ehlo_or_helo_if_needed()
    5960
    6061            # TLS/SSL are mutually exclusive, so only attempt TLS over
    6162            # non-secure connections.
    6263            if not self.use_ssl and self.use_tls:
    63                 self.connection.ehlo()
    6464                self.connection.starttls(keyfile=self.ssl_keyfile, certfile=self.ssl_certfile)
    65                 self.connection.ehlo()
    6665            if self.username and self.password:
    6766                self.connection.login(self.username, self.password)
    6867            return True

Anyway, tests are still needed.

comment:4 by Claude Paroz, 8 years ago

In my opinion, both ehlo() calls could be deleted, and nothing should be added. starttls, login and other connection methods are caring for that themselves. However, they should not be harmful either.

If there is a real problem with EmailBackend not calling ehlo(), please provide more details.

comment:5 by Tim Graham, 8 years ago

The docs for starttls() say, "You should then call ehlo() again." This might be an obsolete requirement as the sendmail() and login() method's in Python's Lib/smtplib.py call self.ehlo_or_helo_if_needed(). I'm not familiar with the SMTP protocol though so I'm not positive what this all means.

comment:6 by Tim Graham, 7 years ago

Needs tests: unset

I'll remove the ehlo() calls and close this ticket as "needsinfo" unless anyone can explain why the change proposed in the ticket's description is needed.

comment:7 by GitHub <noreply@…>, 7 years ago

In bcae045d:

Refs #26487 -- Removed unneeded ehlo() calls in SMTP backend.

starttls(), login(), and other connection methods already call
the method as needed.

comment:8 by Tim Graham, 7 years ago

Resolution: needsinfo
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top