{"action":"created","issue":{"url":"https://api.github.com/repos/strongloop/loopback/issues/3023","repository_url":"https://api.github.com/repos/strongloop/loopback","labels_url":"https://api.github.com/repos/strongloop/loopback/issues/3023/labels{/name}","comments_url":"https://api.github.com/repos/strongloop/loopback/issues/3023/comments","events_url":"https://api.github.com/repos/strongloop/loopback/issues/3023/events","html_url":"https://github.com/strongloop/loopback/pull/3023","id":195010602,"number":3023,"title":"Inject remoting context to options arg (iteration 2)","user":{"login":"bajtos","id":1140553,"avatar_url":"https://avatars.githubusercontent.com/u/1140553?v=3","gravatar_id":"","url":"https://api.github.com/users/bajtos","html_url":"https://github.com/bajtos","followers_url":"https://api.github.com/users/bajtos/followers","following_url":"https://api.github.com/users/bajtos/following{/other_user}","gists_url":"https://api.github.com/users/bajtos/gists{/gist_id}","starred_url":"https://api.github.com/users/bajtos/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bajtos/subscriptions","organizations_url":"https://api.github.com/users/bajtos/orgs","repos_url":"https://api.github.com/users/bajtos/repos","events_url":"https://api.github.com/users/bajtos/events{/privacy}","received_events_url":"https://api.github.com/users/bajtos/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":{"login":"bajtos","id":1140553,"avatar_url":"https://avatars.githubusercontent.com/u/1140553?v=3","gravatar_id":"","url":"https://api.github.com/users/bajtos","html_url":"https://github.com/bajtos","followers_url":"https://api.github.com/users/bajtos/followers","following_url":"https://api.github.com/users/bajtos/following{/other_user}","gists_url":"https://api.github.com/users/bajtos/gists{/gist_id}","starred_url":"https://api.github.com/users/bajtos/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bajtos/subscriptions","organizations_url":"https://api.github.com/users/bajtos/orgs","repos_url":"https://api.github.com/users/bajtos/repos","events_url":"https://api.github.com/users/bajtos/events{/privacy}","received_events_url":"https://api.github.com/users/bajtos/received_events","type":"User","site_admin":false},"assignees":[{"login":"bajtos","id":1140553,"avatar_url":"https://avatars.githubusercontent.com/u/1140553?v=3","gravatar_id":"","url":"https://api.github.com/users/bajtos","html_url":"https://github.com/bajtos","followers_url":"https://api.github.com/users/bajtos/followers","following_url":"https://api.github.com/users/bajtos/following{/other_user}","gists_url":"https://api.github.com/users/bajtos/gists{/gist_id}","starred_url":"https://api.github.com/users/bajtos/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bajtos/subscriptions","organizations_url":"https://api.github.com/users/bajtos/orgs","repos_url":"https://api.github.com/users/bajtos/repos","events_url":"https://api.github.com/users/bajtos/events{/privacy}","received_events_url":"https://api.github.com/users/bajtos/received_events","type":"User","site_admin":false}],"milestone":null,"comments":10,"created_at":"2016-12-12T15:57:14Z","updated_at":"2016-12-14T18:25:33Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/strongloop/loopback/pulls/3023","html_url":"https://github.com/strongloop/loopback/pull/3023","diff_url":"https://github.com/strongloop/loopback/pull/3023.diff","patch_url":"https://github.com/strongloop/loopback/pull/3023.patch"},"body":"Based on the feedback in #2762 and #1676, I would like to discuss a slightly improved proposal for [Injecting Remote Context via Options](https://github.com/strongloop/loopback/issues/1495).\r\n\r\nImportant points I am addressing:\r\n\r\n 1. The \"options\" argument must be initialised in such way that HTTP clients cannot inject unwanted properties. E.g. the client must not be able to inject `options.accessToken` to override the access token set by the authorisation layer.\r\n 2. The injected \"options\" argument must be \"hidden\" and excluded from the Swagger resource generated by `loopback-swagger`\r\n 3. Developers must be able to configure the \"options\" argument via model JSON files\r\n 4. It should be easy to customise the way how the \"options\" argument is built, preferably via a mixin. It should be easy to share and reuse such customisation via a loopback component (a standalone npm package).\r\n 5. Ideally, multiple mixins can contribute additional context properties.\r\n 6. Some context properties (e.g. full `currentUser`) may require async computation. This use case should be supported too.\r\n\r\nBecause we are unlikely to get this new API right on the first try, and because experimenting with features is difficult in LoopBack core, I would like to ship a minimal infrastructure that will allow 3rd party components to experiment with different approaches for building the \"options\" object, as we are already seeing to happen:\r\n\r\n - https://github.com/snowyu/loopback-component-remote-ctx.js\r\n - https://gist.github.com/ebarault/fdc39c5d07cc40f08664256f9e00905a\r\n\r\n**Solution details**\r\n\r\nMethods accepting an `options` argument should declare this argument in their remoting metadata and use a special value `optionsFromRequest` as the `http` mapping. This way there is no magic happening in `beforeRemote` hooks and method authors are in full control of how the `options` argument is (or is not) initialized.\r\n\r\nUnder the hood, this \"magic string\" value is converted by `Model.remoteMethod` to a function for strong-remoting, i.e. strong-remoting receives `accepts.http` as a function, which is already a well-supported use case.\r\n\r\n\r\nAn example using a trimmed-down metadata of `PersistedModel.create`:\r\n\r\n``` js\r\nPersistedModel.remoteMethod('create', {\r\n  accepts: [\r\n    {arg: 'data', type: 'object', model: typeName, http: {source: 'body'}},\r\n    {arg: 'options', type: 'object', http: 'optionsFromRequest'},\r\n  ],\r\n  returns: {arg: 'data', type: typeName, root: true},\r\n  http: {verb: 'post', path: '/'},\r\n});\r\n```\r\n\r\nAt runtime, `optionsFromRequest` invokes a static Model method called `createOptionsFromRemotingContext` to build the argument value. Subclasses and mixins are free to override this method, e.g. to return a class instance as shown in https://gist.github.com/ebarault/fdc39c5d07cc40f08664256f9e00905a.\r\n\r\nIn line with my goal of keeping this feature minimal, the built-in method returns options with a single property `accessToken`:\r\n\r\n```js\r\n  Model.createOptionsFromRemotingContext = function(ctx) {\r\n    return {\r\n      accessToken: ctx.req.accessToken,\r\n    };\r\n  };\r\n```\r\n\r\n**Extending the \"options\" object**\r\n\r\n 1. Override `createOptionsFromRemotingContext` in your Model:\r\n\r\n    ```js\r\n    MyModel.createOptionsFromRemotingContext = function(ctx) {\r\n      var base = this.base.createOptionsFromRemotingContext(ctx);\r\n      return extend(base, {\r\n        currentUserId: base.accessToken && base.accessToken.userId,\r\n      });\r\n    };\r\n    ```\r\n\r\n 1. Use a \"beforeRemote\" hook\r\n\r\n    ```js\r\n    MyModel.beforeRemote('saveOptions', function(ctx, unused, next) {\r\n      if (!ctx.args.options.accessToken) return next();\r\n      User.findById(ctx.args.options.accessToken.userId, function(err, user) {\r\n        if (err) return next(err);\r\n        ctx.args.options.currentUser = user;\r\n        next();\r\n      });\r\n    });\r\n    ```\r\n\r\n 3. Use a custom strong-remoting phase that's run before any remoting hooks are triggered\r\n\r\n    ```js\r\n      app.remotes().phases\r\n        .addBefore('invoke', 'options-from-request')\r\n        .use(function(ctx, next) {\r\n          if (!ctx.args.options.accessToken) return next();\r\n          User.findById(ctx.args.options.accessToken.userId, function(err, user) {\r\n            if (err) return next(err);\r\n            ctx.args.options.currentUser = user;\r\n            next();\r\n          });\r\n        });\r\n    ```\r\n\r\n**Tasks**\r\n\r\n- [x] Implement `optionsFromContext`\r\n- [x] Modify `PersistedModel` methods\r\n- [ ] Modify relation methods\r\n- [ ] Propagate `options` in the authentication layer, see https://github.com/ebarault/loopback-sandbox/commit/09b9a8ccb76d426f8b3e20bc4b4f774f5a25fe61  \r\n\r\n**What's next**\r\n\r\nI would like to get more feedback on my proposal before moving forward.\r\n\r\n - @fabien @ebarault @snowyu @doublemarked  I hope that the solution proposed here will allow you to simplify the implementation of your custom \"options\"/\"remotingContext\" builders, so that you can remove the code injecting the \"options\" arg and focus on building the \"options\" arg instead. What's your opinion? Is there anything missing in my proposal, something that you would have to work-around in your mixins/components?\r\n - @raymondfeng @ritch What's your take? I know this proposal is probably not ideal and we may want to implement a different/better solution in long term. However, my intention here is to offer a reasonable solution that LoopBack users can start using *now*, instead of waiting for a hypothetical unicorn from the future.\r\n - @pulkitsinghal @diadara @azatoth @digitalsadhu @dancingshell @a-legrand @bostondv @yagobski @nidhhoggr @ryedin @ezraroi @rimvydas-urbonas @Morriz @wprater @eggerdo @hotaru355 @kevinsschmidt @tomhallam @pkgulati @souluniversal @pbtcjasper @denkan @mrfelton @zanemcca @guanbo @pongstr  @skyserpent @pkgulati @Saganus @chris-hydra @aorlic @fperks I am pinging you, since you were involved in the discussion at some point in the past. Do you have any opinion on my proposal? (I apologise to everyone who may feel spammed.)"},"comment":{"url":"https://api.github.com/repos/strongloop/loopback/issues/comments/267114202","html_url":"https://github.com/strongloop/loopback/pull/3023#issuecomment-267114202","issue_url":"https://api.github.com/repos/strongloop/loopback/issues/3023","id":267114202,"user":{"login":"ebarault","id":12000935,"avatar_url":"https://avatars.githubusercontent.com/u/12000935?v=3","gravatar_id":"","url":"https://api.github.com/users/ebarault","html_url":"https://github.com/ebarault","followers_url":"https://api.github.com/users/ebarault/followers","following_url":"https://api.github.com/users/ebarault/following{/other_user}","gists_url":"https://api.github.com/users/ebarault/gists{/gist_id}","starred_url":"https://api.github.com/users/ebarault/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ebarault/subscriptions","organizations_url":"https://api.github.com/users/ebarault/orgs","repos_url":"https://api.github.com/users/ebarault/repos","events_url":"https://api.github.com/users/ebarault/events{/privacy}","received_events_url":"https://api.github.com/users/ebarault/received_events","type":"User","site_admin":false},"created_at":"2016-12-14T18:25:33Z","updated_at":"2016-12-14T18:25:33Z","body":"@bajtos : so... yes, indeed you're right, i was a bit too hastly in my tests, my custom method was not triggering the operation hook at all, i was misguided by the logs from the first call to user model in the beforeRemoteMethod hook (enriching the ctx.options with currentUser). **My bad**.\r\n\r\nwhen propagating the ctx inside the custom method as you did with findById, all is good.\r\nI also tested when authorizing the findById method to a custom dynamic role through a custom role resolver : injection in the custom role resolver is also available in operation hooks.\r\n\r\n---\r\nAs for the business requirements underneath this technical demand: generally speaking once you decide to offload a certain business logic outside the remote method themselves, directly inside model operation hooks, you expect to find similar information in those hooks whatever the remote method at the origin of the operation on the model.\r\n\r\nGenerally speaking : it can be for write purposes as described by @PaddyMann, such as inserting information on who performed the change on data, it can also be for read purposes such as more precisely filtering the data returned according to the destinating user, which is complementary to access control.\r\n\r\n**But is there anything more to be done here?** your pr seems to solve this requirement. Yes, it's not direct since we need to propagate the ctx from the custom remote method to the built-in method invocation, but do we have another solution without introducing some kind of magic under the hook (though not easily documentable, maintainable, and so on)?\r\n\r\n--- \r\nabout built-in smart role resolvers: yes having the option to inject ctx.options args directly at authorization stage in built-in resolvers would be nice, in general manner, not only for role $owner.\r\nNote: this can already be done with custom role resolvers and either your pr here, or the solution from community contribution. I already use this kind of injection to get more context on user's authorization in methods or hooks.\r\n\r\n"}}
{"actor":{"display_login":"ebarault"}}