| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| Expand Up | @@ -713,28 +713,12 @@ class FTP_TLS(FTP): | |
| '221 Goodbye.' | ||
| >>> | ||
| ''' | ||
| ssl_version = ssl.PROTOCOL_TLS_CLIENT | ||
|
|
||
| def __init__(self, host='', user='', passwd='', acct='', | ||
| keyfile=None, certfile=None, context=None, | ||
|
Comment thread
Outdated
Copy link
Copy Markdown
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityWhen you remove positional parameters, you have to make the following parameters keyword-only.
Sorry, something went wrong.
All reactions
Copy link
Copy Markdown
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityOh right, done. I didn't document this change. Is it worth to document it?
Sorry, something went wrong.
All reactions
Copy link
Copy Markdown
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityWere similar changes documented in the past? Please add tests that only such positional arguments are accepted. In most cases None is a valid argument both for the first deleted parameter and for the first remaining parameter, so you can just add None as an excessive positional argument.
Sorry, something went wrong.
All reactions
Copy link
Copy Markdown
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
In commit 9baf242, I removed a buffering parameter in bz2.BZ2File. I see that I wrote ".. versionchanged:: 3.9 (...) The compresslevel parameter became keyword-only." in the doc. Honestly, I don't think that it should be documented. I don't expect people to call such constructor with 6 parameters or more. I'm only aware of people who called CodeType() with tons of arguments: problem solved with CodeType.replace() method addition.
Sorry, something went wrong.
All reactions
|
||
| timeout=_GLOBAL_DEFAULT_TIMEOUT, source_address=None, *, | ||
| encoding='utf-8'): | ||
| if context is not None and keyfile is not None: | ||
| raise ValueError("context and keyfile arguments are mutually " | ||
| "exclusive") | ||
| if context is not None and certfile is not None: | ||
| raise ValueError("context and certfile arguments are mutually " | ||
| "exclusive") | ||
| if keyfile is not None or certfile is not None: | ||
| import warnings | ||
| warnings.warn("keyfile and certfile are deprecated, use a " | ||
| "custom context instead", DeprecationWarning, 2) | ||
| self.keyfile = keyfile | ||
| self.certfile = certfile | ||
| *, context=None, timeout=_GLOBAL_DEFAULT_TIMEOUT, | ||
| source_address=None, encoding='utf-8'): | ||
| if context is None: | ||
| context = ssl._create_stdlib_context(self.ssl_version, | ||
| certfile=certfile, | ||
| keyfile=keyfile) | ||
| context = ssl._create_stdlib_context() | ||
| self.context = context | ||
| self._prot_p = False | ||
| super().__init__(host, user, passwd, acct, | ||
| Expand All | @@ -749,7 +733,7 @@ def auth(self): | |
| '''Set up secure control connection by using TLS/SSL.''' | ||
| if isinstance(self.sock, ssl.SSLSocket): | ||
| raise ValueError("Already using TLS") | ||
| if self.ssl_version >= ssl.PROTOCOL_TLS: | ||
|
Comment thread
Outdated
Copy link
Copy Markdown
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityIf it is the right way to do, should this change be backported as a bug fix? I think it deserves a separate issue.
Sorry, something went wrong.
All reactions
Copy link
Copy Markdown
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityThat's a good question. Sadly, I don't know well the ftplib module. I'm not comfortable to change it in a stable Python version. In case of doubt, I prefer to leave the code as it is in stable branches, unless someone reports a bug.
Sorry, something went wrong.
All reactions
Comment thread
Outdated
Copy link
Copy Markdown
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualitySSLv2 and SSLv3 are no longer supported. You can remove the AUTH SSL block.
Sorry, something went wrong.
All reactions
Copy link
Copy Markdown
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityWhen I read Modules/_ssl.c, it's not obvious that SSL is no longer supported. There is still conditonal code: #ifndef OPENSSL_NO_SSL2
PyModule_AddIntConstant(m, "PROTOCOL_SSLv2",
PY_SSL_VERSION_SSL2);
#endif
#ifndef OPENSSL_NO_SSL3
PyModule_AddIntConstant(m, "PROTOCOL_SSLv3",
PY_SSL_VERSION_SSL3);
#endif
These constants are still documented but marked as "deprecated" in the doc: https://docs.python.org/dev/library/ssl.html#ssl.PROTOCOL_SSLv2 I would prefer to fully remove support for SSL in the _ssl/ssl modules, before changing the ftplib module. Another example: https://docs.python.org/dev/library/ssl.html#ssl.OP_NO_SSLv3 still exists (at least in the doc).
Sorry, something went wrong.
All reactions
Copy link
Copy Markdown
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityIt's documented in ssl.rst. I'm using the word "supported" in the sense of a social contract. The presence of the constants do not mean that a user can expect they are doing anything sensible.
Sorry, something went wrong.
All reactions
|
||
| if self.context.protocol >= ssl.PROTOCOL_TLS: | ||
| resp = self.voidcmd('AUTH TLS') | ||
| else: | ||
| resp = self.voidcmd('AUTH SSL') | ||
| Expand Down | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| Expand Up | @@ -1285,40 +1285,23 @@ class IMAP4_SSL(IMAP4): | |
|
|
||
| """IMAP4 client class over SSL connection | ||
|
|
||
| Instantiate with: IMAP4_SSL([host[, port[, keyfile[, certfile[, ssl_context[, timeout=None]]]]]]) | ||
| Instantiate with: IMAP4_SSL([host[, port[, ssl_context[, timeout=None]]]]) | ||
|
Comment thread
Copy link
Copy Markdown
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityThis line is completely redundant, because the output of pydoc already contains the constructor signature. The same in other classes in this module, and maybe in other modules. I would prefer to remove this line and edit the following parameters descriptions, and do it before merging this PR to make backporting easier.
Sorry, something went wrong.
All reactions
Copy link
Copy Markdown
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityThis PR is already big enough. If you want to enhance the documentation, please change it in a separated change. I don't see how updating this doc is a pre-requirement to remove the two parameters.
Sorry, something went wrong.
All reactions
|
||
|
|
||
| host - host's name (default: localhost); | ||
| port - port number (default: standard IMAP4 SSL port); | ||
| keyfile - PEM formatted file that contains your private key (default: None); | ||
| certfile - PEM formatted certificate chain file (default: None); | ||
| ssl_context - a SSLContext object that contains your certificate chain | ||
| and private key (default: None) | ||
| Note: if ssl_context is provided, then parameters keyfile or | ||
| certfile should not be set otherwise ValueError is raised. | ||
| timeout - socket timeout (default: None) If timeout is not given or is None, | ||
| the global default socket timeout is used | ||
|
|
||
| for more documentation see the docstring of the parent class IMAP4. | ||
| """ | ||
|
|
||
|
|
||
| def __init__(self, host='', port=IMAP4_SSL_PORT, keyfile=None, | ||
| certfile=None, ssl_context=None, timeout=None): | ||
| if ssl_context is not None and keyfile is not None: | ||
| raise ValueError("ssl_context and keyfile arguments are mutually " | ||
| "exclusive") | ||
| if ssl_context is not None and certfile is not None: | ||
| raise ValueError("ssl_context and certfile arguments are mutually " | ||
| "exclusive") | ||
| if keyfile is not None or certfile is not None: | ||
| import warnings | ||
| warnings.warn("keyfile and certfile are deprecated, use a " | ||
| "custom ssl_context instead", DeprecationWarning, 2) | ||
| self.keyfile = keyfile | ||
| self.certfile = certfile | ||
| def __init__(self, host='', port=IMAP4_SSL_PORT, | ||
| *, ssl_context=None, timeout=None): | ||
| if ssl_context is None: | ||
| ssl_context = ssl._create_stdlib_context(certfile=certfile, | ||
| keyfile=keyfile) | ||
| ssl_context = ssl._create_stdlib_context() | ||
| self.ssl_context = ssl_context | ||
| IMAP4.__init__(self, host, port, timeout) | ||
|
|
||
| Expand Down | ||
| Back | FazBrowse Home | New Git URL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityWhy is it removed?
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityIf you provide a SSLContext, you get different protocol than FTP_TLS.ssl_version. This removal is not required, it's more to make the API consistent with the other modified modules: the SSLContext constructor is the only place setting default parameter values. Do you prefer to keep it?
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityI do not know whether it is documented, but it specifies the default protocol if you do not provide an SSLContext. The user can override it in FTP_TLS or subclasses.
Who added this attribute? Ask him.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityAll protocols except PROTOCOL_TLS, TLS_CLIENT, and TLS_SERVER are deprecated anyways because OpenSSL is going to remove them eventually.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality@tiran: Do you see a reason to keep ssl_version to specify a different "ssl version" when context parameter is omitted?
The practical problem is more that applications relying on this feature may not notice that overriden ssl_version are now ignored silently.
Note: I hate mentions of "ssl", since SSL no longer exists, it was replaced with TLS, but the Python module is still called "ssl"... as OpenSSL ;-)
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityThere is no reason to keep the variable here. ssl.PROTOCOL_TLS_CLIENT is the only correct and fully supported value. In 3.13 I will remove all the other protocols. Only ssl.PROTOCOL_TLS_CLIENT and ssl.PROTOCOL_TLS_SERVER will stay.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.