-
Notifications
You must be signed in to change notification settings - Fork 89
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
Fix image serving in direct editing #2059
Conversation
$imageFile = $this->imageService->getImage($documentId, $imageFileName, $this->userId); | ||
$session = $this->sessionService->getSession($documentId, $sessionId, $sessionToken); | ||
$userId = $session->getUserId(); | ||
$imageFile = $this->imageService->getImage($documentId, $imageFileName, $userId); |
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.
Mh, $userId
seems to be needed in each public function of the controller. It gets injected into the constructor by DI, but in all functions but this one (getImage()
), it's fetched again using $session->getUiserId()
. So does this mean that $userId
as injected by DI is not reliable? Then probably $this->userId
should be removed from the class altogether?
Maybe a function like the following could work?
private function getUserId(int $documentId, int $sessionId, string $sessionToken): string {
if ($this->userId === null) {
$this->userId = $this->sessionService->getSession($documentId, $sessionId, $sessionToken)->getUserId();
}
return $this->userId;
}
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.
It's the same in all methods of ImageController
. Whether the user is authenticated or not, we always choose to rely on the edition session rather than the "classic" authentication. This way it works with the mobile clients which are making unauthenticated requests but our UI passes the edition session ID and token.
This was suggested by @juliushaertl in #1900 (comment)
The unused $userId
attribute has been removed.
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.
Looks good to me now 😊 You could still move the logic to get $userId
into a dedicated private function to lower code-duplication, but that's really just nitpicking 😉
51c3365
to
99c58ed
Compare
$imageFile = $this->imageService->getImage($documentId, $imageFileName, $this->userId); | ||
$session = $this->sessionService->getSession($documentId, $sessionId, $sessionToken); | ||
$userId = $session->getUserId(); | ||
$imageFile = $this->imageService->getImage($documentId, $imageFileName, $userId); |
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.
Looks good to me now 😊 You could still move the logic to get $userId
into a dedicated private function to lower code-duplication, but that's really just nitpicking 😉
…hareToken, use the edition session to get the user ID Signed-off-by: Julien Veyssier <eneiluj@posteo.net>
99c58ed
to
466a6b5
Compare
@mejo- I completely agree. Factorization done. Rebased on master |
Cypress failure is unrelated. It's the "share" test. |
This fixes 2 mistakes:
Image serving then works in our mobile clients (tested NC Android app).
refs #1900