generated from biigle/module
-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add s3 disk validation step #22
Open
lehecht
wants to merge
25
commits into
main
Choose a base branch
from
add-s3-disk-validation-step
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
0d4851a
Add s3 config validation
lehecht d1c3dc6
Show error message if s3 config is invalid
lehecht 7857719
Move help texts to only highlight errors
lehecht 705a721
Rewrite code
lehecht 7bf5a70
Change error messages
lehecht 8ece2ff
Throw exception if s3 disk config is invalid
lehecht 8a0f2f3
Change code structure to increase code readability
lehecht 846cf59
Throw validation exeptions that uses array messages
lehecht 2deebc0
Fix transaction return statement
lehecht 4e1bc98
Refactor code to enable testing
lehecht a1e01e6
Add and update tests
lehecht 9b9bea2
Refactor code
lehecht 9c3afcf
Fix failing tests
lehecht 16ee9be
Minor changes
lehecht 2f4c758
Update test.yml
lehecht 5ed1d25
Change error message
lehecht bbcd9bb
Rename method and add php doc
lehecht af08c9a
Update tests
lehecht 05094c8
Remove redundant error handling
lehecht 22a8865
Fix indentation
lehecht 3b56180
Fix exception handling
lehecht f587e3a
Update tests
lehecht 0da95d3
Remove old return value
lehecht 788cf75
Add tests to check validation error message
lehecht 380c3e8
Add tests to check invalid bucket
lehecht File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,11 +2,16 @@ | |
|
||
namespace Biigle\Modules\UserDisks\Http\Controllers\Api; | ||
|
||
use Exception; | ||
use Illuminate\Support\Str; | ||
use Illuminate\Support\Facades\DB; | ||
use Biigle\Modules\UserDisks\UserDisk; | ||
use Illuminate\Support\Facades\Storage; | ||
use Biigle\Http\Controllers\Api\Controller; | ||
use Illuminate\Validation\ValidationException; | ||
use Biigle\Modules\UserDisks\Http\Requests\StoreUserDisk; | ||
use Biigle\Modules\UserDisks\Http\Requests\UpdateUserDisk; | ||
use Biigle\Modules\UserDisks\Http\Requests\ExtendUserDisk; | ||
use Biigle\Modules\UserDisks\UserDisk; | ||
use Biigle\Modules\UserDisks\Http\Requests\UpdateUserDisk; | ||
|
||
class UserDiskController extends Controller | ||
{ | ||
|
@@ -35,21 +40,33 @@ class UserDiskController extends Controller | |
*/ | ||
public function store(StoreUserDisk $request) | ||
{ | ||
$disk = UserDisk::create([ | ||
'name' => $request->input('name'), | ||
'type' => $request->input('type'), | ||
'user_id' => $request->user()->id, | ||
'expires_at' => now()->addMonths(config('user_disks.expires_months')), | ||
'options' => $request->getDiskOptions(), | ||
]); | ||
|
||
if ($this->isAutomatedRequest()) { | ||
return $disk; | ||
} | ||
try { | ||
$disk = DB::transaction(function () use ($request) { | ||
$disk = UserDisk::create([ | ||
'name' => $request->input('name'), | ||
'type' => $request->input('type'), | ||
'user_id' => $request->user()->id, | ||
'expires_at' => now()->addMonths(config('user_disks.expires_months')), | ||
'options' => $request->getDiskOptions(), | ||
]); | ||
|
||
$this->validateS3Config($disk); | ||
|
||
return $disk; | ||
}); | ||
|
||
return $this->fuzzyRedirect('storage-disks') | ||
->with('message', 'Storage disk created') | ||
->with('messageType', 'success'); | ||
if ($this->isAutomatedRequest()) { | ||
return $disk; | ||
} | ||
|
||
return $this->fuzzyRedirect('storage-disks') | ||
->with('message', 'Storage disk created') | ||
->with('messageType', 'success'); | ||
} catch (ValidationException $e) { | ||
return $this->fuzzyRedirect() | ||
->withErrors($e->errors()) | ||
->withInput(); | ||
} | ||
} | ||
|
||
/** | ||
|
@@ -77,18 +94,27 @@ public function store(StoreUserDisk $request) | |
*/ | ||
public function update(UpdateUserDisk $request) | ||
{ | ||
$request->disk->name = $request->input('name', $request->disk->name); | ||
$request->disk->options = array_merge( | ||
$request->disk->options, | ||
$request->getDiskOptions() | ||
); | ||
try { | ||
DB::transaction(function () use ($request) { | ||
$request->disk->name = $request->input('name', $request->disk->name); | ||
$request->disk->options = array_merge( | ||
$request->disk->options, | ||
$request->getDiskOptions() | ||
); | ||
|
||
$request->disk->save(); | ||
$request->disk->save(); | ||
$this->validateS3Config($request->disk); | ||
}); | ||
|
||
if (!$this->isAutomatedRequest()) { | ||
if (!$this->isAutomatedRequest()) { | ||
return $this->fuzzyRedirect() | ||
->with('message', 'Storage disk updated') | ||
->with('messageType', 'success'); | ||
} | ||
} catch (ValidationException $e) { | ||
return $this->fuzzyRedirect() | ||
->with('message', 'Storage disk updated') | ||
->with('messageType', 'success'); | ||
->withErrors($e->errors()) | ||
->withInput(); | ||
} | ||
Comment on lines
+114
to
118
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also no need for try/catch here. |
||
} | ||
|
||
|
@@ -142,4 +168,51 @@ public function destroy($id) | |
->with('messageType', 'success'); | ||
} | ||
} | ||
protected function validateS3Config(UserDisk $disk) | ||
mzur marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please still add a docblock here. |
||
{ | ||
if ($disk->type != 's3') { | ||
return; | ||
} | ||
|
||
$options = $disk->options; | ||
$endpoint = $options['endpoint']; | ||
$bucket = $options['bucket']; | ||
|
||
// Check if endpoint contains bucket name | ||
if (!preg_match("/(\b\/" . $bucket . "\.|\b" . $bucket . "\b)/", $endpoint)) { | ||
throw ValidationException::withMessages(['endpoint' => 'The endpoint URL must contain the bucket name. Please check if the name is present and spelled correctly.']); | ||
} | ||
|
||
try { | ||
$this->validateDiskAccess($disk); | ||
} catch (Exception $e) { | ||
$msg = $e->getMessage(); | ||
|
||
if (Str::contains($msg, 'timeout', true)) { | ||
throw ValidationException::withMessages(['error' => 'The endpoint URL could not be accessed. Does it exist?']); | ||
} elseif (Str::contains($msg, ['cURL error', 'Error parsing XML'], true)) { | ||
throw ValidationException::withMessages(['endpoint' => 'This does not seem to be a valid S3 endpoint.']); | ||
} elseif (Str::contains($msg, ["AccessDenied", "NoSuchBucket", "NoSuchKey"], true)) { | ||
throw ValidationException::withMessages(['error' => 'The bucket could not be accessed. Please check for typos or missing access permissions.']); | ||
} else { | ||
throw ValidationException::withMessages(['error' => 'An error occurred. Please check if your input is correct.']); | ||
} | ||
} | ||
return; | ||
} | ||
|
||
/** | ||
* Checks if the endpoint url is valid and the disk can be accessed | ||
* | ||
* @param mixed $disk Disk configured by the user that should be tested | ||
* @return void | ||
* @throws Exception If the disk cannot be accessed | ||
*/ | ||
protected function validateDiskAccess($disk) | ||
{ | ||
$disk = Storage::disk("disk-{$disk->id}"); | ||
$files = $disk->getAdapter()->listContents('', false); | ||
// Need to access an element to check if endpoint url is valid | ||
$files->current(); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
No need for the try/catch here. The ValidationException handles both browser and script requests, so there is no need to call
fuzzyRedirect()
. This is only needed in the success state (as it was before).