diff --git a/Account/Sources/Account/AccountDelegates/FeedlyAccountDelegate+OAuth.swift b/Account/Sources/Account/AccountDelegates/FeedlyAccountDelegate+OAuth.swift index 5ee8dd3e7..e75193dbb 100644 --- a/Account/Sources/Account/AccountDelegates/FeedlyAccountDelegate+OAuth.swift +++ b/Account/Sources/Account/AccountDelegates/FeedlyAccountDelegate+OAuth.swift @@ -62,6 +62,7 @@ extension FeedlyAccountDelegate: OAuthAuthorizationGranting { } extension FeedlyAccountDelegate: OAuthAccessTokenRefreshing { + func refreshAccessToken(with refreshToken: String, client: OAuthAuthorizationClient) async throws -> OAuthAuthorizationGrant { let request = OAuthRefreshAccessTokenRequest(refreshToken: refreshToken, scope: nil, client: client) diff --git a/Account/Sources/Account/AccountDelegates/FeedlyAccountDelegate.swift b/Account/Sources/Account/AccountDelegates/FeedlyAccountDelegate.swift index d8e02ae22..935174019 100644 --- a/Account/Sources/Account/AccountDelegates/FeedlyAccountDelegate.swift +++ b/Account/Sources/Account/AccountDelegates/FeedlyAccountDelegate.swift @@ -610,7 +610,7 @@ final class FeedlyAccountDelegate: AccountDelegate { static func validateCredentials(transport: Transport, credentials: Credentials, endpoint: URL?, secretsProvider: SecretsProvider) async throws -> Credentials? { - assertionFailure("An `account` instance should enqueue an \(FeedlyRefreshAccessTokenOperation.self) instead.") + assertionFailure("An `account` instance should refresh the access token first instead.") return credentials } @@ -642,34 +642,36 @@ final class FeedlyAccountDelegate: AccountDelegate { extension FeedlyAccountDelegate: FeedlyAPICallerDelegate { - @MainActor func reauthorizeFeedlyAPICaller(_ caller: FeedlyAPICaller, completionHandler: @escaping (Bool) -> ()) { + @MainActor func reauthorizeFeedlyAPICaller(_ caller: FeedlyAPICaller) async -> Bool { + guard let account = initializedAccount else { - completionHandler(false) - return + return false } - - /// Captures a failure to refresh a token, assuming that it was refreshed unless told otherwise. - final class RefreshAccessTokenOperationDelegate: FeedlyOperationDelegate { - - private(set) var didReauthorize = true - - func feedlyOperation(_ operation: FeedlyOperation, didFailWith error: Error) { - didReauthorize = false - } + + do { + try await refreshAccessToken(account: account) + return true + } catch { + return false } - - let refreshAccessToken = FeedlyRefreshAccessTokenOperation(account: account, service: self, oauthClient: oauthAuthorizationClient, log: log) - refreshAccessToken.downloadProgress = refreshProgress - - /// This must be strongly referenced by the completionBlock of the `FeedlyRefreshAccessTokenOperation`. - let refreshAccessTokenDelegate = RefreshAccessTokenOperationDelegate() - refreshAccessToken.delegate = refreshAccessTokenDelegate - - refreshAccessToken.completionBlock = { operation in - assert(Thread.isMainThread) - completionHandler(refreshAccessTokenDelegate.didReauthorize && !operation.isCanceled) + } + + private func refreshAccessToken(account: Account) async throws { + + guard let credentials = try account.retrieveCredentials(type: .oauthRefreshToken) else { + os_log(.debug, log: log, "Could not find a refresh token in the keychain. Check the refresh token is added to the Keychain, remove the account and add it again.") + throw TransportError.httpError(status: 403) } - - MainThreadOperationQueue.shared.add(refreshAccessToken) + + os_log(.debug, log: log, "Refreshing access token.") + let grant = try await refreshAccessToken(with: credentials.secret, client: oauthAuthorizationClient) + + os_log(.debug, log: log, "Storing refresh token.") + if let refreshToken = grant.refreshToken { + try account.storeCredentials(refreshToken) + } + + os_log(.debug, log: log, "Storing access token.") + try account.storeCredentials(grant.accessToken) } } diff --git a/Account/Sources/Account/Feedly/FeedlyAPICaller.swift b/Account/Sources/Account/Feedly/FeedlyAPICaller.swift index bc2f89a63..58f98e704 100644 --- a/Account/Sources/Account/Feedly/FeedlyAPICaller.swift +++ b/Account/Sources/Account/Feedly/FeedlyAPICaller.swift @@ -12,9 +12,10 @@ import Secrets import Feedly protocol FeedlyAPICallerDelegate: AnyObject { + /// Implemented by the `FeedlyAccountDelegate` reauthorize the client with a fresh OAuth token so the client can retry the unauthorized request. /// Pass `true` to the completion handler if the failing request should be retried with a fresh token or `false` if the unauthorized request should complete with the original failure error. - @MainActor func reauthorizeFeedlyAPICaller(_ caller: FeedlyAPICaller, completionHandler: @escaping (Bool) -> ()) + @MainActor func reauthorizeFeedlyAPICaller(_ caller: FeedlyAPICaller) async -> Bool } @MainActor final class FeedlyAPICaller { @@ -102,47 +103,46 @@ protocol FeedlyAPICallerDelegate: AnyObject { } private func send(request: URLRequest, resultType: R.Type, dateDecoding: JSONDecoder.DateDecodingStrategy = .iso8601, keyDecoding: JSONDecoder.KeyDecodingStrategy = .useDefaultKeys, completion: @escaping (Result<(HTTPURLResponse, R?), Error>) -> Void) { + transport.send(request: request, resultType: resultType, dateDecoding: dateDecoding, keyDecoding: keyDecoding) { [weak self] result in - - MainActor.assumeIsolated { - + + Task { @MainActor [weak self] in + switch result { case .success: completion(result) case .failure(let error): switch error { case TransportError.httpError(let statusCode) where statusCode == 401: - + assert(self == nil ? true : self?.delegate != nil, "Check the delegate is set to \(FeedlyAccountDelegate.self).") - + guard let self = self, let delegate = self.delegate else { completion(result) return } - + /// Capture the credentials before the reauthorization to check for a change. let credentialsBefore = self.credentials - - delegate.reauthorizeFeedlyAPICaller(self) { [weak self] isReauthorizedAndShouldRetry in - assert(Thread.isMainThread) - - guard isReauthorizedAndShouldRetry, let self = self else { - completion(result) - return - } - - // Check for a change. Not only would it help debugging, but it'll also catch an infinitely recursive attempt to refresh. - guard let accessToken = self.credentials?.secret, accessToken != credentialsBefore?.secret else { - assertionFailure("Could not update the request with a new OAuth token. Did \(String(describing: self.delegate)) set them on \(self)?") - completion(result) - return - } - - var reauthorizedRequest = request - reauthorizedRequest.setValue("OAuth \(accessToken)", forHTTPHeaderField: HTTPRequestHeader.authorization) - - self.send(request: reauthorizedRequest, resultType: resultType, dateDecoding: dateDecoding, keyDecoding: keyDecoding, completion: completion) + + let isReauthorizedAndShouldRetry = await delegate.reauthorizeFeedlyAPICaller(self) + guard isReauthorizedAndShouldRetry else { + completion(result) + return } + + // Check for a change. Not only would it help debugging, but it'll also catch an infinitely recursive attempt to refresh. + guard let accessToken = self.credentials?.secret, accessToken != credentialsBefore?.secret else { + assertionFailure("Could not update the request with a new OAuth token. Did \(String(describing: self.delegate)) set them on \(self)?") + completion(result) + return + } + + var reauthorizedRequest = request + reauthorizedRequest.setValue("OAuth \(accessToken)", forHTTPHeaderField: HTTPRequestHeader.authorization) + + self.send(request: reauthorizedRequest, resultType: resultType, dateDecoding: dateDecoding, keyDecoding: keyDecoding, completion: completion) + default: completion(result) } @@ -150,7 +150,7 @@ protocol FeedlyAPICallerDelegate: AnyObject { } } } - + func importOPML(_ opmlData: Data) async throws { guard !isSuspended else { throw TransportError.suspended } diff --git a/Account/Sources/Account/Feedly/Operations/FeedlyRefreshAccessTokenOperation.swift b/Account/Sources/Account/Feedly/Operations/FeedlyRefreshAccessTokenOperation.swift deleted file mode 100644 index 4bcd8061d..000000000 --- a/Account/Sources/Account/Feedly/Operations/FeedlyRefreshAccessTokenOperation.swift +++ /dev/null @@ -1,60 +0,0 @@ -// -// FeedlyRefreshAccessTokenOperation.swift -// Account -// -// Created by Kiel Gillard on 4/11/19. -// Copyright © 2019 Ranchero Software, LLC. All rights reserved. -// - -import Foundation -import os.log -import Web -import Secrets -import Feedly - -final class FeedlyRefreshAccessTokenOperation: FeedlyOperation { - - let service: OAuthAccessTokenRefreshing - let oauthClient: OAuthAuthorizationClient - let account: Account - let log: OSLog - - init(account: Account, service: OAuthAccessTokenRefreshing, oauthClient: OAuthAuthorizationClient, log: OSLog) { - self.oauthClient = oauthClient - self.service = service - self.account = account - self.log = log - } - - override func run() { - - Task { @MainActor in - - do { - guard let credentials = try account.retrieveCredentials(type: .oauthRefreshToken) else { - os_log(.debug, log: log, "Could not find a refresh token in the keychain. Check the refresh token is added to the Keychain, remove the account and add it again.") - throw TransportError.httpError(status: 403) - } - - // Ignore cancellation after the request is resumed otherwise we may continue storing a potentially invalid token! - os_log(.debug, log: log, "Refreshing access token.") - let grant = try await service.refreshAccessToken(with: credentials.secret, client: oauthClient) - - // Store the refresh token first because it sends this token to the account delegate. - os_log(.debug, log: log, "Storing refresh token.") - if let refreshToken = grant.refreshToken { - try account.storeCredentials(refreshToken) - } - - // Now store the access token because we want the account delegate to use it. - os_log(.debug, log: log, "Storing access token.") - try account.storeCredentials(grant.accessToken) - - didFinish() - - } catch { - didFinish(with: error) - } - } - } -} diff --git a/Account/Tests/AccountTests/Feedly/FeedlyRefreshAccessTokenOperationTests.swift b/Account/Tests/AccountTests/Feedly/FeedlyRefreshAccessTokenOperationTests.swift deleted file mode 100644 index 30c6fab3b..000000000 --- a/Account/Tests/AccountTests/Feedly/FeedlyRefreshAccessTokenOperationTests.swift +++ /dev/null @@ -1,218 +0,0 @@ -// -// FeedlyRefreshAccessTokenOperationTests.swift -// AccountTests -// -// Created by Kiel Gillard on 4/11/19. -// Copyright © 2019 Ranchero Software, LLC. All rights reserved. -// - -import XCTest -@testable import Account -import Web -import Secrets - -class FeedlyRefreshAccessTokenOperationTests: XCTestCase { - - private var account: Account! - private let support = FeedlyTestSupport() - - override func setUp() { - super.setUp() - account = support.makeTestAccount() - } - - override func tearDown() { - if let account = account { - support.destroy(account) - } - super.tearDown() - } - - class TestRefreshTokenService: OAuthAccessTokenRefreshing { - var mockResult: Result? - var refreshAccessTokenExpectation: XCTestExpectation? - var parameterTester: ((String, OAuthAuthorizationClient) -> ())? - - func refreshAccessToken(with refreshToken: String, client: OAuthAuthorizationClient, completion: @escaping (Result) -> ()) { - - guard let result = mockResult else { - XCTFail("Missing mock result. Test may time out because the completion will not be called.") - return - } - parameterTester?(refreshToken, client) - DispatchQueue.main.async { - completion(result) - self.refreshAccessTokenExpectation?.fulfill() - } - } - } - - func testCancel() { - let service = TestRefreshTokenService() - service.refreshAccessTokenExpectation = expectation(description: "Did Call Refresh") - service.refreshAccessTokenExpectation?.isInverted = true - - let client = support.makeMockOAuthClient() - let refresh = FeedlyRefreshAccessTokenOperation(account: account, service: service, oauthClient: client, log: support.log) - - // If this expectation is not fulfilled, the operation is not calling `didFinish`. - let completionExpectation = expectation(description: "Did Finish") - refresh.completionBlock = { _ in - completionExpectation.fulfill() - } - - MainThreadOperationQueue.shared.add(refresh) - - MainThreadOperationQueue.shared.cancelOperations([refresh]) - - waitForExpectations(timeout: 1) - - XCTAssertTrue(refresh.isCanceled) - } - - class TestRefreshTokenDelegate: FeedlyOperationDelegate { - var error: Error? - var didFailExpectation: XCTestExpectation? - - func feedlyOperation(_ operation: FeedlyOperation, didFailWith error: Error) { - self.error = error - didFailExpectation?.fulfill() - } - } - - func testMissingRefreshToken() { - support.removeCredentials(matching: .oauthRefreshToken, from: account) - - let service = TestRefreshTokenService() - service.refreshAccessTokenExpectation = expectation(description: "Did Call Refresh") - service.refreshAccessTokenExpectation?.isInverted = true - - let client = support.makeMockOAuthClient() - let refresh = FeedlyRefreshAccessTokenOperation(account: account, service: service, oauthClient: client, log: support.log) - - let delegate = TestRefreshTokenDelegate() - delegate.didFailExpectation = expectation(description: "Did Fail") - refresh.delegate = delegate - - // If this expectation is not fulfilled, the operation is not calling `didFinish`. - let completionExpectation = expectation(description: "Did Finish") - refresh.completionBlock = { _ in - completionExpectation.fulfill() - } - - MainThreadOperationQueue.shared.add(refresh) - - waitForExpectations(timeout: 1) - - XCTAssertNotNil(delegate.error, "Should have failed with error.") - if let error = delegate.error { - switch error { - case let error as TransportError: - switch error { - case .httpError(status: let status): - XCTAssertEqual(status, 403, "Expected 403 Forbidden.") - default: - XCTFail("Expected 403 Forbidden") - } - default: - XCTFail("Expected \(TransportError.httpError(status: 403))") - } - } - } - - func testRefreshTokenSuccess() { - let service = TestRefreshTokenService() - service.refreshAccessTokenExpectation = expectation(description: "Did Call Refresh") - - let mockAccessToken = Credentials(type: .oauthAccessToken, username: "Test", secret: UUID().uuidString) - let mockRefreshToken = Credentials(type: .oauthRefreshToken, username: "Test", secret: UUID().uuidString) - let grant = OAuthAuthorizationGrant(accessToken: mockAccessToken, refreshToken: mockRefreshToken) - service.mockResult = .success(grant) - - let client = support.makeMockOAuthClient() - service.parameterTester = { serviceRefreshToken, serviceClient in - if let accountRefreshToken = try! self.account.retrieveCredentials(type: .oauthRefreshToken) { - XCTAssertEqual(serviceRefreshToken, accountRefreshToken.secret) - } else { - XCTFail("Could not verify correct refresh token used.") - } - XCTAssertEqual(serviceClient, client) - } - - let refresh = FeedlyRefreshAccessTokenOperation(account: account, service: service, oauthClient: client, log: support.log) - - // If this expectation is not fulfilled, the operation is not calling `didFinish`. - let completionExpectation = expectation(description: "Did Finish") - refresh.completionBlock = { _ in - completionExpectation.fulfill() - } - - MainThreadOperationQueue.shared.add(refresh) - - waitForExpectations(timeout: 1) - - do { - let accessToken = try account.retrieveCredentials(type: .oauthAccessToken) - XCTAssertEqual(accessToken, mockAccessToken) - - let refreshToken = try account.retrieveCredentials(type: .oauthRefreshToken) - XCTAssertEqual(refreshToken, mockRefreshToken) - } catch { - XCTFail("Could not verify refresh and access tokens because \(error).") - } - } - - func testRefreshTokenFailure() { - let accessTokenBefore: Credentials - let refreshTokenBefore: Credentials - - do { - guard let accessToken = try account.retrieveCredentials(type: .oauthAccessToken), - let refreshToken = try account.retrieveCredentials(type: .oauthRefreshToken) else { - XCTFail("Initial refresh and/or access token does not exist.") - return - } - accessTokenBefore = accessToken - refreshTokenBefore = refreshToken - } catch { - XCTFail("Caught error getting initial refresh and access tokens because \(error).") - return - } - - let service = TestRefreshTokenService() - service.refreshAccessTokenExpectation = expectation(description: "Did Call Refresh") - service.mockResult = .failure(URLError(.timedOut)) - - let client = support.makeMockOAuthClient() - service.parameterTester = { serviceRefreshToken, serviceClient in - if let accountRefreshToken = try! self.account.retrieveCredentials(type: .oauthRefreshToken) { - XCTAssertEqual(serviceRefreshToken, accountRefreshToken.secret) - } else { - XCTFail("Could not verify correct refresh token used.") - } - XCTAssertEqual(serviceClient, client) - } - - let refresh = FeedlyRefreshAccessTokenOperation(account: account, service: service, oauthClient: client, log: support.log) - - // If this expectation is not fulfilled, the operation is not calling `didFinish`. - let completionExpectation = expectation(description: "Did Finish") - refresh.completionBlock = { _ in - completionExpectation.fulfill() - } - - MainThreadOperationQueue.shared.add(refresh) - - waitForExpectations(timeout: 1) - - do { - let accessToken = try account.retrieveCredentials(type: .oauthAccessToken) - XCTAssertEqual(accessToken, accessTokenBefore) - - let refreshToken = try account.retrieveCredentials(type: .oauthRefreshToken) - XCTAssertEqual(refreshToken, refreshTokenBefore) - } catch { - XCTFail("Could not verify refresh and access tokens because \(error).") - } - } -}