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.

Sign up for GitHub

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

Jump to bottom

Feat/thumbor bucket in url #521

Closed
wants to merge 8 commits into from
Closed

Feat/thumbor bucket in url #521

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f135101
feat(thumbor-urls): add logic to parse bucket from path
bennet-esyoil Nov 30, 2023
2589355
feat: add tests
bennet-esyoil Nov 30, 2023
7aed95c
fix: replace bucket-name in url if present
bennet-esyoil Dec 1, 2023
57137dd
fix: also replace key
bennet-esyoil Dec 1, 2023
7fad979
fix: well
bennet-esyoil Dec 1, 2023
bf21c19
chore(tests): added test to verify that correct bucket gets selected …
bennet-esyoil Dec 11, 2023
52038a1
fix: typo in test title
bennet-esyoil Dec 11, 2023
eabd936
feat: switch to s3:bucket syntax
bennetgallein Dec 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions source/image-handler/image-request.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ export class ImageRequest {

imageRequestInfo.requestType = this.parseRequestType(event);
imageRequestInfo.bucket = this.parseImageBucket(event, imageRequestInfo.requestType);
imageRequestInfo.key = this.parseImageKey(event, imageRequestInfo.requestType);
imageRequestInfo.key = this.parseImageKey(event, imageRequestInfo.requestType, imageRequestInfo.bucket);
imageRequestInfo.edits = this.parseImageEdits(event, imageRequestInfo.requestType);

const originalImage = await this.getOriginalImage(imageRequestInfo.bucket, imageRequestInfo.key);
Expand Down Expand Up @@ -223,6 +223,13 @@ export class ImageRequest {
} else if (requestType === RequestTypes.THUMBOR || requestType === RequestTypes.CUSTOM) {
// Use the default image source bucket env var
const sourceBuckets = this.getAllowedSourceBuckets();
// Take the path and split it at "/" to get each "word" in the url as array
let potentialBucket = event.path.split("/");
// filter out all parts that are not an bucket-url
potentialBucket = potentialBucket.filter(e => sourceBuckets.includes(e));
// return the first match
if (potentialBucket.length > 0) return potentialBucket[0];

return sourceBuckets[0];
} else {
throw new ImageHandlerError(
Expand Down Expand Up @@ -265,7 +272,7 @@ export class ImageRequest {
* @param requestType Type of the request.
* @returns The name of the appropriate Amazon S3 key.
*/
public parseImageKey(event: ImageHandlerEvent, requestType: RequestTypes): string {
public parseImageKey(event: ImageHandlerEvent, requestType: RequestTypes, bucket: string = null): string {
if (requestType === RequestTypes.DEFAULT) {
// Decode the image request and return the image key
const { key } = this.decodeRequest(event);
Expand Down Expand Up @@ -299,6 +306,7 @@ export class ImageRequest {
.replace(/\/fit-in(?=\/)/g, "")
.replace(/^\/+/g, "")
.replace(/^\/+/, "")
.replace(new RegExp(bucket + "\/"), '')
);
}

Expand Down
26 changes: 26 additions & 0 deletions source/image-handler/test/image-request/parse-image-bucket.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,4 +126,30 @@ describe("parseImageBucket", () => {
});
}
});

it("should parse bucket-name from first part in thubor request but fail since it's not allowed", () => {
Copy link

@t4colingough t4colingough Dec 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thumbor is spelled incorrectly in the above string and in the next test.

Does this test cover when no bucket is provided? For instance, in my use case I do not want to specify a bucket name in the URL.

Copy link
Author

@bennet-esyoil bennet-esyoil Dec 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've found some unrelated cases where the tests did not cover, thus my "akward" commits after creating the PR.

the only problem I currently see with my approach is that if you have a path that contains the bucket-name which you do not want to use, this would fail.

e.g. you have a bucket bucket-1 and want to request the file /bucket-1/test.jpg, from that bucket, the logic would remove the bucket-1 since it's a name. I was thinking about prefixing the parameter with something like bucket:bucket-1/bucket-1/test.jpg which would resolve that issue.

Does this test cover when no bucket is provided? For instance, in my use case I do not want to specify a bucket name in the URL.

If you dont, behaviour is the same as before (takes the first bucket provided in the env-variables)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok thanks for the reply.

Yes, I agree with using a keyword like "bucket:"/ "s3:" similar to the "filters:" in the URL. That would work around the issue and hopefully not cause an issue with the path/bucket name

Example

    const event = { path: "/filters:grayscale()/bucket:test-bucket/test-image-001.jpg" };
    const event = { path: "/filters:grayscale()/s3:test-bucket/test-image-001.jpg" };

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the s3:name looks pretty sleek, kinda ressembles the normal s3://bucket/object syntax. I'll get this changed.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kinda don't like how it's not part of the ThumborMapper class, but it's kinda not part of thumbor itself and also needs to happen at an earlier stage, so it makes sense to have it where it is now..

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @t4colingough any update?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello again @t4colingough & @dougtoppin ,

any chance to get this released any time soon?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @bennet-esyoil,

This change has been made internally and is currently set to be included in the next minor/major release. While our internal processes had us merge the code manually, once the release with this change is out, you'll see yourself included in the external contributors section towards the bottom of the readme and this PR will be closed.

Thanks for your contributions,
Simon

// Arrange
const event = { path: "/filters:grayscale()/test-bucket/test-image-001.jpg" };
process.env.SOURCE_BUCKETS = "allowedBucket001, allowedBucket002";

// Act
const imageRequest = new ImageRequest(s3Client, secretProvider);

const bucket = imageRequest.parseImageBucket(event, RequestTypes.THUMBOR);
// Assert
expect(bucket).toEqual("allowedBucket001")
})

it("should parse bucket-name from first part in thubor request and return it", () => {
// Arrange
const event = { path: "/filters:grayscale()/test-bucket/test-image-001.jpg" };
process.env.SOURCE_BUCKETS = "allowedBucket001, test-bucket";

// Act
const imageRequest = new ImageRequest(s3Client, secretProvider);

const bucket = imageRequest.parseImageBucket(event, RequestTypes.THUMBOR);
// Assert
expect(bucket).toEqual("test-bucket")
})
});