Use lb access token #421

Traksewt · 2017-09-14T12:57:00Z

Description

Allow the rest connector to call other loopback services by passing on the user authorization from the options where available. This is the fallback if no other authorization is specified.

This allows loopback servers to communicate securely with other loopback servers by passing on the auth token. Useful for using loopback as microservices.

Related issues

connect to Pass auth header from client to loopback rest-connector loopbackio/loopback-connector-rest#89
connect to Add accessToken support loopback-connector-remote#3

Checklist

New tests added or existing tests modified to cover all changes
Code conforms with the style
guide

slnode · 2017-09-14T12:57:02Z

Can one of the admins verify this patch?

slnode · 2017-09-14T12:57:02Z

Can one of the admins verify this patch?

slnode · 2017-09-14T12:57:03Z

Can one of the admins verify this patch?

bajtos

Hello @Traksewt, thank you for the pull request. The changes looks mostly good at the first quick read. I am little bit worried that the tests are covering only units in isolation, I'd like to see a test doing real REST invocation to make sure all things fit together as expected.

I think this is the place where to add such test:

strong-remoting/test/rest.test.js

Lines 2379 to 2380 in 565aba1

    
           describe('client', function() { 
        
             describe('call of constructor method', function() {

bajtos · 2017-09-14T14:42:34Z

lib/rest-adapter.js

+  // check for the loopback options to pass along.
+  // It may have the access token that can be used
+  if (!auth && args.length) {
+    var options = args[args.length - 1];


This assumes that args do not contain the callback as the last argument, can we rely on this? I don't remember how exactly remote invocation works.

bajtos · 2017-09-14T14:43:32Z

lib/rest-adapter.js

+ * @param remotes
+ * @param args
+ */
+RestAdapter.prototype.extractAuth = function(remotes, args) {


I think this method should be private - the name should start with underscore and jsdoc should include @private annotation.

bajtos · 2017-09-14T14:44:01Z

lib/rest-adapter.js

+  var auth = remotes.auth;
+  // check for the loopback options to pass along.
+  // It may have the access token that can be used
+  if (!auth && args.length) {


Let's simplify this method by using reverse logic please.

if (auth || !args.length) return auth; // process options ...

bajtos · 2017-09-14T14:47:21Z

@slnode ok to test

Traksewt · 2017-09-17T11:48:43Z

Thanks @bajtos, I was having trouble testing the REST in that unit test. As the fix was the setting auth for one loopback server hitting another loopback server, should I be creating an extra express/loopback (so 2 in total) in this unit test? I couldn't tell of how I could test the rest adapter setting the access token using the single server.

I also added a config to enable passing through the LB Access Token, otherwise, people may inadvertently send their loopback token to other 3rd party REST endpoints.

bajtos · 2017-09-20T12:27:22Z

lib/remote-objects.js

-  var adapter = new Adapter(this, options);
+  // send the set options for this remote object and
+  // the dynamic options passed in to create the adapter
+  var adapter = new Adapter(this, Object.assign({}, this.options, options));


This does not look right to me. Remoting options are different from transport/adapter-specific options. Why is this change needed? Can we revert it please?

OK Bajtos. Thanks for the heads up. I got a neater solution using the actual Rest properties that doesn't need config code changes. the 'options.rest' config goes directly to the rest adapter already.

bajtos · 2017-09-20T12:28:24Z

lib/remote-objects.js

@@ -132,7 +134,7 @@ RemoteObjects.prototype.connect = function(url, name) {
  }

  var Adapter = this.adapter(name);
-  var adapter = new Adapter(this);
+  var adapter = new Adapter(this, this.options);


As I mentioned above, this.options are usually different from adapter options. For example (and IIRC), the configuration of REST adapter is specified in rest property of remoting options.

I think we should add options argument to connect, to make this consistent with handler method.

OK I don't need this at all anymore, not even for the connect. As the Adapter gets it off the rest config.

bajtos · 2017-09-20T12:31:21Z

lib/rest-adapter.js

+ * @param args
+ * @private
+ */
+RestAdapter.prototype._extractAuth = function(remotes, args) {


On the second thought, this implementation is very loopback-specific. We are trying to keep strong-remoting decoupled from loopback itself - this package should provide extension hooks allowing loopback to inject loopback-specific behaviour.

In this particular case, the function parsing the accessToken from the options argument should live in LoopBack. It can be provided to RestAdapter for example via the options object which contains other related configuration too.

The main difference for this one, is that it is executed per request. E.g. for access tokens it changes for each different user hitting the server, that is why it doesn't fit in the other options object that is used to initialise the adapter.

I'll think about it to see if I can set this through loopback code. One option is making it like a basic function to get the auth, that I can override in some sort of boot script that iterates over all datasources and sets the more complex extractAuth programmatically on the adapter. Apart from this boot script solution feeling a bit hacky, I was also hoping that this might have been useful to developers that want loopback servers to hit other loopback servers securely (e.g. microservices). I was hoping it would have worked out of the box (after setting the config flag). Open to ideas here.

I did your solution below of using the meta to find the options. All it is looking for is an accessToken in an options object. I made a generic way to get an arg by name on RestMethod.

bajtos · 2017-09-20T12:32:38Z

lib/rest-adapter.js

+  // check for the loopback options to pass along.
+  // It may have the access token that can be used
+  if (auth || !args.length) return auth;
+  var options = args[args.length - 1];


I am not sure if it's a good idea to always assume the last argument is options. Can we perhaps parse remoting metadata for the invoked method to verify that it has options argument at the expected position?

Done, with a getArgByName in RestMethod.

bajtos · 2017-09-20T12:34:14Z

test/http-invocation.test.js

+          Authorization: 'abc'
+        }
+      };
+      expect(inv.createRequest()).to.eql(expectedReq);


This assert is checking too much. If we change the underlying implementation in the future (e.g. by adding more fields to the returned request), then this test will fail despite the fact that the implementation may still be correct.

Please check only that request headers contain the expected authorization.

expect(inv.createRequest().headers).to.have.property('Authorization', 'abc');

Np. I was copying all the following methods, but this change is more stable.

bajtos · 2017-09-20T12:35:10Z

I also added a config to enable passing through the LB Access Token, otherwise, people may inadvertently send their loopback token to other 3rd party REST endpoints.

👍

bajtos · 2017-09-20T12:36:40Z

As the fix was the setting auth for one loopback server hitting another loopback server, should I be creating an extra express/loopback (so 2 in total) in this unit test? I couldn't tell of how I could test the rest adapter setting the access token using the single server.

I think a single server is enough. You can create the access token via javascript API by calling something like server.models.User.create and server.models.User.login, and then pass this access token in the options argument of the client method (doing remote invocation).

Traksewt · 2017-10-12T05:13:18Z

@bajtos please check

bajtos · 2017-10-13T13:54:00Z

@Traksewt thank you for the pull request, I'll take a look next week. I see that our CI builds are failing because one of our dependencies dropped support for Node.js 0.x, I think I'll need to fix our CI config before landing this patch, to get a green build first.

bajtos · 2017-10-16T10:24:45Z

Here is a pull request that should fix our CI in the 2.x branch: #428

bajtos · 2017-10-16T10:29:53Z

@Traksewt sorry for realizing this only now: the changes you are proposing are effectively adding a new feature. Per our LTS rules, we are not adding any new features to the version lines that are in LTS or maintenance mode, and the version line 2.x is in LTS right now.

Could you please rework your patch on top of the master (version 3.x) branch?

GitHub made it recently possible to change the target branch of a pull request: click in "edit" button on the right end of pull request title, and pick master from the branch drop-down. I haven't done this myself before, I think you may need to run git rebase -i master and git push -f afterwards to fix the commit history.

Traksewt · 2017-10-19T03:48:43Z

There were too many conflicts so made a new pull #430

Traksewt added 2 commits September 14, 2017 21:50

handle auth in options

dce1438

add UTs

c56fa65

bajtos suggested changes Sep 14, 2017

View reviewed changes

bajtos self-assigned this Sep 14, 2017

code review changes

ae31137

Traksewt mentioned this pull request Sep 18, 2017

Add support for configuring remoting options strongloop/loopback-connector-remote#77

Merged

2 tasks

code review changes

140f7b6

bajtos suggested changes Sep 20, 2017

View reviewed changes

Traksewt added 2 commits October 12, 2017 15:51

code review changes

81220a8

removing logs

810fd84

Traksewt changed the base branch from 2.x to master October 18, 2017 23:31

Traksewt changed the base branch from master to 2.x October 19, 2017 00:08

Traksewt mentioned this pull request Oct 19, 2017

Add support for pass-through authentication #430

Merged

2 tasks

Traksewt closed this Oct 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use lb access token #421

Use lb access token #421

Traksewt commented Sep 14, 2017 •

edited

Loading

slnode commented Sep 14, 2017

slnode commented Sep 14, 2017

slnode commented Sep 14, 2017

bajtos left a comment

bajtos Sep 14, 2017

bajtos Sep 14, 2017

bajtos Sep 14, 2017

bajtos commented Sep 14, 2017

Traksewt commented Sep 17, 2017

bajtos Sep 20, 2017

Traksewt Oct 12, 2017

bajtos Sep 20, 2017

Traksewt Oct 12, 2017

bajtos Sep 20, 2017

Traksewt Oct 12, 2017

Traksewt Oct 12, 2017

bajtos Sep 20, 2017

Traksewt Oct 13, 2017

bajtos Sep 20, 2017

Traksewt Oct 12, 2017

bajtos commented Sep 20, 2017

bajtos commented Sep 20, 2017

Traksewt commented Oct 12, 2017

bajtos commented Oct 13, 2017

bajtos commented Oct 16, 2017

bajtos commented Oct 16, 2017

Traksewt commented Oct 19, 2017

	describe('client', function() {
	describe('call of constructor method', function() {

Use lb access token #421

Use lb access token #421

Conversation

Traksewt commented Sep 14, 2017 • edited Loading

Description

Related issues

Checklist

slnode commented Sep 14, 2017

slnode commented Sep 14, 2017

slnode commented Sep 14, 2017

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Sep 14, 2017

Traksewt commented Sep 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Sep 20, 2017

bajtos commented Sep 20, 2017

Traksewt commented Oct 12, 2017

bajtos commented Oct 13, 2017

bajtos commented Oct 16, 2017

bajtos commented Oct 16, 2017

Traksewt commented Oct 19, 2017

Traksewt commented Sep 14, 2017 •

edited

Loading