Add support for pass-through authentication #430
Conversation
|
Can one of the admins verify this patch? |
2 similar comments
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
recreated #421 to be based on master branch |
|
@Traksewt great, thank you for the updated patch. I'm short on time today and off work tomorrow, I'll take a look next week. |
|
@slnode ok to test |
Traksewt
force-pushed
the
allow-access-token
branch
from
eef2d99 to
aa10234
Compare
|
@bajtos I'm not sure how to fix the PR Builder as I don't have access to see the Details on the build machine. |
|
Hey, sorry for the delay, I am going to review this right now.
Unfortunately those CI machines are accessible only from the internal IBM network :( The error seems unrelated to your changes to me, I think we can safely ignore it. cc @rmg is this a known problem of our CI? |
|
The changes look good to me at high level. I cleaned the code a bit for you, PTAL at b7c3564 and let me know if this is ok with you. |
There was a problem hiding this comment.
See my comments below for an explanation of the changes I made.
In addition to that, I changed var to let and const per our style guide (http://loopback.io/doc/en/contrib/style-guide.html#variable-declarations) and reworked test functions to use () => { notation.
lib/http-invocation.js
Outdated
| } | ||
| } else if (auth.accessToken) { | ||
| req.headers = req.headers || {}; | ||
| req.headers.Authorization = auth.accessToken.id; |
There was a problem hiding this comment.
I find this structure difficult to comprehend, there are too many different code paths possible. I reworked it by moving the branch where we are setting req.headers to the top and preserving the old code mostly intact.
lib/rest-adapter.js
Outdated
| // check for the options to pass along to the rest endpoint. | ||
| // It may have the access token that can be used | ||
| if (auth || !invokeOptions) return auth; | ||
| if (this.options && this.options.passAccessToken && invokeOptions && |
There was a problem hiding this comment.
Instead of checking if this.options is set everywhere, it's usually easier to ensure it's at least an empty object in the constructor.
I have also rearranged the code in this function to make it easier to follow for me (hopefully it still looks clear to you too!)
lib/rest-adapter.js
Outdated
| * @param args | ||
| * @private | ||
| */ | ||
| RestAdapter.prototype._extractAuth = function(remotes, invokeOptions) { |
There was a problem hiding this comment.
No need to pass remotes, since they are already available as this.remotes.
test/rest-adapter.test.js
Outdated
| }, { | ||
| arg: 'argName2', | ||
| type: String, | ||
| }]; |
There was a problem hiding this comment.
I reformatted the code to follow the convention we are using in LoopBack, where each arg definition is on a single line.
[
{arg: 'argName1', /*...*/},
{arg: 'argName2', /*...*/},
]
I have also renamed the variable to make the intent more clear.
test/rest-adapter.test.js
Outdated
| it('should find the first arg', function() { | ||
| var method = givenRestStaticMethod({accepts: anArg}); | ||
| expect(method.getArgByName('argName1', | ||
| ['firstArg', 'secondArg'])).to.equal('firstArg'); |
There was a problem hiding this comment.
Breaking the line in the middle of argument list make the code difficult to follow. Here is a better solution that we are already using in other parts of our codebase:
expect(method.getArgByName('argName1', ['firstArg', 'secondArg']))
.to.equal('firstArg');
|
thanks @bajtos looks good. Good to hear we can use let & const. I was used to the 2.x codebase. |
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.
This new options is disabled by default, it can be enabled by adding
the following "options" to the datasource configuration:
{
"connector": "remote",
// ...
"options": {
"passAccessToken": "true"
}
}
bajtos
force-pushed
the
allow-access-token
branch
from
4a3cbef to
e8ef11e
Compare
This failure is unrelated to the changes made by this patch. I am going to ignore it. |
|
Landed, thank you for the contribution! |
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
Checklist
guide