weavejester / ring-oauth2

OAuth 2.0 client middleware for Ring
147 stars 38 forks source link

Supporting multiple profiles for the same provider? #52

Open lukaszkorecki opened 1 year ago

lukaszkorecki commented 1 year ago

This might be somewhat an edge case, although I dealt with this in the past when working with Ruby's Omniauth where this issue was addressed IIRC.

Here's my setup:

I have two profiles for Google OAuth:


{:google-sso {:authorize-uri "https://accounts.google.com/o/oauth2/v2/auth?access_type=offline&prompt=select_account"
              :access-token-uri "https://www.googleapis.com/oauth2/v4/token"
              :client-id "test"
              :client-secret "test"
              :redirect-uri "/api/auth/google/callback"
              :landing-uri "/api/auth/google/finalize"

              ;; specific for SSO
              :launch-uri "/api/auth/google/auth"
              :scopes ["openid" "email" "profile"]}

 :google-calendar {:authorize-uri "https://accounts.google.com/o/oauth2/v2/auth?access_type=offline&prompt=select_account&include_granted_scopes=true"
                   :access-token-uri "https://www.googleapis.com/oauth2/v4/token"
                   :client-id "test"
                   :client-secret "test"
                   :redirect-uri "/api/auth/google/callback"
                   :landing-uri "/api/auth/google/finalize"

                   ;; for connecting Google Calendar
                   :launch-uri "/api/auth/google/auth"
                   :scopes ["https://www.googleapis.com/auth/calendar"]}}

Note that :redirect-uri are the same, but scopes, :authorize-uri and :launch-uri are different.

Why this setup? Because Google encourages staged authorization to different resources - as in, if all you need Google access for is signing-in, don't request calendar access as well - not every user will need it. It's something I dealt with in the past with multiple providers, not only Google but also Slack, Qualtrics and probably more that I'm forgetting.

The issue is - because of how the middleware detects the profile during redirect stage:

https://github.com/weavejester/ring-oauth2/blob/d711e017a55ee193e81ddffb6140fb08bbf00c11/src/ring/middleware/oauth2.clj#L161

a couple of things happen:

The workaround might look trivial: configure Google's OAuth screen to accept unique redirect URLs. That might work if there's only two, but in realistic scenarios there's at least six (for each environment) and there will be more if/when my application needs to support different regions. And no, Google verifies the whole URL, so appending query params won't work. To make this even more difficult - any change to the OAuth screen requires going through Google's verification process, which in my experience can be anything from 2hrs to 2 months 😢

On top of that: some API providers support a fixed number of redirect urls (or just one, and you're forced to create separate workspaces/accounts/etc for different environments).

I have a fork which fixes this issue and while the code works, it's a bit hairy - you can see it here: https://github.com/weavejester/ring-oauth2/compare/master...lukaszkorecki:tmp/figure-out-profile-confusion?expand=1

It's all working as expected and all tests are passing, but:

I'll be more than happy to take this to the finish line, but I'm wondering if there's another way to deal with this.

Thank you

weavejester commented 1 year ago

So because Google and other providers may not support an arbitrary number of redirect URLs, instead your solution is to set a session variable and use that to determine the profile being used? I think that's fine, but I also think it should be an option that's off by default. Something like :session-chooses-profile?. What do you think?

lukaszkorecki commented 1 year ago

I think that could work - let me play this back, just so I understand your approach:

Since launch-uris have to be unique per-profile (as per the documentation), with that option we could set the session variable if the new option is active, and use that to find the right profile during the redirect phase. If the session var is missing, we fallback to the original approach of using the redirects map.

Something like that? If that's the case - where does this option go? Currently wrap-oauth2 accepts the handler and profile map.

weavejester commented 1 year ago

Yes, I think that would work. I think the option would go in the profile, as you might not want to use the session approach for some providers. The session has the disadvantage of not being "thread safe", so you'd only want to use it where there's no other reasonable option.

lukaszkorecki commented 1 year ago

Sounds good - I'll make changes in my fork and open a proper PR 👍

Thinking out loud though - when the authorization process starts, we know exactly which profile started it for the current user, so carrying over the profile ID should be safe, just like holding on to the state parameter is. While yes, it's possible that the same person starts two auth flows at the same time and things will go south, but that's more of a user error based on my experience.

weavejester commented 1 year ago

Yeah, it's unlikely to occur, but I don't believe it's technically incorrect for two OAuth2 flows to be active for the same user at the same time. It's safer, if only incrementally, to use the redirect URL to determine the profile.