Skip to content
New issue

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

Support keyword parameters for authenticators #71

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

nevans
Copy link
Contributor

@nevans nevans commented Oct 14, 2023

Several SASL mechanisms have zero or one required parameters, so it doesn't make sense to require positional "user" and "secret" parameters. And in some cases, the semantics of the parameters don't align cleanly with the semantics implied by "user" and "secret".

And many SASL mechanisms will need to be able to take extra arguments. For example: authzid for many mechanisms, warn_deprecations for deprecated mechanisms, min_iterations for SCRAM-*, anonymous_message for ANONYMOUS, and so on. Also, although it is convenient to use username as an (ambiguous) alias for authcid or authzid, and secret as an (ambiguous) alias for password or oauth2_token, it is also useful to have keyword parameters that keep stable semantics across many different mechanisms.

So, the API needs to be updated so that 1) positional parameters are not required, 2) keyword parameters are enabled. For semantic clarity, positional parameters should be seen as a convenience, and keyword parameters should be considered the basic form.

This PR does several things (each split into their own commit):

  • #authenticate changes:
  • Net::SMTP.start and #start changes
    • Add auth parameter: a hash of keyword arguments for #authenticate. For backward compatibility, the existing username, secret, etc are still sent as positional arguments. (Forward authenticate keyword arguments #74)
  • Adds keyword parameters to all existing authenticators. This makes user and secret positional arguments optional, as keyword args can be used instead.

As currently written, this PR depends on the following other PRs:

@nevans nevans force-pushed the sasl/support-keyword-parameters branch 2 times, most recently from 0d8db36 to c7d7243 Compare October 14, 2023 20:30
@nevans nevans force-pushed the sasl/support-keyword-parameters branch 4 times, most recently from 34b1f85 to f7d0851 Compare October 15, 2023 11:24
@nevans nevans force-pushed the sasl/support-keyword-parameters branch from f7d0851 to 4f9e040 Compare October 21, 2023 00:29
@nevans nevans force-pushed the sasl/support-keyword-parameters branch 2 times, most recently from f8e201f to a6280b6 Compare November 7, 2023 23:26
@nevans
Copy link
Contributor Author

nevans commented Nov 7, 2023

@tmtm Thanks for taking the time to review all of my PRs! I've extracted #74 from this PR, because those are the most important and (I hope) least controversial changes. However, the tests for #74 are still in this PR. Let me know if you want anything changed in either PR. Thanks again!

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}`.
@nevans nevans force-pushed the sasl/support-keyword-parameters branch from a6280b6 to a47a7c3 Compare May 5, 2024 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant