The allow/deny() are function callbacks, so somewhere in the code there would be something that calls such callbacks. We just need to find the one that calls the deny() callback for the collection and make it assume it is always true.
I guess change it to say
insert: { allow: [], deny: [ _.constant(false) ] },
update: { allow: [], deny: [ _.constant(false) ] },
remove: { allow: [], deny: [ _.constant(false) ] },
upsert: { allow: [], deny: [ _.constant(false) ] }If anyone wants a quick fix, this should do:
//populate collectionList with a pointer to each collection
var deny = function(){return true}
var all = {insert:deny, update:deny, remove:deny}
collectionList.forEach(function(c){
c.deny(all);
})
But this should only be necessary for Meteor.users collection, I think every action to other collections are auto-denied by default.
Otherwise, I think @trajano is on the right trackā¦ the entire API should just go.
Interestingā¦ just have noticed that with 42 posts on this thread, there is a golden ratio distribution in the poll results:
38% - use
62% - do not use 
I wonder if the same ratio is with Blaze now :))) Just started a poll
Ease of use is one thing, but knowing every client-side entry point to your db (and being able to predict the ways in which those entry points could be hacked/manipulated) seems too important to me not to use methods 100%.
Which is partially why I created rlivingston:simple-schema-mixin. Include that mixin and the validate option can be replaced by a schema option. There are also techniques that can allow you to reuse your existing schema for most parameters via a combination of SimpleSchemaās pick and schema functions. See the README.md for examples.
Honestly, in my own code I also wrote a package with a class wrapper for ValidatedMethod that I call SimplyValidatedMethod. It automatically includes the SimpleSchemaMixin so that I donāt have to specify mixins on every ValidatedMethod definition. Iāve thought about developing that into a general purpose package that allows someone to specify whatever mixin list they choose and any other options that might be the same in all of their ValidateMethod definitions. But, I think that would undermine ValidatedMethod. It would be better if an extension were made within the ValidatedMethod package to help eliminate that type of duplication.
I feel youāre misunderstanding the fetch rule. This only limits the database fields fetched and made available for the code that you place in your insert, update, and delete rules. From the meteor docs:
When calling update or remove Meteor will by default fetch the entire document doc from the database. If you have large documents you may wish to fetch only the fields that are actually used by your functions. Accomplish this by setting fetch to an array of field names to retrieve.
So, my understanding is that the fetch within allow/deny rules doesnāt limit what your collection find operations can access. Only the publication can do that.
This * 10,000. With allow/deny thereās one place that decides whether that user can edit it the document or not. With 50 methods that can modify a collection, itās a lot more likely that someone forgot to do that check.
Youāll typically only ever have one or two insert, update, and remove methods per collection. If you have 50 then you might want to consider rewriting your application in a more concise and efficient manner.
Allow/deny is basically just a wrapper for methods - because allow/deny ultimately uses/transpiles into methods.
Also, the check package is great for letting you know when you havenāt done a check in a method.
Okay, 50 was purposely excessive. But one or two is not realistic either (outside of a basic TODO app), youāll end up with just as much of a mess as you would if you just used allow/deny in the first place.
The check package is used for checking methods, audit-argument-checks is what youāre thinking of. Which is definitely a good package. Unfortunately it doesnāt allow you to use ES6 property destructing or rest operator.
Hey, I know whatās the problem is The framework needs 2-3 (or even 20-30) āreference appsā so that it is really stays up to its purpose. Probably it already has behind the scenes (high paying customers on Galaxy).
Otherwise all this theoretical talk is similar to āhow to educate a kidā - with single truth āallow/deny listā or with explaining that in each of methods he or she hasā¦ what are the users he/she will interact with? are they good and will not try to abuse him/her? But we have only simple to-do kid, zygote, not even embryo and trying to predict his/her behaviour in university.
Iām on the method side of this debate and yet I donāt buy one or two. Iām probably about half-way in between these two extremes.
I find that the number of methods tends to be directly related to the number of fields, and meteor development tends to drive me to a relatively flat schema. I write methods that update single fields much of the time. The exception might be when fields are highly related and tend to change in concert with each other.
I think that reactivity is the largest factor driving this approach. The old approach of holding onto changes and sending them all at once with a submit is mostly dead. It still throws me sometimes as a user that very few applications have me apply or submit option, setup, and profile changes for example. So, a field change usually results in a method call that updates a single field at the server - reactively driving immediate response to the change back to the client.
But, I donāt consider this a factor in why I threw away allow/deny. I threw them away because the decisions as to what to allow or deny are complex. In my app they are very often per field and depend on the userās relationship to the data (owner, contributor, stakeholder, disseminator, etc.) and even sometimes the exact nature of the change. Whether to allow or deny an update is just not as simple as whether or not the user is an admin or logged in anymore.
So, I feel that I need to carefully consider the rights for the data being updated by each method, not for each collection.
Of course, if you implement allow/deny at the method level, youāve essentially eliminated this argument. At that point, itās just terminology.
Iām on the method side when I have to work with a team and I am responsible for working on the server side. Primarily because I would like to know how things are controlled. Thatās from my experience on larger scale enterprise applications.
However, when the UI is still not finalized and itās more of a collaboration/shared documents app installed on an intranet, I would use use allow/deny and just have a simple security check to ensure that someone is logged in and in the proper role. This simplifies the development on both sides of the fence. Mind you the security and data set of those systems are a bit more lax since its a limited number of users that know each other and itās more of helping them manage their workflow rather than putting things out to the public.
Agree. Itās sometimes nice to keep things simple while prototyping or for an intranet, and you often can because your exposure is less.
In my case, Iām a team of one but, because my apps tend to be large scale, I donāt trust myself to think of all angles all of the time. So I impose the same structure on myself I used to impose on my team. I deny everything and carefully punch through with methods.
Itās entirely possible. And may be evidence that the allow/deny documentation was never clear to begin with. Iāve understood the following line to be equivalent of an allow all or allow * firewall rule for a few years now. Maybe thatās incorrect. But based on how it was written up on docs.meteor.com, thatās how some people have been interpreting it.
Iām sympathetic to this view, and think itās useful to trace the history of how allow/deny came to be. Originally, it was an extension of the livedata system right? And livedata is a pub/sub system. Why did Meteor choose to implement pub/sub? Because it was a javascript version of the pubsubhubbub protocol. And atom/rss style publication feeds were the early aughts real-time alternative to prefetching data with polling intervals.
So with basic stateless HTTP Ajax calls, data gets fetched and thatās it. Stateless, scalable, secure, and all that. But because itās stateless, thereās no mechanism for real-time updates; so people resort to polling, which leads to prefetching, and then publications/subscriptions. As that happens, security becomes a concern, and somebody along the way slaps together an app-level firewall. The firewall has allow() and deny() rules. And since weāre implementing subscription/prefetching functionality, lets just assume that the firewall is going to have an implicit allow all rule on fetching data; because it would sort of defeat the purpose of livedata to firewall the outbound updates, right? And then, hey, lets do some refactoring, and move the firewall stuff to itās own package; and the prefetching stuff to itās own package.
I could be completely wrong about that sequence of events. But it seems pretty obvious to me that something like that is what happened, more or less.
So, just a couple questions:
Firewall.allow() and Firewall.deny() be a good namespace to move the functionality under? Does it behoove Apollo to have a Firewall object of some sort? Semantically and educationally, this is a coherent API that people are familiar with.allow all rule for the rest of livedata. Maybe other people donāt see that. But it seems obvious to me that there are remnants of that era of design. Simply curious to know if we have any idea yet what the new mechanism for publications/prefetching/livedata is going to be in a stateless method-centric Apollo world. (Last I heard, livedata was going to live side-by-side with Apollo; and that polling was going to be the recommend strategy???)I have been using allow/deny since day one.
Since when has this become an antipattern and for what reason ???
@rlivingston valid points!
@rlivingston @Volodymyr @workman Just to clarify, when I said one or two of each per collection, I meant one or two inserts, updates, and removes, per collection.
For example:
Theoretical Posts Collection
Thatās usually how it plays out per collection. Iām running a production app right now that literally has a single insert, update, and remove method per collection, with a few rare cases where I need more than one of each.
Really, the argument canāt be made against methods being bad for apps with lots of inserts, updates, and removes. If anything, methods are better when your app has more inserts, updates, and removes - you can have one place for all of your method logic as opposed to having twenty places in the UI code where an insert, update, or remove is called.
Regardless of the arguments that will ensue, itās good that weāre having this conversation.
@repsonsive, @sashko
Thatās a good point ā being new dev friendly. Apps grow and change and get hardened. Making it simple to start with is NOT a bad idea. No one starts with a 1k lines of code day one to start testing and prototyping.
I personally donāt have a problem with allow/deny. But since Iāve been working on bigger projects with several people involved in the code part of things, doing security in methods have always made our apps more secure. In the end there might be more code, but at least everyone is more confident when working with said code.
Over the past 3-4 years, Iāve gone from āimplicit is nice, I donāt have to write so much codeā to āexplicit is almost always better, because me or anyone else can come in and understand stuff more easilyā
Agree. I know about this issue because it bit me. I was misled as well. But, I feel that it isnāt entirely docs.meteor.com here that is at fault. My understanding came pretty quickly when I actually read the parameter description for fetch.
**fetch** Array of Strings
Optional performance enhancement. Limits the fields that will be fetched
from the database for inspection by your update and remove functions.
The most important documents by far in any interface are the method and parameter names themselves. It is unfortunate in this case that in the context of insert, update and remove parameters that do one thing, they chose to name an adjacent parameter āfetchā. It seems to be a natural sibling to insert, update, and remove, but it is nothing of the sort. Instead, it is an optional means of optimizing the performance of the insert, update, and remove allow/deny functions by reducing or eliminating the data fetch performed in support of them.
This misunderstanding which I have found to be fairly common (more than one in this thread has shown it) is another of the reasons I have a distaste for the allow/deny mechanism.