clojusc / friend-oauth2

OAuth2 Workflow for Friend (https://github.com/cemerick/friend)
MIT License
102 stars 24 forks source link

Support multiple outh providers in the workflow #15

Open tutysara opened 10 years ago

tutysara commented 10 years ago

Enhance outh2 workflow to allow multiple providers where the user can authenticate using any of them.

Many sites allow the users to login using their credentials with various providers like Google/Facebook/Github etc and adding this will allow clj web apps to do this easily.

ddellacosta commented 10 years ago

hey, you mentioned to me on IRC that you had a flow working for multiple providers--would you mind adding an example in friend-oauth2-examples?

tutysara commented 10 years ago

Sure, I had to make some modification in friend-oauth2. I am using /login to display the possible providers by redirecting to a form to select the provider (GET "/login" [] (response/file-response "login.html" {:root "resources"}))

login.html

<p>
<table>
<tr>
<td><a href="/auth/google"> Google </a></td>
<td><a href="/auth/github"> Github </a></td>
<td><a href="/auth/facebook"> Facebook </a></td>
</table>
</p>

To support this I had to modify is-oauth2-callback? in workflow.clj to not intercept /login by our workflow. Workflow intercepts - /auth/:provider to initiate the authentication process. Are you fine with this approach?

ddellacosta commented 10 years ago

If you had to modify friend-oauth2 to get this to work correctly, then it's probably best first to make a pull request for that--I don't want to provide any examples that require one to modify library code to get them to work!

So, if I understand the issue correctly, it sounds like we need to allow for a page before the login redirect that would give the user a chance to ask which provider they want to use. Is that correct? Seems like it may be useful to provide a configuration value or function to allow client developers to generically inject an action at this stage of the process if so.

tutysara commented 10 years ago

You are right, we have to provide a means for the user to allow them to select a provider. This is how I am planning to do it

Modify workflow function to allow for providers config by including :providers-uri in config

(defn workflow
  "Workflow for OAuth2"
  [config]
  (fn [request]
    (if (is-oauth2-callback? config request)
      (if (and (= (request/path-info request)
                  (or (:login-uri config)
                      (-> request ::friend/auth-config :login-uri)))
               (:providers-uri config))
        (ring.util.response/redirect (:providers-uri config))
      ;; Extracts code from request if we are getting here via OAuth2 callback.
      ;; http://tools.ietf.org/html/draft-ietf-oauth-v2-31#section-4.1.2
        (let [{:keys [state code error]} (:params request)
              session-state        (util/extract-anti-forgery-token request)]
          (println :state state :code code :error error)
          (if (and (not (nil? code))
                   (= state session-state))
            ;; we have a valid code and state
            (when-let [access-token (request-token config code)]
              (let [user-info-fn (or (:user-info-fn config) (fn[_] {}))
                    user-info (user-info-fn access-token)]             
                (when-let [auth-map ((:credential-fn config default-credential-fn)
                                     (merge user-info {:access-token access-token}))]
                  (vary-meta auth-map merge {::friend/workflow :oauth2
                                             ::friend/redirect-on-auth? true
                                             :type ::friend/auth}))))
            ;; when we don't have a valid code and state : may be
            ;; because of error response from provider or we haven't redirected to provider yet
            (if error                      
              (redirect-to-errorpage! config request)
              (redirect-to-provider! config request))))))))

Then modify the application to use multiple providers

(def app
  (handler/site
   (friend/authenticate
    ring-app
    {:allow-anon? true
     :workflows [;; google handler
                 (oauth2/workflow
                  {:client-config google-client-config
                   :uri-config google-uri-config
                   :user-info-fn get-user-info
                   :providers-uri "/login.html"
                   :unauthorized-handler (fn [data]
                                           (println "this request is unauthorised" data))
                   :credential-fn (fn [token]
                                    {:identity token
                                     :roles #{::user ::admin}}) })
                 ;; facebook handler
                 (oauth2/workflow
                  {:client-config facebook-client-config
                   :uri-config facebook-uri-config
                   :access-token-parsefn get-access-token-from-params
                   :providers-uri "/login.html"
                   :credential-fn (fn [token]
                                    {:identity token
                                     :roles #{::user ::admin}}) })

                 ;; github handler
                 (oauth2/workflow
                  {:client-config github-client-config
                   :uri-config github-uri-config
                   :access-token-parsefn get-access-token-from-params
                   :providers-uri "/login.html"
                   :credential-fn (fn [token]
                                    {:identity token
                                     :roles #{::user ::admin}}) })]})))

I had also modified the workflow function to accept an user info function that can be used to fetch more user data after we get the access token

https://www.refheap.com/26854

I will open a pull request if you are ok with this approach.

ddellacosta commented 10 years ago

It's hard to say without seeing the actual implementation (along with tests), but I have a few comments:

1) There is a lot of redundancy in the configuration--it should be simpler to configure, and I'm not sure if this should really be three workflows vs. one...perhaps it should? 2) it should not go to the provider-uri by default--it should be optional. 3) provider-uri should perhaps be something more like pre-login-fn or something. Not sure, I'll think on it.

Right now I feel like the flow is a bit obscure compared to how the function worked previously. But it's probably best to first submit a pull request and work through this there.

Thanks!

komarov commented 10 years ago

Hi, i'm new to both friend and oauth, so if the following doesn't make much sense, please just ignore :)

Representing multiple authorization providers as separate workflows vs one hydra-workflow actually has a benefit of being able login/logout into/out of separate roles simultaneously.

Example: an application that aggregates various external profiles and lets you manage a single identity and specify what you share from those external services. Let's assume that this application assigns some ID to a user when this user authenticates with provider-A as user-A and fills in some info related to A. ID is now linked to user-A, and when she authenticates later she is authorized to change stored info for ID.

Now assume that we want to add info from provider-B. You have to authenticate as user-B, still maintaining authentication you have for user-A.

If you allow for only one authentication at the same time, implementing this feature would require some additional protocol for identity equality validation.

Friend, on the other hand, if i get it right, specifically allows for several simultaneous workflows and several simultaneous authentications. So isn't it a more natural way to go?

As for the configuration redundancy, could we use something like this:

(defn client-config [provider]
  (condp = provider
    :github {:client-id ....}
    ...))
...
(friend/authenticate {:workflows
                                 (map #({:client-config (client-config %) ...})
                                   [:github :twitter :facebook ...])})
tutysara commented 10 years ago

Redundancy is solved by using a single workflow with configuration for many providers. Now it looks a lot simpler than the one given in the comments above, I have an working example here - multiworkflow It might require a clean up or rewrite based on ddellacosta comments before it is merged, but you can take a look and see if you like it. I am not sure on how to do the login with the multiple providers at the same time.

ddellacosta commented 10 years ago

@cemerick would love it if you could scan this quickly to see if this makes sense to you.

@komarov Thanks for the feedback! Very helpful.

Thinking about it I tend to agree with your assessment. Despite the fact that I don't like having such a large amount of configuration, that's a separate issue. However, incorporating the oauth2 workflows into one large workflow is coupling authorization in an inappropriate way, and could introduce a lot of unnecessary complexity. It should be one provider per oauth2 workflow.

Sorry @tutysara but I really think this is the correct design decision.

cemerick commented 10 years ago

@ddellacosta As you know, I am blissfully mostly ignorant of oauth-specific details; that said, all of the above looks sane. I agree with @komarov re: configuration; that complexity can be ameliorated by ensuring that workflows accept a simple map of options, so that e.g. a base configuration map can be defined somewhere, and modified minimally when actually setting up the authenticate middleware.

cemerick commented 10 years ago

And to be clear, something like @tutysara's "multiworkflow" is very viable as a sugared API, but everything it needs should be available piecemeal (and likely needs to be be, anyway, in order to implement such a convenience measure).

ddellacosta commented 10 years ago

Thank you @cemerick --very helpful.

pjlegato commented 10 years ago

With the "one workflow per auth provider" paradigm, is it still necessary to change friend-oauth2 to not intercept "/login"?

goya commented 9 years ago

so i assume @tutysara didnt get his pull request in, so there is currently no way to use multiple providers with this library?

ddellacosta commented 9 years ago

@goya Hey, apologies for the extremely slow action on this. I'm in the process of doing some serious refactoring right now in fact, and should have a solution for you shortly (shooting for within a week). Thanks for your patience!