Refresh during network loss causes ASWebAuthenticationSession to show up again

[nylki]

Hi there,

This is less of a feature request or bug, but more of question.
I observed that when the refresh is being called during a network outage (or airplane mode) clearLogin() is called, due to the network error.
This in effect causes the ASWebAuthenticationSession sheet to re-appear (i think).

Well, what I was wondering how best to handle these situations with OAuthenticator, if already possible.
I suppose I could check inside my storeLogin if the login to be stored is invalid and then simply decide not to store it?
But this feels like an antipattern somehow, because at that point I cannot reason about the cause of the invalid login. Is there a better way to discard certain errors and not handle them as invalid logins, so that future calls/retries to the refresh can be succesful, without the user having to manually log-in again?

Another way would be to simply not do any network calls in such situations in the first place of course, which is what I am currently considering. But in my opinion it could be useful to treat specific errors as recoverable/retrieable in terms of refresh.

thanks :)

[nylki]

A few moments after writing this issue, I looked at the refresh provider one more time and I think I found a solution that could work for me.
The RefreshProvider already allows handling errors of course, what I missed was that I can simply return the passed/current login back in certain situations (URLError, .notConnectedToInternet) (so that the old login keeps being stored in memory and via LoginStorage, until network is back and a refresh is hopefully going through succesfully).

For posterity, this is my current refreshProvider (https://github.com/nylki/CommonsFinder/blob/9db4d031f3eb9bb356e7f6a6d4e79fa26d670fab/CommonsFinder/Networking/MediaWikiOAuth.swift#L259):

    @Sendable
    static func refreshProvider(serverConfig: ServerConfig) -> TokenHandling.RefreshProvider {
        return { login, appCredentials, urlLoader in

            let request = try authenticationRefreshRequest(
                serverConfig: serverConfig,
                login: login,
                appCredentials: appCredentials
            )

            let data: Data

            do {
                let (fetchedData, _) = try await urlLoader(request)
                data = fetchedData
            } catch let urlError as URLError {
                switch urlError.code {
                case .notConnectedToInternet, .cannotLoadFromNetwork, .networkConnectionLost:
                    logger.notice("!!! Tried to refresh with no internet connection. Treat as recoverable and return the old login for now.")
                    return login
                default:
                    throw urlError
                }
            } catch {
                throw error
            }

            do {
                let response = try jsonDecoder.decode(AuthLoginResponse.self, from: data)
                return response.login
            } catch let decodingError as DecodingError {
                if let apiError = try? jsonDecoder.decode(AuthAPIErrorResponse.self, from: data) {
                    throw apiError
                }
                throw decodingError
            } catch {
                logger.error("Error during refresh \(error)")
                throw error
            }
        }
    }

[mattmassicotte]

Hmm, ok this is an interesting case. I think you are on to something here and the system might be too aggressive. You can definitely use the existing hooks to modify the behaviors, but I wonder if perhaps another system could be more appropriate? Or, if the networking wrapper could be augmented somehow?