Repository: aio-libs/aiosmtpd

Issue:
LMTP server offers HELO, doesn't provide correct LHLO response
As initailly reported here: https://gitlab.com/mailman/mailman/issues/348

Telnet to the LMTP server, and type "HELP".

It responds with 
250 Supported commands: EHLO HELO MAIL RCPT DATA RSET NOOP QUIT VRFY

This isn't correct.  LMTP uses LHLO; the EHLO and HELO should not be offered.

LHLO foo

Responds with
250 foo.example.net

RFC2033 says the implementation MUST support PIPELINING and ENHANCEDSTATUSCODES, and SHOULD support 8BITMIME

Thus, LHLO should be responding with the ESMTP keywords for these extensions.

8BITMIME (RFC 1652)

ENHANCEDSTATUSCODES (RFC 2034)

PIPELINING (RFC 1854)

Since it also responds to HELP HELP should also be listed (RFC 1869)

Thus, the dialog should look like:
````
S: 220 foo.example.net GNU Mailman LMTP ...
C: LHLO foo
S: 250-foo.example.net
S: 250-HELP
S: 250-ENHANCEDSTATUSCODES
S: 250-8BITMIME
S: 250 PIPELINING
````

(The order doesn't matter.)

And, obviously, 'HELP' should respond with the correct list of supported commands - including LHLO.



Discussion:

Patch:
diff --git a/aiosmtpd/docs/NEWS.rst b/aiosmtpd/docs/NEWS.rst
index dc6631a..7ade980 100644
--- a/aiosmtpd/docs/NEWS.rst
+++ b/aiosmtpd/docs/NEWS.rst
@@ -21,6 +21,10 @@
 * Widen the catch of ``ConnectionResetError`` and ``CancelledError`` to also
   catch such errors from handler methods.  (Closes #110)
 * Added a manpage for the ``aiosmtpd`` command line script.  (Closes #116)
+* Added much better support for the ``HELP``.  There's a new decorator called
+  ``@syntax()`` which you can use in derived classes to decorate ``smtp_*()``
+  methods.  These then show up in ``HELP`` responses.  This also fixes
+  ``HELP`` responses for the ``LMTP`` subclass.  (Closes #113)
 
 1.0 (2017-05-15)
 ================
diff --git a/aiosmtpd/docs/smtp.rst b/aiosmtpd/docs/smtp.rst
index f46a65e..de2aebc 100644
--- a/aiosmtpd/docs/smtp.rst
+++ b/aiosmtpd/docs/smtp.rst
@@ -28,8 +28,9 @@ All methods implementing ``SMTP`` commands are prefixed with ``smtp_``; they
 must also be coroutines.  Here's how you could implement this use case::
 
     >>> import asyncio
-    >>> from aiosmtpd.smtp import SMTP as Server
+    >>> from aiosmtpd.smtp import SMTP as Server, syntax
     >>> class MyServer(Server):
+    ...     @syntax('PING [ignored]')
     ...     async def smtp_PING(self, arg):
     ...         await self.push('259 Pong')
 
@@ -61,6 +62,17 @@ command, we have to use the lower level interface to talk to it.
     >>> message
     b'Pong'
 
+Because we prefixed the ``smtp_PING()`` method with the ``@syntax()``
+decorator, the command shows up in the ``HELP`` output.
+
+    >>> print(client.help().decode('utf-8'))
+    Supported commands: DATA EHLO HELO HELP MAIL NOOP PING QUIT RCPT RSET VRFY
+
+And we can get more detailed help on the new command.
+
+    >>> print(client.help('PING').decode('utf-8'))
+    Syntax: PING [ignored]
+
 
 Server hooks
 ============
diff --git a/aiosmtpd/lmtp.py b/aiosmtpd/lmtp.py
index 65b5fd3..53ed087 100644
--- a/aiosmtpd/lmtp.py
+++ b/aiosmtpd/lmtp.py
@@ -1,12 +1,14 @@
-from aiosmtpd.smtp import SMTP
+from aiosmtpd.smtp import SMTP, syntax
 from public import public
 
 
 @public
 class LMTP(SMTP):
+    @syntax('LHLO hostname')
     async def smtp_LHLO(self, arg):
         """The LMTP greeting, used instead of HELO/EHLO."""
         await super().smtp_HELO(arg)
+        self.show_smtp_greeting = False
 
     async def smtp_HELO(self, arg):
         """HELO is not a valid LMTP command."""
diff --git a/aiosmtpd/smtp.py b/aiosmtpd/smtp.py
index dfdbcb5..780d11a 100644
--- a/aiosmtpd/smtp.py
+++ b/aiosmtpd/smtp.py
@@ -51,6 +51,15 @@ def make_loop():
     return asyncio.get_event_loop()
 
 
+def syntax(text, extended=None, when=None):
+    def decorator(f):
+        f.__smtp_syntax__ = text
+        f.__smtp_syntax_extended__ = extended
+        f.__smtp_syntax_when__ = when
+        return f
+    return decorator
+
+
 @public
 class SMTP(asyncio.StreamReaderProtocol):
     command_size_limit = 512
@@ -281,6 +290,7 @@ class SMTP(asyncio.StreamReaderProtocol):
                     await self.push(status)
 
     # SMTP and ESMTP commands
+    @syntax('HELO hostname')
     async def smtp_HELO(self, hostname):
         if not hostname:
             await self.push('501 Syntax: HELO hostname')
@@ -293,6 +303,7 @@ class SMTP(asyncio.StreamReaderProtocol):
             status = '250 {}'.format(self.hostname)
         await self.push(status)
 
+    @syntax('EHLO hostname')
     async def smtp_EHLO(self, hostname):
         if not hostname:
             await self.push('501 Syntax: EHLO hostname')
@@ -320,10 +331,12 @@ class SMTP(asyncio.StreamReaderProtocol):
             status = '250 HELP'
         await self.push(status)
 
+    @syntax('NOOP [ignored]')
     async def smtp_NOOP(self, arg):
         status = await self._call_handler_hook('NOOP', arg)
         await self.push('250 OK' if status is MISSING else status)
 
+    @syntax('QUIT')
     async def smtp_QUIT(self, arg):
         if arg:
             await self.push('501 Syntax: QUIT')
@@ -333,6 +346,7 @@ class SMTP(asyncio.StreamReaderProtocol):
             self._handler_coroutine.cancel()
             self.transport.close()
 
+    @syntax('STARTTLS', when='tls_context')
     async def smtp_STARTTLS(self, arg):
         log.info('%r STARTTLS', self.session.peer)
         if arg:
@@ -390,43 +404,39 @@ class SMTP(asyncio.StreamReaderProtocol):
             result[param] = value if eq else True
         return result
 
+    def _syntax_available(self, method):
+        if getattr(method, '__smtp_syntax__', None) is None:
+            return False
+        if method.__smtp_syntax_when__:
+            return bool(getattr(self, method.__smtp_syntax_when__))
+        return True
+
+    @syntax('HELP [command]')
+    @asyncio.coroutine
     async def smtp_HELP(self, arg):
+        code = 250
         if arg:
-            extended = ' [SP <mail-parameters>]'
-            lc_arg = arg.upper()
-            if lc_arg == 'EHLO':
-                await self.push('250 Syntax: EHLO hostname')
-            elif lc_arg == 'HELO':
-                await self.push('250 Syntax: HELO hostname')
-            elif lc_arg == 'MAIL':
-                msg = '250 Syntax: MAIL FROM: <address>'
-                if self.session.extended_smtp:
-                    msg += extended
-                await self.push(msg)
-            elif lc_arg == 'RCPT':
-                msg = '250 Syntax: RCPT TO: <address>'
-                if self.session.extended_smtp:
-                    msg += extended
-                await self.push(msg)
-            elif lc_arg == 'DATA':
-                await self.push('250 Syntax: DATA')
-            elif lc_arg == 'RSET':
-                await self.push('250 Syntax: RSET')
-            elif lc_arg == 'NOOP':
-                await self.push('250 Syntax: NOOP')
-            elif lc_arg == 'QUIT':
-                await self.push('250 Syntax: QUIT')
-            elif lc_arg == 'VRFY':
-                await self.push('250 Syntax: VRFY <address>')
-            else:
-                await self.push(
-                    '501 Supported commands: EHLO HELO MAIL RCPT '
-                    'DATA RSET NOOP QUIT VRFY')
-        else:
-            await self.push(
-                '250 Supported commands: EHLO HELO MAIL RCPT DATA '
-                'RSET NOOP QUIT VRFY')
-
+            method = getattr(self, 'smtp_' + arg.upper(), None)
+            if method and self._syntax_available(method):
+                help_str = method.__smtp_syntax__
+                if (self.session.extended_smtp
+                        and method.__smtp_syntax_extended__):
+                    help_str += method.__smtp_syntax_extended__
+                await self.push('250 Syntax: ' + help_str)
+                return
+            code = 501
+        commands = []
+        for name in dir(self):
+            if not name.startswith('smtp_'):
+                continue
+            method = getattr(self, name)
+            if self._syntax_available(method):
+                commands.append(name.lstrip('smtp_'))
+        commands.sort()
+        await self.push(
+            '{} Supported commands: {}'.format(code, ' '.join(commands)))
+
+    @syntax('VRFY <address>')
     async def smtp_VRFY(self, arg):
         if arg:
             try:
@@ -444,6 +454,7 @@ class SMTP(asyncio.StreamReaderProtocol):
         else:
             await self.push('501 Syntax: VRFY <address>')
 
+    @syntax('MAIL FROM: <address>', extended=' [SP <mail-parameters>]')
     async def smtp_MAIL(self, arg):
         if not self.session.host_name:
             await self.push('503 Error: send HELO first')
@@ -510,6 +521,7 @@ class SMTP(asyncio.StreamReaderProtocol):
         log.info('%r sender: %s', self.session.peer, address)
         await self.push(status)
 
+    @syntax('RCPT TO: <address>', extended=' [SP <mail-parameters>]')
     async def smtp_RCPT(self, arg):
         if not self.session.host_name:
             await self.push('503 Error: send HELO first')
@@ -556,6 +568,7 @@ class SMTP(asyncio.StreamReaderProtocol):
         log.info('%r recip: %s', self.session.peer, address)
         await self.push(status)
 
+    @syntax('RSET')
     async def smtp_RSET(self, arg):
         if arg:
             await self.push('501 Syntax: RSET')
@@ -568,6 +581,7 @@ class SMTP(asyncio.StreamReaderProtocol):
         status = await self._call_handler_hook('RSET')
         await self.push('250 OK' if status is MISSING else status)
 
+    @syntax('DATA')
     async def smtp_DATA(self, arg):
         if not self.session.host_name:
             await self.push('503 Error: send HELO first')


Test patch:
diff --git a/aiosmtpd/tests/test_lmtp.py b/aiosmtpd/tests/test_lmtp.py
index 0af6594..d77816e 100644
--- a/aiosmtpd/tests/test_lmtp.py
+++ b/aiosmtpd/tests/test_lmtp.py
@@ -40,3 +40,13 @@ class TestLMTP(unittest.TestCase):
             code, response = client.ehlo('example.com')
             self.assertEqual(code, 500)
             self.assertEqual(response, b'Error: command "EHLO" not recognized')
+
+    def test_help(self):
+        # https://github.com/aio-libs/aiosmtpd/issues/113
+        with SMTP(*self.address) as client:
+            # Don't get tricked by smtplib processing of the response.
+            code, response = client.docmd('HELP')
+            self.assertEqual(code, 250)
+            self.assertEqual(response,
+                             b'Supported commands: DATA HELP LHLO MAIL '
+                             b'NOOP QUIT RCPT RSET VRFY')
diff --git a/aiosmtpd/tests/test_smtp.py b/aiosmtpd/tests/test_smtp.py
index ab52cf6..8d2a07d 100644
--- a/aiosmtpd/tests/test_smtp.py
+++ b/aiosmtpd/tests/test_smtp.py
@@ -287,8 +287,8 @@ class TestSMTP(unittest.TestCase):
             code, response = client.docmd('HELP')
             self.assertEqual(code, 250)
             self.assertEqual(response,
-                             b'Supported commands: EHLO HELO MAIL RCPT '
-                             b'DATA RSET NOOP QUIT VRFY')
+                             b'Supported commands: DATA EHLO HELO HELP MAIL '
+                             b'NOOP QUIT RCPT RSET VRFY')
 
     def test_help_helo(self):
         with SMTP(*self.address) as client:
@@ -354,7 +354,7 @@ class TestSMTP(unittest.TestCase):
         with SMTP(*self.address) as client:
             code, response = client.docmd('HELP', 'NOOP')
             self.assertEqual(code, 250)
-            self.assertEqual(response, b'Syntax: NOOP')
+            self.assertEqual(response, b'Syntax: NOOP [ignored]')
 
     def test_help_quit(self):
         with SMTP(*self.address) as client:
@@ -374,8 +374,8 @@ class TestSMTP(unittest.TestCase):
             code, response = client.docmd('HELP me!')
             self.assertEqual(code, 501)
             self.assertEqual(response,
-                             b'Supported commands: EHLO HELO MAIL RCPT '
-                             b'DATA RSET NOOP QUIT VRFY')
+                             b'Supported commands: DATA EHLO HELO HELP MAIL '
+                             b'NOOP QUIT RCPT RSET VRFY')
 
     def test_expn(self):
         with SMTP(*self.address) as client:
diff --git a/aiosmtpd/tests/test_starttls.py b/aiosmtpd/tests/test_starttls.py
index e8608c4..f7c1fde 100644
--- a/aiosmtpd/tests/test_starttls.py
+++ b/aiosmtpd/tests/test_starttls.py
@@ -103,6 +103,18 @@ class TestStartTLS(unittest.TestCase):
             code, response = client.docmd('STARTTLS', 'TRUE')
             self.assertEqual(code, 501)
 
+    def test_help_after_starttls(self):
+        controller = TLSController(Sink())
+        controller.start()
+        self.addCleanup(controller.stop)
+        with SMTP(controller.hostname, controller.port) as client:
+            # Don't get tricked by smtplib processing of the response.
+            code, response = client.docmd('HELP')
+            self.assertEqual(code, 250)
+            self.assertEqual(response,
+                             b'Supported commands: DATA EHLO HELO HELP MAIL '
+                             b'NOOP QUIT RCPT RSET STARTTLS VRFY')
+
 
 class TestTLSForgetsSessionData(unittest.TestCase):
     def setUp(self):