[Notifier] Fixed authorization problems #58088

jirikmik · 2024-08-26T07:13:23Z

Q	A
Branch?	6.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Issues	Fix #58085
License	MIT

Fixed creating authorization header for username and password auth and added DSN example for token auth to README.

carsonbot · 2024-08-26T07:13:28Z

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

Always add tests
Keep backward compatibility (see https://symfony.com/bc).
Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
Features and deprecations must be submitted against the 7.2 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

xabbuh · 2024-08-26T07:35:45Z

src/Symfony/Component/Notifier/Bridge/Ntfy/NtfyTransport.php

@@ -87,7 +87,7 @@ protected function doSend(MessageInterface $message): SentMessage
        $headers = [];

        if (null !== $this->user && null !== $this->password) {
-            $headers['Authorization'] = 'Basic '.rtrim(base64_encode($this->user.':'.$this->password), '=');
+            $headers['Authorization'] = 'Basic '.base64_encode($this->user.':'.$this->password);


@mikaelkael Would be great if you could have a look here as you contributed the initial code in #50131.

@xabbuh: as explain here https://docs.ntfy.sh/publish/#username-password, you don't have final sign equal and this modification breaks associated test https://github.com/symfony/symfony/actions/runs/10555314977/job/29238683518#step:8:1929. As all tests use mocks without direct access to a ntfy server, perhaps your modification is possible but you need to also change tests to be compliant

@xabbuh: as explain here https://docs.ntfy.sh/publish/#username-password, you don't have final sign equal and this modification breaks associated test https://github.com/symfony/symfony/actions/runs/10555314977/job/29238683518#step:8:1929. As all tests use mocks without direct access to a ntfy server, perhaps your modification is possible but you need to also change tests to be compliant

Suggested page not explain you don't have final equal sign. But in case you encode testuser:fakepassword string from Ntfy docs base64 output is without equal sign(s).

Doing tests is totally out of my scope. I just only wanted report bug I have found. Somebody asks me for PR, I did ... sorry :-(

The fix looks correct to me, as Base64 encoded strings do not always end with an = character. The = character is used as padding to ensure that the length of the encoded string is a multiple of 4 characters, which is required by the Base64 encoding standard.

So yes, the tests should be adjusted

https://3v4l.org/BHa9R

@jirikmik: you have the mention of no equal sign in https://docs.ntfy.sh/publish/#query-param, specifically for query param and as @OskarStark said, the case of the doc 'testuser:fakepassword' doesn't need padding. I will try to reproduce #58085 with a real server (not mocked)

I think screenshot from Ntfy.sh docs is related to "query param" auth, not Basic auth ...

In none of the example in the doc, we can find padding equal (for any langage, not only PHP) but it's not a valid reason. During my dev, it didn't made problem. There is perhaps a change with recent version of ntfy. The only way is to have a reproductible test case. As I know in symfony, we didn't depend on external service in test. So I will try to write a failing test case in my https://github.com/mikaelkael/ntfy-notifier (the repository on which I work before integration in symfony).

If I read the linked documentation correctly, stripping the trailing = characters is documented for authenticating via the query parameter,but not for basic auth. This means the proposed change is correct.

I was able to reproduce the bug mikaelkael/ntfy-notifier@a9e19dd, I confirm that the correction is good and the only thing to change in test is: mikaelkael/ntfy-notifier@a9e19dd#diff-cfb9cdd160d285443c2bf73a6307b616ce74d3431a19d887ad290425606d2471L114

nicolas-grekas · 2024-08-26T08:21:10Z

src/Symfony/Component/Notifier/Bridge/Ntfy/README.md

@@ -5,16 +5,24 @@ Provides [Ntfy](https://docs.ntfy.sh/) integration for Symfony Notifier.

 DSN example
 -----------
-


let's keep this blank line

nicolas-grekas · 2024-08-26T08:22:24Z

src/Symfony/Component/Notifier/Bridge/Ntfy/README.md

 ```
 where:
 - `URL` is the ntfy server which you are using
-    - if `default` is provided, this will default to the public ntfy server hosted on [ntfy.sh](https://ntfy.sh/).


Looking at README in other bridges, the example DSN uses @default directly. I think it'd be better to follow their lead as using something non-default is going to be the exception.

Ok, I understand. But text is talking about URL which is not in example DSN. With self-hosted Ntfy is default unclear

fabpot · 2024-08-27T06:33:32Z

src/Symfony/Component/Notifier/Bridge/Ntfy/NtfyTransport.php

@@ -73,29 +73,26 @@ protected function doSend(MessageInterface $message): SentMessage
            $opts = NtfyOptions::fromNotification($notification);
        }

-        $options = $opts ? $opts->toArray() : [];
+        $json_options = $opts? $opts->toArray() : [];


Suggested change

$json_options = $opts? $opts->toArray() : [];

$jsonOptions = $opts ? $opts->toArray() : [];

fabpot · 2024-08-27T06:33:39Z

src/Symfony/Component/Notifier/Bridge/Ntfy/NtfyTransport.php

        }

-        $headers = [];
+        $client_options = ['json' => $json_options];


Suggested change

$client_options = ['json' => $json_options];

$clientOptions = ['json' => $json_options];

OskarStark · 2024-09-03T17:56:03Z

Any plans to finish this PR @jirikmik ?

fabpot · 2024-09-06T06:44:32Z

src/Symfony/Component/Notifier/Bridge/Ntfy/README.md

 ```
-NTFY_DSN=ntfy://[USER:PASSWORD]@default[:PORT]/TOPIC?[secureHttp=[on]]
+NTFY_DSN=ntfy://URL[:PORT]/TOPIC?[secureHttp=[on]]
+```


Can you please add a whiteline after and before each code delimiter line?

fabpot · 2025-01-05T10:14:12Z

Closing as there are no more activities. Feel free to reopen when you have time or if someone wants to take over.

jirikmik added 2 commits August 26, 2024 09:04

Fix authorization header with self-hosted Ntfy

e96beb4

Added example for token auth

db86f7f

jirikmik requested a review from OskarStark as a code owner August 26, 2024 07:13

carsonbot added Status: Needs Review Bug labels Aug 26, 2024

carsonbot added this to the 7.2 milestone Aug 26, 2024

This comment was marked as resolved.

Sign in to view

jirikmik changed the base branch from 7.2 to 7.1 August 26, 2024 07:18

jirikmik requested review from xabbuh, lyrixx, dunglas, yceruto, welcoMattic, kbond, chalasr and jderusse as code owners August 26, 2024 07:18

xabbuh changed the base branch from 7.1 to 7.2 August 26, 2024 07:34

xabbuh removed request for dunglas, kbond, lyrixx, jderusse, welcoMattic, OskarStark, xabbuh, yceruto and chalasr August 26, 2024 07:34

xabbuh added the Notifier label Aug 26, 2024

carsonbot changed the title ~~Fixed authorization problems~~ [Notifier] Fixed authorization problems Aug 26, 2024

xabbuh removed this from the 7.2 milestone Aug 26, 2024

xabbuh added this to the 6.4 milestone Aug 26, 2024

xabbuh reviewed Aug 26, 2024

View reviewed changes

nicolas-grekas reviewed Aug 26, 2024

View reviewed changes

Auth headers created by http client

b6e9732

fabpot requested changes Aug 27, 2024

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Aug 27, 2024

fabpot reviewed Sep 6, 2024

View reviewed changes

fabpot closed this Jan 5, 2025

		@@ -5,16 +5,24 @@ Provides [Ntfy](https://docs.ntfy.sh/) integration for Symfony Notifier.

		DSN example
		-----------

	$json_options = $opts? $opts->toArray() : [];
	$jsonOptions = $opts ? $opts->toArray() : [];

	$client_options = ['json' => $json_options];
	$clientOptions = ['json' => $json_options];

Uh oh!

[Notifier] Fixed authorization problems #58088

[Notifier] Fixed authorization problems #58088

Uh oh!

Conversation

jirikmik commented Aug 26, 2024 • edited by xabbuh Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carsonbot commented Aug 26, 2024

Uh oh!

This comment was marked as resolved.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikaelkael Aug 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot Aug 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OskarStark commented Sep 3, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commented Jan 5, 2025

Uh oh!

Uh oh!

jirikmik commented Aug 26, 2024 •

edited by xabbuh

Loading

mikaelkael Aug 26, 2024 •

edited

Loading

fabpot Aug 27, 2024 •

edited

Loading