Companion server s3 configuration to let bucket key to be a function and passing down the request

[rmoura-92]

had a very specific need to target dynamically buckets based on some req property defined in the implementation of the server config.

typically we do this config for s3.

{
  getKey: (req, filename, metadata) => {
    if (metadata.folder) {
      return metadata.folder + filename;
    }
    return filename;
  },
  bucket: process.env.S3_BUCKET,
  key: process.env.S3_ACCESS_KEY,
  secret: process.env.S3_ACCESS_PKEY,
  region: process.env.S3_REGION
}

now we can augment the bucket key passing a function, the previous way still valid.

{
  getKey: (req, filename, metadata) => {
    if (metadata.folder) {
      return metadata.folder + filename;
    }
    return filename;
  },
  getBucket: (req) => {
    return req.context.bucket; // do whatever u need based on req to manipulate the target bucket
  },
  bucket: process.env.S3_BUCKET_FALLBACK, // ?
  key: process.env.S3_ACCESS_KEY,
  secret: process.env.S3_ACCESS_PKEY,
  region: process.env.S3_REGION
}

[mifi]

Thanks for the PR ! I think we should refactor this to allow more code reuse

[aduh95]

I agree with Mikael, we should avoid the code repetition when we can

[rmoura-92]

adjusted with the suggestions

[Murderlon]

Does this mean you're blindly setting the bucket based on the client request? Could this be abused?

bucket: (req) => {
    return req.context.bucket;
},

[aduh95]

We should validate the bucket after the function has been called. Also we should probably not disclose to the client the actual error but instead log an error on the server console.

[rmoura-92]

should we fallback to config.bucket? in case of error in the condition from isValidBucket since theres 2 config keys now and bucket is required.

[rmoura-92]

@Murderlon >

you should not use this with uncontrolled inputs for sure like req.params, req.query etc..
just augments the possibilities like getKey if u have that requirement.

in my case i have a middleware that finds a DB record and i add it to req with bucket target.

[mifi]

good! now we just need to actually use the new getBucket function

[aduh95]

I think we'd want to support passing a function here, right?

Suggested change

	if (!client || typeof config.bucket !== 'string') {
	if (!client) {

[rmoura-92]

it's using config.getBucket key for the function, lines: 27-29

[aduh95]

huh that's confusing, I think there should be only one config key for the bucket.

[mifi]

shouldn't we use config.bucket instead of config.getBucket? then it can be either a string or a function, and it will be backwards compatible

[mifi]

This code is repeated many times. I feel like it could be rewritten to something like this:

function validateBucket(req, res) {
    const bucket = getBucket(req)

    if (isValidBucket(bucket)) return true
    logger.error(ERROR_BAD_BUCKET)
    res.status(400).json({ error: ERROR_BAD_CONFIG })
    return false
}

then call:

if (!validateBucket(req, res)) return

(also I don't think we need to wrap the log message in a TypeError)

[aduh95]

@rmoura-92 are you still working on this? The diff now shows 0 file changes.

[rmoura-92]

yes tomorrow will try to submit a final version. i clicked on discard by mistake due commits ahead