senecajs / seneca-auth

A Seneca user authentication plugin for Hapi and Express
http://senecajs.org
MIT License
33 stars 29 forks source link

Add support for token based Auth in addition to cookie #68

Open girishla opened 8 years ago

girishla commented 8 years ago

Add support for token based Auth

At the moment seneca-auth supports only a cookie(session) based authentication scheme. In order to support API-Only servers for native mobile apps (which is a common use-case in the Hapi world), seneca-auth should ideally support at least one additional token based scheme.

This can be implemented Using Hapi's chained strategies like so:

server.auth.strategy('session', 'cookie', token_auth_opts)
server.auth.strategy('token', 'bearer-access-token', cookie_auth_opts)
server.auth.default({
    strategies: ['token','session']
})

In the above chained case, Hapi would first look for the specified token in the header, and if not found would then look for a cookie. The seneca login token can be used as the bearer token and as normal for session management.

Setting chained strategies as default would mean that endpoints that must not be restricted such as /auth/login should have auth set to false on the route options.

If we do want to take this forward, I've taken a brief look at the necessary changes - it looks like in addition to the small changes to seneca-auth, I believe we would need the following minor changes:

1) minor changes to auth-token-cookie to add and register the chained strategy.
2) One liner change to seneca-web (line 452)

add the || part to pass auth=false to the route config - like so:

if ((routespecs.auth && routespecs.auth !== 'none') || (routespecs.auth===false)) {
  hapi_route.config.auth = routespecs.auth

    console.log('modified seneca-web...:',hapi_route)
  internals.server.route( hapi_route )
  done()
}
3) One liner change to seneca-local-auth (hapi-local-auth.js line 23)

add auth:false to avoid the default chained strategy from applying to the login route like so:

map: {
  login: {GET: true, POST: true, data: true,auth:false, alias: options.urlpath.login}
}

If we do want to take this fwd and you are ok with the changes in principle, I'm happy to make the proposed changes and submit relevant PRs.

mirceaalexandru commented 8 years ago

@girishla yes, I think is a good idea, but let's discuss a little about this.

Few comments:

seneca.use('auth', {restrict: '/api'})

to restrict only /api paths. If we are placing the strategies as default then we will broke this feature, as all paths will be automatically restricted and seneca-auth will not be in control for this feature. So here I think we can add more strategies, but not as default, and maybe we can extend the options for seneca-auth to select the auth strategy to be used for a restrict path.

@rjrodger, @mihaidma comments?

girishla commented 8 years ago

@mirceaalexandru I agree, strategy registration should be in their own separate plugins.

Re. the restrict, I reckoned we should be retaining as-is functionality if we change from

function get_restriction (msg, done) {
    var path = msg.path

    var restrict_mode = restrict.restriction()(path)
    if (restrict_mode) {
      done(null, {auth: 'session'})
    }
    else {
      done()
    }
  }

to something like this so routes that aren't restricted will be set to false and restricted ones would get the server default ?

  function get_restriction (msg, done) {
    var path = msg.path

    var restrict_mode = restrict.restriction()(path)
    if (restrict_mode) {
      done(null)
    }
    else {
        done(null, {auth: false})
    }
  }

thoughts ?

mirceaalexandru commented 8 years ago

Well, I will do it like this:

seneca
.use('auth'...)
.use('auth-cookie',...)
.use('auth-token',...)
....

and each of the above plugins (auth-cookie and auth-token) register in seneca-auth their strategies. And then the code will be:

function get_restriction (msg, done) {
    var path = msg.path

    var restrict_mode = restrict.restriction()(path)
    if (restrict_mode) {
      done(null, {auth: registered_auth_strategies_array})
    }
    else {
        done(null, {auth: false})
    }
  }

where registered_auth_strategies_array will be an array containing the strategies registered using those two plugins.

It seems unnecessary/unsafe for me to use those two auth plugins but user should also register the auth strategies as default in Hapi. In this way, seneca-auth will restrict, based on what plugins are registered in seneca.

thoughts?

girishla commented 8 years ago

yes, I concur - that's a better way to do it. This will allow us to add more strategies - each as separate plugins. Nice and clean.

Rgds Girish

On Fri, Feb 5, 2016 at 10:17 AM, Mircea Alexandru notifications@github.com wrote:

Well, I will do it like this:

seneca .use('auth'...) .use('auth-cookie',...) .use('auth-token',...) ....

and each of the above plugins (auth-cookie and auth-token) register in seneca-auth their strategies. And then the code will be:

function get_restriction (msg, done) { var path = msg.path

var restrict_mode = restrict.restriction()(path)
if (restrict_mode) {
  done(null, {auth: registered_auth_strategies_array})
}
else {
    done(null, {auth: false})
}

}

It seems unnecessary/unsafe for me to use those two auth plugins but user should also register the auth strategies as default in Hapi. In this way, seneca-auth will restrict, based on what plugins are registered in seneca.

thoughts?

— Reply to this email directly or view it on GitHub https://github.com/senecajs/seneca-auth/issues/68#issuecomment-180284284 .

Regards, Girish Lakshmanan

mirceaalexandru commented 8 years ago

Great, then we have a plan!

Now I don't know if you can get some time to create PRs for this, if you can do that it would be great. If not then it might take some time until this will be implemented.

Regards, Mircea

girishla commented 8 years ago

I should be able to find some time on Monday hopefully. I'll get these done and submit PRs.

Thanks.

girishla commented 8 years ago

Had a brief look at this today. Looks like there is active work going on in mirceaalexandru/seneca-auth. Tests are failing at present - so it would be better to hold off until it is in decent shape before I add this in.

mirceaalexandru commented 8 years ago

Yes, it is WIP, also on other plugins that are used by seneca-auth. We are publishing all these and then seneca-auth tests will be ok, hopefully

nfantone commented 8 years ago

Any movement here? I'd also like to support something like JWT on Express. This looks like following a similar path.

girishla commented 8 years ago

@nfantone I'm waiting for @mirceaalexandru to publish and get tests working. I will then add token header support for hapi. I'll start with a first plugin to use a simple bearer token and then subsequently another plugin to support jwt

nfantone commented 8 years ago

@girishla Want me to give you a hand on that? I'm still trying to get a grasp on how things work, but this is something I need to implement, sooner or later.

girishla commented 8 years ago

@nfantone sure extra hands are always good :) We can agree on an approach with @mircealexandru and then I can perhaps pick up the hapi plugins and you the express ones

nfantone commented 8 years ago

@girishla Fantastic. ETA on that?

girishla commented 8 years ago

@nfantone the changes aren't that complex but I reckon it would be best to wait until changes in mircea's fork is merged into the main repo. I assume all tests would pass in that stage

nfantone commented 8 years ago

@girishla Roger. What's holding the merge back?

girishla commented 8 years ago

@nfantone lack of time I would guess :) the number of plugins in the Seneca ecosystem is quite large and there is a lot of action currently. Maintaining all of them is a superhuman effort. I'm sure they'll get to this soon enough.

nfantone commented 8 years ago

That's totally understandable. Let's wait on @mirceaalexandru, then.

mirceaalexandru commented 8 years ago

Yes, thank you for understanding @girishla @nfantone . Hapi auth tests are passing, some small issues on Express support, so one-two days.

nfantone commented 8 years ago

@mirceaalexandru Gotcha. Will wait over here for your changes.

mirceaalexandru commented 8 years ago

@nfantone @girishla that PR is merged. The only thing stopping now from publishing is https://github.com/senecajs/seneca-auth/issues/71

girishla commented 8 years ago

Thanks @mirceaalexandru, I can see the tests are passing now.

I'll pick this one up when I get some free time. Before I make the changes, I wanted to get everyone on the same page:

I want to add support for 2 types of token strategies.

With jwt, I wanted to agree with everyone about some fundamental differences that it brings to the table:

first, some points to bring everyone on the same page:

with the above background, I propose to

If everyone agrees, I'll make required changes when time permits.

Thoughts @rjrodger @mirceaalexandru @mcdonnelldean @nfantone ?

nfantone commented 8 years ago

@girishla Some things to consider are how will the tokens be provided and who manages expirations. Will seneca-auth even take that into account? Or should we leave it to some third party? Maybe integrate seneca-jwt into the mix?

if the client sends both a jwt header and cookie, the jwt takes precedence

Do you mean if both things are sent and the user has both authentication mechanism configured? I imagine, support for JWT will come in the form of something like seneca-jwt-auth. Am I right?

girishla commented 8 years ago

@nfantone I was thinking I'll do the hapi versions first which will be integrated into the existing auth-token-header plugin. "auth-token-header" uses a simple bearer token based on the login entity. This can be extended to support jwt.

The hapi version will use this hapi plugin to handle token validation. For Signing we can just use jsonwebtoken which the hapi plugin sits on. I haven't looked at jwt for express in detail but I presume we can use the passport-jwt strategy for it.

nfantone commented 8 years ago

@girishla I've always used express-jwt for JWT on Express. But there is, indeed, a passport-jwt module.

nfantone commented 8 years ago

@girishla Any updates on this? Something I could help with?

girishla commented 8 years ago

@nfantone I'm waiting for feedback from the main contributors before I do anything :) This is a fairly significant change that requires an agreement in principle first.

mirceaalexandru commented 8 years ago

@girishla I think is OK what you said, just that maybe you should create a new plugin, do not change the existing "auth-token-header", as this implements another feature.

About Express I think it is easy for current implementation to use passport-jwt as we are using passport for all our authentication strategies, but I did not use any of them so we can discuss if you think otherwise.

Plugin should be loaded in the same way for Express/Hapi, internally it must choose the correct implementation.

nfantone commented 8 years ago

@mirceaalexandru Don't mind using passport-jwt. Agreed on creating new seneca-auth-jwt, even if it borrows most of its implementation from auth-token-header. Seems like the better approach to me.

mirceaalexandru commented 8 years ago

@nfantone the purpose is to let anyone use what he wants, just by seneca.use() a plugin that implemented that feature. We try to avoid using options that change plugin behavior.

nfantone commented 8 years ago

@mirceaalexandru That's my vision too. And the reason I voted in favor of creating a new plugin.

girishla commented 8 years ago

@mirceaalexandru not sure I'm on the same page. This new jwt plugin cannot be optional going by the way seneca-auth is written at the moment. I could be wrong and please correct me if that is the case.

The new jwt plugin has to be a hard dependency loaded by default into seneca-auth. The reason is the init action of hapi-auth adds hapi routes for all the auth endpoints. Once added, there is no way to override the routes(Hapi provides no API to delete or modify already added routes).

This means that the auth strategies(i.e session, jwt ) have to be registered by the respective plugins before the above init method.

thoughts or suggestions ?

mirceaalexandru commented 8 years ago

@girishla

Well you are right, this is a problem. The workaround I think could be to implement our custom strategy that will used by default and from that strategy try to use all registered available strategies.

So we have something like this: https://github.com/hapijs/hapi/blob/master/API.md#serverauthstrategyname-scheme-mode-options

and from there we can call registered strategies: https://github.com/hapijs/hapi/blob/master/API.md#serverauthapi

But I am maybe wrong, we should check more carefully the Hapijs documentation.

girishla commented 8 years ago

@mirceaalexandru I like the idea of a custom hybrid hapi auth scheme but I don't think it will be possible without duplicating code from existing hapi plugins. The hapi plugins we are interested in(such as hapi-auth-cookie and hapi-auth-jwt etc) do not expose the methods we would be interested in. They are private unfortunately.

Although it is less than ideal, I'm wondering if it would it be too bad if we did create a new hybrid scheme duplicating code from multiple existing plugins ? Perhaps it's not such a bad idea ?

mirceaalexandru commented 8 years ago

@girishla I do not have any other idea how to solve this.

My problem is that I do not want to add JWT support by default - we should support what was the initial implementation for Express (so minimal local strategy and cookies) so we must do something to avoid adding this as default strategy.

All other features should come as additional plugins.

This hybrid strategy might be the only way to do it.

@geek @rjrodger @mcdonnelldean @mihaidma any other idea how to solve this?

nfantone commented 8 years ago

Maybe add the discussion label to this issue?

hotrush commented 8 years ago

any news?

segunafo commented 7 years ago

Any update on this discussion? @mirceaalexandru @girishla