Allow/Deny Deprecation Discussion

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

[hwillson]

The idea of deprecating allow-deny has been gaining a bit of traction in several channels, but at this point it is just an idea. That being said, the deprecation discussions have been centered around how much allow-deny should be further supported / promoted by Meteor, when in its current form, it can be quite easy to misuse. I think we can all agree that the functionality behind allow-deny has definitely proved its value in Meteor. The outstanding question is, however, what can we do to improve allow-deny functionality, to make it safer to use and easier to understand, while at the same time retaining its powerful capabilities? That’s where the deprecation discussions have been headed; how can we progress this paradigm?

Putting all of this aside however, Meteor is definitely committed to backwards compatibility. Even if it was decided that allow-deny in its current form should be deprecated, it would still remain part of Meteor core (e.g. like the deprecated spiderable package). It would no longer be under active development by MDG, but it would still be usable as is (and can always be extended as a local package). Given that allow-deny functionality hasn’t really changed in years, we should be alright …

So just to re-cap; allow-deny in its current form might be deprecated at some point in the future, but that will only happen if there is a better solution to replace it. The exciting takeaway from all of this is - what does a better allow-deny look like? How can we make this awesome part of Meteor, that was likely one of the main reasons we started using Meteor, even better?

[ralof]

Given that allow-deny functionality hasn’t really changed in years, we should be alright

At least for me, “deprecated” means that we cannot rely on bug fixes and since we are talking about a mechanism that, when used, is a vital part of the systems security, then I do not think that we can be “alright” if it is being deprecated.

[hwillson]

I think the problem here is that the “deprecated” label can mean different things, based on the context it’s used in. In the case of allow-deny, “deprecation” would mean that its use is no longer recommended (which is the case now), it’s no longer being actively developed (has pretty much been the case for years), and won’t be accepting or implementing new feature requests (I’ve seen one allow-deny FR in the past couple of years). The version that exists currently would continue to work (and would still be part of the Meteor core), but it wouldn’t be taken further. Given that allow-deny has been such an integral piece of the Meteor infrastructure pie however, if it was deprecated I’m pretty sure it would still have critical issues resolved. Meteor is an active open source project, and pull requests are reviewed/accepted weekly. If a bug was found in allow-deny and a PR was submitted, even if it was deprecated, I’m pretty sure it would be accepted. This has happened with other Meteor core deprecated packages, so there is precedent.

All of this sums up what I meant by “we should be alright” .

But just a reminder - at this point all talk of deprecating allow-deny is just that, talk. Nothing has been made official, and a deprecation decision wouldn’t be made lightly (or without more community involvement/discussion).

[ralof]

Yes, but words are important. If I’d, as a consultant, would develop something for a client and that clients security officer comes back with “Hey, you are using deprecated methods in the security mechanism, you must change that!”, then it probably helps very little to say “Well, it is deprecated because it is so good and secure that they will not need to develop it anymore”

You know that could, and would, happen. Somewhere. One day.

[hluz]

The word ‘deprecated’ also commonly has the connotation with an intended discontinuity. As in: “Please find another way of doing this because we intend to remove the capability in the next functional version. If you don’t you will not be able to migrate to the new version”. Because of this, in my view we shouldn’t use the word deprecated when that is not the case, like when saying “Blaze is deprecated”.

[sabrina]

Do you have your original code from the allow/deny challenge? Can’t see it anymore since Meteorpad is no longer.