Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Misc Header and Auth support #11

Closed
wants to merge 1 commit into from
Closed

Add Misc Header and Auth support #11

wants to merge 1 commit into from

Conversation

BerkeleyTrue
Copy link

This adds support for authorization and misc headers.
Misc Headers can be added in the remote connector settings
in client/datasource.json.
Authorization happens automatically using local/session storage.
While headers are tested and working, authorization requires
two other PRs to work.

  1. Loopback needs to use User.remoteMethod instead of
    loopback.remoteMethod in common/models/user.js
    see Use User.remoteMethod instead of loopbacks method loopback#943
  2. Strong-Remoting needs to support hooks when
    using http.prototype.invoke
    see Add remote hooks to restAdapter.invoke method strong-remoting#152

See strongloop/strong-remoting#105

@BerkeleyTrue
Copy link
Author

Any feedback on this?

@BerkeleyTrue BerkeleyTrue changed the title Add Misc Header and Auth support [WIP] Add Misc Header and Auth support Jan 12, 2015
@bajtos
Copy link
Member

bajtos commented Jan 13, 2015

Hey @BerkeleyTrue, sorry for not responding sooner. We use waffle.io to track PRs across all loopback-related repositories and it seems that the integration was recently :(

@ritch @kraman I think you two are best suited to review the patch, PTAL.

@bajtos
Copy link
Member

bajtos commented Jan 13, 2015

@BerkeleyTrue please rebase the patch on top of the current master (see #10).

@BerkeleyTrue
Copy link
Author

@bajtos Done

var keys = Object.keys(settings.headers);
for (var i = 0; i < keys.length; i++) {
this.headers.push({
http: { source: 'header', },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stray comma

This adds support for authorization and misc headers.
Misc Headers can be added in the remote connector settings
in client/datasource.json.
Authorization happens automatically using local/session storage.
While headers are tested and working, authorization requires
two other PRs to work.

  1. Loopback needs to use User.remoteMethod instead of
     loopback.remoteMethod in common/models/user.js
     see strongloop/loopback#943

  2. Strong-Remoting needs to support hooks when
     using http.prototype.invoke
     see strongloop/strong-remoting#152

See strongloop/strong-remoting#105
@BerkeleyTrue
Copy link
Author

@ritch Setting headers using remote hook.

@ritch
Copy link
Member

ritch commented Feb 4, 2015

Need to remove the opinionated User.login / logout hooks before merging this.

};

if (typeof window !== 'undefined') {
_localStorage = window.localStorage;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think storage belongs in this module. All we need to do in the connector itself is ensure the token and any other metadata required by authentication is set on the context and or request.

Everything else is really outside the domain of this module. I think it actually belongs in the LoopBack AccessToken or User models.

I think we just need something like this in the connector itself:

// the dataSource
var remote = loopback.createDataSource();

// set the accessToken to be sent along with each request / invocation
remote.setAccessToken(accessToken);

// or set the accessToken during creation (the Connector would just call `remote.setAccessToken()`)
var remote = loopback.createDataSource({
  connector: 'remote',
  url: 'http://localhost:3000/api',
  accessToken: accessToken
});

@bajtos @kraman @BerkeleyTrue

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think storage belongs in this module. All we need to do in the connector itself is ensure the token and any other metadata required by authentication is set on the context and or request.

I agree that browser-specific stuff should be left out of this pull request.

@ritch your proposal may be putting too much burden on the users of this module. So we may end up with including some part of the browser storage in the connector itself. However, even if that happens, then it should be build on top of the non-browser-specific API provided by this PR.

Here is my proposal:

  • In this PR, implement the API proposed by @ritch above
  • Once it is landed, look into ways how to persist and restore the access token in the browser.

@bajtos
Copy link
Member

bajtos commented Apr 2, 2015

@ritch is this patch still relevant after strongloop/strong-remoting#181 was landed in strong-remoting?

@slnode
Copy link

slnode commented Apr 2, 2015

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@bajtos
Copy link
Member

bajtos commented Apr 2, 2015

@slnode ok to test

@bajtos
Copy link
Member

bajtos commented Apr 2, 2015

@ritch @BerkeleyTrue I think this patch is no longer relevant, see strongloop/strong-remoting#105 (comment)

Code snippet showing how to configure authentication:

app.dataSources.remote.connector.remotes.auth = {
  bearer: new Buffer(token).toString('base64'),
  sendImmediately: true
};

@ritch
Copy link
Member

ritch commented Apr 2, 2015

Closing this for now. If we want to add miscellaneous header support let's create an issue to track that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants