PROPOSAL: Implement refresh token functionality

[microtherion]

EDITED 2025-11-09

This PR features a proposed implementation of refresh token functionality. Unfortunately, to do so, it had to make architectural changes that may not be acceptable to the maintainers.

The fundamental problem I was facing is that:

1. The refreshing needs to be done from client code.
2. The client code needs access to the FederatedService doing the refresh to initiate the refreshing.

So the code to some extent needed to punch a hole in the existing abstractions.

Usage

Client configuration

A client wishing to use the refresh functionality needs to store a handle to the service in an accessible location:

nonisolated(unsafe) static var dropboxAuth: RefreshableFederatedService!

dropboxAuth  =
        try routes.oAuthRefreshable(from: Dropbox.self, authenticate: "login/dropbox",
                         callback: host+"/auth/dropbox", completion: processDropboxLogin)

Refreshing

Clients needs to test calls for authorization failures. On failure, they need to refresh their access token and retry their request:

private func refresh(request: Request) throws -> EventLoopFuture<Void> {
    let promise = request.eventLoop.makePromise(of: Void.self)
    promise.completeWithTask {
        let refreshToken = try request.session.refreshToken()

        try await dropboxAuth.refreshAccessToken(request, refreshToken: refreshToken) { request, accessToken in
                request.session.setAccessToken(accessToken)
        }
    }
        
    return promise.futureResult
}

var headers = HTTPHeaders()
let token = try request.session.accessToken()
headers.bearerAuthorization = BearerAuthorization(token: token)

var retriableRequest = ClientRequest(method: .POST, url: URI(string: url), headers: headers)
try beforeSend(&retriableRequest)

return request.client.send(retriableRequest)
.tryFlatMap { response in
    if response.status == .unauthorized {
        // Refresh and retry once
        return try self.refresh(request: request)
        .tryFlatMap {
            // Update authorization
            let token = try request.session.accessToken()
            retriableRequest.headers.bearerAuthorization = BearerAuthorization(token: token)
            return request.client.send(retriableRequest)
         }
    } else {
         return request.eventLoop.makeSucceededFuture(response)
    }
}

[codecov]

Codecov Report

❌ Patch coverage is 56.25000% with 35 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.19%. Comparing base (e1ecc85) to head (51862eb).

Files with missing lines	Patch %	Lines
Sources/ImperialCore/FederatedServiceRouter.swift	20.83%	19 Missing ⚠️
Sources/ImperialCore/RoutesBuilder+oAuth.swift	0.00%	14 Missing ⚠️
...ces/ImperialCore/FederatedServiceRefreshBody.swift	0.00%	1 Missing ⚠️
Sources/ImperialImgur/ImgurRouter.swift	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #121      +/-   ##
==========================================
- Coverage   82.98%   79.19%   -3.79%     
==========================================
  Files          63       63              
  Lines         852      817      -35     
==========================================
- Hits          707      647      -60     
- Misses        145      170      +25     

Files with missing lines	Coverage Δ
Sources/ImperialAuth0/Auth0.swift	100.00% <100.00%> (ø)
Sources/ImperialCore/Sessions+Imperial.swift	33.33% <100.00%> (+7.40%)	⬆️
Sources/ImperialDeviantArt/DeviantArt.swift	100.00% <100.00%> (ø)
Sources/ImperialDeviantArt/DeviantArtRouter.swift	84.61% <100.00%> (+1.28%)	⬆️
Sources/ImperialDiscord/Discord.swift	100.00% <100.00%> (ø)
Sources/ImperialDropbox/Dropbox.swift	100.00% <100.00%> (ø)
Sources/ImperialDropbox/DropboxRouter.swift	96.77% <100.00%> (-0.11%)	⬇️
Sources/ImperialFacebook/Facebook.swift	100.00% <100.00%> (ø)
Sources/ImperialGitHub/GitHub.swift	100.00% <100.00%> (ø)
Sources/ImperialGitlab/Gitlab.swift	100.00% <100.00%> (ø)
... and 12 more

... and 29 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[fpseverino]

IIRC the original issue was that FederatedServiceRouter and FederatedServiceTokens were no longer public. Can we leverage this to improve this PR?

[microtherion]

Yes, if there is a good likelihood that #122 will be adopted, I definitely think I could work off that. Let me give it a shot.

[fpseverino]

Good, I just merged it

[microtherion]

@fpseverino here's the latest iteration. With FederatedServiceRouter being public, the implementation indeed becomes more straightforward, and the distinction between oAuth and oAuthRefreshable you proposed also simplifies things.

One potentially problematic element I now see is the new router requirement in the FederatedService protocol. I could provide a default implementation, but that would be somewhat awkward. What do you think? Does this still count as a breaking change?

[fpseverino]

Does this still count as a breaking change?

Yeah, there are still a couple of breaking changes

[microtherion]

I forgot to mention that 45433ac and 179a3cc are technically fixes for bug I discovered during testing, not part of the core functionality under discussion. Should I break these out into 1 or 2 separate PRs?

[fpseverino]

I forgot to mention that 45433ac and 179a3cc are technically fixes for bug I discovered during testing, not part of the core functionality under discussion. Should I break these out into 1 or 2 separate PRs?

Nah, let's keep them here, they're small enough

[fpseverino]

2 breaking changes detected in ImperialCore:
💔 API breakage: var FederatedService.router has been added as a protocol requirement
💔 API breakage: func Session.setRefreshToken(_:) has parameter 0 type change from Swift.String to Swift.String?

Ok, so, the second one is easy enough to fix, you just have to make a little if let... dance either calling the original method or not calling it when the value is nil

For the first one, technically if we add router as a computed variable, and offer a default implementation (that maybe returns nil, idk), it shouldn't be a breaking change.

Let me know if you're still interested and if you think it's worth adding these changes now, or waiting for the first alpha release of Vapor 5 and preparing a new major release of Imperial (I can't tell you when the alpha of Vapor 5 will be released, though).

[microtherion]

Thanks for looking into this! Would be happy to make those changes.