Skip to content

Open MailKit secure socket option setting #87

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

Merged
merged 2 commits into from
Jun 4, 2021

Conversation

arthurchiakh
Copy link
Contributor

related to Issue #69

Enable user to set secure socket option for MailKit

@arthurchiakh
Copy link
Contributor Author

Hi, @nblumhardt ,
do you mind checking this PR?
This change is to let user to specify secure socket option setting for MailKit, which can increase the range of supported email servers for this library.
In my case, I can only connect to Office365 email server when the secure socket option set to SecureSocketOptions.StartTls.
Please see also related issue #69 Mailkit client use SecureSocketOptions .

@Gondost
Copy link

Gondost commented Nov 13, 2020

I would like to see this merged as well, I need to set the SecureSocketOptions in order to send email in my environment.

@nblumhardt
Copy link
Member

Thanks for the nudge. I would prefer not to expose MailKit types directly on the email sink's API - is there a higher-level way this can be expressed, akin to how we currently set EnableSSL?

@Gondost
Copy link

Gondost commented Nov 16, 2020

@nblumhardt

EnableSSL is a bit simpler because it's a boolean on both ends, whereas SecureSocketOptions is an enum. At some point, the enum will have to be validated to ascertain whether it's valid. This seems to lead to paths like creating your own enum with the same values as SecureSocketOptions, etc. Or, casting it in some manner and doing an automatic mapping. One way or the other, enum/type mapping seems unavoidable, so it seems like it's a matter of how you prefer to structure the sink code.

@andriysavin
Copy link

Thanks for the nudge. I would prefer not to expose MailKit types directly on the email sink's API - is there a higher-level way this can be expressed, akin to how we currently set EnableSSL?

I think this sink should introduce an enum similar or the same to SecureSocketOptions to avoid dependency on MailKit. And have one-to-one mapping between these enums. Come on, guys, lets finish this, it will be very useful!

@nblumhardt
Copy link
Member

Introducing an enum that maps to MailKit's one sounds reasonable 👍

@arthurchiakh arthurchiakh force-pushed the dev branch 2 times, most recently from 259ce91 to 295ab0a Compare May 25, 2021 15:13
@arthurchiakh
Copy link
Contributor Author

I'm sorry guys, better late than never.
I have added a dedicated Enum to map the MailKit socket options.

@nblumhardt please review my latest change. Thanks loads.

@arthurchiakh
Copy link
Contributor Author

@nblumhardt @andriysavin
I already changed the code accordingly.
Please let me know anything that I can improve.
Thanks.

@arthurchiakh
Copy link
Contributor Author

@nblumhardt Hi,
can you have a look on this?

@andriysavin
Copy link

@nblumhardt please, take a look

Copy link
Member

@nblumhardt nblumhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good 👍

@arthurchiakh arthurchiakh requested a review from nblumhardt June 4, 2021 15:49
@nblumhardt
Copy link
Member

Righto! 👍 Let's give this a try on dev 😎

@nblumhardt nblumhardt merged commit c34bc37 into serilog:dev Jun 4, 2021
@andriysavin
Copy link

@nblumhardt Is there a package feed for dev branch?

@arthurchiakh
Copy link
Contributor Author

@nblumhardt Is there a package feed for dev branch?

I believe this is the one.
https://www.nuget.org/packages/Serilog.Sinks.Email/2.4.1-dev-00128

@lenlenya
Copy link

Thank you for your contribution, It's very helpful to me.

@chekm8
Copy link

chekm8 commented Oct 15, 2021

Thanks for this, The pre-release code resolves my issue as well. Any idea when the final release will be published?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants