search

tzeumer / SIP2-Client-for-Python

MIT License
8 stars 11 forks source link

Bugfixes: case sensitivity in import, rebuild connections, and error handling #8

Closed yefuwang closed 3 years ago

yefuwang commented 3 years ago

Hi Tobias,

Here is a PR with 4 commits to addressing some issues described in the commit messages. Thank you very much for the good project.

Regards, Yefu

tzeumer commented 3 years ago

Hello Yefu,

thank for sharing. It's been quite I while since I last checked anything in Python.

I'm just not sure about the connection rebuilding commit, because if "tlsEnable" is set to false, that "if (mode == 'plain')" in line 531 should never be reached, because it would exit in line 449. And if TLS is enabled, it should not connect unencrypted by accident.

I have to think about it at least. But I'll merge the other commits.

Thanks again and greetings Tobias

yefuwang commented 3 years ago

Hi Tobias,

In my tests, it reached this branch with the connection closed. TLS is enabled but the server probably does not support TLS. I think this is what happened:

  1. Mode is set to "plain" in Line 424
  2. An exception is thrown from line 474, from the log in the exception handling, I am assuming your intention was to use plain: "CONNECTION INFO: SSL is not supported by host (using plain connection)."

When this happened, the plain exception was closed when it reaches the place I made the change. Therefore I tried to reopen it.

Regards, Yefu

tzeumer commented 3 years ago

Hello Yefu,

I think I just did it somewhat messy.

The best way is this: one knows if the SIP2 server supports TSL or not and sets the configuration variable self.tlsEnable accordingly. If the server does not support TSL and I know this, then it would quit at about line 448. This would be the cleanest thing (I know what my server support) and should work correctly.

But you're right about 474, I tried to offer an easy way if someone does not know if the server settings are plain or TSL (memories are coming back :)). And even though I didn't test it, I assume you're right that the plain connection breaks in the described way. Thanks for your explanation and the workaround! I'll push it in a second.

It's really been a long time :)

Greetings Tobias

tzeumer commented 3 years ago

I merged all four commits by cherry picking. Therefore I close this PR without the built in merge action.

Thanks again :)