Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Notifier] Fixed authorization problems#58088
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
carsonbot commentedAug 26, 2024
Hey! I see that this is your first PR. That is great! Welcome! Symfony has acontribution guide which I suggest you to read. In short:
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! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
This comment was marked as resolved.
This comment was marked as resolved.
| 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); |
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.
@mikaelkael Would be great if you could have a look here as you contributed the initial code in#50131.
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.
@xabbuh: as explain herehttps://docs.ntfy.sh/publish/#username-password, you don't have final sign equal and this modification breaks associated testhttps://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
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.
@xabbuh: as explain herehttps://docs.ntfy.sh/publish/#username-password, you don't have final sign equal and this modification breaks associated testhttps://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 encodetestuser: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 :-(
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 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
mikaelkaelAug 26, 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.
@jirikmik: you have the mention of no equal sign inhttps://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)
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.
Ah it is:
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 think screenshot from Ntfy.sh docs is related to "query param" auth, not Basic auth ...
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 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 myhttps://github.com/mikaelkael/ntfy-notifier (the repository on which I work before integration in symfony).
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.
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.
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 was able to reproduce the bugmikaelkael/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
| DSN example | ||
| ----------- | ||
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.
let's keep this blank line
| ``` | ||
| 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/). |
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.
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.
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.
Ok, I understand. But text is talking aboutURL which is not in example DSN. With self-hosted Ntfy isdefault unclear
| } | ||
| $options =$opts?$opts->toArray() : []; | ||
| $json_options =$opts?$opts->toArray() : []; |
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.
| $json_options =$opts?$opts->toArray() : []; | |
| $jsonOptions =$opts?$opts->toArray() : []; |
| } | ||
| $headers = []; | ||
| $client_options = ['json' =>$json_options]; |
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.
| $client_options = ['json' =>$json_options]; | |
| $clientOptions = ['json' =>$json_options]; |
OskarStark commentedSep 3, 2024
Any plans to finish this PR@jirikmik ? |
| ``` | ||
| NTFY_DSN=ntfy://[USER:PASSWORD]@default[:PORT]/TOPIC?[secureHttp=[on]] | ||
| NTFY_DSN=ntfy://URL[:PORT]/TOPIC?[secureHttp=[on]] | ||
| ``` |
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.
Can you please add a whiteline after and before each code delimiter line?
fabpot commentedJan 5, 2025
Closing as there are no more activities. Feel free to reopen when you have time or if someone wants to take over. |
Uh oh!
There was an error while loading.Please reload this page.
Fixed creating authorization header for username and password auth and added DSN example for token auth to README.