search

supabase-community / supabase-kt

A Kotlin Multiplatform Client for Supabase.
https://supabase.com/docs/reference/kotlin/introduction
MIT License
408 stars 37 forks source link

[Bug]: UserSession is deleted when backend is unavailable (503) #724

Closed jacobian2020 closed 1 month ago

jacobian2020 commented 1 month ago

General Info

Version(s)

2.6.1

Kotlin Target(s) and their respective versions

JVM 17 Android 14

What happened? (include your code)

When backend is unavailable (503), UserSession is deleted.

Offending line is at https://github.com/supabase-community/supabase-kt/blob/246fe23ebe590f7346b4590107d200fb62947c25/GoTrue/src/commonMain/kotlin/io/github/jan/supabase/gotrue/AuthImpl.kt#L438

Our corporate reverse proxy returned 503 due to supabase backend being unavailable and it logged out all our users due to a background job that retried to upload images when users were not even using the app.

Suggestions: Please don't clear the session for all http error codes. Only clear it when refresh returns 401 due to the token being incorrect. If your backend does not support this, make it configurable for now to ignore at least 503s as this will be returned by most webservers if server is having issues/under load etc. At it's current state this is unusable in production for us.

Steps To Reproduce (optional)

class Test {

    @get:Rule
    val wireMockRule = WireMockRule(wireMockConfig().port(0))

    @Test
    fun does_not_delete_session_on_error() {

        stubFor(any(urlPathMatching(".*"))
            .willReturn(aResponse()
                .withStatus(503)
                .withBody("Service Unavailable")));

        val mockSessionManager: SessionManager = mockk()
        val userSession: UserSession = mockk()
        every { userSession.accessToken } returns "xxx"
        every { userSession.refreshToken } returns "xxx"
        every { userSession.expiresAt } returns Instant.DISTANT_PAST
        coEvery { mockSessionManager.loadSession() } returns userSession
        coEvery { mockSessionManager.deleteSession() } just runs

        val client = createSupabaseClient(
            supabaseUrl = wireMockRule.baseUrl(),
            supabaseKey = "xxxx",
        ) {
            useHTTPS = false
            install(Auth) {
                sessionManager = mockSessionManager
            }
        }

        runBlocking { client.auth.awaitInitialization() }

        coVerify(inverse = true) { mockSessionManager.deleteSession() }
    }

}

Relevant log output (optional)

Info: (Supabase-Auth) Successfully loaded session from storage!
Error: (Supabase-Auth) Couldn't refresh session. The refresh token may have been revoked.
io.github.jan.supabase.gotrue.exception.AuthRestException: 
    at io.github.jan.supabase.gotrue.AuthImpl.checkErrorCodes(AuthImpl.kt:523)
    at io.github.jan.supabase.gotrue.AuthImpl.parseErrorResponse(AuthImpl.kt:491)
    at io.github.jan.supabase.gotrue.AuthenticatedSupabaseApiKt$authenticatedSupabaseApi$3.invoke(AuthenticatedSupabaseApi.kt:60)
    at io.github.jan.supabase.gotrue.AuthenticatedSupabaseApiKt$authenticatedSupabaseApi$3.invoke(AuthenticatedSupabaseApi.kt:60)
    at io.github.jan.supabase.network.SupabaseApi.rawRequest$suspendImpl(SupabaseApi.kt:25)
    at io.github.jan.supabase.network.SupabaseApi$rawRequest$1.invokeSuspend(SupabaseApi.kt)
jan-tennert commented 1 month ago

Thanks for the issue, will publish a fix later!

jan-tennert commented 1 month ago

make it configurable for now to ignore at least 503s as this will be returned by most webservers if server is having issues/under load etc

Just to make sure, if the refresh fails with such a status code, what exactly should happen? Should the exception just be silently ignored (which causes auto refresh to stop essentially until restart), or should the client retry refreshing after AuthConfig#retryDelay? I'd assume the latter, prepared a PR #725