jagi / meteor-astronomy

Model layer for Meteor
https://atmospherejs.com/jagi/astronomy
MIT License
604 stars 67 forks source link

Possibly bad practice in documentation #567

Closed arggh closed 7 years ago

arggh commented 7 years ago

In the documentation, part "Building with Astronomy", there's this:

...
fields: {
    /**
     * The userId of the user who created the conversation, or 'SYSTEM'
     * if created by the app.
     **/
    ownerId: {
      type: String,
      default () {
        return Meteor.userId() || 'SYSTEM';
      },
      immutable: true
    },
...

So basically, a malicious user could add new conversations to other users simply by defining the ownerId. The default value never gets asked in that case. Would it be better to just create an event hook, beforeInsert, that would populate this field with the appropriate value?

Sidenote: The same part of the docs could probably be refactored slightly to use meteorMethods ?

lukejagodzinski commented 7 years ago

That section was not written by me and I didn't review it in details but yes you're right about security problems in this example. And I agree that security should be defined in the meteorMetods. Default values shouldn't be used in such an important case as saving user ID. I will probably remove this section as I will be creating video tutorials for Astronomy this or the next week.

arggh commented 7 years ago

What do you think about using the beforeInsert hook for setting the owner field?

So, a Class definition would be like this in the essential parts:

Doc = Class.create({
  name: 'Doc',
  collection: Docs,
  secured: false,

  events: {
    beforeInsert: [
      auth.authenticate, // Check that user is logged in
      auth.setOwner // Populate the doc.ownerId field
    ],
    beforeUpdate: [
      auth.authorize // Check that user is logged in and doc.ownerId matches this.userId
    ],
    beforeRemove: [
      auth.authorize // Check that user is logged in and doc.ownerId matches this.userId
    ]
  },

  fields: {
    ownerId: {
      type: String,
      validators: CustomValidators.meteorId,
      immutable: true
    }
    ...
  }
});
arggh commented 7 years ago

Also, I do find it very beneficial to a have a complete "all bells and whistles" example of using Astronomy and all it's features. It really helps to understand the whole picture.

lukejagodzinski commented 7 years ago

You can authenticate and put all security code in before events as well as meteorMethods. It depends on if you're set secured property to true or false.