ory / fosite

Extensible security first OAuth 2.0 and OpenID Connect SDK for Go.
https://www.ory.sh/?utm_source=github&utm_medium=banner&utm_campaign=fosite
Apache License 2.0
2.33k stars 365 forks source link

Improve support for password credentials grant #129

Closed kujenga closed 7 years ago

kujenga commented 7 years ago

I've run into two issues while setting up a password credentials using this library. I've been looking into ways to fix them but wanted to post an issue first.

  1. The oidc package does not support the password credentials flow. Here is one source saying that is is an acceptable part of the spec: http://stackoverflow.com/questions/24047047/does-openid-connect-support-the-resource-owner-password-credentials-grant Would you be okay with adding this functionality, and what would be the most "idiomatic" way to approach this?

  2. The password credentials flow makes it difficult to efficiently fill out a full jwt token session with the appropriate claims. Because the user is authenticated behind the Authenticate call [1], it is not possible to use the entity behind that to add information to the session that builds the token returned to the caller. I ran into the specific issue that the sub claim cannot be filled out during the password credentials grant without re-parsing the request to get the username, doing another identical database lookup, and filling out the session using that information. Perhaps there could be an alternate Authenticate call that passes in an access requester or session for modification?

[1] https://github.com/ory-am/fosite/blob/master/handler/oauth2/flow_resource_owner.go#L39

aeneasr commented 7 years ago

The oidc package does not support the password credentials flow. Here is one source saying that is is an acceptable part of the spec: http://stackoverflow.com/questions/24047047/does-openid-connect-support-the-resource-owner-password-credentials-grant Would you be okay with adding this functionality, and what would be the most "idiomatic" way to approach this?

If you want oidc and the ropc grant, you need to activate both. The fosite-example repository shows how that works. It supports both things as well.

The password credentials flow makes it difficult to efficiently fill out a full jwt token session with the appropriate claims. Because the user is authenticated behind the Authenticate call [1], it is not possible to use the entity behind that to add information to the session that builds the token returned to the caller.

I get the problem. One issue that I have however is that the act of authenticating the user credentials should not be mixed with hydrating the response. Side-effects in logically separated units are tricky when it comes to libraries. Those are two separate concerns and it could lead to anti-patterns and hacks by developers (in fact, hydrating the response during authentication is an anti-pattern imho). One thing to note is that after the HandleTokenEndpointRequest call, you can be sure that the POST'ed username and password are correct, if the request is a ropc grant. So you could use that username as subject. If that isn't enough and database performance is really an issue for you, because you are authenticating thousand of users per second, you can always opt for some caching strategies and keep data around locally for a few minutes.

There is one thing to note, which is on premature optimization: Be aware that hashing algorithms such as BCrypt use a million times more CPU & memory time than extracting a key from a map or fetching data over the network. No matter how much you optimize the user look up, you will always hit the hasing bottleneck.

I hope this is a satisfying answer

aeneasr commented 7 years ago

Closing due to inactivity