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

Adds Backblaze B2 proxy function for Cloudflare workers #11

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

lukestanley
Copy link
Member

@lukestanley lukestanley commented Dec 10, 2024

It made sense to have a service specific proxy for the S3 like API, Backblaze B2 to share all (audio files) from a file bucket in a way that is transparent to the end user.
The most simple way to deploy seemed to be a Cloudflare worker.

@lukestanley lukestanley marked this pull request as ready for review December 10, 2024 10:39
@lukestanley
Copy link
Member Author

lukestanley commented Dec 10, 2024

I have tested this on a Cloudflare worker in my name, and need to test it on a more central Kendraio organisation worker.
Then I need to use it for Fireflies meetings.

Copy link
Contributor

@CodeKrakken CodeKrakken left a comment

Choose a reason for hiding this comment

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

Just one small point - otherwise very nice work!

console.log('Download authorisation successful');

const signedUrlData = await downloadAuthorisation.json();
const downloadUrl = `${authData.downloadUrl}/file/${B2_BUCKET_NAME}/${fileName}?Authorization=${signedUrlData.authorizationToken}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks a bit untidy - can you split it onto multiple lines more gracefully?

Copy link
Contributor

Choose a reason for hiding this comment

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

Mh, I don't fully agree @CodeKrakken
I think that is good to have it in one line as it is 1 URL.
To me, the naming is clear enough to make it readable, and I would probably feel a bit more lost if they started to be in separate lines or even more split.
Anyway, just because is clear to me doesn't mean it is for everybody.
Can you make a example of how it would be written if it were on multiple lines @CodeKrakken ?

I am thinking maybe also something like that can be helpful for you:

const b2AuthUrl = authData.downloadUrl;
const authorizationParam =  `Authorization=${signedUrlData.authorizationToken}`;
const downloadUrl = `${b2AuthUrl}/file/${B2_BUCKET_NAME}/${fileName}? authorizationParam`;

What do you all think?

Copy link
Contributor

@gsambrotta gsambrotta left a comment

Choose a reason for hiding this comment

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

Great job @lukestanley !
Just some minor styling comments and a bit more clarity in regard of Cloudflare.
Thank you 🙏


const url = new URL(request.url);

// Return hello world if no path specified
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seems very helpful to me.
Can we not return random text which can result in confusion?
I would suggest to either return an error or a message like "please specify path" (if we want people to have hints)

console.log(`Using bucket: ${B2_BUCKET_NAME} (${B2_BUCKET_ID})`);

// Step 1: Authorise with Backblaze B2
const authResponse = await fetch('https://api.backblazeb2.com/b2api/v2/b2_authorize_account', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please wrap the fetch() in a try/catch block?

const apiUrl = authData.apiUrl;

// Step 2: Get Download Authorisation
const downloadAuthorisation = await fetch(`${apiUrl}/b2api/v2/b2_get_download_authorization`, {
Copy link
Contributor

Choose a reason for hiding this comment

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

same here. A try/catch block pls

console.log('Download authorisation successful');

const signedUrlData = await downloadAuthorisation.json();
const downloadUrl = `${authData.downloadUrl}/file/${B2_BUCKET_NAME}/${fileName}?Authorization=${signedUrlData.authorizationToken}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Mh, I don't fully agree @CodeKrakken
I think that is good to have it in one line as it is 1 URL.
To me, the naming is clear enough to make it readable, and I would probably feel a bit more lost if they started to be in separate lines or even more split.
Anyway, just because is clear to me doesn't mean it is for everybody.
Can you make a example of how it would be written if it were on multiple lines @CodeKrakken ?

I am thinking maybe also something like that can be helpful for you:

const b2AuthUrl = authData.downloadUrl;
const authorizationParam =  `Authorization=${signedUrlData.authorizationToken}`;
const downloadUrl = `${b2AuthUrl}/file/${B2_BUCKET_NAME}/${fileName}? authorizationParam`;

What do you all think?


// Step 3: Fetch the content from signed URL
console.log(`Fetching content from signed URL for: ${fileName}`);
const fileResponse = await fetch(downloadUrl);
Copy link
Contributor

Choose a reason for hiding this comment

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

and try/catch again :)

@@ -0,0 +1,113 @@
/**
* Cloudflare Workers script to proxy requests to Backblaze B2 with
Copy link
Contributor

Choose a reason for hiding this comment

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

I am a bit confuse, why is this a Cloudflare workers script? I don't see anywhere in the code that this script talks to Cloudflare. Or am I missing something?

Or maybe you means that this script is written and meant to be hosted in Cloudflare?
If so, I would encourage to change the comment as one day we could decided to host it somewhere else and i believe it will still work.


// Return response if no path is specified
if (url.pathname === "/" || !url.pathname) {
return new Response("The proxy server is active. This is the root path. Note: To access a file resource, append the filename to the path.", {
Copy link
Contributor

Choose a reason for hiding this comment

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

🤩

Copy link
Contributor

@gsambrotta gsambrotta left a 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! Thank you for your changes @lukestanley 🙏

Copy link
Contributor

@CodeKrakken CodeKrakken left a comment

Choose a reason for hiding this comment

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

Lovely stuff!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants