-
-
Notifications
You must be signed in to change notification settings - Fork 271
feat: add user metadata support for RLS policies #825
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
base: master
Are you sure you want to change the base?
Conversation
Pull Request Test Coverage Report for Build 21370655160Details
💛 - Coveralls |
|
@TylerHillery awesome! Can we also pass in the |
I thought we couldn't do this because we don't get the We could switch the order but then we have to ensure to delete the object if user can't upload and we would be do the "expensive" part uploading when we didn't have to. Much quicker to test RLS first and skip uploading if we don't have to. Here is the /**
* Extracts file information from the request and upload the buffer
* to the remote storage
* @param request
* @param options
*/
async upload(request: UploadRequest) {
// NOTE: canUpload() gets called inside prepare upload
const version = await this.prepareUpload(request)
try {
const file = request.file
const s3Key = this.location.getKeyLocation({
tenantId: this.db.tenantId,
bucketId: request.bucketId,
objectName: request.objectName,
})
// NOTE: we don't get object metadata until here
const objectMetadata = await this.backend.uploadObject(
storageS3Bucket,
s3Key,
version,
file.body,
file.mimeType,
file.cacheControl,
request.signal
)
if (request.file.xRobotsTag) {
objectMetadata.xRobotsTag = request.file.xRobotsTag
}
if (file.isTruncated()) {
throw ERRORS.EntityTooLarge()
}
return this.completeUpload({
...request,
version,
objectMetadata: objectMetadata,
userMetadata: { ...request.userMetadata },
})
} catch (e) {
await ObjectAdminDelete.send({
name: request.objectName,
bucketId: request.bucketId,
tenant: this.db.tenant(),
version: version,
reqId: this.db.reqId,
})
throw e
}
} |
Other fields like etag, lastModified, it's fine if they are empty on the first RLS check |
| .from(bucketName) | ||
| .signUploadObjectUrl(objectName, urlPath as string, uploadSignedUrlExpirationTime, owner, { | ||
| upsert: request.headers['x-upsert'] === 'true', | ||
| userMetadata: userMetadata, |
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.
I think we don't actually support userMetadata on upload signed urls (which we should) so I'm glad you found this.
I believe we need to store the userMetadata in the returned JWT payload; otherwise, this information will be lost during the signedUpload request. We need to be careful, though, if we decide to store the metadata in the JWT, because it can be abused with large payloads. We can for now restrict the amount of data that we can store in the JWT to something like 1MB i think we already have a constant MAX_CUSTOM_METADATA_SIZE, which can be reused (re-refactored as a validation function, potentially in limits.ts)
But we also need to add the second part (on the upload route) to store the userMetadata received from the JWT. This way, the metadata can't be tempered.
What kind of change does this PR introduce?
resolves #757 by adding support for referencing
user_metadatafield in RLS policies for insertsWhat is the current behavior?
Currently
user_metadatais not passed intocanUploadchecks, so RLS policies can't reference it during file uploads.What is the new behavior?
Now
user_metadatais passed through all upload paths (standard HTTP, S3, TUS, multipart) so RLS policies can use it.Additional context
canUploadto acceptuserMetatdata. I updated all callers to pass inuserMetadata. Becauseuploadgets called inprepareUploadwhich gets called inuploadI also updated alluploadcalls to ensureuserMetadatais passed through. I found it difficult to ensure I caught every cause though, e.g. it was very easy to missuploadNewObjectneeded to be updated in theuploadFromRequest. I'm open to feedback to see if there is a more robust solution we could implement that could be caught with typescript types.canUploadchecks for multipart uploads. It required changing the order of certain operations around so thatuser_metadatawas available. Wasn't entirely sure if these changes were okay.userMetadatafrom theFileUploadinterface to theUploadRequestbut wasn't sure if this was the right approach so would appreciate some feedback