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

Correct sieve connection when using scheme #1199

Merged

Conversation

Shadow243
Copy link
Member

@Shadow243 Shadow243 commented Aug 27, 2024

I added a checkbox that will be used to enable or disable TLS, instead of relying on the Sieve host starting with 'tls://'. With Migadu, even when TLS mode is set to true, we use the host without 'tls://'. However, with Stalwart, 'tls://' is required. In the older version, we were removing it before sending the request to the server at https://github.com/cypht-org/cypht/pull/1199/files#diff-f546b2284fe2ff1bd3c96ced190bc15d442480a4ebe1da5c82a2a59dd1b97505R1511 in parse_url function.

@Shadow243 Shadow243 force-pushed the correct-sieve-error-with-scheme branch 4 times, most recently from f5476dc to af3bc1e Compare August 27, 2024 02:48
modules/smtp/hm-smtp.php Outdated Show resolved Hide resolved
@josaphatim
Copy link
Member

@Shadow243 can you please rebase

Personally I'm not convinced with this fix. I have tested it with Postale and Migadu which do not work. @kroky can you please give your opinion ?

@Shadow243
Copy link
Member Author

@Shadow243 can you please rebase

Personally I'm not convinced with this fix. I have tested it with Postale and Migadu which do not work. @kroky can you please give your opinion ?

#1199 (comment)

@Shadow243
Copy link
Member Author

@Shadow243 can you please rebase

I have tested it with Postale and Migadu which do not work. @kroky can you please give your opinion ?

There is a small change on the User interface, here some screenshorts that will show you:
Screenshot 2024-09-12 at 16 29 05

I added a checkbox that will be used to enable or disable TLS, instead of relying on the Sieve host starting with 'tls://'. With Migadu, even when TLS mode is set to true, we use the host without 'tls://'. However, with Stalwart, 'tls://' is required. In the older version, we were removing it before sending the request to the server.
Screenshot 2024-09-12 at 16 33 53

@Shadow243 Shadow243 force-pushed the correct-sieve-error-with-scheme branch from 7ac9050 to d9f26cb Compare September 12, 2024 13:49
@kroky
Copy link
Member

kroky commented Sep 12, 2024

I think we need these changes - relying on tls scheme is not enough, it is better to specify host and port separately and then turn on or off the tls via a checkbox. So, in terms of UI it is ok. Just check if the changed method signatures (connect_to_imap_server) is OK when used throghout the app. I don't see other problems but I haven't test this code.

@josaphatim
Copy link
Member

Thanks @kroky
@Shadow243 Then you need also to refactor IMAP_AUTH_SIEVE_CONF_HOST found in .env

@Shadow243 Shadow243 force-pushed the correct-sieve-error-with-scheme branch 2 times, most recently from cdbce5d to 0074054 Compare September 28, 2024 14:33
@Shadow243
Copy link
Member Author

Thanks @kroky @Shadow243 Then you need also to refactor IMAP_AUTH_SIEVE_CONF_HOST found in .env

@josaphatim it's now done. just a concern, if i add for example a wrong sieve url i'l getting time out error, it coming from henrique-borba/php-sieve-manager: readResponse function.

config/app.php Outdated Show resolved Hide resolved
config/app.php Show resolved Hide resolved
modules/nux/services.php Show resolved Hide resolved
.env.example Outdated Show resolved Hide resolved
lib/auth.php Outdated Show resolved Hide resolved
modules/sievefilters/hm-sieve.php Outdated Show resolved Hide resolved
@Shadow243 Shadow243 force-pushed the correct-sieve-error-with-scheme branch from 0074054 to 01dd12a Compare October 6, 2024 21:35
@Shadow243
Copy link
Member Author

@josaphatim corrections applied.

@Shadow243 Shadow243 force-pushed the correct-sieve-error-with-scheme branch 5 times, most recently from 76147c4 to d756905 Compare October 18, 2024 12:06
@Shadow243 Shadow243 force-pushed the correct-sieve-error-with-scheme branch from d756905 to d522c61 Compare October 18, 2024 13:21
@josaphatim josaphatim merged commit 28b39ac into cypht-org:master Oct 18, 2024
4 of 5 checks passed
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.

3 participants