Establish STARTTLS connection without authentication #17

schengawegga · 2020-09-22T11:09:42Z

For using smartHost, wich authenticates the client via his ip address, to establish the STARTTLS connection without sending credentials.

schengawegga · 2021-12-20T14:51:31Z

Is there anyone who can do a code-review and merge this request?

till

Change looks sane to me. Maybe @alecpl has thoughts?

alecpl

Please describe the new option in the constructor doc. It need to be noted somewhere that the option requires Net_SMTP 1.10.0.

alecpl · 2021-12-22T11:01:37Z

Mail/smtp.php

@@ -402,6 +415,16 @@ public function getSMTPObject()
                return PEAR::raiseError($error, PEAR_MAIL_SMTP_ERROR_AUTH);
            }
        }
+
+        /* Attempt to establishe a TLS encrypted connection. */


"establishe" -> "establish".

alecpl · 2021-12-22T11:01:53Z

Mail/smtp.php

@@ -402,6 +415,16 @@ public function getSMTPObject()
                return PEAR::raiseError($error, PEAR_MAIL_SMTP_ERROR_AUTH);
            }
        }
+
+        /* Attempt to establishe a TLS encrypted connection. */
+        if ($this->starttls && $this->auth === False) {


it would be better to do just !$this->auth here.

alecpl · 2021-12-22T11:02:50Z

Mail/smtp.php

+        /* Attempt to establishe a TLS encrypted connection. */
+        if ($this->starttls && $this->auth === False) {
+            $starttls = $this->_smtp->starttls();
+            if(PEAR::isError($starttls)){


A space after if, and elseif below. And after closing bracket.

alecpl · 2021-12-22T11:03:24Z

Mail/smtp.php

+            $starttls = $this->_smtp->starttls();
+            if(PEAR::isError($starttls)){
+                return PEAR::raiseError($starttls);
+            } elseif($starttls === False){


false and spacing.

For using smartHost, wich authenticates the client via his ip address, to establish the STARTTLS connection without sending credentials. Change request from alecpl

alecpl · 2021-12-22T14:18:29Z

It looks good now. I see one point of possible confusion. If you set auth=PLAIN and starttls=false, STARTTLS will still be used, as this is the default behavior for of the auth() method. However, I don't know how to solve that without a BC break.

schengawegga · 2021-12-22T15:15:43Z

You´re right. Thank you. I´ve not seen this. I will do a new commit in a few hours, so we will be compatible.

For using smartHost, wich authenticates the client via his ip address, to establish the STARTTLS connection without sending credentials. Another change request from alecpl

schengawegga · 2021-12-22T20:06:01Z

It looks good now. I see one point of possible confusion. If you set auth=PLAIN and starttls=false, STARTTLS will still be used, as this is the default behavior for of the auth() method. However, I don't know how to solve that without a BC break.

Now it should be compatible with the Net_SMTP workflow.

alecpl · 2021-12-23T08:20:06Z

Mail/smtp.php

@@ -185,7 +204,8 @@ class Mail_smtp extends Mail {
     * passed in. It looks for the following parameters:
     *     host        The server to connect to. Defaults to localhost.
     *     port        The port to connect to. Defaults to 25.
-     *     auth        SMTP authentication.  Defaults to none.
+     *     auth        SMTP authentication. Defaults to none.
+     *     starttls    Should STARTTLS connection be used? Defaults to false. PEAR/Net_SMTP >= 1.10.0 required.


The default is not false anymore.

Hawkeye ;-) I´m sorry. I have pushed another commit with "No default".

For using smartHost, wich authenticates the client via his ip address, to establish the STARTTLS connection without sending credentials. Another change request from alecpl

schengawegga · 2022-02-28T15:55:31Z

Hi, what are the next steps?

schengawegga · 2022-05-30T13:15:10Z

Hi there. Do I need to do anything else to complete the merge?

MegaphoneJon · 2022-07-13T23:54:04Z

Is there anything that someone can do to help the maintainers to complete this merge? It appears to be approved, but I'm willing to put it into production if that would help.

alecpl · 2022-07-14T05:27:55Z

Merged. However, I'm not interested in this project anymore.

schengawegga · 2022-07-14T12:17:52Z

So, if alecpl is not interested anymore, is there anyone who is interested? to do merges at this project in the future?
i´m interested, so if nobody else is interested, give me the permission to do merges on this project, please.

MegaphoneJon · 2022-07-14T18:18:22Z

@alecpl Thank you for all your work as a maintainer over the years!

@gauthierm I see that you're the maintainer on Packagist. Are you able to merge changes? Given that this library almost never changes, if we can tag a new version for Packagist, I think the maintenance work will be done for a long time to come.

schengawegga · 2022-08-03T06:25:36Z

Change looks sane to me. Maybe @alecpl has thoughts?

@till can you publish a new release? and i am very interested in maintaining these package. Can you give me permissions to maintain these package too? It is a very helpful and widespreaded package, so it should be good maintained in the future.

ashnazg · 2022-08-03T10:14:11Z

I'll look at getting a release out in the next week or so.

till · 2022-08-03T10:58:42Z

@ashnazg I would help, but I unlearned how to do the PEAR releases, and also don't want to burden my brain. But if you document it somewhere, I can help next time around. Or did we stop doing these? :D

till · 2022-08-03T11:00:28Z

@schengawegga you can use the branch/commit in composer already:

{
    "name": "your/package",
    "repositories": [
        {
            "type": "vcs",
            "url": "https://github.com/pear/Mail.git"
        }
    ],
    "require": {
        "pear/mail": "dev-master#PUT-COMMIT-HERE"
    }
}

schengawegga · 2022-08-03T11:08:05Z

I'll look at getting a release out in the next week or so.

@ashnazg Thank you very very much. I am very happy about your effort. This will help us very much.

@ashnazg @till How do you think about my proposal to helping you in developing this package?
But not only this package, i plan a pull-request for PEAR/DB, because this is also a very helpful package in our project, and i will help to keep all these packages well maintained in the future.
So can you help me please to getting a part of the development team, to help you and take a little bit of work off your hands?

schengawegga · 2022-08-03T11:12:07Z

@schengawegga you can use the branch/commit in composer already:

{
    "name": "your/package",
    "repositories": [
        {
            "type": "vcs",
            "url": "https://github.com/pear/Mail.git"
        }
    ],
    "require": {
        "pear/mail": "dev-master#PUT-COMMIT-HERE"
    }
}

@till Thanks for the reply. This is also a very good solution. But i am also interested in learning to keep these packaging well maintained. Thats the reason why i am asking you all to be part of the development team. I think these PEAR packages are so valuable and helpful, so they have to be well maintained in the future. And to ensure that, i will help you all with the development work.

ashnazg · 2022-11-12T15:26:47Z

@alecpl , should I mark you as inactive on the maintainer list?

alecpl · 2022-11-12T20:35:29Z

Yes, please.

ashnazg · 2022-11-24T14:49:34Z

@schengawegga , I've added you as a Maintainer on the Mail project now.

schengawegga · 2022-11-29T23:12:50Z

@ashnazg Thank you for adding me as a maintainer to this project.
@MegaphoneJon i published a new release on GitHub and pear.php.net.
Thanks all for your support.

Establish STARTTLS connection without authentication

e49cac5

For using smartHost, wich authenticates the client via his ip address, to establish the STARTTLS connection without sending credentials.

till approved these changes Dec 20, 2021

View reviewed changes

alecpl requested changes Dec 22, 2021

View reviewed changes

Establish STARTTLS connection without authentication

818a806

For using smartHost, wich authenticates the client via his ip address, to establish the STARTTLS connection without sending credentials. Change request from alecpl

schengawegga requested a review from alecpl December 22, 2021 13:13

Establish STARTTLS connection without authentication

6e6cf62

For using smartHost, wich authenticates the client via his ip address, to establish the STARTTLS connection without sending credentials. Another change request from alecpl

alecpl requested changes Dec 23, 2021

View reviewed changes

Establish STARTTLS connection without authentication

47ea96f

For using smartHost, wich authenticates the client via his ip address, to establish the STARTTLS connection without sending credentials. Another change request from alecpl

alecpl approved these changes Dec 23, 2021

View reviewed changes

MegaphoneJon mentioned this pull request Jul 14, 2022

net_smtp upgrade to 1.10.0 civicrm/civicrm-core#23998

Merged

alecpl merged commit 66ac6d1 into pear:master Jul 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Establish STARTTLS connection without authentication #17

Establish STARTTLS connection without authentication #17

schengawegga commented Sep 22, 2020

schengawegga commented Dec 20, 2021

till left a comment

alecpl left a comment

alecpl Dec 22, 2021

alecpl Dec 22, 2021

alecpl Dec 22, 2021

alecpl Dec 22, 2021

alecpl commented Dec 22, 2021

schengawegga commented Dec 22, 2021

schengawegga commented Dec 22, 2021

alecpl Dec 23, 2021

schengawegga Dec 23, 2021

schengawegga commented Feb 28, 2022

schengawegga commented May 30, 2022

MegaphoneJon commented Jul 13, 2022

alecpl commented Jul 14, 2022

schengawegga commented Jul 14, 2022

MegaphoneJon commented Jul 14, 2022

schengawegga commented Aug 3, 2022

ashnazg commented Aug 3, 2022

till commented Aug 3, 2022

till commented Aug 3, 2022

schengawegga commented Aug 3, 2022

schengawegga commented Aug 3, 2022

ashnazg commented Nov 12, 2022

alecpl commented Nov 12, 2022

ashnazg commented Nov 24, 2022

schengawegga commented Nov 29, 2022

Establish STARTTLS connection without authentication #17

Establish STARTTLS connection without authentication #17

Conversation

schengawegga commented Sep 22, 2020

schengawegga commented Dec 20, 2021

till left a comment

Choose a reason for hiding this comment

alecpl left a comment

Choose a reason for hiding this comment

alecpl Dec 22, 2021

Choose a reason for hiding this comment

alecpl Dec 22, 2021

Choose a reason for hiding this comment

alecpl Dec 22, 2021

Choose a reason for hiding this comment

alecpl Dec 22, 2021

Choose a reason for hiding this comment

alecpl commented Dec 22, 2021

schengawegga commented Dec 22, 2021

schengawegga commented Dec 22, 2021

alecpl Dec 23, 2021

Choose a reason for hiding this comment

schengawegga Dec 23, 2021

Choose a reason for hiding this comment

schengawegga commented Feb 28, 2022

schengawegga commented May 30, 2022

MegaphoneJon commented Jul 13, 2022

alecpl commented Jul 14, 2022

schengawegga commented Jul 14, 2022

MegaphoneJon commented Jul 14, 2022

schengawegga commented Aug 3, 2022

ashnazg commented Aug 3, 2022

till commented Aug 3, 2022

till commented Aug 3, 2022

schengawegga commented Aug 3, 2022

schengawegga commented Aug 3, 2022

ashnazg commented Nov 12, 2022

alecpl commented Nov 12, 2022

ashnazg commented Nov 24, 2022

schengawegga commented Nov 29, 2022