Proof that no one uses Allow/Deny

It’s in the Angular-Meteor tutorial

What’s the recommendation now? I kind of liked having a check like this

// Simple checks to ensure that the user is logged in before making changes.
Tasks.allow({
  insert: (userId, doc) => !!userId,
  update: (userId, doc, fields, modifier) => !!userId,
  remove: (userId, doc) => !!userId
})

The recommendation is to use ValidatedMethod and you can use a mixin to ensure login: https://github.com/meteor/validated-method#community-mixins

It’s quite unfortunate that the Angular-Meteor tutorial uses that feature.

3 Likes

I presume we still need to do a .deny or is it deny by default now? At least I can make the adjustments to boilerplate to remove it.

You can use deny just in case to make sure no other code or package calls allow. But by default client writes are denied. See here: http://guide.meteor.com/security.html#allow-deny

I got like 90% of the way to making allow-deny completely removable from Meteor, but never quite finished it.

3 Likes

Wow – nice. I didn’t think to use deny in order to prevent other packages from using allow. It’s a bit poetic – using allow/deny api so that you don’t use it… :confused: And you should totally finish that :sweat_smile:

2 Likes

@sashko pls take a look at the other thread.

I think allow/deny can make it very elegant to write isomorphic code, but since the current behavior is only server specific it is therefore not really useful.

In reality, it is Meteor.methods that should be deprecated because it really is just the same old client/server pattern.

1 Like

Note that allow/deny is actually implemented on top of methods, so it is a strictly less powerful pattern.

4 Likes

I strongly disagree.

When you want a finite amount of things from an infinite set.

Choosing what you want out of an infinite set of patterns is far more convenient than choosing what you don’t want out of an infinite set of patterns.

Allow/Deny is a beat around the bush. If you don’t want a user to update the location.lat field, but want them to update the location.lng field, and the name.first field but not the name.last field and definitely not the location.lng field is the value isn’t between [-180, and 180], and you only want them to update the posts field if the post in question satisfies a certain structure and has no words in the array [shit, damn, ...], and you definitely cannot have them messing around with the password field unless it has at least 10 characters, 3 uppercase, four special characters, and no spaces… eh…

JUST WRITE METHODS!!!

3 Likes

Suppose Meteor.methods were deprecated by an act of god :smiley: , will it be possible just to use Meteor.isClient and Meteor.isServer and allow/deny to achieve the same result? I think so… simplistically, I think just making every js function defined in the common area a Meteor.method may do it. Thanks!

I really have to disagree with the sentiment that somehow allow/deny is inferior to meteor methods. Obviously this will all become moot when Apollo is finally polished and migrated to, but for the time being allow/deny rules can be easily used in conjunction with SimpleSchema to create a very secure application in about half the time it would take you to implement the same mess using a method call. I proved this quite well with my submission to the Discover Meteor Allow/Deny Security Challenge and I currently use this technique for all of the packages in the Socialize set. Despite this fact the same nonsense is repeated time and time again until it becomes gospel. :disappointed:

3 Likes

We use allow / deny quite a bit. Like @copleykj mentioned, it is a secure and easy way to restrict write access.

At its infancy, Meteor was labelled insecure (I wasn’t around, so can’t comment). A big part of proper security is making it easy for developers to harden their code / server / DB. Asking us to start writing methods will surely result in some (or many) insecure apps.

Again, when dealing with security, it has to be SUPER easy for an idiot to harden their code. The moment you are labelled insecure you are toast. It’s a human thing, not just code.

1 Like

Given two arbitrary code bases that are currently not secure, one using allow/deny and the other using methods, I’d much rather be given the job of securing the methods one. Here would be my workflow:

  1. List all of the methods
  2. Go through them one by one and enumerate the possible inputs, and the desired effects in those cases
  3. Write tests for all of them to check various kinds of inputs, and use argument validation to disallow any other kind of input
  4. Done

I can’t think of such a workflow for client-side updates and allow/deny. Is there one?

1 Like

@sashko,

Maybe I am missing something.
If client-side updates went through a single pipe (allow/deny) your workflow would simply be:

  1. Check your allow/deny code

Sounds too easy? Look forward to your reply.

The problem is exactly that. “Check your allow/deny code” is a hard to do when it’s something you wrote yesterday, very hard when it’s something from last week. You’ll be a hero fixing others allow/deny rules.

1 Like

Yeah I mean the workflow for securing an allow-deny app is something like:

  1. Predict all possible updates that could be done from the client
  2. Write a function that somehow secures all possible forms of input that could come in
  3. Hope that none of your client code is dynamically generating mongo update queries such that they might fail in certain cases
  4. Cry?

Sure, if you use client updates in a very principled way and stick to only $set for example, then maybe it’s not so bad. But in that case, why give yourself all of the rope of sending Mongo queries over the wire if you could just send pre-agreed upon arguments, AKA method calls?

Plus, good luck ever changing your database schema if all of your client operations assume a specific set of fields for their updates.

@sashko & @josmardias

I haven’t faced the issues you are describing. I think it’s because we are very principled in our design and in our usage, so our allow / deny mostly look at permissions.

Writing methods does work, but it’s cumbersome. Not only that, but dev time is so much shorter when you can have real client-side updates.

1 Like

I’ve heard this argument, but I’d really love to have some sort of head-to-head competition where two people try to build a relatively complex yet secure app, one with methods and one with allow/deny. I’d put my money on the method approach resulting in a secure, functional, and well-tested app faster. But I could be wrong, of course.

1 Like

OK, just modified my boilerplate code to use ValidatedMethod. Reminds me of JEE programming where we hide direct database access and wrap everything in a DAO. Well that’s what I am more familiar with to begin with so I think that’s fine if I am wearing my security conscious hat, rather than let’s-get-the-bloody-product-out-the-door-first hat. :slight_smile:

I looked at the guide on how it is implemented, sort of left a bit of a bad taste in my mouth though. Even the way the secret server code was implemented in the guide didn’t really seem right. So after a few iterations, I came up with something else. (I only show the example of one method, but the concept is the same for other methods)

import { Mongo } from 'meteor/mongo'
import { ValidatedMethod } from 'meteor/mdg:validated-method'
import { LoggedInMixin } from 'meteor/tunifight:loggedin-mixin'
import { SimpleSchema } from 'meteor/aldeed:simple-schema'

class TasksCollection extends Mongo.Collection {
  constructor () {
    super('tasks')
    this.insertTaskMethod = 
      new ValidatedMethod({
        name: 'tasks.insert',
        mixins: [LoggedInMixin],
        checkLoggedInError: {
          error: 'notLoggedin'
        },
        validate: new SimpleSchema({
          text: {
            type: String,
            optional: false
          }
        }).validator(),
        run: (params) => {
          super.insert(params)
        }
      })
  }
  get insertTask () {
    return this.insertTaskMethod
  }
}
export const Tasks = new TasksCollection()

When invoking, I used

Tasks.insertTask.call(this.entry, (err) => {
  if (err) {
    this.error = err
  }
  this.state.go('^.list')
})

For @Urigo This pattern should be closer to what you would need in socially.

@sashko the one thing that is annoying in the pattern is the need for .call, becauseI can’t seem to get away with return new ValidatedMethod({..}).call to just get the call function, I wonder if there’s a way around it, maybe Function.prototype.apply() or something.

Also for those who are thinking of using the same pattern I have above, if you use insert, update, delete … you won’t be able to easily change the run function on the server side like the following example of secret sever code

Tasks.insertTask.run = (params) => {
  Tasks.insert({
    text: params.text,
    createdOn: new Date()
  })
}
1 Like

Yeah we probably need to add something like this.call.bind(this) in the constructor here: https://github.com/meteor/validated-method/blob/master/validated-method.js

Glad to take a PR!

Also, ValidatedMethod doesn’t have to be the end-user API - you could easily think of some nice wrapper packages to make it a lot more concise.

The main point here is, we’re using a real programming language. Don’t like the tools you have? Make new tools!

1 Like

Problem with making new tools is I need to support them, I can’t just tell the next guy just go follow @sashko on github and the forums. :slight_smile:

But honestly with the exception of the mixins and name and the fact that I have to keep on saying new SimpleSchema every time it looks good already.

1 Like