Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Notifier] [Telegram] Add support for local API server#57769
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:7.4
Are you sure you want to change the base?
[Notifier] [Telegram] Add support for local API server#57769
Uh oh!
There was an error while loading.Please reload this page.
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 OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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 toac9edcaCompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Notifier/Bridge/Telegram/Tests/TelegramTransportTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Notifier/Bridge/Telegram/TelegramTransportFactory.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
After my comments
| $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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
something is wrong here: the condition is always true now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 useempty() function in the condition for better readability? (e.g.if (!empty($formattedOptions)))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Tests are passing well also
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I just tested it, as I was always confused, but http_build_query behaves likearray_filter and if chatChannel and disableHttps is null,$formattedOptions is not true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actually, I don't get what is wrong with thehttp_build_query or theif 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 usehttp_build_query in the same function to build the transport DSN.
If that's not a good approach, what is the alternative then?
ahmedghanem00Aug 25, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ahmedghanem00Jan 9, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hola@nicolas-grekas@OskarStark, Can you have a recheck on here? Thanks
…'s default behavior of using `https` protocolSigned-off-by: Ahmed Ghanem <ahmedghanem7361@gmail.com>
4f8cc4d to7cdb55fCompare@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In AmazonSns, this option is namedsslmode. I don't know if this name is better, but I think it's better to be consistent between adapters.
symfony/src/Symfony/Component/Notifier/Bridge/AmazonSns/AmazonSnsTransportFactory.php
Line 34 in9ad4353
| $protocol ='disable' ===$dsn->getOption('sslmode') ?'http' :'https'; |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I likesslmode 👍
Uh oh!
There was an error while loading.Please reload this page.
Since Telegram'slocal 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