Be sure to differentiate between user and server certificates#21668
Be sure to differentiate between user and server certificates#21668
Conversation
If we try to list the certificates there is a difference between a user and the system. If the user does not have customer certificates but the server does. Then we should still fetch the customer server bundle This slightly extends the ICertificateManager. But since it fixes a perforamnce issue I'm kind of fine with it. Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
rullzer
requested review from
ChristophWurst,
MorrisJobke,
juliusknorr,
kesselb and
nickvergessen
| * @since 8.0.0 | ||
| */ | ||
| public function listCertificates(); | ||
| public function listCertificates($uid = ''); |
There was a problem hiding this comment.
| public function listCertificates($uid = ''); | |
| public function listCertificates(?string $uid = ''); |
or
| public function listCertificates($uid = ''); | |
| public function listCertificates(string $uid = null); |
There was a problem hiding this comment.
then it should be the first, because the second one would change the behavior from "return the users list" to "return the global list" ...
what a mess of an API 🙈
Maybe it should be listUserCertificates(string $uid = '') and listGlobalCertificates() to make this visible in the code and not hidden in some magic flags.
There was a problem hiding this comment.
Let me do it this way. I'll revert the original PRs to the maintenance releases. They will then have to do it with slightly less performant code. And we add the new stuff for 20
| } | ||
|
|
||
| // If there are server wide customer certifcates use those | ||
| if ($this->certificateManager->listCertificates(null) !== []) { |
There was a problem hiding this comment.
$uid is be null and not '' -> is that intended?
There was a problem hiding this comment.
yeah it is the weird certificate store magic
There was a problem hiding this comment.
null requests the global cert list, '' uses the one of the current user 🙈
|
See also #21693 |
3 participants
If we try to list the certificates there is a difference between a user
and the system. If the user does not have customer certificates but the
server does. Then we should still fetch the customer server bundle
This slightly extends the ICertificateManager. But since it fixes a
perforamnce issue I'm kind of fine with it.
Signed-off-by: Roeland Jago Douma roeland@famdouma.nl