From f885a49a7ae75b3b727fef3e2e5c6ff034f9f32d Mon Sep 17 00:00:00 2001 From: Mathieu Date: Mon, 23 Oct 2023 21:39:00 +0200 Subject: [PATCH] fix: Request pre-signed url to upload picture via API --- .../040_Users/Avatar/070_S3Signature.robot | 138 ++++++++++++++++++ tests-qa/acceptance/ressources/vars/moves.yml | 13 ++ .../Pages/AbstractPictureUpload.php | 92 +++++++++++- .../app/GeoKrety/Controller/Pages/Base.php | 5 + .../Controller/Pages/GeokretAvatarUpload.php | 12 ++ .../Controller/Pages/MoveAvatarUpload.php | 12 ++ .../GeoKrety/Controller/Pages/MoveDelete.php | 9 +- .../Controller/Pages/Traits/MoveLoader.php | 8 +- .../Controller/Pages/UserAvatarUpload.php | 12 ++ website/app/GeoKrety/Service/Config.php | 1 + website/app/GeoKrety/Service/Xml/Generic.php | 46 ++++++ website/app/routes-api.ini | 7 + website/app/routes.ini | 6 +- 13 files changed, 347 insertions(+), 14 deletions(-) create mode 100644 tests-qa/acceptance/040_Users/Avatar/070_S3Signature.robot create mode 100644 website/app/GeoKrety/Service/Xml/Generic.php diff --git a/tests-qa/acceptance/040_Users/Avatar/070_S3Signature.robot b/tests-qa/acceptance/040_Users/Avatar/070_S3Signature.robot new file mode 100644 index 0000000000..7960df2239 --- /dev/null +++ b/tests-qa/acceptance/040_Users/Avatar/070_S3Signature.robot @@ -0,0 +1,138 @@ +*** Settings *** +Library RequestsLibrary +Library XML +Resource ../../ressources/vars/Urls.robot +Resource ../../ressources/Moves.robot +Variables ../../ressources/vars/users.yml +Variables ../../ressources/vars/moves.yml +Variables ../../ressources/vars/geokrety.yml +Test Setup Test Setup + +*** Variables *** + +@{ENDPOINTS_FOR_USER_1} /api/v1/geokrety/${GEOKRETY_1.ref}/avatar/request-s3-file-signature +... /api/v1/moves/${1}/avatar/request-s3-file-signature +... /api/v1/users/${USER_1.id}/avatar/request-s3-file-signature +... +@{ENDPOINTS_FOR_USER_2} /api/v1/geokrety/${GEOKRETY_2.ref}/avatar/request-s3-file-signature +... /api/v1/moves/${2}/avatar/request-s3-file-signature +... /api/v1/users/${USER_2.id}/avatar/request-s3-file-signature + +&{SECID_NOTHING} +&{SECID_EMPTY} secid= +&{SECID_USER_1} secid=${USER_1.secid} +&{SECID_USER_2} secid=${USER_2.secid} + + +*** Test Cases *** + +Valid Parameters + [Template] Post Request Valid + ${ENDPOINTS_FOR_USER_1}[0] ${SECID_USER_1} + ${ENDPOINTS_FOR_USER_1}[1] ${SECID_USER_1} + ${ENDPOINTS_FOR_USER_1}[2] ${SECID_USER_1} + + ${ENDPOINTS_FOR_USER_2}[0] ${SECID_USER_2} + ${ENDPOINTS_FOR_USER_2}[1] ${SECID_USER_2} + ${ENDPOINTS_FOR_USER_2}[2] ${SECID_USER_2} + + +Required Parameters + [Template] Post Return Error + ${ENDPOINTS_FOR_USER_1}[0] ${SECID_NOTHING} 200 Invalid "secid" length + ${ENDPOINTS_FOR_USER_1}[1] ${SECID_NOTHING} 200 Invalid "secid" length + ${ENDPOINTS_FOR_USER_1}[2] ${SECID_NOTHING} 200 Invalid "secid" length + + ${ENDPOINTS_FOR_USER_1}[0] ${SECID_EMPTY} 200 Invalid "secid" length + ${ENDPOINTS_FOR_USER_1}[1] ${SECID_EMPTY} 200 Invalid "secid" length + ${ENDPOINTS_FOR_USER_1}[2] ${SECID_EMPTY} 200 Invalid "secid" length + + ${ENDPOINTS_FOR_USER_1}[0] ${SECID_USER_2} 403 You are not the GeoKret owner + ${ENDPOINTS_FOR_USER_1}[1] ${SECID_USER_2} 403 You have no write permission on this move + ${ENDPOINTS_FOR_USER_1}[2] ${SECID_USER_2} 403 This is not your profile + + ${ENDPOINTS_FOR_USER_2}[0] ${SECID_USER_1} 403 You are not the GeoKret owner + ${ENDPOINTS_FOR_USER_2}[1] ${SECID_USER_1} 403 You have no write permission on this move + ${ENDPOINTS_FOR_USER_2}[2] ${SECID_USER_1} 403 This is not your profile + +*** Keywords *** + +Test Setup + Clear Database And Seed ${2} users + Seed ${1} geokrety owned by ${1} + Seed ${1} geokrety owned by ${2} + Post move ${MOVE_1} + Post move ${MOVE_41} + + +Check Error In XML List + [Arguments] ${root} ${error} + ${value} = Get Elements ${root} xpath=.//error[.='${error}'] + Length Should Be ${value} 1 + +Response Should be XML Error + [Arguments] ${root} @{errors} + Should Be Equal ${root.tag} gkxml + + ${errors_count} = Get Length ${errors} + ${count} = XML.Get Element Count ${root} errors/error + Should Be Equal As Numbers ${count} ${errors_count} + + FOR ${error} IN @{errors} + Check Error In XML List ${root} ${error} + END + +Post Return Error + [Arguments] ${url} ${data} ${code} @{errors} + Create Session geokrety ${GK_URL} + ${auth} = GET On Session geokrety url=/ + + ${resp} = POST On Session geokrety url=${url} data=${data} expected_status=${code} + Should Not Be Empty ${resp.content} + + ${root} = Parse XML ${resp.content} + Response Should be XML Error ${root} @{errors} + + +Response Should be XML Valid + [Arguments] ${root} + Should Be Equal ${root.tag} gkxml + + + ${count} = XML.Get Element Count ${root} image-upload + Should Be Equal As Numbers ${count} ${1} + + ${count} = XML.Get Element Count ${root} image-upload/success + Should Be Equal As Numbers ${count} ${1} + ${count} = XML.Get Element Count ${root} image-upload/uploadUrl + Should Be Equal As Numbers ${count} ${1} + ${count} = XML.Get Element Count ${root} image-upload/s3Key + Should Be Equal As Numbers ${count} ${1} + ${count} = XML.Get Element Count ${root} image-upload/headers + Should Be Equal As Numbers ${count} ${1} + ${count} = XML.Get Element Count ${root} image-upload/headers/key + Should Be Equal As Numbers ${count} ${1} + ${count} = XML.Get Element Count ${root} image-upload/headers/X-Amz-Credential + Should Be Equal As Numbers ${count} ${1} + ${count} = XML.Get Element Count ${root} image-upload/headers/X-Amz-Algorithm + Should Be Equal As Numbers ${count} ${1} + ${count} = XML.Get Element Count ${root} image-upload/headers/X-Amz-Date + Should Be Equal As Numbers ${count} ${1} + ${count} = XML.Get Element Count ${root} image-upload/headers/Policy + Should Be Equal As Numbers ${count} ${1} + ${count} = XML.Get Element Count ${root} image-upload/headers/X-Amz-Signature + Should Be Equal As Numbers ${count} ${1} + + +Post Request Valid + [Arguments] ${url} ${data} + Create Session geokrety ${GK_URL} + ${auth} = GET On Session geokrety url=/ + + ${resp} = POST On Session geokrety url=${url} data=${data} + Should Not Be Empty ${resp.content} + + ${root} = Parse XML ${resp.content} + Response Should be XML Valid ${root} + + Delete All Sessions diff --git a/tests-qa/acceptance/ressources/vars/moves.yml b/tests-qa/acceptance/ressources/vars/moves.yml index bed06960cf..7de7a614ca 100644 --- a/tests-qa/acceptance/ressources/vars/moves.yml +++ b/tests-qa/acceptance/ressources/vars/moves.yml @@ -232,3 +232,16 @@ MOVE_36: comment: Hello app: robotframework app_ver: 3.2.1 + +### Moves by Author 2 For GeoKret 2 +# DROP +MOVE_41: + gkid: 2 + move_type: 0 + author: 2 + waypoint: GC0001 + lat: "43.60000" + lon: "7.00000" + comment: Hello + app: robotframework + app_ver: 3.2.1 diff --git a/website/app/GeoKrety/Controller/Pages/AbstractPictureUpload.php b/website/app/GeoKrety/Controller/Pages/AbstractPictureUpload.php index 381460939b..93f23c164c 100644 --- a/website/app/GeoKrety/Controller/Pages/AbstractPictureUpload.php +++ b/website/app/GeoKrety/Controller/Pages/AbstractPictureUpload.php @@ -4,14 +4,83 @@ use Aws\S3\PostObjectV4; use GeoKrety\Model\Picture; +use GeoKrety\Service\RateLimit; use GeoKrety\Service\S3Client; +use GeoKrety\Service\Xml\Error; +use GeoKrety\Service\Xml\Generic; use Sugar\Event; +class UploadPermissionException extends \Exception { +} + abstract class AbstractPictureUpload extends Base { private string $imgKey; - public function request_s3_file_signature(\Base $f3) { - header('Content-Type: application/json; charset=utf-8'); + public const CONTENT_TYPE_JSON = 'Content-Type: application/json; charset=utf-8'; + public const CONTENT_TYPE_XML = 'Content-Type: application/xml; charset=utf-8'; + + private function wanted_response_content_type(\Base $f3) { + // Requests having secid are known to be XML + if ($f3->exists('REQUEST.secid')) { + return self::CONTENT_TYPE_XML; + } + + // Default client is dropzone and is talking json + return self::CONTENT_TYPE_JSON; + } + + private function set_response_content_type(\Base $f3) { + header($this->wanted_response_content_type($f3)); + } + + private function render_response(\Base $f3, array $response) { + if ($this->wanted_response_content_type($f3) === self::CONTENT_TYPE_XML) { + return Generic::buildGeneric(true, 'image-upload', $response); + } + // Default is Json + return json_encode($response); + } + + /** + * Request a presigned url from the xml api + * Authentication is done by using a secid. + * + * @return void + */ + public function request_s3_file_signature_api(\Base $f3) { + $this->set_response_content_type($f3); + $this->authenticate_via_secid($f3); + RateLimit::check_rate_limit_raw('API_V1_REQUEST_S3_FILE_SIGNATURE', $this->f3->get('REQUEST.secid')); + $data = $this->request_s3_file_signature($f3); + // Remove some dropzone internal headers on the public API + foreach (['success', 'uploadUrl', 's3Key'] as $key) { + $response[$key] = $data[$key]; + unset($data[$key]); + } + $response = array_merge($response, ['headers' => $data]); + echo $this->render_response($f3, $response); + } + + /** + * Request a presigned url from dropzone + * Authentication is done by the framework. + * + * @return void + */ + public function request_s3_file_signature_ajax(\Base $f3) { + $this->set_response_content_type($f3); + $response = $this->request_s3_file_signature($f3); + echo $this->render_response($f3, $response); + } + + private function request_s3_file_signature(\Base $f3) { + try { + $this->check_permission($f3); + } catch (UploadPermissionException $e) { + http_response_code(403); + Error::buildError(true, [$e->getMessage()]); + exit; + } $s3 = S3Client::instance()->getS3Public(); @@ -55,8 +124,8 @@ public function request_s3_file_signature(\Base $f3) { 'success' => 0, 'text' => $f3->get('validation.error'), ]; - echo json_encode($response); - exit; + + return $response; } try { @@ -68,13 +137,13 @@ public function request_s3_file_signature(\Base $f3) { 'success' => 0, 'text' => 'Failed to store upload url into database.', ]; - echo json_encode($response); - exit; + + return $response; } Event::instance()->emit(sprintf('%s.presigned_request', $this->getEventNameBase()), $picture, $response); - echo json_encode($response); + return $response; } public function getImgKey() { @@ -116,4 +185,13 @@ abstract public function getEventNameBase(): string; abstract public function getPictureType(): int; abstract public function setRelationships(Picture $picture): void; + + /** + * Check if the current user has permission on this object. + * + * @return void + * + * @throws UploadPermissionException + */ + abstract protected function check_permission(\Base $f3); } diff --git a/website/app/GeoKrety/Controller/Pages/Base.php b/website/app/GeoKrety/Controller/Pages/Base.php index 1f6273632e..5a04bc8eee 100644 --- a/website/app/GeoKrety/Controller/Pages/Base.php +++ b/website/app/GeoKrety/Controller/Pages/Base.php @@ -63,6 +63,11 @@ public function loadCurrentUser() { } } + public function authenticate_via_secid(\Base $f3) { + $login = new Login(); + $user = $login->secidAuth($this->f3, $this->f3->get('REQUEST.secid')); + } + public function isLoggedIn(): bool { return !is_null($this->current_user); } diff --git a/website/app/GeoKrety/Controller/Pages/GeokretAvatarUpload.php b/website/app/GeoKrety/Controller/Pages/GeokretAvatarUpload.php index 44e583f16d..9990fcf505 100644 --- a/website/app/GeoKrety/Controller/Pages/GeokretAvatarUpload.php +++ b/website/app/GeoKrety/Controller/Pages/GeokretAvatarUpload.php @@ -32,4 +32,16 @@ public function getEventNameBase(): string { public function setRelationships(Picture $picture): void { $picture->geokret = $this->geokret->id; } + + /** + * Check if the current user has permission on this object. + * + * @throws UploadPermissionException + */ + protected function check_permission(\Base $f3): void { + if ($this->geokret->isOwner()) { + return; + } + throw new UploadPermissionException(_('You are not the GeoKret owner')); + } } diff --git a/website/app/GeoKrety/Controller/Pages/MoveAvatarUpload.php b/website/app/GeoKrety/Controller/Pages/MoveAvatarUpload.php index eb35b7e3f2..49cc245f8f 100644 --- a/website/app/GeoKrety/Controller/Pages/MoveAvatarUpload.php +++ b/website/app/GeoKrety/Controller/Pages/MoveAvatarUpload.php @@ -31,4 +31,16 @@ public function getEventNameBase(): string { public function setRelationships(Picture $picture): void { $picture->move = $this->move; } + + /** + * Check if the current user has permission on this object. + * + * @throws UploadPermissionException + */ + protected function check_permission(\Base $f3): void { + if ($this->hasWritePermission($this->move)) { + return; + } + throw new UploadPermissionException(_('You have no write permission on this move')); + } } diff --git a/website/app/GeoKrety/Controller/Pages/MoveDelete.php b/website/app/GeoKrety/Controller/Pages/MoveDelete.php index c7370b709c..6ebcb5a1f7 100644 --- a/website/app/GeoKrety/Controller/Pages/MoveDelete.php +++ b/website/app/GeoKrety/Controller/Pages/MoveDelete.php @@ -6,7 +6,14 @@ use Sugar\Event; class MoveDelete extends Base { - use \MoveLoader; + use \MoveLoader { + beforeRoute as protected traitBeforeRoute; + } + + public function beforeRoute(\Base $f3) { + $this->traitBeforeRoute($f3); + $this->checkAuthor($this->move); + } public function get(\Base $f3) { Smarty::render('extends:full_screen_modal.tpl|dialog/move_delete.tpl'); diff --git a/website/app/GeoKrety/Controller/Pages/Traits/MoveLoader.php b/website/app/GeoKrety/Controller/Pages/Traits/MoveLoader.php index 3b77ddbaa3..39c7ba68b2 100644 --- a/website/app/GeoKrety/Controller/Pages/Traits/MoveLoader.php +++ b/website/app/GeoKrety/Controller/Pages/Traits/MoveLoader.php @@ -22,13 +22,15 @@ public function beforeRoute($f3) { $f3->error(404, _('This move does not exist.')); } - $this->checkAuthor($move); - Smarty::assign('move', $this->move); } + protected function hasWritePermission(Move $move) { + return $move->isAuthor() || $move->geokret->isOwner(); + } + protected function checkAuthor(Move $move) { - if (!($move->isAuthor() || $move->geokret->isOwner())) { + if (!$this->hasWritePermission($move)) { \Base::instance()->error(403, _('You are not allowed to edit this move.')); } } diff --git a/website/app/GeoKrety/Controller/Pages/UserAvatarUpload.php b/website/app/GeoKrety/Controller/Pages/UserAvatarUpload.php index 55bf53e35e..292ddb63ba 100644 --- a/website/app/GeoKrety/Controller/Pages/UserAvatarUpload.php +++ b/website/app/GeoKrety/Controller/Pages/UserAvatarUpload.php @@ -31,4 +31,16 @@ public function getEventNameBase(): string { public function setRelationships(Picture $picture): void { $picture->user = $this->user; } + + /** + * Check if the current user has permission on this object. + * + * @throws UploadPermissionException + */ + protected function check_permission(\Base $f3): void { + if ($this->user->isCurrentUser()) { + return; + } + throw new UploadPermissionException(_('This is not your profile')); + } } diff --git a/website/app/GeoKrety/Service/Config.php b/website/app/GeoKrety/Service/Config.php index a7604bf89e..968892a8a9 100644 --- a/website/app/GeoKrety/Service/Config.php +++ b/website/app/GeoKrety/Service/Config.php @@ -374,6 +374,7 @@ public function __construct() { 'API_V1_EXPORT2' => [1500, 60 * 60 * 24], // 1500/day 'API_V1_EXPORT' => [12, 60], // 12/minute 'API_V1_EXPORT_OC' => [12, 60], // 12/minute + 'API_V1_REQUEST_S3_FILE_SIGNATURE' => [50, 60 * 60 * 24], // 50/day 'API_GKT_V3_SEARCH' => [10000, 60 * 60 * 24], // 10000/day 'API_GKT_V3_INVENTORY' => [1500, 60 * 60 * 24], // 1500/day 'USERNAME_CHANGE' => [3, 60 * 60 * 24 * 28], // 3/month diff --git a/website/app/GeoKrety/Service/Xml/Generic.php b/website/app/GeoKrety/Service/Xml/Generic.php new file mode 100644 index 0000000000..c12bdab718 --- /dev/null +++ b/website/app/GeoKrety/Service/Xml/Generic.php @@ -0,0 +1,46 @@ +xml->startElement($root); + } + + public function end() { + $this->xml->endElement(); + parent::end(); + } + + public function addItem($key, $value) { + $this->xml->startElement($key); + $this->xml->writeCdata($value); + $this->xml->endElement(); + } + + /** + * Create xml response from an array of errors. + * + * @param bool $stream Prepare to render as xml output + * @param string $root The XML root element name + */ + public static function buildGeneric(bool $stream, string $root, array|string $data) { + $data = gettype($data) === 'string' ? [$data] : $data; + $xml = new \GeoKrety\Service\Xml\Generic($root, $stream); + + foreach ($data as $key => $value) { + if (gettype($value) === 'array') { + $xml->xml->startElement($key); + foreach ($value as $k => $v) { + $xml->addItem($k, $v); + } + $xml->xml->endElement(); + } else { + $xml->addItem($key, $value); + } + } + $xml->end(); + $xml->finish(); // may return raw gzipped data + } +} diff --git a/website/app/routes-api.ini b/website/app/routes-api.ini index eec5ad538f..6100901c96 100644 --- a/website/app/routes-api.ini +++ b/website/app/routes-api.ini @@ -13,6 +13,9 @@ GET @api_v1_export_oc: /api/v1/export_oc = \GeoKrety\Controller\ExportOCXML->get GET @api_v1_geokret_stats_altitude_profile: /api/v1/geokrety/@gkid/statistics/altitude-profile = \GeoKrety\Controller\API\v1\Statistics->altitude_profile +POST @api_v1_geokret_avatar_upload_get_s3_signature: /api/v1/geokrety/@gkid/avatar/request-s3-file-signature = \GeoKrety\Controller\GeokretAvatarUpload->request_s3_file_signature_api +POST @api_v1_user_avatar_upload_get_s3_signature: /api/v1/users/@userid/avatar/request-s3-file-signature = \GeoKrety\Controller\UserAvatarUpload->request_s3_file_signature_api +POST @api_v1_move_picture_upload_get_s3_signature: /api/v1/moves/@moveid/avatar/request-s3-file-signature = \GeoKrety\Controller\MoveAvatarUpload->request_s3_file_signature_api [ACCESS.rules] @@ -27,3 +30,7 @@ allow @api_v1_geokret_stats_altitude_profile = * allow @gkt_v3_search = * allow @gkt_v3_inventory = * + +allow @api_v1_geokret_avatar_upload_get_s3_signature = * +allow @api_v1_user_avatar_upload_get_s3_signature = * +allow @api_v1_move_picture_upload_get_s3_signature = * diff --git a/website/app/routes.ini b/website/app/routes.ini index b01864ffe6..5a0de3fa5b 100644 --- a/website/app/routes.ini +++ b/website/app/routes.ini @@ -155,13 +155,13 @@ GET @picture_proxy: /pictures/@key/proxy = \GeoKrety\Controller\PictureProxy->ge GET @picture_proxy_thumbnail: /pictures/@key/proxy/thumbnail = \GeoKrety\Controller\PictureProxy->get_thumbnail ; GeoKret Avatars File upload -POST @geokret_avatar_upload_get_s3_signature: /geokrety/@gkid/avatar/request-s3-file-signature = \GeoKrety\Controller\GeokretAvatarUpload->request_s3_file_signature +POST @geokret_avatar_upload_get_s3_signature: /geokrety/@gkid/avatar/request-s3-file-signature = \GeoKrety\Controller\GeokretAvatarUpload->request_s3_file_signature_ajax ; Users Avatar File upload -POST @user_avatar_upload_get_s3_signature: /users/@userid/avatar/request-s3-file-signature = \GeoKrety\Controller\UserAvatarUpload->request_s3_file_signature +POST @user_avatar_upload_get_s3_signature: /users/@userid/avatar/request-s3-file-signature = \GeoKrety\Controller\UserAvatarUpload->request_s3_file_signature_ajax ; Users Avatar File upload -POST @move_picture_upload_get_s3_signature: /moves/@moveid/avatar/request-s3-file-signature = \GeoKrety\Controller\MoveAvatarUpload->request_s3_file_signature +POST @move_picture_upload_get_s3_signature: /moves/@moveid/avatar/request-s3-file-signature = \GeoKrety\Controller\MoveAvatarUpload->request_s3_file_signature_ajax GET @picture_edit: /pictures/@key/edit [sync] = \GeoKrety\Controller\PictureEdit->get GET @picture_edit: /pictures/@key/edit [ajax] = \GeoKrety\Controller\PictureEdit->get_ajax