add in a client builder that abstracts away connecting to TLS or non-TLS connections and what TLS provider is used.

[urkle]

• this allows a more transparent and versatile usage of the library as one can simply compile it as-is and then use the builder to configure where we connect and how we connect without having to be concerned about what type is used for the imap::Client / imap::Session

This is inspired by how the ldap3 library hides-away the connection of ldap, ldaps, or ldapi (unix socket)

IMHO this should just be the main ClientBuilder, however, the methodology used in this PR requires that only either rustls OR nativetls be active, not both. So this PR adds it as a parallel client builder.

Implements issue described in #244

---

This change is 

[tjni]

Disclaimer: I'm a rust novice and new to this crate.

I haven't reviewed in detail, but I really like that enabling only one of native-tls or rustls-tls uses code that does not need to specify which one is used.

Personally, I think this is the overwhelming common case and would vote to make this part of the current builder even if it removes the option of using both TLS features at the same time, unless someone has a need for it.

If we want to support that need, would it still be possible by requiring a TLS library enum as a parameter to tls when multiple TLS library features are enabled?

[urkle]

@jonhoo I did the first part of the refactoring that uses your Boxed trait suggestion. Now the code works with both native and rust enabled.

I'll look next at condensing the builders so we have a single builder

[urkle]

@jonhoo I added another commit that combines the boxed client builder into the now single client builder.

Note a few contract changes happened.

1) the native_tls() and rustls() methods have been removed as tls_kind replaces that functionality. 2) the old connect(FnOne) method has been renamed connect_tls_with(FnOnce) I could not find a nice way to get it to behave and convert the C to a Box<dyn ImapConnection> the compiler just yelled at me, so it still returns a Client<C>

• if we can find a generic way to allow the user to set custom certs we may be able to just do away with this method completely :)

[codecov[bot]]

Codecov Report

Merging #245 (bb39460) into main (ce87f7d) will increase coverage by 0.3%. The diff coverage is 89.5%.

Files	Coverage Δ
src/types/deleted.rs	95.8% <ø> (ø)
src/client.rs	93.4% <96.5%> (+0.3%)	:arrow_up:
src/conn.rs	0.0% <0.0%> (ø)
src/extensions/idle.rs	0.0% <0.0%> (ø)
src/error.rs	25.0% <0.0%> (-0.9%)	:arrow_down:
src/client_builder.rs	96.0% <95.6%> (+12.7%)	:arrow_up:

[urkle]

@jonhoo I rebased, squashed and cleaned things up a bit based on the latest comments.

Can you re-review things?

[urkle]

@jonhoo I rebased + squashed and then added a fixup addressing the latest comments

• added new Error for TlsNotConfigured (when no tls is compiled in) and StartTlsNotAvailable (server does not provide but was requested)
• added AutoTls mode (default when tls available) which will refuse to connect via Plaintext
  • when tls is NOT available should we default to Plaintext vs Auto?
• renamed Tcp to Plaintext
• more cfg guards for when tls is disabled to return TlsNotConfigured (even in your unreachable suggestion)
• danger_no_tls_verify -> danger_skip_tls_verify
• removed public "helpers" starttls, tls, native_tls, and rusttls

Other items I am wondering about 1) remove connect_with completely you had made one comment about this that if the user want's to set it it up themselves they can simply wrap it in the Client. Though if we do this, should we expose into_inner() publicly? 2) change mode and tls_kind, and danger_skip_tls_verify (the 3 config methods) not be &mut self but instead be simply mut self (consuming setters). (fn mode(mut self, mode: ConnectionMode) -> Self) This would make it so we don't need the configuration to be mut in the common case.

[bsodmike]

Hello! @urkle Running tests locally for your wip branch I am seeing:

---- auto_tls stdout ----
thread 'auto_tls' panicked at tests/builder_integration.rs:103:31:
called `Result::unwrap()` on an `Err` value: StartTlsNotAvailable

Do we expect to enforce needing StartTLS even in the AutoTLS scenario? / More detail over here.

This test passes with

    builder
        .danger_skip_tls_verify(true)
        .mode(ConnectionMode::Auto);

... although I'm not sure if that's our expectation here.

[urkle]

---- auto_tls stdout ----
thread 'auto_tls' panicked at tests/builder_integration.rs:103:31:
called `Result::unwrap()` on an `Err` value: StartTlsNotAvailable

@bsodmike the intention here is that if you use AutoTls, then it will ONLY connect via a secured connection. If you use port 993 then strait Tls, if you use any other port than it'll use StartTls, and if no starttls is supported it'll yield that StartTlsNotAvailable error.

I just pushed up a force commit to correct an issue in the AutoTls branch where if no TLS provider is compiled in it returns the wrong error message, it should have yielded TlsNotConfigured in that scenario.

The intention of the "Auto" mode is that it'll allow connecting to an Imap server via plain text if no starttls is available. Thus that, in general, is not preferred as it is less secure.

[bsodmike]

@bsodmike the intention here is that if you use AutoTls, then it will ONLY connect via a secured connection.

My confusion was around the integration test; running this out of the box throws up the error I shared before, but that's because the test attempts to connect to the test backend (Greenmail) and it isn't configured to support StartTLS (at least for this test).

Perhaps we should make it clearer as to the objective of the test, and then ensure Greenmail is configured (if needed) to support its requirements. Probably makes sense to test for the 'Happy path'.

[urkle]

@jonhoo I pushed a fixup with the changes you requested.

Once you give me the OK I'll rebase + squash the fixups (as there is some other items in master I'll need to resolve)

[jonhoo]

Ah, looks like we also now specifically have to set Auto (not AutoTls) for the greenmail tests, and fix some doctests.

[urkle]

@jonhoo I rebased + squashed and fixed up the test errors.

for connect_with I simply removed the comments & doc example entirely since it's now an internal method.

[urkle]

@jonhoo we are green except for the persisting code-coverage one. I am reviewing those to at least add some coverage to my code changes where possible.

[urkle]

@jonhoo 100% green!

[jonhoo]

Released in alpha.12 — thanks again for sticking with this for so long!