Add option novalidatecert to connect(); closes #63

[trasher]

Q	A
Documentation	no
Bugfix	no
BC Break	no
New Feature	yes
RFC	no
QA	no

Description

Allow connection with self signed SSL certificates; see #63; port of existing zendframework/zend-mail#247.

[trasher]

Thanks @cedric-anne :))

[weierophinney]

First off, thanks for this patch! I'm sure a number of people will appreciate having it in place, and it will make testing locally, where you might be forced to use self-signed certs, far easier!

I think you can improve this substantially by moving the various functionalities around the $novalidatecert property and setter into the ProtocolTrait, as well as the functionality for creating the socket options. I've provided guidance in the comments below.

Additionally, it would be great if you could figure out a way to unit test this, as the new functionality is not covered at all. While I'm reasonably certain it will be fine, a unit test ensures we don't break it in the future.

[weierophinney]

Please use camelCase for variable names for consistency (this is part of our coding standard).

[trasher]

Changed. I've also factorized this piece of code, since this was entirely duplicated in both Imap and Pop3 classes

[weierophinney]

Please call $this->setNoValidateCert($novalidatecert) to ensure you get proper validation of the value (see my notes on that method as well).

[trasher]

Done

[weierophinney]

Please call $this->setNoValidateCert($novalidatecert) to ensure you get proper validation of the value (see my notes on that method as well).

[trasher]

Done aswell

[weierophinney]

Please see my notes on the same method in the Imap class.

[weierophinney]

Please see my notes on this in the Imap class.

Since this code is reproduced in both classes, I'd argue that you should:

• Move the $novalidatecert property into ProtocolTrait
• Move the setNoValidateCert() method into ProtocolTrait
• Create a new method in ProtocolTrait:

/**
 * @return array
 */
private function prepareSocketOptions()
{
    return $this->novalidatecert
        ? [
            'ssl' => [
                'verify_peer_name' => false,
                'verify_peer'            => false,
            ],
        ]
        : [];
}

Then, in the connect() method of each, you can do:

$this->socket = stream_socket_client(
    $host . ':' . $port,
    $errno,
    $errstr,
    self::TIMEOUT_CONNECTION,
    STREAM_CLIENT_CONNECT,
    stream_context_create($this->prepareSocketOptions())
);

(This also means the comments about camelCase names become irrelevant, as the variables are never created.)

[trasher]

I've moved then entire socket preparation, and splitted options preparation to another method

[trasher]

Hi @weierophinney thank you for the feedbacks :)

I will fix according to your review comments.
I do not know if this is possible to add tests on that; I guess some case should be added on the existing "Storage" ones; I can try that and we'll see the result.

[trasher]

Tests are failing: Laminas\Mail\Protocol\AbstractProtocol and Laminas\Mail\Protocol\ProtocolTrait define the same property ($socket); maybe the socket creation method should be moved in AbstractProtocol.

[trasher]

I've added a very basic test; but I do not know if it is possible to do more; and I do not really have time to investigate on that.

[glensc]

@trasher let's hope @weierophinney is happy with the changes. 3️⃣ pairs of 👀 reviewed the code now.

[weierophinney]

Looks good! I've made some minor changes locally (pushing momentarily) to cover some nitpicks I had but didn't want to bother you with.

🚢

[weierophinney]

I rebased locally and made edits... but don't have rights to push back to the origin repo. Rest assured, though - your commits are in!