Proof that no one uses Allow/Deny

announce
40

Right unfortunately that’s currently the only way. If anyone has a design or pr to make it easy to turn off allow deny please let’s make it happen!

1 Like
41

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) ] }
42

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.

2 Likes
43

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 :slight_smile:

I wonder if the same ratio is with Blaze now :))) Just started a poll

1 Like
44

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%.

1 Like
45

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.

3 Likes
46

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.

47

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.

1 Like
48

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.

1 Like
49

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.

1 Like
50

Hey, I know what’s the problem is :slight_smile: 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 :slight_smile: and trying to predict his/her behaviour in university.

1 Like
51

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.

1 Like
52

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.

53

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.

54

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:

  • Would 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.
  • As we move to ValidatedMethods, is Apollo going to re-address the use-cases of prefetching data? There’s this tiny sliver of functionality in allow/deny that acts as an 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???)
1 Like
55

I have been using allow/deny since day one.
Since when has this become an antipattern and for what reason ???

2 Likes
56

@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

  • Insert method
  • Insert method
  • Update method
  • Update method
  • Remove method
  • Remove method

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.

57

@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.

1 Like
58

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”

3 Likes
59

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.