search

thomas4019 / expressa

API creation middleware with an admin interface
MIT License
439 stars 27 forks source link

Fields with GET Bug #153

Closed kane-mason closed 2 years ago

kane-mason commented 3 years ago

If you are trying to retrieve a list of docs of which you are the owner, and you use the &fields={} url parameter, it will return zero docs unless you specify meta:1 in the fields list.. This seems wrong to me.. Yes the user has permission to view their own docs in this collection, it works fine if you dont have the fields url parameter at all `

thomas4019 commented 3 years ago

@kane-mason Good find! This is a very relevant issue with the way I implemented fields. The query is passed all the way to the database which means the listeners don't get the full document either and thus don't work correctly. One solution here is I could make it always request the meta fields, but I still feel like there could easily be issues with people who write custom listeners expecting full documents and then being surprised to get partial documents. What's your thoughts?

kane-mason commented 3 years ago

Mmmm it is a tough one. I think it depends on what you interpret as more important.

From my side, i like the fact that the fields are restricted all the way from the database. I have not done any research, but i imagine there is a benefit to only requesting a few fields from what could potentially be a huge document.

My thoughts are that for document database, because documents can be widly large, a good practise is to always request only the fields you are after. Smaller reads from the database, smaller objects within API, smaller payload sent to client, seems like it should definitely improve performance, but yeah thats not exactly scientific.

But if you have that mindset, i think how it is setup is ok. I think there is even an argument to leave meta: 1 off, though because this affects directly an internal permissions feature i may be inclined to argue meta.owner should be a default field.

Lastly, GET requests dont often have complex custom listeners, its def more of a POST/PUT feature, so less of a concern, though still a concern. Would be nice if we could somehow warn a client that there are a few fields they need as default, maybe could be configured? Only if update is not too invasive

kane-mason commented 3 years ago

We could easilly configre default getFields in the schema now that i think of it.. This would allow the backend developers to ensure fields they need for their listeners are always present

kane-mason commented 3 years ago

@thomas4019 I have an idea which will improve efficency and will solve the original problem.

When the request comes in, if the user is logged in, and if they have the permission {collection}: view own, but not {collection}: view (might need to check if collection documentsHaveOwners is true), then before the request to the database is made, include 'meta.owner': req.uid on the query url param.

This will only retrieve documents from the database that are owned by the user. And whether or not meta.owner: 1 field is specified is irrelevant now because the logic that filters out documents they dont own is no longer needed!

thomas4019 commented 3 years ago

This approach sounds good and very efficient to me! I can implement in a week or two or feel to put up a PR for it if you have time.

kane-mason commented 3 years ago

I will certainly try if i get a chance!

Though, i am nervous to remove the editingOwn permission check. So do you think we remove the current 'post database read' editingOwn check, in favour for my suggested 'pre database read' check, or a hybrid? I personally dont like the idea of having both, but not sure if it might create any vulnerabilities. I guess could be mitigated with good unit tests, do you think what exists is sufficient?