search

prototypsthlm / taco

An improved interface for LLMs like ChatGPT
https://www.tacoai.app
MIT License
29 stars 2 forks source link

Split user pw into extra table #62

Closed brokelyn closed 1 year ago

brokelyn commented 1 year ago

In order to never expose a user password we could move the user password into a separate table like we did with the user session (though for another reason). At the moment we are risking with every query to forget about removing the user password and exposing it to the frontend

brokelyn commented 1 year ago

@tonioriol or @eriklindgren what do you think? Any other ideas or best practices how this stuff is solved?

tonioriol commented 1 year ago

That was also my thinking to solve this issue, so I like it. It is in fact how it's done in EventBlender db!

eriklindgren commented 1 year ago

You guys know way more about this project than I, but while I think this solution is better than nothing, it does feel wrong to let the libraries we use dictate the database schema. I would prefer to do something like this if possible: https://www.prisma.io/docs/concepts/components/prisma-client/excluding-fields

tonioriol commented 1 year ago

This is mostly what we already have: the exclude method. So, I guess it's fine to leave it as is in that case.

Of course, another option is to use a select clause. I don't understand why this isn't mentioned in the Prisma docs. I already read about it the other day in those very same docs for this very same reason, and I was surprised they don't mention it.

I even investigated how to use select for that purpose, and it turns out you can't combine it with include, which I used for loading relations. So what you have to do in places where your query needs a relation, and you want to select only some fields is to load the relations also using the select clause. It's weird, but that's how it is.

But both solutions have the same root problem to me, and is that you have to be explicit with either of them. It's too easy to accidentally leak the password to the client if you have to do it by hand. And if/when new developers join the project who may not even be aware of this, it could be too risky.

So, while I agree it sounds bad that the framework forces us to design the schema in a certain way, I don't know, I still think it's the best option.

brokelyn commented 1 year ago

Yeah absolutely agree with you here Toni! Its also not just about new people joining. You make a complex query where somewhere we have a user and then you just forget. Its just too risky. But what would we add to this new table? The email as well? All personal information?

eriklindgren commented 1 year ago

I think this is a great example of why something like the views/templates in Phoenix makes a lot of sense: make sure you explicitly state what data you return from your endpoints. Moves the whole issue away from the db interaction.

Anyway, I rest my case, it seems like you're in agreement over the best path forward ⭐