Enclose collection's allow/deny rules within schema definition

[serkandurusoy]

While designing a fairly large domain, I came to the conclusion that, incorporating collection-wide allow/deny rules within the schema definition itself could greatly improve code structuring.

Perhaps with a this similar to that of autovalue's and/or val, obj, operator like valueisallowed's.

I can't imagine how and if it should be implemented on the server, though.

[mquandalle]

As @aldeed explains in this post on meteor-core, allow rules might be ambiguous, especially when we talk about a "partial" or "limited to a single field" rule.

For instance if I define this rule:

{
  pseudo: {
    type: String,
    allow: function (val) {
      return val.length > 3;
    }
  }
}

I actually expect a pseudo to be invalid if it has 3 chars or less. But that not what this rule mean: a operation is allowed only if at least one allow rule returns true. This one doesn't but another one could returns true and the operation will be executed even if the pseudo is two chars long.

So I'm against the allow rule for fields. But deny might be a good idea.

It would allow us to define a function for a particular field that will check it on both insert and update operations. And as you say, the this context could be basically the same as the autoValue one: this.isInsert/this.isUpdate (how do we handle the upsert case?) and then a field getter this.field("fieldName").

[serkandurusoy]

I don't quite think allow is dangerous if it is used on purpose and knowing how it works.

For example, I might want a "superadmin" to be able to insert/update/remove at will, without any validation. Or I may have set a docIsDraft: true and want that to bypass any validation and be allowed insert.

Such rules better be defined as simple allow rules. Easier to follow and maintain.

So I believe, the decision to be able to use either one or both of allow and deny should be the developer's, not the package author's.

[aldeed]

@serkandurusoy, I don't think your docIsDraft example makes sense because allow/deny are applied on the collection, not individual fields, and so if any other fields had deny that would negate your allow.

Unless you were thinking this should be an entirely separate layer of allow/deny rules that are per field. I'm not sure I see the need for adding that extra level of complexity.

But I do think this could generally be helpful to add to the schema. The obvious problem is interpreting how the various rules for various fields should be combined, i.e., which takes precedence in the event of a conflict. I'd need to see a pretty thorough plan or example implementation before I'd be on board.

[mquandalle]

Yes you can use a allow rule for those two examples. As you say it makes sense for a superadmin to have every rights. But the rule will be defined on the collection object (like today), not on a single field.

The operation validation happen at the document level, so if you know all the code you can define a allow rule at this level for some cases. But at the field level that doesn't make sense anymore because even if a single field allow rule return true it doesn't mean that the operation is allowed for the all document. It just mean that the operation is allowed at a field level, ie not denied at the document level.

[aldeed]

The confusion might be because @mquandalle and I are thinking in terms of the behind-the-scenes implementation while @serkandurusoy might be thinking more of the API/schema options. It might be possible to have allow/deny schema options BUT we would implement BOTH of these through logic defined in only a single deny function behind the scenes. So the trick would be figuring out the correct (and most logical) logic for that function.

[serkandurusoy]

To clarify my point/requirement further, I am suggesting that we define allow/deny rules within the schema definition for the collection as opposed to defining them outside. Other than that, it would basically be the same (not per field, per document as it is currently already intended to be) but with an addition of this or val, obj, operator to increase versatility. It would basically be a pass-through to Meteor.Collection.allow/deny

[aldeed]

It might work, but I feel like there would be areas of confusion. I'll have to test it out and see how logical it is.

[aldeed]

@serkandurusoy, do you have some good examples of where schema allow/deny would be helpful? I'm having trouble thinking of good use cases, and I don't like to add a feature until there's a good use case to justify it. The way I see it, anything related to validating specific fields is part of schema validation, which is basically just one giant deny function already. So the main remaining purpose of allow/deny would be checking user role/permissions, but I'd like to see an example of a case where defining these role/permission checks per field in the schema is better/simpler/more maintainable than using a regular allow/deny would be.

[serkandurusoy]

Since it would basically be the same as defining allow/define outside where it currently is done, I cannot suggest a good example. My point being, this would allow the code be tidier, easier to follow. Other than that, nothing special.

Apart from this, I think my initial comments were misguided due to the fact that I misinterpreted how allow/deny works.

I think I was also looking for something along the lines of a "superallow" where if conditions are met, it would precede any deny rules as well any schema validations.

So to sum this up, if you don't find value in enclosing allow/deny rules as simple pass through in order to increase code organization, then please go ahead and close this.

[testbird]

To me it currently looks quite ok organized to call allow/deny on the collection, after defining the schema and creating it, usually in the same collection/CollectionName.js or .coffee file. A non-standard, special way to define allow/deny in a schema (with the same collection wide effect) could potentially confuse people.

[aldeed]

I'm closing this. I think that with the current custom and autoValue options and the fact that they provide the userId, there's very little need (maybe no need) for any specific field checking in allow/deny.

[geekyme]

hi @aldeed ,

I think that with the current custom and autoValue options and the fact that they provide the userId, there's very little need (maybe no need) for any specific field checking in allow/deny.

Can you give an example of not writing any allow / deny and putting the validation in the custom function?

I'm trying to do it but I can't seem to return my own custom error:

  // ... schema definitions 
  likes: {
      type: [String],
      custom: function(){
        if(!this.userId){
          return 'needLogin';
        }
      }
    },  
 // ... schema definitions
Lines.simpleSchema().messages({
    needLogin: "You must login to do this"
});
Lines.simpleSchema().namedContext().addInvalidKeys([
  {name: 'likes', type: 'needLogin'}
]);

[aldeed]

@geekyme, I'd have to see more code. Your example works for me.

[geekyme]

My app is gigantic I'm not sure which parts is affecting this behavior. I did some checks like placing console.log in custom but it doesn't seem to be logging anything to the console when I perform my collection updates.

    likes: {
      type: [String],
      autoValue: function(){
        if(this.isInsert){
          var arr = [];
          return [];
        }
      },
      custom: function(){
        console.log('test')
        if(!this.userId){
          return 'needLogin';
        }
      }
    },

I also have a collection allow callback on updates as follows:

Lines.allow({
  update: canLike,
});
canLike = function(userId, doc, fields, modifier){
  if(!isLogined(userId)) return false
  if(!modifier.$addToSet) return false // only use $addToSet
  if(!onlyHas(fields, 'likes')) return false   // only goods
  if(_.contains(doc.likes, userId)) return false // unique likes
  return true
}

[aldeed]

I don't see anything obvious, but if you can create a small example app that reproduces the issue, I can take a look.

[aldeed]

Maybe submit it as a new github issue, too.

[serkandurusoy]

@geekyme how is that relevant? I define my collections in files that are available to both client and the server while keeping the allow/deny code within the same file. Therefore, my allow/deny rules are specified everywhere like yours. I don't see any problem though.