-
-
Notifications
You must be signed in to change notification settings - Fork 274
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
https://demo.toolpad.io/ data fetching feels limited #1213
Comments
I agree, the Functions and Fetch should be available by default like we show in the staging environment. We had purposely limited the data sources as the intent was to give a basic idea of Toolpad. Should we disable Connection button and through an info icon show that we support 2 more data sources in the self-hosted version? We can show Fetch default, Functions default and Fake movies API directly when user will go to create a query. Also, as there is no Home view, users will create 3-4 apps in the first session itself (until they realize they can't see the apps). Either we should show apps for that session or an info saying please save this URL if you want to open this again. We can also show Self-host CTA in the header bar. |
Queries run serverside without any rate-limiting, meaning it's quite straightforward to turn a public facing toolpad instance into a DDOS or scraping bot. render.com will kick us off its servers faster than we can say "it wasn't me" 🙂. I will implement #1196 to provide a similar experience that doesn't have the same problems.
I can run I will implement a client-side variant of function query analog to #1196 that can be used in the demo mode.
Currently passwords are plaintext readable in the editor (not the runtime). We will reimplement connections (#1065) with the consideration in mind that plaintext passwords should never end up on the browser.
Same to 3, the token is stored the same way as the connection details
The intent was avoiding cloud related security and performance issues so that we can concentrate on the app building experience instead.
If we do that we should add very clearly that the app can disappear any moment in time. edit: Happy to make these datasources available in the demo if the team doesn't share these same concerns. |
Could we add a rate limiting on the server side? Say to 5 requests a second or any other value that feels right and wouldn't allow for ddos? |
@prakhargupta1 On my end, I didn't feel pain about this. I can see all its features with a single app. I did create a lot of apps because I couldn't find the previous ones back, but I guess it's OK? To fix this we would need to have a login, at which point, we have a Cloud which makes the Demo legacy 😁.
It would improve the UX 👍
@Janpot Ohhhh, I didn't realize there are only two APIs that are allowed: Ok, then I think it simplifies the framing of the problem this GitHub issue is about. Say I'm a developer and I have a problem to solve, the demo helps me envision how the tool could solve my pain without doing the local installation, but if I have a concrete pain, and I would like to try how the tool solves it, I can't see this. Maybe it's OK like this but I think that we could do a lot in terms of improving the UX.
Then for 3 and 4, I problem for me is that as a developer it feels like these data source options don't exist. I think that the data source option should be visible and: either explain why it's disabled, for security reasons, or (better, if it doesn't take too much time to implement) allow the person to acknowledge the risk and use dummy credentials/accounts.
@bytasv This might require a Redis instance or equivalent? If it's too much work, I think that solving the UX part of this GitHub issue could be a great alternative. What I mean by UX, is: I land on the tool, I see very few fetching options and I jump to the conclusion: it's not ready for my use case. It's also about, when I click on "Add query", I see an empty select: so I can't fetch data? |
To be honest, I'd look into client side fetch and function datasources. It will give a good idea on what's possible, and the requests happen fully on the browser side, so our IP addresses are not tainted. And it will be valuable outside the demo as well
Just to note some constraints: I believe we need to rate limit on outbound IP address (perhaps combined with appId). This because we don't control hostname resolution. It will also have to take into account redirects. This means we'll have to use an agent and pass it to all Our options are:
|
I don't think it has to be so complex. In the past I've used https://www.npmjs.com/package/express-rate-limit it's pretty straight forward. Found a blog post that explains how that could be done for next/server - https://kittygiraudel.com/2022/05/16/rate-limit-nextjs-api-routes/ All of that should work with in-memory handlers, but IMO it shouldn't be an issue at least for demo. I might be wrong, but it seems that all we would need is to provide |
Regarding the UX improvements. I would suggest that we do these changes:
|
I like this option, I think any of the 3 would work right now but it seems like the most future-proof, as long as it does not take a lot more time/effort. And rate limiting is definitely something we should add to improve security overall. If we're rate limiting data source calls only/mostly, I guess we could use About the UX, I like the list Prakhar made + allowing only client-side fetch and functions. |
We could also store (in Not sure if we want to do that but just an idea. |
Just want to point out that I'm proposing rate-limiting outbound requests through a forwarding proxy on the fetch calls that the backend is making and @bytasv is proposing rate-limiting inbound requests by wrapping the handler that serves data calls from the frontend. Which are two whole different things. I believe there could be some benefits to rate limiting outbound requests over inbound:
Anyway, some food for thought. |
I think this might be bit extreme of what we actually need for demo purposes. Initial assumption was that we COULD potentially get bad actors trying to abuse fetches, so the inbound rate limiting should take care of that for this demo purposes. I don't think we are currently looking for a way to have a bullet proof solution for a future cloud version? Even if we do, we could still try to do the fast a cheap way so that demo doesn't feel so limited and in parallel continue with setting up whatever is needed with the option that you proposed (unless that is even faster/cheaper to do than the rate limit middleware) |
Ok, so if I summarize the options maze, we have by the level of quality of result and often cost, for each problem:
I'm OK with all these options B/C. If we have the resources to do more (A), then it's also great. |
Adding as per last meeting: And a note on rate-limiting that a in-memory solution wouldn't work well if we need to scale horizontally |
Regarding the UX changes we discussed. My textual inputs for the pop-up below. Styling isn't apt.
We have to add Self-host button at the top, I guess it can be best placed on the right side, just left of 'all changes saved icon'. Another point, To some 'Demo version' may appear like a button, maybe we can alter its appearance. |
All of those seem like good suggestions to me - i think we can do them all. |
@prakhargupta1 When a user creates a new blank app in the demo, should there already be connections for the Covid API and the Dogs API? |
I was thinking that we should just show them in the queries drop-down. |
Based on the current state of https://demo.toolpad.io/ and my first comment #1213 (comment)
Based on #1213 (comment)
|
Oh, it didn't occur to me that by hiding connections this would not be showing anymore: What if we showed every option as self-host only? As the client-side fetch/function don't really use connections. |
@apedroferreira This sounds good. |
Yep, once we have server-side fetch/functions we can show that dropdown with only Postgres and Google Sheets disabled like in the image above. |
@apedroferreira But to be clear, eventually, we will have these options? (Likely no longer in a select as it doesn't scale to many data source) |
I wasn't thinking about separating client/server side fetch and functions in that dropdown, so that's why the UX would be a bit weird if I disabled every option for now... But I also don't think we should separate client and server side connections. It is possible to create client-side connections already in the connection selector if I show them, but the problem is that client-side fetch and functions don't use any of the connection parameters such as base url, headers and authentication. Those parameters don't do anything in client-side queries - which means that creating a client-side connection instead of using the default fetch/function queries is useless...
|
@oliviertassinari Wait, actually Jan is working on global connections so we're eventually going to get rid of this connection picker anyway. Maybe we can keep the demo as it is, and once we have global connections we can somehow show to demo users that we support those options in the self-hosted version, somewhere in the page where global connections can be created? |
OK, not an ideal outcome in the short term, but we eventually reach the outcome we looked for, so why not, especially if it saves us time overall. |
@apedroferreira Yeah, it didn't occur to me as well, that by hiding Connection we are also hiding that info. For now, We can show a message in the footer. Let's append the current message with. "Note: PostgreSQL, Google sheets are supported in the self-hostable version." This will be followed by Self-host CTA. So I think it should go fine. |
We can do that. |
We will be moving connections globally, which will make them impossible to include in the demo as it currently is. Not until we add authentication and workspaces anyway. |
Ok. Let's then add the message in the footer. That should be fine. |
Related to #592
I was hoping I could rebuild most of the mini-apps that I had created on the staging environment https://master--toolpad.mui.com/ in the demo environment: https://demo.toolpad.io/ but it's not the case. This makes me wonder:
fetch
calls that can be made inside the function. I think that we would also need to limit the number of queries that can be added to the page, because why close one door (fetch) when the other is open (multi queries)?The text was updated successfully, but these errors were encountered: