Skip to content
This repository has been archived by the owner on Mar 22, 2022. It is now read-only.

Authentication Documentation #328

Closed
wants to merge 14 commits into from
Closed

Authentication Documentation #328

wants to merge 14 commits into from

Conversation

ekryski
Copy link
Member

@ekryski ekryski commented Oct 24, 2016

Summary

This is a PR so that we can discuss and iterate on the documentation and terminology around authentication and the authentication flow.

@ekryski ekryski added this to the 1.0 milestone Oct 24, 2016
- malformed
- has not been tampered with
5. Server decodes the payload
6. Server looks up refreshId on some service (TBD)
Copy link
Member

Choose a reason for hiding this comment

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

This should be the aauthorizations collection

7. Service `before` hook, `isPermitted` checks to see if the request is authenticated.
- If it is proceed to next hook or service method
- If it isn't it rejects with a `Forbidden` error
8. Service method is called and returns through after hooks

Choose a reason for hiding this comment

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

Steps 6 and 7 above seem too much alike? afaik, 401 Not authenticated is meant for when the client has not yet presented any credentials (so pre-login), whereas 403 Forbidden is meant for clients that did show credentials (so post-login) but are simply not allowed access. So I would expect the one hook to check whether the client is authenticated, whereas the other should check whether he is authorized to perform the action.

1. Client sends expired JWT access token or it's attached to the connected socket
2. Server verifies JWT and determines it is expired
- returns a `NotAuthenticated` error with a "token expired" message to the client (current)
- **Proposed Change:** Return a special error type of `TokenExpired` and flags authorization as expired/revoked

Choose a reason for hiding this comment

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

Actually I would think 401 Not Authenticated would be the correct response (at least HTTP status code wise). The refresh token is effectively a form of credentials (so login, but automated)

- malformed
- tampered with
3. Server decodes the token payload and passes it down the auth middleware chain
4. Server looks up authorization record by the `authorizationId` and passes it down the auth middleware chain (populateAuthorization)

Choose a reason for hiding this comment

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

Not stateless :(

Choose a reason for hiding this comment

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

@Download I think the idea here is that we will allow you to have the ability to retain statelessness however we also need to support identity provider that require state. There will be a callback here that you can implement, or to make this decision.

Copy link
Member

Choose a reason for hiding this comment

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

@Download maybe @ekryski or @daffl can clarify for me if I'm mistaken, but the auth middleware chain will be called like a normal hook. This will allow you to pass params that will customize which middleware will run. You have complete control over the middleware. Here's a contrived example:

app.service('myservice').hooks({
  before: {
    all: [app.authenticate(flow: 'stateless'), otherHook()]
  }
});

app.service('my-super-secure-service').hooks({
  before: {
    all: [app.authenticate(flow: 'strict'), otherHook()]
  }
});

Copy link
Member Author

Choose a reason for hiding this comment

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

@marshallswain is right you will be able to pass options and do whatever you want in your authentication chain by registering whatever verification hooks you would like.

As a side note, @marshallswain they aren't called middleware anymore. They really are just special hooks used to do verification in the authentication chain. Which can be stateless (ie. just using the JWT verify function) or can be stateful (ie. you can look up whatever you want in order to do verification).

Choose a reason for hiding this comment

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

That's great to hear. Thanks for clarifying!

@Download
Copy link

I've added some comments to the commits.

Are you giving up on stateless access tokens? And why exactly? I understand the discussion of changed access permissions only taking effect after the access token has expired, but there are various ways to mitigate that issue. And stateless access tokens have great benefits when it comes to scalability.

Possible mitigations for the long retention time of access permissions:

  • Just set a (very) short expiry time for access tokens. Even when set to just a couple of minutes, stateless access tokens could help save a lot of DB calls for sites that have lots of short, frequent interactions with the server (games etc).
  • Keep the access token around for longer, but apply stricter security for specific actions/services. Much like how most sites will ask you to type your old password before allowing you to set a new one, specific services could choose to force a check of the permissions beforehand. This could be implemented with a standard hook.

@kc-dot-io
Copy link

kc-dot-io commented Oct 27, 2016

@Download see the comment above. We want to support JWT stateless-ness as the sane default but add hooks to allow for stateful authorizations to be stored on the server if needed. See oAuth, Open ID etc. Think about this like Passport does it. There will be "strategies" you can control.

@ekryski
Copy link
Member Author

ekryski commented Oct 27, 2016

I also think that we should be stateful by default. What I mean by that is the special verification hooks would be registered by default when using the generator to create an app.

My justification is this:

  • people are used to stateful, so it's easier for newer people to grok
  • If we went stateless by default we will get a lot of people asking questions about why a user can still login when they have been removed, or still has access to things when their permissions were changed, etc. etc.
  • the stateless approach is more scalable but that's not something every app needs and in my mind you need to explicitly opt-in to eventual consistency.

In the end I see stateless authentication and authorization being more of an upgrade or a power user feature once you need it and understand how authentication works. Ideally that is just removing or re-arranging a couple hooks.

@marshallswain
Copy link
Member

I couldn't agree more. Lock it down for the average users who aren't well versed on this stuff. Let the pros open it up. Hopefully big banks & corporations will stay stateful, regardless of the track record. :)

@Download
Copy link

If we went stateless by default we will get a lot of people asking questions about why a user can still login when they have been removed, or still has access to things when their permissions were changed, etc. etc.

Maybe I'm totally underestimating things... But the way I see it, these are rare events. If by default the access token lives only for a few minutes, I bet you would not see any question at all about why users are still able to login. Because when a user is removed there would be a maximum time window of 5 minutes for him to still do things. It's an extremely short time.

people are used to stateful, so it's easier for newer people to grok
True. But people are used to a lot of stuff that Feathers does differently. In fact I'm working with Feathers right now because it does things differently.

In the end I see stateless authentication and authorization being more of an upgrade or a power user feature once you need it and understand how authentication works.

If it is indeed just a couple of hooks then that's a very strong argument indeed. I'm just hoping it will be well documented how to do this because it's definitely something that I want to use (need is a string word :) )

@daffl
Copy link
Member

daffl commented Oct 31, 2016

I think #336 is going to change not all but quite a bit of this again right?

@Download basically @ekryski found a really nice way to just integrate Feathers as a framework into Passport so you can just do anything Passport supports. I think one of the ideas was to provide some convenience wrappers for standard use cases (e.g. user with username/password login and OAuth2 for creating a JWT and JWT for request authentication) but leave it to Passport and their custom handlers if you want to customize anything other than that.

@ekryski
Copy link
Member Author

ekryski commented Oct 31, 2016

@daffl 💯 . Hoping to cut an initial alpha release today. Working hard on docs and tests today as I need something fairly stable for a couple projects ASAP.

I'll update the migration guide and the spec over the course of the week.

@ekryski
Copy link
Member Author

ekryski commented Nov 16, 2016

An alpha release for the server side is out feathers-authentication@1.0.0-alpha and #336 has been merged into master now. This is no longer applicable. Many of the things put forth by this proposal were answered in a much more flexible way by creating passport adapter and leveraging passport as the authentication chain.

Thanks for the input everyone! 🍻

@ekryski ekryski closed this Nov 16, 2016
@daffl daffl deleted the docs branch July 22, 2017 18:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants