Closed demetriusfeijoo closed 1 year ago
The latest updates on your projects. Learn more about Vercel for Git ↗︎
Name | Status | Preview | Comments | Updated (UTC) |
---|---|---|---|---|
plugin-sandbox | ✅ Ready (Inspect) | Visit Preview | 💬 Add feedback | Aug 30, 2023 9:43am |
After scanning trough your RFC, here is just me thinking out loud:
What if we in a way combined the approaches? So per default, we would store the information inside the local storage. In case the user wants to share the sandbox, there would be a button that would generate the URL with the encrypted query parameter 'options' (just an example). In our Sandbox we would check if this parameter is present and decrypt it and if it is valid then populate the options form.
This way we would have the option locally and only once we decide to share it, we would encrypt the values and put it into the query param.
However, there might be drawbacks to this approach too, such as:
Another thing I am thinking of is, what if we said: This link is sharable only for one hour. When the user clicks the share button, the options would be stored in a database as an entry that would expire after one hour. After the entry is saved into the database it will create an id, that will be used in the query parameter. When the sandbox is open with the id n the query parameter it will check if it is valid and populates the option fields.
Here I am not sure about the potential security issues.
@demetriusfeijoo @BibiSebi @johannes-lindgren In my opinion, it's too risky to automatically include the options in the URL. If we include the options automatically in the URL before the user consents, there's a chance the user may share or leak this URL without knowing. It should be more clear. Instead, we can display a button to copy the URL with options included. When the button is clicked, we can show a warning that sharing the link with others may pose a security risk.
Also, this RFC is missing another approach which is to get the options from the Manifest file. We also discussed about this a lot, so we need to thoroughly discuss it here as well. As Bibi said, it can be a combination of some options.
I think maybe 3 is too much and it's difficult to understand which options take priority over others.
@BibiSebi when you said "encrypted query," which secret key do you think we could use? Between a person sharing a URL and a person receiving a shared URL, it's hard to imagine a secret key that both people have (either manually or programmatically on Storyblok). As long as user explicitly clicks a button to create a URL with options included, it's perhaps not too necessary to encrypt it. What do you think?
And yeah, we could consider using KV to store the information which expires in an hour (for example), and only those who have the URL can access to it. That makes sense. We could consider it later, when we really need to cover the browser limit.
@demetriusfeijoo @BibiSebi
https://saturncloud.io/blog/what-is-the-maximum-length-of-a-url-in-different-browsers/
2KB for Chrome seems good enough for most of use-cases, as our options is literally configurations for the field plugin, but not any user-created large data to store.
Thanks, @BibiSebi and @eunjae-lee.
I think we're going to have so many ways and approaches that it'd be nice to categorize them and, after, decide which would be our first step.
These would be the aspects I think we should analyze:
A. Bootstrapping the plugin options from the URL B. Bootstrapping from preset C. Synchronizing the data locally in real-time D. Sharing our options thru url query-strings
So, following the RFC approach and the approaches mentioned by @eunjae-lee and @BibiSebi, I would categorize the options like that:
With these approaches, we can see that:
Now, we may need to answer how important is for the first step to:
@BibiSebi, I liked your suggestion of storing all data remotely and giving it an expiration time. It seems the most robust and bulletproof option IMO. It would improve the security while sharing the URL, security while fetching the data, and cover any browser limitation. However, it adds some extra complexity to dealing with a new service and the infrastructure required for that (I mean, should we use MongoDB, Redis, or some scheduler for validating expired items)? If we have SSR for the sandbox, we could still take benefit of this idea and add an expiration time as a required encrypted query parameter (secret would be stored in the backend) when the user hits the share button.
@eunjae-lee, about the limitations of the browser, I agree with you that the actual size is probably enough, however, I think we should validate it and give the user some feedback because It would certainly be something that a QA, for example, would try to stress out.
If any approach appears, we may start excluding the one we are sure it's not appropriate or big enough for the first step.
What do you think?
@demetriusfeijoo In my opinion,
Synchronize locally in real-time;
it's not that important for the first version,
Bootstrapping from preset
this should be included in the initial version, because if we don't have it, then the Manifest file will be quite useless.
Having the Manifest file will help us improve the CLI so that the deploy
command can apply the default options as well. So I'm thinking B -> A -> D (-> C?).
What do you think @demetriusfeijoo @johannes-lindgren ?
B -> A -> D (-> C?)
I agree with this prioritization order and with C being the last item on this list @eunjae-lee.
starting from the manifest would make the evolution path clearer and allow us to improve all these features progressively, with small steps.
@demetriusfeijoo awesome. could you then update the markdown file to reflect all this discussion? :)
Here's my reasoning:
Regardless of the feature that we consider in this thread, we know that we want to persist the default options in version control so that we can deploy the complete field plugin with the CLI. At the moment, the CLI deploys the code using the name, but the default options are missing. Thus, we know that we will have a manifest file at some point. The question is whether we will use it for this feature.
There are two features that would be nice to have:
Number 1) seems more important for me than number 2).
To share options, the options somehow need to be serialized (1):
Using a database seems complex to me. The sandbox doesn't have authentication at the moment, which probably would be required.
We should also consider other properties such as
It might be nice to make these shareable as well. These cannot be included in the manifest file though, as they do not describe the plugin itself. But they could be included in the URL.
I guess I reached the same conclusion:
A. Both option B and D require this feature. B. The most important thing is to bootstrap from the manifest. But how do we get the options in the manifest into the app? It's going to require you to first be able to load the options via the URL. A Vite plugin could read the manifest file and generate a URL. D. Second priority is to be able to generate a new URL after modifications of the default options
I think we probably should sync the URL query. It could lead to leaked secrets, but do we protect against this by requiring a button click? (The dream scenario is one with the app bridge, where we are no longer forced to store secrets in options)
@johannes-lindgren thanks for your thorough feedback. You're right. In order to do B, we need to do A first. I missed that part.
About D, I think a button click is safer than syncing the URL automatically. Once the button is clicked, we can show a warning to user that says something like
The URL to this sandbox is copied to clipboard. It contains the following options:
SHOPIFY_TOKEN: ABCDE BLAHBLAH_TOKEN: DEF
Be aware in case the URL contains sensitive information.
Even after clicking the button, the URL in the url bar doesn't change. It's just copied to the clipboard, meaning the URL stays the same as the initially loaded one (either the one from the manifest file, or previously copied one).
The URL to this sandbox is copied to clipboard. It contains the following options: SHOPIFY_TOKEN: ABCDE BLAHBLAH_TOKEN: DEF Be aware in case the URL contains sensitive information.
Even after clicking the button, the URL in the url bar doesn't change. It's just copied to the clipboard, meaning the URL stays the same as the initially loaded one (either the one from the manifest file, or previously copied one).
Good idea, makes sense.
Thanks @eunjae-lee and @johannes-lindgren
great insights!
IMO, even with A being the 'base', it alone doesn't deliver value, once the user will not be going to create/update the query parameters on their own (even if the user wants to update something directly in the URL, the need to know there is such way).
So, IMO, the "shareable" URL needs to be created either during yarn dev
(reading the options from the manifest file) or from the D (Sharing our options thru url query-strings after hitting a button).
I don't know if you all agree, but for the first release (which delivers value), would be one of these options below:
What are your thoughts? Am I missing something?
@demetriusfeijoo If I understand correctly, @johannes-lindgren and I agree on A -> B -> D.
A: We are able to use options from the URL
B: We read manifest file and print out proper URL with those query parameters from vite dev
command
D: We provide a button for the "click-to-copy-url" experience.
So, we'd better ship these 3 altogether for better DX, but the order of implementing this would be A -> B -> D.
What do you think?
I agree with you, @eunjae-lee, releasing A, B, and D would be the best scenario.
However, we may do it in 2 releases/steps:
IDK...but it's another story, although.
The point now is that I think we're aligned in doing A, B, and D, and we also have some insights on how to tackle them. 🚀
May I update the RFC now with what we discussed...
@eunjae-lee, @johannes-lindgren, do you think we should discuss about obfuscating these options somehow?
Because even if the URL is copied straightly to the user's clipboard, and we alert him about not sharing confidential data, he may do it. Do we need to concern about it right now or we may improve it in a future version?
If we do not handle obfuscating the options, we may discuss another aspect, such as query parameter collision, since both options
and sandbox configurations can use the same name. For example, the URL parameter is used as a configuration in the Sandbox but could also be used as a plugin option.
Does it make sense for you @johannes-lindgren @eunjae-lee ?
Obfuscating options might be too much at this point, in my opinion.
Can you give me an example about query parameter collision? I'm not sure if I understand it.
And what do you mean by "2 releases/steps"?
Thanks for your opinion @eunjae-lee, I appreciate it a lot.
I agree that obfuscation is too much for this moment, the idea was to bring up the collision discussion (that may we also ignore for now) and how we are going to structure the query parameter for options.
By "options collision" I meant providing, for example, an option named url
.
In this scenario, we would have a url
parameter (used by the sandbox to load the preview) and also a url
parameter for the created option.
Example of request: https://plugin-sandbox.storyblok.com/field-plugin?url=http://localhost:8080&url=http://second-url.com
We have several approaches for handling this situation and I'd like your opinion on that...
I'm going just to list some of the approaches that are on top of my mind:
Do you see any other option @eunjae-lee?
We can also discuss it when we start developing but maybe here and now would be more appropriate. What is your opinion on that @eunjae-lee? Should we discuss it right now?
Ps: Calling it "collision" is not really appropriate because it does not collide, but creates an array of parameters for items with the same name.
By 2 releases I meant:
Release 1 - We have one first delivery on our side just for A and B Release 2 - We deliver D.
I agree that the best DX would be A, B, and C. Although, I think A and B deliver already some value.
But as I said in my earlier comment, I think it's not a topic for the RFC discussion phase... We can easily discuss it after...
@demetriusfeijoo I see what you mean now.
I think putting whole json in the url parameter makes the sense here. I'd even include the whole manifest object instead of only options. ?manifest={"options": {...
By 2 releases I meant: Release 1 - We have one first delivery on our side just for A and B Release 2 - We deliver D. I agree that the best DX would be A, B, and C.
You meant, A, B, and D, right? 😅
Could you summarize all these discussions back in the RFC please? It doesn't have to include everything, but it'd be nice to options that we have discussed and why we picked something, and not the others.
After reviewing the final RFC, I'll approve and merge this, and I think you can get started working on this, if you want.
You meant, A, B, and D, right?
Yeah, that's right @eunjae-lee. 😅
I think putting whole json in the url parameter makes the sense here. I'd even include the whole manifest object instead of only options. ?manifest={"options": {...
I think in the future we can add all the manifest inside the URL but without grouping them in a manifest
parameter as you suggested, @eunjae-lee. I mean, as these shareable options could be added to a URL through a share button
, IMO it may not be strictly related to a manifest
all the time.
Could you summarize all these discussions back in the RFC please?
@eunjae-lee, since we discussed about:
- Bootstrapping from preset (manifest file);
- Click to copy the URL with options included;
- persisting options in the local storage;
I think it was too much to have in a single RFC, so I've updated this one to have only the persisting discussion
and created another for the manifest and the Click to copy
button.
Manifest RFC
Click to copy RFC
Ps: All of them are already updated based on our discussion here.
What are your thoughts on that @eunjae-lee, @johannes-lindgren, and @BibiSebi?
The idea is to keep them simple and focused on only one aspect of the whole issue we're trying to address here.
@demetriusfeijoo Hey Demetrius, reading this commit, I'm a bit confused.
If I may have misunderstood you, I'm sorry, but when I said A, B, and D, I didn't mean localStorage.
A: We are able to use options from the URL B: We read manifest file and print out proper URL with those query parameters from vite dev command D: We provide a button for the "click-to-copy-url" experience.
Hey @eunjae-lee, you didn't misunderstand me at all, after our call I realized I took a way different from the way you were expecting, so that was the confusion, no worries.
The RFC was updated accordingly to what we agreed in our call: to cover what main aspects we discussed here and also to be as simple as possible.
If there is any other point I missed to include in the RFC, please let me know.
What?
Include an RFC for persisting options and data values.
Why?
Each field plugin usually has a sort of different options defined, and it's common either to refresh the Field Plugin Sandbox page during development/test or to switch from one field plugin to another.
Doing so, it would be nice to, somehow, persist both options and their corresponding values. This way the user would not need to recreate all of them while refreshing the Sandbox page or switching between field plugins.
This PR opens a discussion on how we should proceed in dealing with this scenario.
[View Rendered →]