classification
Title: NNTP authentication should check capabilities
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.2, Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Nathan.Clayton, hynek, jelie, pitrou, python-dev
Priority: normal Keywords: patch

Created on 2010-11-01 21:56 by jelie, last changed 2012-02-14 22:34 by pitrou. This issue is now closed.

Files
File name Uploaded Description Edit
nntplib_optcapabilities.patch Nathan.Clayton, 2011-11-06 04:18 Patch to optionally disable capabilities at connection.
nntp-retry-capas-after-login.diff hynek, 2012-02-09 12:56 Patch to retry CAPABILITIES after login
0cf0b06e1d31.diff hynek, 2012-02-12 13:07 Add AUTH tests, don't fail on temporary errors in getcapabilities, get caps after login again review
e986dd8bb47d.diff hynek, 2012-02-12 17:58 Same as above with review comments fixed review
b33bcf179df4.diff hynek, 2012-02-12 21:02 Send MODE READER only if we're not in READER mode already. Add some tests. review
Repositories containing patches
https://bitbucket.org/hynek/cpython-nntp-capas#nntp-capas
Messages (20)
msg120183 - (view) Author: Julien ÉLIE (jelie) Date: 2010-11-01 21:56
RFC 4643:

   The server MAY list the AUTHINFO capability with no arguments, which
   indicates that it complies with this specification and does not
   permit any authentication commands in its current state.  In this
   case, the client MUST NOT attempt to utilize any AUTHINFO commands,
   even if it contains logic that might otherwise cause it to do so
   (e.g., for backward compatibility with servers that are not compliant
   with this specification).

Yet, nntplib attempts to authenticate.


self.capabilities() should be sent at startup.

If "READER" is advertised, no need to send a "MODE READER" command at all...

If "MODE-READER" is advertised, then "MODE READER" (if wanted) can be sent.
Then, self.capabilities() should be sent again.  Capabilities changed!

Then authentication if "AUTHINFO USER" is advertised with NNTP version >=2.  If "AUTHINFO" without "USER", no authentication at all.

And after authentication, self.capabilities() should be sent again.



Please note that the readermode_afterauth variable I see in the source code should normally not be used by a client...  RFC 4643 mentions:

   o  the MODE READER command MUST NOT be used in the
      same session following successful authentication.
msg147141 - (view) Author: Nathan Clayton (Nathan.Clayton) Date: 2011-11-06 04:18
By always sending capabilities at connection, some servers immediately throw an error. I've modified the class initialization to include an optional parameter to indicate if this should be disabled.
msg147156 - (view) Author: Julien ÉLIE (jelie) Date: 2011-11-06 13:03
A drawback is that CAPABILITIES will still being unsent after an upgrade of the news server to a more up-to-date version supporting the command.

If CAPABILITIES is not understood by the news server, it is not an issue.  A 500 response code is usually answered by the news server, which is RFC-compliant.  nntplib should then behave properly.  (getcapabilities() returns an empty dictionary when CAPABILITIES is not implemented.)
msg147157 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-11-06 13:33
> By always sending capabilities at connection, some servers immediately
> throw an error. I've modified the class initialization to include an
> optional parameter to indicate if this should be disabled.

Which kind of error? As Julien says, we could simply catch it and
proceed.
By the way, when sending a patch, please send a unified diff if possible
("diff -u"; if you are using Mercurial, this should be the default).
msg147184 - (view) Author: Nathan Clayton (Nathan.Clayton) Date: 2011-11-06 20:56
When using Easynews, it sends a 480 error (e.g. "nntplib.NNTPTemporaryError: 480 You must log in.")
msg152944 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-02-09 12:56
I think I've fixed it to do as described. Alas, I have no Easynews account to test it (I mailed their support about that so maybe that'll change).

Would someone with an account mind to test, if it works? Nathan?
msg153147 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-02-12 03:02
Hynek, the patch doesn't apply properly here. Are you soon it's been generated against an up-to-date working copy?

Also, I think the logic is wrong: the capabilities should *always* be queried after auth, even if they are already known. That's because they can change after auth is successful.

See RFC 4643: “Other capabilities returned in response to a CAPABILITIES
   command received after authentication MAY be different from those
   returned before authentication.  For example, an NNTP server may not
   want to advertise support for a specific extension unless a client
   has been authenticated.”
msg153148 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-02-12 03:02
(oh, and better if it can come with a test)
msg153195 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-02-12 10:58
Sure, I wanted to add tests as soon as I know that the flow is correct (which it isn't :)).

So essentially we want always

CAPABILITIES
LOGIN
CAPABILITIES

?

That's rather simple to achieve. The tests are going to be the harder part. ;)

Re the patch: I tried to generate it using SourceTree but probably did something wrong – will use hg next time again.
msg153205 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-02-12 13:06
I've added general tests of AUTH USER/PASS as there weren't any present yet.

As I haven't alas heard back from Easynews, I can only presume they allow CAPABILITIES after a login – I've added a test case that models this behavior.

Otherwise I did as it was asked: getcapabilities() is tolerant to temporary errors and we get them again after a login now.

(I'm also using the remote repo tool now to avoid further patch fuckups – sorry for the extra mails.)
msg153209 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-02-12 17:26
I've made a couple of small comments (see "review" link next to the patch).
msg153210 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-02-12 18:00
Thanks! I think I've addressed everything in the latest patch.
msg153211 - (view) Author: Roundup Robot (python-dev) Date: 2012-02-12 18:18
New changeset d789f453387d by Antoine Pitrou in branch '3.2':
Issue #10287: nntplib now queries the server's CAPABILITIES again after authenticating (since the result may change, according to RFC 4643).
http://hg.python.org/cpython/rev/d789f453387d

New changeset 98ad67529241 by Antoine Pitrou in branch 'default':
Issue #10287: nntplib now queries the server's CAPABILITIES again after authenticating (since the result may change, according to RFC 4643).
http://hg.python.org/cpython/rev/98ad67529241
msg153212 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-02-12 18:18
Thank you :) Committed now.
msg153213 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-02-12 18:19
Perhaps I closed too quickly... Julien's original message had two other requests:

“If "READER" is advertised, no need to send a "MODE READER" command at all...

If "MODE-READER" is advertised, then "MODE READER" (if wanted) can be sent.
Then, self.capabilities() should be sent again.  Capabilities changed!”
msg153217 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-02-12 18:37
There's one more issue Julien has raised: nntplib attempts to authenticate when "AUTHINFO" is sent w/o USER. (haven't tested it but I presume it's still valid)

It's trivial to catch that but I'd say that it's fine to let the server handle it if the user has specifically told us to authenticate by giving credentials – no? Maybe if dealing with not-entirely-rfc-compliant servers?

I will look into the READER/MODE-READER part.
msg153229 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-02-12 21:03
So my take on the whole issue and Antoine "tends to agree". ;)

1. If the user tells us (s)he _wants_ us to authenticate or send MODE READER, we do it even if capabilities don't advertise it. There's a lot of rfc-non-conformant servers out there. Permanent errors (i.e. not supported) will be gracefully swallowed.

That's already status quo.

2. If the server tells us it already _is_ in READER mode, we don't send it because in that case we can assume the server knows what it's doing.

The next patch checks whether we're in READER mode before trying to switch the mode.

I've also added a test for the basic case where a connection is made to a MODE-READER advertising server.

I also added a handler to the nntp2 tests that raises an exception if "mode reader" is called although the server advertises itself as "reader".

If there's a consensus of not sending "MODE READER" if not advertised, it's a matter of two lines to fix that. Personally, I like this middle ground best.
msg153370 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-02-14 21:04
I think the patch is fine. Will apply soon (thanks!)
.
msg153373 - (view) Author: Roundup Robot (python-dev) Date: 2012-02-14 22:33
New changeset a7f1ffd741d6 by Antoine Pitrou in branch '3.2':
Issue #10287: nntplib now queries the server's CAPABILITIES first before sending MODE READER, and only sends it if not already in READER mode.
http://hg.python.org/cpython/rev/a7f1ffd741d6

New changeset 03019bb62d83 by Antoine Pitrou in branch 'default':
Issue #10287: nntplib now queries the server's CAPABILITIES first before sending MODE READER, and only sends it if not already in READER mode.
http://hg.python.org/cpython/rev/03019bb62d83
msg153374 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-02-14 22:34
This issue can now be closed for good.
History
Date User Action Args
2012-02-14 22:34:12pitrousetstatus: open -> closed

messages: + msg153374
2012-02-14 22:33:49python-devsetmessages: + msg153373
2012-02-14 21:04:16pitrousetstatus: closed -> open

messages: + msg153370
2012-02-12 21:03:28hyneksetmessages: + msg153229
2012-02-12 21:02:56hyneksetfiles: + b33bcf179df4.diff
2012-02-12 18:37:33hyneksetmessages: + msg153217
2012-02-12 18:19:42pitrousetmessages: + msg153213
2012-02-12 18:18:48pitrousetstatus: open -> closed
resolution: fixed
messages: + msg153212

stage: resolved
2012-02-12 18:18:13python-devsetnosy: + python-dev
messages: + msg153211
2012-02-12 18:00:07hyneksetmessages: + msg153210
2012-02-12 17:58:55hyneksetfiles: + e986dd8bb47d.diff
2012-02-12 17:26:30pitrousetmessages: + msg153209
2012-02-12 13:07:34hyneksetfiles: + 0cf0b06e1d31.diff
2012-02-12 13:06:51hyneksethgrepos: + hgrepo113
messages: + msg153205
2012-02-12 10:58:32hyneksetmessages: + msg153195
2012-02-12 03:02:44pitrousettype: behavior
messages: + msg153148
versions: + Python 3.3
2012-02-12 03:02:16pitrousetmessages: + msg153147
2012-02-09 12:56:26hyneksetfiles: + nntp-retry-capas-after-login.diff

messages: + msg152944
2012-02-07 21:19:59hyneksetnosy: + hynek
2011-11-06 20:56:59Nathan.Claytonsetmessages: + msg147184
2011-11-06 13:33:35pitrousetmessages: + msg147157
2011-11-06 13:03:02jeliesetmessages: + msg147156
2011-11-06 04:18:55Nathan.Claytonsetfiles: + nntplib_optcapabilities.patch

nosy: + Nathan.Clayton
messages: + msg147141

keywords: + patch
2010-11-01 23:06:28pitrousetnosy: + pitrou
2010-11-01 21:56:31jeliecreate