VulcanJS / Vulcan

🌋 A toolkit to quickly build apps with React, GraphQL & Meteor
http://vulcanjs.org
MIT License
7.98k stars 1.88k forks source link

Access to the totalCount in private collections viewable only by collection owner #2134

Open saravanabalagi opened 5 years ago

saravanabalagi commented 5 years ago

Originally posted by @saravanabalagi in https://github.com/VulcanJS/Vulcan/issues/2066#issuecomment-441442088

saravanabalagi commented 5 years ago

Example:

Topics is a collection owned by users and viewable/editable/deletable only by the owner. In this case, the API shouldn't expose the totalCount of Topics except for admin, but it is currently accessible: Notice 2/6 on the load more button

image

React debugger output:

image

SachaG commented 5 years ago

In your screenshot the "Conferences" and "Papers" topics are visible, so it seems like that collection is readable by the current logged in user in your example? Or am I misunderstanding?

saravanabalagi commented 5 years ago

@SachaG yes, Conferences and Papers are Topics created by user someone and are visible to someone and admin. But someone is also able to see that there are 4 other Topics created by others.

SachaG commented 5 years ago

Oh ok so I think it's the same issue as the original linked issue, there is no way for the server to know how many database documents are viewable by the current user unless it checked every single document in the db for every single request, which is obviously not doable.

So I think what you're asking for is an option you could pass when initializing the resolver to either disable total altogether, or maybe pass a check function so you can selectively allow access to that total?

SachaG commented 5 years ago

I think we could address this together with #2115. Just make it:

const MySecureCollection = createCollection({
  ..., 
  check:{ 
    read: (user, document) => {...}, 
    create: (user, document) => {...},
    update: (user, document) => {...},
    delete: (user, document) => {...},
    total: (user, document) => {...},
  }
})
saravanabalagi commented 5 years ago

unless it checked every single document in the db for every single request, which is obviously not doable.

How about we have a separate function exclusively to just get the count? That way we don't have to process this for all requests (which would be too much), and at the same time, we can serve it if required.

Does it make sense to have the count not shown as the default behaviour and when required we can override this by adding something like canRead: count thing?

SachaG commented 5 years ago

How about we have a separate function exclusively to just get the count?

Yes that's what I'm suggesting with the total function above.

Apollinaire commented 5 years ago

Actually I've been thinking about this one, the totalCount is wrong most of the times because you query for all the documents of the collection. Instead you could write a view to filter directly in the mongodb request:

// this alone is not enough to have the right totalCount
Movies.checkAccess = function( currentUser, movie ) {
  // restrict viewing a movie to only those who own it
  return movie.userId === currentUser._id;
}

// with this, we query only the documents that actually belong to said user so the totalCount is right
Movies.addDefaultView((terms, apolloClient, context) => {
  // in case of server side and not server-rendering, select only the items owned by the user who queries the resolver
  if (Meteor.isServer && !getRenderContext()) {
    return { selector: {userId: context.currentUser._id} }
  } else { // when client-side or server-rendering, we don't have context so no access to currentUser
    return {}
  }
})

The problem with this solution is that we don't have access to the currentUser on clientside or server-render, because context is undefined. that's the reason for the test. But it works, does not throw errors, and (strangely) server-rendering works too (the right totalCount is rendered)