Skip to content
This repository has been archived by the owner on Oct 24, 2021. It is now read-only.

"Methods" package #54

Closed
tmeasday opened this issue Oct 23, 2015 · 7 comments
Closed

"Methods" package #54

tmeasday opened this issue Oct 23, 2015 · 7 comments
Assignees
Labels

Comments

@tmeasday
Copy link
Contributor

Build a wrapper around methods, of the form:

Todos.methods.insert = new Method({
  name: '/todos/insert',
  validator(doc) => { Todo.Schema.check(doc); },
  permission(doc, userId) => {
    if (!doc.ownerId == userId) {
      throw new ForbiddenException("...");
    }
  },
  body(doc) {
    Todos.insert(doc);
  }
});
@stubailo
Copy link
Contributor

Yeah I dig this. Did you assign it to me since I should build it?

@aldeed
Copy link
Contributor

aldeed commented Oct 23, 2015

This is cool because I was planning to make a package almost exactly like this soon, but I'll let you do it. :) Here are a couple other things I was going to include:

runRestricted: true

Turns on restricted mode so that all the insert/update/remove in the method body go through allow/deny checks. Allows pattern of defining all security in one place, which I do using ongoworks:security pkg. (I also added a Security.can feature to that package recently, which can be used in the method permission for isomorphic security.)

permission: false

If permission is omitted, I was going to throw an error. That way you need to explicitly set permission: false to expose. A big help to reduce security holes. Also permission: "loggedIn" could be supported.

url: '/'

I was also going to try to merge @stubailo's simple:rest stuff in, checking for json-routes pkg if options like url are passed. But actually I think a better approach would be to provide a method of registering custom properties, and then simple:rest can depend on this and do add-on support for url, etc.

@stubailo
Copy link
Contributor

Yeah, this really would make simple:rest a lot more legit instead of monkey-patching all of DDP!

@tmeasday
Copy link
Contributor Author

@stubailo I was umming and ahhing and then I just assigned you. Wasn't sure about what our decision on outside-of-core coding work assignment was. I figured this one was your baby anyway.

@aldeed:

  1. Re: restricted - I think we are de-emphasizing allow/deny in the guide so we probably won't build that, but'll take PRs and just not mention that feature in the guide. This seems a good approach.

  2. I like the permission required + shortcutting names. Although I think I like something simpler and less magic like:

    // somewhere global
    Permissions.loggedIn = (userId) => { return !!userId; }
  3. Re: URLs -- so the bit that maybe didn't come through yet is that we were planning on the method name being the URL automagically via simple:rest. It's all part of @stubailo's secret plan to turn Meteor into Rails ;)

@aldeed
Copy link
Contributor

aldeed commented Oct 25, 2015

@tmeasday that all sounds good. I figured the secret plan might be something like that based on the way the name was written. :) But simple:rest would still need to add other options and should definitely provide a way to disable http route. Actually, in the end this methods pkg would just be a slight rewrite of the simple:rest pkg.

@stubailo
Copy link
Contributor

Yeah the biggest limitation until now was that there was no way to provide options to a single method. I think declaring methods individually will be a lot better than having a Meteor.methods dictionary.

@mitar
Copy link
Contributor

mitar commented Oct 31, 2015

I think that any Method wrapper should provide an easy extension point to wrap existing functionality. JavaScript constructors sadly cannot be monkey-patched. So it is great if a constructor calls some other method which can then be.

So in general, provide an API that Method can be extended for all users of Method please.

@stubailo stubailo closed this as completed Jan 8, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants