-
Notifications
You must be signed in to change notification settings - Fork 36
feat: add new methods Dataset.createItemsPublicUrl & KeyValueStore.createKeysPublicUrl #720
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
Conversation
* Any other options (like `limit` or `prefix`) will be included as query parameters in the URL. | ||
* | ||
*/ | ||
async getPublicKeysUrl(options: KeyValueClientListKeysOptions = {}, expiresInMillis?: number) { |
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.
A "public key" already means something completely different. Any chance that the name could be changed?
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.
Sure, we can change the name. How about getPublicKeyListUrl
?
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.
Maybe we could do getKeysPublicUrl
?
I was thinking about it already on the API PR... to me it makes more sense language-wise. It's not URL for public keys, it's a public URL for keys. But AI disagreed with my thinking so I let it be 😄
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.
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.
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.
Gemini actually liked Tobi's proposal 🙃
Well, you know, both brains were trained by Google ;D
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.
Generally makes sense but I left a few nits and would like to take another look once it is finished. (Curious mainly about the naming.)
Also kudos for the nice PR description 👏
* Any other options (like `limit` or `prefix`) will be included as query parameters in the URL. | ||
* | ||
*/ | ||
async getPublicKeysUrl(options: KeyValueClientListKeysOptions = {}, expiresInMillis?: number) { |
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.
Maybe we could do getKeysPublicUrl
?
I was thinking about it already on the API PR... to me it makes more sense language-wise. It's not URL for public keys, it's a public URL for keys. But AI disagreed with my thinking so I let it be 😄
src/utils.ts
Outdated
expiresInMillis: number | undefined; | ||
version?: number; | ||
}) { | ||
const expiresAt = expiresInMillis ? new Date().getTime() + expiresInMillis : null; |
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.
const expiresAt = expiresInMillis ? new Date().getTime() + expiresInMillis : null; | |
const expiresAt = expiresInMillis ? new Date().getTime() + expiresInMillis : '0'; |
(opt) Maybe this?
Then in API you'll be able to do:
const parsedExpiresAtMillis = parseInt(expiresAtMillis, 10);
if (parsedExpiresAtMillis > 0 && parsedExpiresAtMillis < new Date().getTime()) {
// signature expired
return false;
}
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.
Honestly, I'd keep it as `null, but I updated comments in API 👇
// For non-expiring signatures, we pass 'null' as timestamp.
// If the timestamp is 'null', we don't need to check for expiration.
if (expiresAtMillis !== 'null' && parseInt(expiresAtMillis, 10) < new Date().getTime()) {
// signature expired
return false;
}
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.
Uff, well, if this is what you want 😅
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.
well, but it is not required to check for expiresAtMillis !== 'null'
, I thought it would be more self-explanatory 😄
NaN < new Date().getTime()
would always return false
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.
Okay, you're right 😄
In python it will looks ugly, since python has None
not null
.
expires_at_str = str(expires_at) if expires_at is not None else 'null'
Changing it to 0
src/resource_clients/dataset.ts
Outdated
* If not provided, the URL will not expire. | ||
* | ||
* Any other options (like `limit` or `prefix`) will be included as query parameters in the URL. | ||
* |
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.
* |
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.
Please update the PR title and description to match the new name 🙂 After that, feel free to merge
After release of new methods `KeyValueStore.createKeysPublicUrl` and `Dataset.createItemsPublicUrl` ([PR here](#720)), I noticed that in `KeyValueStore.createKeysPublicUrl` I used incorrect path `items` instead of `keys`. This PR fixes URL path and improves tests.
When storage resources (Datasets or Key-Value Stores) are set to Restricted, accessing or sharing their data externally becomes difficult due to limited permissions. This PR introduces functionality to generate signed URLs that allow controlled external access to these resources without adding token to the request.
This PR introduces methods to generate signed URLs for Dataset items and Key-Value Store records:
Datasets
dataset(:datasetId).createItemsPublicUrl(options, expiresInMillis)
→ Returns a signed URL like:
/v2/datasets/:datasetId/items?signature=xxx
Key-Value Stores
keyValueStore(:storeId).createKeysPublicUrl(options, expiresInMillis)
→ Returns a signed URL like:
/v2/key-value-stores/:storeId/keys?signature=xxx
🕒 Expiration:
The
expiresInMillis
parameter defines how long the signature is valid.Note: The signature is included only if the token has WRITE access to the storage. Otherwise, an unsigned URL is returned.
P.S. We're not yet exposing
urlSigningSecretKey
for datasets, it will be released after PR is merged.More context here