-
Notifications
You must be signed in to change notification settings - Fork 109
Fix image serving in direct editing #2059
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
| $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 <[email protected]>
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