OAuth functionality doesn't retry because a 401/403 doesn't throw an error

[theisegeberg]

Line #43 of APIClient has this:

    public func send(_ request: URLRequest) async throws -> (Data, URLResponse) {
        do {
            return try await actuallySend(request)
        } catch {
            guard await delegate.shouldClientRetry(self, withError: error) else { throw error }
            return try await actuallySend(request)
        }
    }

Perhaps I'm mistaken, but a 401/403 doesn't actually throw an error here, it'll just continue a normal right? The shouldClientRetry would only be called if the network had some actually throwing error here, right?

I usually approach OAuth in a completely different way, and if you agree I'd love to submit a PR with a different approach where auth'ed endpoints are wrapped at the call site, rather than handled inside of the API framework.

[kean]

actuallySend performs response validation and throws an error for any status code no in 2xx...3xx range.

I usually approach OAuth in a completely different way, and if you agree I'd love to submit a PR

I'm interested to see it.

[theisegeberg]

The way I read the code validation occurs on line #44 in the initial send function. I’m guessing it should be actuallySend though. Am I misreading it completely?

[kean]

You are right, it's in the wrong place. The validation should happen in actuallySend for authentication to work. I was combining multiple samples and ended up placing them in the wrong place.

[kean]

Addressed in 0.0.4

[theisegeberg]

Here's a slightly quick and dirty example of my usual approach. Sorry, I've never had to write text about this approach before, so it got a bit long winded. 😄 - The callAsFunction is a matter of taste. It could just be a function, but this shifts a tiny bit of cognitive load.

Pro: All potential retries are visual at the call site. Con: You can forget to put these retries there of course, potentially you'd just make a single point where you go sendAuthed(_ request:Request). You get a bit of a nesting hell there. Which COULD be fixed with result builders, but would be some syntactic overhead.

Pro: You can pretty easily wrap different kinds of retries inside each other, and you'll have fall backs that are structured. If the outer one fails, it won't retry to the first one again. So it's deterministic, you will never have an infinite loop of retries. Con: The order of the fallbacks become important. If the errors that are thrown can happen in a completely mixed order, and they all demand retries, then it needs to be more free. But I'm not exactly seeing that as something that should be handled at such high level.

Pro: It'll fit the standard JWT + 2FA system neatly and can salvage calls all the way down. Most common scenario would be: 1. User has stale refreshToken, 2. User glitches login, 3. User needs to re-auth with some 2FA system. This is normally pretty complex and hard to read. Usually it's just not built with a final retry of the original call that failed in mind. And the code for doing this "without a retry" is often messier than handling the retry I think. Because you'll need a method to exit to the login screen from a failed call. Con: It requires the login screen to be presentable in a meaningful place at all places, and it doesn't take into account any background call that fails.

struct Retry {
  let client:APIClient
  let doBeforeRetry:() async throws -> Void
  let retryIfTrue:(Error) -> Bool

  func callAsFunction<T:Decodable>(_ f:(APIClient) async throws ->(T)) async rethrows -> T {
    do {
      return try await f(client)
    } catch {
      guard retryIfTrue(error) else {
        throw error
      }
      try await doBeforeRetry()
      return try await f(client)
    }
  }
}

let SharedAPIClient = APIClient(configuration:
                                    .init(host: Configuration.API.API_BASE_URL_STRING,
                                          port: Configuration.API.API_PORT,
                                          isInsecure: Configuration.API.API_SECURE == false,
                                          delegate: APIDelegate()
                                         ))

struct APIPath {
  static func with(ext:String) -> String {
    return "\(Configuration.API.API_BASE_PATH)\(ext)"
  }
}

let Auth = Retry(client: SharedAPIClient) {
  fatalError() // Refresh token
} retryIfTrue: { error in
  fatalError() // Check if error indicates that token needs to be refreshed
}

let Relogin = Retry(client: SharedAPIClient) {
  fatalError() // Trigger the app to popover a login screen and log the user in via that
} retryIfTrue: { error in
  fatalError() // Check if error indicates that login was wrong
}

struct User:Codable {}

func getUser() async throws -> User {
  try await Relogin { _ in
    try await Auth { client in
      try await client.send(.get(APIPath.with(ext: "/users"))).value
    }
  }
}

[kean]

Got it. Yeah, I used a similar approach in one of the apps I worked on in the past. But I think centralization has more advantages:

• Individual requests don't need to worry about authorization anymore – no duplication
• Token refresh is handled in a single place making it easier to coalesce requests (multiple requests fail with 401 but only one refresh needs to be performed)
• It's generally easier to implement logout with a centralized approach