-
Notifications
You must be signed in to change notification settings - Fork 24
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
optionally parse refresh tokens from authorization code exchange #25
base: master
Are you sure you want to change the base?
Conversation
workflow/request-token can be passed a response-parse-fn in place of access-token-parsefn, e.g. for google’s oauth2 (defn response-parse-fn [req] (when-let [response (some-> req :body (chesire.core/parse-string true))] {:access-token (:access_token response) :refresh-token (:refresh_token response)}))
Okay, I've read through this and I think it's the right direction, but I want to propose some more drastic changes. It's clear based on other conversations with folks that I've had that we need to allow the lib user access to all possible fields that the token endpoint could pass back. I think the goal with this lib should be to extend it to follow the spec for OpenID Connect, as that is (ostensibly) a real authentication protocol. This will require, in this section, supporting the extensions to allow for parsing an ID Token. Anyways, what you have certainly enables that by giving the API user access to the entire response through a function they've passed in. That part of it is exactly what we want. But I think at this point it's overly confusing to have access-token-parsefn as well as response-parse-fn--I think we should only have one, and it should have a default implementation using the default RFC-supported method for parsing tokens, and we can leave the alternate in place if it helps anyone implementing this for a specific provider who strays from the RFC. So I think what I'd like to see is something a bit more all-encompassing that replaces access-token-parsefn, that handles all the values in the OpenID connect spec, and is named something a bit more general like token-response-parsefn or token-response-handler. I also would like to have tests for these. By the way, I'm not saying I expect all of these in this pull request, more just that this is how I think it should be structured, and whatever you contribute toward that is great. Let me know what you think. |
It seems that "offline access" involving refresh tokens is a feature that at least Google and Twitter implement using Bearer tokens. I wanted to add that to the conversation as something that we should be aware of. Since every provider seems to roll their own flavor of oauth (especially oauth2) I personally think covering every corner case would be a slippery slope (who would maintain implementations for the myriad apis that exist?) One example of a mature oauth/openid library that does this is python social auth. With enough people provider-specific workflows might be feasible, but right now I don't see that happening realistically. I definitely agree that the library should be flexible. Perhaps allowing dispatch on tokens that are part of the RFC? To elaborate, may be friend-oauth2 could handle the boilerplate of server-to-server communication, allowing end users to inject a map of custom token handling functions in which the keys are RFC token-types. This might help mitigate the explosion of In the case of "offline access", there's the need for initial user authentication (after which refresh tokens are retrieved and stored), as well as access token requests using the stored refresh tokens. I'm working on this use case at the moment, but I'm not entirely sure how I'm going to implement it. I think dispatch on token types might help, but I'm not sure. |
What about providing protocols that hook into the token exchange lifecycle? This idea comes from psa's storage feature and the ability to add new backends. If this library were to provide some protocols and a pipeline model like psa it might be easier for users to implement workflows for their needs. Perhaps this could hook into friend more somehow and provide user association features. Again, I'm not sure if any of this is possible, beneficial, or unnecessarily complex. |
@anthonyurena
Yep. My basic guiding principle with this lib has become: follow the spec, provide alternatives for the most common edge-cases, and let the library user do what they need to if it is outside of the spec, but frequently requested and without heavy maintenance costs. I haven't done a great job of the latter so far, but I think resolving the issues around this pull request will move that forward a good deal.
Regarding specific ideas--protocols, a pipeline model, etc.--I'm not necessarily opposed to any of them but I do want to avoid unnecessary complexity. In comparison, I think that python lib is too heavy for what we want with this lib, and fits a different niche. I have a few thoughts in response to your suggestions:
The only place I really have doubts is in the first item--if there are places in the system that a developer needs to hook into that I'm forgetting, then I'm certainly open to considering that. But I've only ever gotten requests for hooking into the very end of the oauth2 flow, which is why I think that's probably the best place to start, even if it does make sense to open something else up later on. Some things that I do think may be useful is to provide default functions to make a call to the server using the access token, request a new refresh token, etc. But I'm still a bit nervous about where these would go--they aren't strictly part of the work friend-oauth2 should be doing, I believe. That said, they could provide a lot of utility to library users. |
3f22d5c
to
0d4af11
Compare
workflow/request-token
can be passed aresponse-parse-fn
(supercedingaccess-token-parsefn
), allowing values in addition to the access token to be parsed (e.g. refresh token). For example, for google’s oauth2response-parse-fn
should return a map containing parsed vals. Addresses #20