Replace PLAIN, CRAM-MD5, and LOGIN implementations
#70
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nevans
force-pushed
the
net-imap-sasl
branch
3 times, most recently
from
b310bd9
to
83f9103
Compare
nevans
force-pushed
the
net-imap-sasl
branch
8 times, most recently
from
f8b744c
to
3ecdaae
Compare
nevans
force-pushed
the
net-imap-sasl
branch
2 times, most recently
from
00b56e2
to
c65246a
Compare
This adds a new `auth` keyword param to `Net::SMTP.start` and `#start` that can be used to pass any arbitrary keyword parameters to `#authenticate`. The pre-existing `username`, `secret`, etc keyword params will retain their existing behavior as positional arguments to `#authenticate`.
Although "user" is a reasonable abbreviation, the parameter is more accurately described as a "username" or an "authentication identity". They are synonomous here, with "username" winning when both are present.
Username can be set by args[0], authcid, username, or user. Secret can be set by args[1], secret, or password. Since all of the existing authenticators have the same API, it is sufficient to update `check_args` in the base class.
This API is a little bit confusing, IMO. But it does preserve backward compatibility, while allowing authenticators that don't allow positional parameters to work without crashing. But, authenticators that require only one parameter—or more than three—will still be inaccessible.
This is convenient for `smtp.start auth: {type:, **etc}`.
Although `#authenticate` can be updated to make username and secret _both_ optional, by placing the mechanism last and making it optional, it's not possible to use an authenticator with a _single_ positional parameter or with more than two positional parameters. By placing `type` first among positional parameters or as a keyword argument, we avoid this problem.
The net-imap dependency requires 2.7.3, to deal with kwargs.
This commit adds the `net-imap` as a default fallback for mechanisms that haven't otherwise been added. In this commit, the original implementation is still used by `#authenticate` for the `PLAIN`, `XOAUTH2`, `LOGIN`, and `CRAM-MD5` mechanisms. Every other mechanism supported by `net-imap` v0.4.0 is added here: * `ANONYMOUS` * `DIGEST-MD5` _(deprecated)_ * `EXTERNAL` * `OAUTHBEARER` * `SCRAM-SHA-1` and `SCRAM-SHA-256` **TODO:** Ideally, `net-smtp` and `net-imap` should both depend on a shared `sasl` or `net-sasl` gem, rather than keep the SASL implementation inside one or the other. See ruby/net-imap#23.
This is mostly to stay consistent with other SASL mechanisms and to remove duplication: We shouldn't have unique SASL mechanism implementations for each individual protocol client. In some cases, the versions from `net-imap` are more full-featured. E.g: they support `authzid` and print deprecation warnings for MD5 authenticators. The tests are updated to silence deprecation warnings, which are printed by the authenticators from `net-imap`.
The XOAUTH2 authenticator that comes from Net::IMAP::SASL does support keyword attributes. But XOAUTH2 is very simple, so this doesn't really add much. This is mostly to stay consistent with other SASL mechanisms and to remove duplication: We shouldn't have unique SASL mechanism implementations for each individual protocol client.
This renames `#do_finish` to `#quit!`, makes it public, adds an option to convert exceptions into warnings, and extracts the contents of the ensure block into a new public `#disconnect` method. The motivation for making these public is given in the rdoc. As documented in the rdoc, `#quit!`: > Calls #quit and ensures that #disconnect is called. Returns the > result from #quit. Returns +nil+ when the client is already > disconnected or when a prior error prevents the client from calling > #quit. Unlike #finish, this an exception will not be raised when the > client has not started. > > When <tt>exception: :warn</tt> is specified, when #quit raises a > StandardError, a warning will be printed and the exception is > returned, not re-raised. When <tt>exception: false</tt> is specified, > a warning will not be printed. This is useful when the connection > must be dropped, for example in a test suite or due to security > concerns. As documented in the rdoc, `#disconnect`: > Disconnects the socket without checking if the connection has started > yet, and without sending a final QUIT message to the server. > > Generally, either #finish or #quit! should be used instead.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Once the SASL implementation in #69 has been merged (whether it is coming from
net-imapor from some new shared gem), the existing authenticators forPLAIN,CRAM-MD5, andLOGINwill be obsolete. This PR removes themThe updated implementation for
PLAINadds support forauthzid.The updated implementations for
CRAM-MD5andLOGINwill both be marked as deprecated and will print warnings when they are used, unlesswarn_deprecation: falseis used.