-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Notifier] Updated the NTFY notifier to run without a user parameter #53594
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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:
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 |
Hi @lostfocus! With your implementation, we store two exclusive ways of authorization, that cannot be used together. The NTFY documentation makes an interesting point: the ntfy uses/accepts access token as "user-less" Basic authorization.
This would be smaller to implement while beeing easier to maintain .. what do you think ? Something similar is done in the Telegram Bridge: private function getToken(Dsn $dsn): string
{
if (null === $dsn->getUser() && null === $dsn->getPassword()) {
throw new IncompleteDsnException('Missing token.', 'telegram://'.$dsn->getHost());
}
if (null === $dsn->getPassword()) {
throw new IncompleteDsnException('Malformed token.', 'telegram://'.$dsn->getHost());
}
return sprintf('%s:%s', $dsn->getUser(), $dsn->getPassword());
} |
@@ -41,6 +42,13 @@ public function getTopic(): string | ||
return $this->topic; | ||
} | ||
|
||
public function setToken(?string $token): self |
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.
public function(#[\SensitiveParameter] ?string $token): self
(Probably useless as you may remove the method :) )
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.
Is #[\SensitiveParameter]
already in the 6.4 branch? setPassword
has it in 7.0 but not in 6.4.
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.
Sorry forgot you targetted 6.4.
But as i'm not sure it will be considered a bug ... you may have to target 7.1 (let's wait to see what admins say)
@smnandre yeah, I was looking into that. But if we have something like |
I'm not sure to understand, were are you losing the password ? i think the "unified" sugegstion would be easier to develop, passing by user:password to the Basic or Bearer header with (almost) no manipulation. And it would be easier for users to switch from Token to user/password :) |
Okay, yeah, that was a brainfart. For some reason I thought |
…thout user parameter per the discussion on the PR symfony#53594
a152a33
to
fa3be01
Compare
Thank you @lostfocus. |
The ntfy integration of the notifier component was missing the option to run without set username.