Harden user_status API #49797
Harden user_status API #49797
Conversation
Signed-off-by: provokateurin <[email protected]>
Signed-off-by: provokateurin <[email protected]>
…user status Signed-off-by: provokateurin <[email protected]>
…f nullable Signed-off-by: provokateurin <[email protected]>
provokateurin
requested review from
nickvergessen,
a team,
ArtificialOwl,
skjnldsv and
come-nc
and removed request for
a team
| @@ -46,7 +46,7 @@ public function __construct( | |||
| * Find statuses of users | |||
| * | |||
| * @param int|null $limit Maximum number of statuses to find | |||
| * @param int|null $offset Offset for finding statuses | |||
| * @param non-negative-int|null $offset Offset for finding statuses | |||
There was a problem hiding this comment.
I’m curious, I guess a lot of places in the API expects a positive int and type it as int, why change this one in particular?
There was a problem hiding this comment.
I found this one because the fuzzer used -1 which triggered an unexpected error.
I didn't search for these problems, but the underlying typing could be improved to find more of them.
| @@ -36,7 +36,7 @@ class UserStatusController extends OCSController { | |||
| public function __construct( | |||
| string $appName, | |||
| IRequest $request, | |||
| private string $userId, | |||
| private ?string $userId, | |||
There was a problem hiding this comment.
Is there any chance to explain psalm and friends that such a property cannot be null when calling an authenticated method?
There was a problem hiding this comment.
I don't think that is possible and even then we'd still have a problem because the DI will fail when you don't send the authentication (regardless if the method needs it or not).
There was a problem hiding this comment.
Yeah we have this error regularly in our logs from various apps.
I'm just always drawn apart how to handle it as allowing null in the constructor basically means you have to handle it manually in each method or have psalm complaining there
There was a problem hiding this comment.
I think we could be handling it more gracefully and letting controllers specify it as non-null if they know it has to be there for every method. Currently if the DI fails weird things happen, for example for a DELETE request it just returns 200 (which is how I found this with the fuzzer).
| @@ -22,7 +22,7 @@ | |||
| * icon: string, | |||
| * message: string, | |||
| * clearAt: ?UserStatusClearAt, | |||
| * visible: ?bool, | |||
| * visible?: bool, | |||
There was a problem hiding this comment.
I want to check this on Monday, so please don't merge before
Summary
Issues found with #47825
Checklist