supabase-community / gotrue-swift

A Swift client library for GoTrue.
MIT License
36 stars 31 forks source link

getSessionFromUrl expects query but receives fragment (using ASWebAuthenticationSession) #22

Closed ky1ejs closed 2 years ago

ky1ejs commented 2 years ago

I'm a bit confused about how this lib is supposed to be used for signing in via a Provider (e.g. Google Oauth).

As mentioned by this issue in the supabase/supabase repo (see screenshot below) callbacks come with params as a fragment, not a query.

However this lib asserts that a query is required to parse a Provider based sign-in: https://github.com/supabase-community/gotrue-swift/blob/5a5aca5aa63fbb4639458dc0c044f17586c3c9d7/Tests/GoTrueTests/GoTrueTests.swift#L45-L68

So the following code fails:

func authenticate() {
  let client = SupabaseClient(supabaseUrl: host, supabaseKey: clientId)

  client.auth.signIn(provider: .google, options: .init(redirectTo: "my-app://supabase-oauth-callback", scopes: nil)) { authReslt in
    switch authReslt {
    case .success(let url):
      let session = ASWebAuthenticationSession(url: url, callbackURLScheme: "my-app") { callbackUrl, error in
        guard error == nil, let callbackUrl = callbackUrl else { return } // handle this error case

        // will fail on the line below because the url will be `my-app//supabase-oauth-callback#access_token=.....`
        // rather than `my-app//supabase-oauth-callback?access_token=.....`
        client.auth.getSessionFromUrl(url: callbackUrl.absoluteString) { sessionResult in
          switch sessionResult {
          case .success:
            // login succeeded
          case .failure:
            // handle login failure           
          }
        }
      }

      session.presentationContextProvider = self
      session.start()             
    case .failure:
      continuation.resume(returning: false)
    }
  }
}

This is the exact callbackUrl that this code receives:

my-app://supabase-oauth-callback#access_token=ACCESS_TOKEN&expires_in=3600&provider_token=PROVIDER_TOKEN&refresh_token=REFRESH&token_type=bearer

supabase/supabase discussions/2133

Screenshot 2022-06-08 at 23 28 30
grdsdev commented 2 years ago

Hi @ky1ejs thanks for opening this, I'll take a look on this and fix it ASAP.

ky1ejs commented 2 years ago

I'm happy to open a PR for this issue, I was more so looking to understand if this is the correct behaviour for mobile clients / the REST API. I was hoping @inian or someone from Supabase would speak to that.

grdsdev commented 2 years ago

@ky1ejs I've opened a PR fixing this, can you take a look at it and maybe point your app to the fixed branch and verify if the issue has been solved?

PR: https://github.com/supabase-community/gotrue-swift/pull/23

grdsdev commented 2 years ago

I'm happy to open a PR for this issue, I was more so looking to understand if this is the correct behaviour for mobile clients / the REST API. I was hoping @inian or someone from Supabase would speak to that.

The behavior should be the same for all platforms, this was a mistake when building.

inian commented 2 years ago

Agree that the callback params should be in the query fragment!

ky1ejs commented 2 years ago

@grsouza I'll take a look and confirm with you asap in this issue

ky1ejs commented 2 years ago

@grsouza confirmed that this fix works 🎉

It would be great if a version bump could be processed soon for grotrue-swift and supabase-swift.

Although, I'd love to provide some tweaks to the API for gotrue, such as adding @discarableResult for functions such as:

let _ = try! await client.auth.session(from: callbackUrl.absoluteURL)