Add pre-built tasks #165

soywod · 2024-05-03T09:04:04Z

I open the PR so you can already have a look at it, but I plan to leave comments here and there to explain the changes.

coveralls · 2024-05-03T09:11:28Z

Pull Request Test Coverage Report for Build 8936654537

Details

10 of 32 (31.25%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.5%) to 53.545%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/send_command.rs	3	5	60.0%
src/client.rs	7	27	25.93%

Totals
Change from base Build 8774441930:	-0.5%
Covered Lines:	657
Relevant Lines:	1227

💛 - Coveralls

soywod

Here some comments to give more context. I have the following questions:

In this PR I missed STARTTLS related stuff. I tried to set it up but I have hard time to do so because of AnyStream holding a dyn Stream. Here a concrete example: I create a TcpStream, then wrap it into a AnyStream. From there I can create a ClientFlow and a Scheduler. After the greetings, if STARTTLS is supported by the server, I need to take back the initial TcpStream in order to upgrade it to a TlsStream. Is the SansIO refactor will have an impact on this?
Regarding SansIO, how is the state of the refactor? Should I wait for it, then re-open a PR?
Would you like me to split now the PR into small ones? One first with the core, then few others (one per task)?

Cargo.toml

soywod · 2024-05-04T08:32:14Z

src/client.rs

@@ -152,8 +177,22 @@ impl ClientFlow {
                return Ok(event);
            }

-            if let Some(event) = self.progress_receive().await? {
-                return Ok(event);
+            if self.is_waiting_for_idle_done_set() {


This is the uggliest part of the PR. For the IDLE command to work with the current flow, I acted the same way the imap crate does: every time an IDLE event is received (or a timeout), we exit the IDLE mode. Lib consumers need to take care of the loop.

Interesting. No strong opinions yet, but some thoughts:

Not sure whether we should add this to ClientFlow or burden the user of ClientFlow with implementing this feature. I don't know the pros and cons yet. Or can we implement this as a task?

Regarding the SansIO refactoring, I would prefer to treat time as an input and somehow pass it into ClientFlow. This would also make testing more feasible.

If we want to implement this as a feature of ClientFlow, we should do this in a separate PR and add documentation and flow-test test cases.

Alright, then I will wait for the SansIO refactoring, and submit a dedicated PR.

Okay! If you want then I can work on this, too.

I see that the new implem with SansIO reads and writes in parallel, so I'm curious to give it a try and see how it behaves.

tasks/src/lib.rs

duesee · 2024-05-04T09:53:22Z

Hey Clement! Thanks for the PR! :-)

I'm still not home but just a few thoughts to (hopefully) unblock you a bit.

I think that we have roughly a few work packages here: Tasks that can already be added, tasks that need changes (IDLE), (START)TLS, and other improvements.

To better focus on any part, we could try to split this PR accordingly and first try to get all things in that are unrelated to SansIO. Most tasks, I guess?

One first with the core, then few others (one per task)?

Sounds good! Feel free to PR multiple tasks at once when you feel there should not be much discussion needed.

We should also treat STARTTLS as a connection "prefix", and only progress the IMAP protocol to the point where both parties start sending TLS data (doing (almost) no parsing).

We don't even need to handle capabilities for STARTTLS:

read greeting (no IMAP parsing, greeting has no literal, just read and discard line as we MUST forget everything we learn here anyway)
send A starttls (don't care if supported)
receive line (don't care if ok or not)
start with TLS (exactly as an implicit TLS client)

This way, we have an isolated STARTTLS prefix and can proceed exactly as with implicit TLS. This would be secure by design. More in section 8 https://www.usenix.org/conference/usenixsecurity21/presentation/poddebniak.

Jakob is currently working on TLS. I would suggest to wait a few more days before coming back to it.

PS: I kinda like splitting and moving commits around :-) So, if you like, I could give it a try early next week. Still, feel free to already move this as much forward as you want!

src/client.rs

src/send_command.rs

src/client.rs

soywod · 2024-05-04T14:28:37Z

Awesome, then I will open small PRs until you come back.

We should also treat STARTTLS as a connection "prefix", and only progress the IMAP protocol to the point where both parties start sending TLS data (doing (almost) no parsing).

It is more clear now, thank you for the post as well, really interesting.

Jakob is currently working on TLS. I would suggest to wait a few more days before coming back to it.

I will wait for Jakob's work then. Thanks for your feedback!

jakoschiko · 2024-05-05T00:15:27Z

fyi: SansIO is ready for review, see #158

soywod · 2024-05-05T09:40:29Z

fyi: SansIO is ready for review

Awesome, I will try to integrate my core changes from your branch to see. As soon as it is merged, I will open the first PR (and close this one).

duesee · 2024-05-05T11:13:50Z

Thanks! :-) Lot's of progress happening, great :-)

feat: Add pre-built tasks

a83efdd

soywod commented May 4, 2024

View reviewed changes