Allow/Deny Deprecation Discussion

[copleykj]

So recently this issue (Remove internal allow-deny use from Accounts #190) was brought to my attention due to the mention of deprecating Allow/Deny. I’d really like to get a discussion going around this topic of Allow/Deny deprecation. I can’t be the only one that thinks this would have a largely negative impact across the Meteor ecosystem. I’m aware of the view that allow/deny is hard to secure and the recent security issue, but that issue has been patched and allow/deny can be made simple and straightforward with the use of some amazing community packages such as Simple Schema and Collection2.

I’ve been working with Meteor since v0.5.0, so I’ve been through my share of changes to this wonderful framework. Of all the changes I’ve seen though, I don’t think I’ve seen the deprecation/loss of a core feature such as this. Is this a completely necessary change, or could the changes be shallow such as removing the dependency from accounts and leaving allow-deny in place to be used by those of us that wish to?

So let’s have a discussion. Hopefully I’m not alone in my view on this.

[vigorwebsolutions]

This is an interesting one to me.

I’ve never been a fan of the allow/deny strategy, because it makes more sense to me to start with no permissions and build them as I go. But I really think that’s personal preference, and it’s been shown here (and in the code) time and again that allow/deny can be just as secure as other methods when done correctly.

I think what it comes down to for me is: is there anything inherent about allow/deny that makes it more susceptible to the kind of security issue which recently surfaced? If the answer is unequivocally yes, then I understand the decision. If it relates more to the implementation of allow/deny, then I think that’s a more difficult decision.

Although this wouldn’t likely impact the projects I work on, it’s kind of similar to view layers in my eyes – the more (stable implementations), the merrier. Bigger ecosystems mean more traction, more people helping people, etc., so to run the risk of alienating allow/deny proponents with this move, I’m hoping it is a legitimate future security risk.

[copleykj]

I agree. As far as I can tell there isn’t anything that would make allow/deny inherently insecure, and I really think this could be a “Have your cake and eat it too” scenario.

[herteby]

I remember some guys had an “allow/deny security competition”. Only one of them managed to submit an entry without any security holes. Maybe moving allow/deny to an optional legacy package that you can include if you wanted to would be a good idea.

[copleykj]

Yes, The Discover Meteor Allow & Deny Security Challenge. There were actually 5 of us that completed the challenge on our first submission.

Most of the submissions used completely ridiculous logic inside allow-deny rules. Of course you’re going to have an issue securing your application that way. You’ll have exactly the same issue if you try to secure your methods this way, only you’ll spread that same logic over multiple method calls. My submission, on the other hand, used Simple Schema with Collection2 and a single additional custom validation rule to disallow updating of the likesCount field from the client. So simple even someone just learning Meteor could do it. Unfortunately at that time in Meteor’s history there was a whole lot of trying to prove how insecure allow-deny was instead of educating people on how to use it properly.

[copleykj]

No one else has any more input on this?

[storyteller]

I have mixed thoughts on this issue. It is a really nice feature, but only a few people are really able to utilize it securely. Even the Meteor Guide advises from using it. If the official recommendation is against it then it makes little sense to keep it in the system. But at the same time, I’m considering prototyping where it finds great use.

From my experience in react right now I prefer using methods, but that is just one possible implementation and I’m not entirely consistent.

As @copleykj pointed out there was always more of a push to not use it rather than educate. Wouldn’t then be better changing the default behavior to the safe way be preferred than abolishing this approach altogether? It is all nice and good to tell people to educate themselves about the security issue, but given how constantly even large companies that have dedicated security teams are leaking confidential data, I find that to be a losing battle and hence prefer secure by design and only touch it if you know what you are doing.
Either way, I’m afraid we will have to have some sort of breakage here to solve this one way or other.

[msavin]

Why not move the functionality into an eternal package, and slap a big warning sign on it? that way, it’s out of the Meteor core but can be used for backwards compatibility, etc

[ralof]

I see allow/deny as kind of gatekeepers, bouncers at the door to your club. They provide basic security but you cannot rely on the for all situations simply because you cannot put that amount of responsibly on them without risking a meltdown.

I think that if, if, allow/deny are not able to do what they are advertised to, if they can be hacked, then they must be either fixed or deprecated because a security mechanism that is defect is very dangerous because the false senso of security will prevent yu from doing what you need.

If the problem is that allow/deny is often implemented in a bad way, then keep it. Expand the documentation to include warning examples and so on, but keep it.

It is great as a gatekeeper.

[copleykj]

I would almost be on board with this; However I would like to see this stay in core because it’s part of Meteor’s original core philosophy of making isomorphic javascript applications dead simple. Meteor has sort of switched gears to try to target us professional developers. I get it, business strategy has to change for the survival of a company, but I don’t think that means that this philosophy needs to be scrapped and the core of the framework hacked apart until meteor is just a slightly advanced build tool.

[veered]

tldr: Make allow/deny easier to use, don’t remove it.

The recent Meteor security disclosure isn’t really related to the reasons people are generally told to avoid allow/deny. Use of allow/deny is discouraged because of the difficulty of directly writing correct allow/deny rules, while the recent security issue was just a bug that happened to be related to allow/deny. You could imagine something similar happening for Meteor methods or whatever.

Personally, I think one of the best parts of Meteor is code isomorphism and the ability to perform DB updates client-side, without having to put everything in a Meteor method. It’s really quite incredible and AFAIK there’s nothing like it anywhere else.

The real core problem is that writing allow/deny rules is hard. And that is undeniably the case. But the solution isn’t removing one of the best parts of Meteor. The solution is providing an easier alternative to allow/deny. The best alternative that I’m aware of is ongoworks:security paired with SimpleSchema. It’s super easy to understand how to use this combo and avoids all of the pitfalls of directly writing allow/deny rules. I wouldn’t be surprised if it’s possible to make this approach even simpler and more tightly integrated.

IMO, the only times that it’s necessary to put DB updates in Meteor methods (that could plausibly be done on the client) are:

• The restrictions on the update are too complicated to succinctly describe with ongoworks:security + SimpleSchema
• There are multiple updates that must be performed in tandem, such that if only a subset of them were performed it could lead to inconsistent state. In other words, you need multi-document transactional updates
• You need access to data not published on the client in order to make the update
• You are worried about race conditions created by multiple clients making an update at the same time and need to implement locking

I might be missing some situations… But in many applications the vast majority of DB updates don’t fall into any of these categories. For example, allowing a user to change their own name is a very safe operation to allow client-side. At Qualia, if we had to put every DB update in a Meteor method we would have an insane number of meteor methods and the code would be much more difficult to read and maintain.

[hwillson]

Great discussion all! Keep the pros/cons coming - this is all being considered. FR #190 will still be going ahead (at some point), since there isn’t a strong reason why the internal Accounts code needs to rely on allow-deny. That being said, the idea to deprecate allow-deny completely may change (there has been more discussion about this in PR #9152).

[korus90]

Would someone mind pointing out the security issue or send me a link? I can’t find anything in my (admittedly brief) google and search on the forums.

EDIT: It’s on the meteor blog. Ignore me…

[vjau]

I use an app, where every document in the DB is attributed an owner field. The owner field is the id of the user who is allowed to modify it.
I find that very easy to check with allow/deny, when you try to update something, it checks that the user id is the same that the owner field of the document you are trying to modify, and deny it if that’s not the case.
Anyone could point me towards what could go wrong with this approach ?
With a method, you are passing arbitratry object between the client and the server and you have to triple check those objects before submitting them to the db, as javascript objects can have any arbitrary (and dangerous !) shape.

[herteby]

If it’s really as simple as you say, then allow/deny is awesome for you.

The “problems” happen when many people are allowed to edit the same document, with different rules. For example, you have a “post”, which has “content”, “authorId” and “likes”. Any user may “like” the post, if they haven’t liked it before, and it must only increase the “likes” counter by one. But only the author and maybe admins are allowed to edit the “content”. Then, you must consider that a hacker for example might “like” the post and at the same try sneak in an update of “content”.

Methods simplifies things by clearly separating each type of action.

Not saying that allow/deny can’t work well when you know what you’re doing.

[copleykj]

Even all of that can be made quite simple with the use of 3rd party packages. Meteor does however attract a lot of new and/or hobby developers which is why I created the Socialize packages. Makes all of what you mentioned dead simple and they are all extremely extensible for more experienced devs, thanks to things like allow/deny.

[hypo]

I have to admit I am indeed a fan of allow/deny – it’s exactly on the right abstraction layer for my needs in several applications. Without it, I’d have to re-implement it. Please don’t deprecate it!

[serkandurusoy]

I have an app in production, developed ~3 years ago, only getting core meteor and dependency upgrades as it is already both feature-complete and stable.

It has 50+ collections, a quite high peek connection count, an admin style backend app and a rather complicated real-time front end app in web, ios and android flavors.

This app has benefited immensely from allow/deny with clever and careful use of it through ongoworks:security (I’m probably one of the first users), simple schema, collection hooks, collection helpers and publish composite which used to be the usual suspects at the time with quite vibrant sub communities of their own.

If the client were to ask for new features today, I’d keep building on the existing infrastructure instead of refactoring allow/deny.

I would imagine the codebase to have been 50% larger (and thus much more unmanageable) had I relied on methods for everything.

I’m not necessarily relying on allow/deny today not because I find it insecure or insufficient for anything, I just have other use cases. But I never considered migrating that one large app to methods, either.

I find the argument around allow/deny not supporting complicated requirements misleading.

Any part of the meteor stack (or javascript ecosystem for that matter) can be considered better for simple/complicated and they all have their use cases and in fact until some clever guy comes up with some quite innovative implementations, many would not even realize that their former impression of it was actually wrong or incomplete.

I don’t agree with the latest security issue consisting any grounds for questioning allow/deny, either because that could have just as likely come up with any other part of the meteor stack. In fact, there had been other security-related patches on meteor (most recently with the version on node) and nobody considered deprecating them. Imagine someone suggesting deprecation of node just becaue a security issue were identified!

In short, deprecating allow/deny would not only be a disservice to those aforementioned innovative people, but it would also be a questionable turn of Meteor philosophy where backwards compatibility is advertised as a core principle.

[juho]

My 2c is that since introduction of imports Meteor has played two roles, the “get something done quick” phase and then the “make it a real product” phase. Most people seem stuck somewhere in between especially when starting up as the guide kind of recommends stuff from both worlds. I’ve been imagining a situation where meteor-tool could take your lazy MVP code written with insecure and autopublish, and turn it into split import files based on best practices. Denying all collection updates would be part of this process I imagine; turning ops into ValidatedMethods, enumerating used field keys in subs and using those as fields for publish, etc.

[babrahams]

Where did all this talk of deprecating allow/deny come from? There’s nothing wrong with it if used properly and it’s highly convenient.