[Notifier] [Telegram] Add support for local API server #57769
Conversation
|
I am afraid that this could lead to security issues, what about combining this flag with kernel debug parameter? Does that make sense? 🤔 |
src/Symfony/Component/Notifier/Bridge/Telegram/TelegramTransport.php
Outdated
Show resolved
Hide resolved
Agree, but usually this "API server" is hosted internally within the same infrastruture/server, so it might not be a big risk in that case? Maybe when it hosted on a remote server and the communication will flow over the internet, a TLS-termination server may be used in combination with it, to secure the data and keep the transmition happens on We also may add an explicit caution in the readme file about this? |
Not sure, but usually I see options of other bridges being controlled via DSN only. |
0e0398c to
ac9edca
Compare
src/Symfony/Component/Notifier/Bridge/Telegram/Tests/TelegramTransportTest.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Notifier/Bridge/Telegram/TelegramTransportFactory.php
Outdated
Show resolved
Hide resolved
| $toString = \sprintf('telegram://%s', $this->getEndpoint()); | ||
| $formattedOptions = http_build_query(['channel' => $this->chatChannel, 'disable_https' => $this->disableHttps ?: null]); | ||
|
|
||
| if ($formattedOptions) { |
There was a problem hiding this comment.
something is wrong here: the condition is always true now
There was a problem hiding this comment.
It will be true only if one of the $chatChannel or $disableHttps properties has been set to something except the default. Unless this, the condition will always be false.
echo http_build_query(['channel' => null, 'disable_https' => false ?: null]); should return an empty string which will not pass the condition.
I may use empty() function in the condition for better readability? (e.g. if (!empty($formattedOptions)))
There was a problem hiding this comment.
Tests are passing well also
There was a problem hiding this comment.
I just tested it, as I was always confused, but http_build_query behaves like array_filter and if chatChannel and disableHttps is null, $formattedOptions is not true
There was a problem hiding this comment.
Actually, I don't get what is wrong with the http_build_query or the if statement 😅. It just tries to build a query string from a given set of class properties ($chatChannel & $disableHttps). I also have seen many other bridges use http_build_query in the same function to build the transport DSN.
If that's not a good approach, what is the alternative then?
There was a problem hiding this comment.
Hola @nicolas-grekas @OskarStark, Can you have a recheck on here? Thanks
…'s default behavior of using `https` protocol Signed-off-by: Ahmed Ghanem <ahmedghanem7361@gmail.com>
4f8cc4d to
7cdb55f
Compare
|
@nicolas-grekas @OskarStark, Hola guys, can this PR get some review on here, whether it will be accepted or rejected😅? Thanks |
| @@ -14,6 +14,27 @@ where: | |||
| - `TOKEN` is your Telegram token | |||
| - `CHAT_ID` is your Telegram chat id | |||
There was a problem hiding this comment.
The option should be added to this list, similar to this: https://github.com/symfony/symfony/blob/7.3/src/Symfony/Component/Notifier/Bridge/AmazonSns/README.md#dsn-example
|
|
||
| Example: | ||
| ``` | ||
| TELEGRAM_DSN=telegram://TOKEN@localhost:5001?channel=CHAT_ID&disable_https=1 |
There was a problem hiding this comment.
In AmazonSns, this option is named sslmode. I don't know if this name is better, but I think it's better to be consistent between adapters.
This would become:
| TELEGRAM_DSN=telegram://TOKEN@localhost:5001?channel=CHAT_ID&disable_https=1 | |
| TELEGRAM_DSN=telegram://TOKEN@localhost:5001?channel=CHAT_ID&sslmode=disable |
There was a problem hiding this comment.
I know you've already changed the name in response to our request #57769 (comment), but perhaps the consistency with other bridges hadn't been examined.
Since Telegram's local API server doesn't support HTTPS traffic natively, this PR introduces a new DSN option/flag to allow the disabling of bridge's default behavior of using
httpsprotocol