Skip to content
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

1.3.0 release broke role based s3 authentication #23

Closed
tractorcow opened this issue Mar 24, 2022 · 4 comments · Fixed by #25
Closed

1.3.0 release broke role based s3 authentication #23

tractorcow opened this issue Mar 24, 2022 · 4 comments · Fixed by #25

Comments

@tractorcow
Copy link
Contributor

tractorcow commented Mar 24, 2022

695f952#diff-d381368b790cbf818204c612d2985bfbac9f3bffd3466c426ff1ffb7f02e9ae2R56-R63

Any application that uses role-based authentication would have worked prior to this change. The new version of this module discards the existing s3 client connected to the filesystem adapter, and constructs a new one, assuming key + secret authentication instead.

When we updated to 1.3 this of course immediately broke. :)

Could the current code work the same if we restore this line?

$this->s3Client = Storage::disk($this->tool->disk)->getDriver()->getAdapter()->getClient();
@ahmedkandel
Copy link
Owner

Laravel 9.x has migrated from Flysystem 1.x to 3.x so the Storage::disk($this->tool->disk)->getDriver()->getAdapter()->getClient() which works for Laravel 8.x is not available anymore starting from Laravel 9.x and will be Storage::disk($this->tool->disk)->getClient() here.

In order to support Laravel 8.x and 9.x we found it is more convenient to decouple from Laravel storage and instantiate S3Client using the disk config.

@ahmedkandel
Copy link
Owner

To solve your problem either you get the client from Laravel filesystem depending on the Laravel version or to use the S3Client directly but with optional credentials as discussed here.

PRs are welcome.

@tractorcow
Copy link
Contributor Author

Thanks for the explanation. It makes sense to me. I'll see about writing up a PR to address.

Is likely we could still manually construct the client the way you are now, but tweak the credentials.

tractorcow added a commit to tractorcow/nova-s3-multipart-upload that referenced this issue Mar 28, 2022
tractorcow added a commit to tractorcow/nova-s3-multipart-upload that referenced this issue Mar 28, 2022
@tractorcow
Copy link
Contributor Author

Please see #25. Hope you find it helpful :)

ahmedkandel pushed a commit that referenced this issue Apr 3, 2022
…ials (#25)

* feat: containerise s3 client generation, support other client credentials

fixes #23

* fix: shift dependency registration from boot to register
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants