GeezerVison is showing github.com — Rework Send-MailMessage cmdlet with MailKit by ThreeFive-O · Pull Request #10246 · PowerShell/PowerShell
Conversation
ThreeFive-O
commented
Jul 29, 2019
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
PR Summary
This PR will fix the underlying architecture of the Send-MailMessage cmdlet using the actively maintained MailKit SMTP client.
Note: System.Net.Mail.SmtpClient receives no further development and only supports STARTTLS secure connections.
Enhancements
Benefits of MailKit
Backward compatibility and breaking changes
All parameters are left as they are, BUT
-
Credential parameter is the only way to provide credentials right now. System.Net.Mail.SmtpClient has the ability to get the default credentials from Windows.
-> Requires change in documentation; X-plat credential provider solution for PowerShell 7 might be the solution to that.
-
DeliverNotificationOption is implemented, but the functionality should be reviewed as all tested platforms either don't support Delivery Status Notfication (DSN) (e.g. Gmail, GMX, Yahoo) or ignore the setting (Outlook.com - btw the only SMTP server I've came accross which reports DSN capability).
-> Subject to discuss in RFC to deprecate this parameter.
-
Encoding parameter no longer has any influence on subject/body encoding as this is handled by MimeKit
-> Requires further research, but may be subject to deprecate.
-
UseSsl is no longer mandatory to allow STARTTLS upgrade from an insecure connection. The parameter is now used to explicitly state the the entire connection should be wrapped in a SSL/TLS connections, also known as SMTPS. -> Using Port 0 (default - automatic port selection will use Port 465 for UseSsl but can of course be overridden with any other port number; If UseSsl is not specified the default port 25 for SMTP is used, although most STARTTLS endpoints use port 587).
-> Critical breaking change for automated scripts; Subject to discuss
Possible workaround: Use another parameter for SMTPS functionality and keep UseSsl parameter for STARTTLS (though it wouldn't have any relevance in the code and would be a non-functional parameter like Encoding, though not breaking any scripts)
Test infrastructure
The Pester tests use netDumbster which only supports insecure connections.
STARTTLS and SMTPS functionality have been tested interactively against:
- Outlook.com (STARTTLS 587)
- Gmail.com (SMTPS 465)
- Yahoo.com (SMTPS 465 and STARTTLS 587)
PR Context
As of #9178 is marked as obsolete and an RFC PR PowerShell/PowerShell-RFC#160 was opened to discuess the future of the cmdlet.
PR Checklist
- PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
- Summarized changes
- Make sure all
.h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
- This PR is ready to merge and is not Work in Progress.
- If the PR is work in progress, please add the prefix
WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
- Breaking changes
- User-facing changes
- Testing - New and feature
- Tooling
- I have considered the user experience from a tooling perspective and don't believe tooling will be impacted.
- OR
- I have considered the user experience from a tooling perspective and enumerated concerns in the summary. This may include:
- Impact on PowerShell Editor Services which is used in the PowerShell extension for VSCode (which runs in a different PS Host).
- Impact on Completions (both in the console and in editors) - one of PowerShell's most powerful features.
- Impact on PSScriptAnalyzer (which provides linting & formatting in the editor extensions).
- Impact on EditorSyntax (which provides syntax highlighting with in VSCode, GitHub, and many other editors).
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Travis Plunk (@TravisEz13) Tests for Remoting are failing for Windows ElevatedPesterTests due to missing dependencies:
New-PSSession : [localhost] Processing data from remote server localhost failed with the following error message: Could not load file or assembly 'MailKit, Version=2.2.0.0, Culture=neutral, PublicKeyToken=4e064fe7c44a8f1b'. The system cannot find the file specified. For more information, see the about_Remote_Troubleshooting Help topic.
Could you please point me in the right direction how to resolve this dependency issue? Thank you very much.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
ThreeFive-O reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
@PowerShell/powershell-committee reviewed this, we appreciate this work, but ask that this be a module published to PSGallery. A decision whether this ships in PowerShell will be decided later.
- We have concerns about potential for breaking changes (types, functionality)
- Size concerns bringing in another assembly
- In general, our direction is to have more modules/cmdlets validated via PSGallery before deciding it should be shipped with PowerShell
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 30 days. It will be closed if no further activity occurs within 10 days of this comment.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
I don't agree that existing functionality should be moved to a separate module. It's too bad SMTPClient was deprecated, but this seems like a punishment to the community to remove a builtin feature and put it elsewhere.
|
lukeb1961, A9G-Data-Droid, and BladeFireLight reacted with thumbs up emoji
VertigoRay reacted with thumbs down emoji
All reactions
-
👍
3 reactions
-
👎
1 reaction
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
This pull request was closed.
Reviewers
Justin Chung (jshigetomi)
Awaiting requested review from jshigetomi
jshigetomi will be requested when the pull request is marked ready for review
jshigetomi is a code owner
Copilot will automatically review once the pull request is marked ready for review
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Labels
CL-General
Indicates that a PR should be marked as a general cmdlet change in the Change Log
Committee-Reviewed
PS-Committee has reviewed this and made a decision
Waiting on Author
The PR was reviewed and requires changes or comments from the author before being accept
Milestone
No milestone
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.
|