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 support for token based Auth in addition to cookie #68

Open
girishla opened this issue Feb 4, 2016 · 37 comments
Open

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

girishla opened this issue Feb 4, 2016 · 37 comments

Comments

@girishla
Copy link
Contributor

girishla commented Feb 4, 2016

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
Copy link
Collaborator

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

Few comments:

  • I am not really sure about changing the seneca-token-cookie that will add the chained strategy, as this plugin should only register its strategy. So, in this case I think that we should have another plugin that will handle the other strategy.
  • seneca-auth is using the {restrict: ...} option to define which paths should be authorized. So I want to use something like:

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
Copy link
Contributor Author

girishla commented Feb 5, 2016

@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
Copy link
Collaborator

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
Copy link
Contributor Author

girishla commented Feb 5, 2016

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 [email protected]
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
#68 (comment)
.

Regards,
Girish Lakshmanan

@mirceaalexandru
Copy link
Collaborator

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
Copy link
Contributor Author

girishla commented Feb 5, 2016

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

Thanks.

@girishla
Copy link
Contributor Author

girishla commented Feb 8, 2016

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
Copy link
Collaborator

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
Copy link

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

@girishla
Copy link
Contributor Author

@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
Copy link

@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
Copy link
Contributor Author

@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
Copy link

@girishla Fantastic. ETA on that?

@girishla
Copy link
Contributor Author

@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
Copy link

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

@girishla
Copy link
Contributor Author

@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
Copy link

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

@mirceaalexandru
Copy link
Collaborator

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

@nfantone
Copy link

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

@mirceaalexandru
Copy link
Collaborator

@nfantone @girishla that PR is merged. The only thing stopping now from publishing is #71

@girishla
Copy link
Contributor Author

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.

  • jwt
  • simple bearer token(using the id of the login entity as the token)

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:

  • seneca-auth at the moment uses the login entity to implement the concept of sessions.
  • jwt fundamentally does not need sessions. In fact the main advantage of jwt is that sessions are not necessary as all user info is derivable from the token itself. sessions take up memory and may cause scalability issues in largescale apps.
  • seneca-auth currently supports the concept of server-side logout. i.e. by invalidating the login(session) entity
  • with jwt, there is no concept of server side logout. Tokens expire after set duration. Logout is typically implemented by losing the token on the client side by clearing it out of local storage/cookies.

with the above background, I propose to

  • keep the existing session based solution intact
  • add jwt support and provide 2 default strategies with seneca-auth - 1) cookie and 2) jwt
  • if the client sends a jwt header, authenticate and derive user info from the jwt without relying on the login entity.
  • if the client sends a cookie, authenticate and derive user info from the login entity
  • if the client sends both a jwt header and cookie, the jwt takes precedence

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

Thoughts @rjrodger @mirceaalexandru @mcdonnelldean @nfantone ?

@nfantone
Copy link

@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
Copy link
Contributor Author

@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
Copy link

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

@nfantone
Copy link

nfantone commented Mar 1, 2016

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

@girishla
Copy link
Contributor Author

girishla commented Mar 1, 2016

@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
Copy link
Collaborator

@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
Copy link

nfantone commented Mar 1, 2016

@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
Copy link
Collaborator

@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
Copy link

nfantone commented Mar 2, 2016

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

@girishla
Copy link
Contributor Author

girishla commented Mar 2, 2016

@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
Copy link
Collaborator

@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
Copy link
Contributor Author

girishla commented Mar 3, 2016

@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
Copy link
Collaborator

@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
Copy link

nfantone commented Mar 3, 2016

Maybe add the discussion label to this issue?

@geek geek added the discussion label Mar 3, 2016
@hotrush
Copy link

hotrush commented Jun 30, 2016

any news?

@segunafo
Copy link

Any update on this discussion? @mirceaalexandru @girishla

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

No branches or pull requests

6 participants