From 6224dfad031e2cb34e1b148aac3c39f3d3ab39f1 Mon Sep 17 00:00:00 2001 From: Stuart Breckenridge Date: Mon, 18 May 2020 08:39:22 +0800 Subject: [PATCH 1/6] Notification Permission Requests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #2057 • On app launch, the app checks if notification permissions are granted and registers with APNS if that is the case. It will not request permissions as part of the app launch. • When a user requests to be notified of new articles, the authorizationStatus is checked: - if `notDetermined` or `provisional`, an authorization request is made, and if successful, the Notify of New Articles status is updated (otherwise it is reverted) - if `denied`, an alert is thrown asking the user to enable in settings (and the change to notify of new articles is reverted) - if `authorized` the update is made. `WebFeedInspectorViewController` also monitors for the app entering the foreground so that it can get the latest notification auth settings. --- iOS/AppDelegate.swift | 6 +- .../WebFeedInspectorViewController.swift | 65 ++++++++++++++++++- 2 files changed, 67 insertions(+), 4 deletions(-) diff --git a/iOS/AppDelegate.swift b/iOS/AppDelegate.swift index 0dd99bad3..b5c7244b8 100644 --- a/iOS/AppDelegate.swift +++ b/iOS/AppDelegate.swift @@ -90,9 +90,9 @@ class AppDelegate: UIResponder, UIApplicationDelegate, UNUserNotificationCenterD DispatchQueue.main.async { self.unreadCount = AccountManager.shared.unreadCount } - - UNUserNotificationCenter.current().requestAuthorization(options:[.badge, .sound, .alert]) { (granted, error) in - if granted { + + UNUserNotificationCenter.current().getNotificationSettings { (settings) in + if settings.authorizationStatus == .authorized { DispatchQueue.main.async { UIApplication.shared.registerForRemoteNotifications() } diff --git a/iOS/Inspector/WebFeedInspectorViewController.swift b/iOS/Inspector/WebFeedInspectorViewController.swift index 9a39d6a78..a518a8777 100644 --- a/iOS/Inspector/WebFeedInspectorViewController.swift +++ b/iOS/Inspector/WebFeedInspectorViewController.swift @@ -9,6 +9,7 @@ import UIKit import Account import SafariServices +import UserNotifications class WebFeedInspectorViewController: UITableViewController { @@ -38,6 +39,8 @@ class WebFeedInspectorViewController: UITableViewController { return webFeed.homePageURL == nil } + private var userNotificationSettings: UNNotificationSettings? + override func viewDidLoad() { tableView.register(InspectorIconHeaderView.self, forHeaderFooterViewReuseIdentifier: "SectionHeader") @@ -51,6 +54,14 @@ class WebFeedInspectorViewController: UITableViewController { feedURLLabel.text = webFeed.url NotificationCenter.default.addObserver(self, selector: #selector(webFeedIconDidBecomeAvailable(_:)), name: .WebFeedIconDidBecomeAvailable, object: nil) + + NotificationCenter.default.addObserver(forName: UIApplication.willEnterForegroundNotification, object: nil, queue: .main, using: { _ in + self.updateNotificationSettings() + }) + } + + override func viewDidAppear(_ animated: Bool) { + updateNotificationSettings() } override func viewDidDisappear(_ animated: Bool) { @@ -67,7 +78,30 @@ class WebFeedInspectorViewController: UITableViewController { } @IBAction func notifyAboutNewArticlesChanged(_ sender: Any) { - webFeed.isNotifyAboutNewArticles = notifyAboutNewArticlesSwitch.isOn + guard let settings = userNotificationSettings else { + notifyAboutNewArticlesSwitch.isOn = !notifyAboutNewArticlesSwitch.isOn + return + } + if settings.authorizationStatus == .denied { + notifyAboutNewArticlesSwitch.isOn = !notifyAboutNewArticlesSwitch.isOn + present(notificationUpdateErrorAlert(), animated: true, completion: nil) + } else if settings.authorizationStatus == .authorized { + webFeed.isNotifyAboutNewArticles = notifyAboutNewArticlesSwitch.isOn + } else { + UNUserNotificationCenter.current().requestAuthorization(options:[.badge, .sound, .alert]) { (granted, error) in + self.updateNotificationSettings() + if granted { + DispatchQueue.main.async { + self.webFeed.isNotifyAboutNewArticles = self.notifyAboutNewArticlesSwitch.isOn + UIApplication.shared.registerForRemoteNotifications() + } + } else { + DispatchQueue.main.async { + self.notifyAboutNewArticlesSwitch.isOn = !self.notifyAboutNewArticlesSwitch.isOn + } + } + } + } } @IBAction func alwaysShowReaderViewChanged(_ sender: Any) { @@ -158,3 +192,32 @@ extension WebFeedInspectorViewController: UITextFieldDelegate { } } + +// MARK: UNUserNotificationCenter + +extension WebFeedInspectorViewController { + + func updateNotificationSettings() { + UNUserNotificationCenter.current().getNotificationSettings { (settings) in + DispatchQueue.main.async { + self.userNotificationSettings = settings + if settings.authorizationStatus == .authorized { + UIApplication.shared.registerForRemoteNotifications() + } + } + } + } + + func notificationUpdateErrorAlert() -> UIAlertController { + let alert = UIAlertController(title: NSLocalizedString("Enable Notifications", comment: "Notifications"), + message: NSLocalizedString("Notifications need to be enabled in the Settings app.", comment: "Notifications need to be enabled in the Settings app."), preferredStyle: .alert) + let openSettings = UIAlertAction(title: NSLocalizedString("Open Settings", comment: "Open Settings"), style: .default) { (action) in + UIApplication.shared.open(URL(string: UIApplication.openSettingsURLString)!, options: [UIApplication.OpenExternalURLOptionsKey.universalLinksOnly : false], completionHandler: nil) + } + let dismiss = UIAlertAction(title: NSLocalizedString("Dismiss", comment: "Dismiss"), style: .cancel, handler: nil) + alert.addAction(openSettings) + alert.addAction(dismiss) + return alert + } + +} From 2297d218c03aad19892b7ac7034377dfa474f7e5 Mon Sep 17 00:00:00 2001 From: Stuart Breckenridge Date: Mon, 18 May 2020 09:23:42 +0800 Subject: [PATCH 2/6] Uses selector syntax. --- iOS/Inspector/WebFeedInspectorViewController.swift | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/iOS/Inspector/WebFeedInspectorViewController.swift b/iOS/Inspector/WebFeedInspectorViewController.swift index a518a8777..bb3522edf 100644 --- a/iOS/Inspector/WebFeedInspectorViewController.swift +++ b/iOS/Inspector/WebFeedInspectorViewController.swift @@ -55,9 +55,8 @@ class WebFeedInspectorViewController: UITableViewController { NotificationCenter.default.addObserver(self, selector: #selector(webFeedIconDidBecomeAvailable(_:)), name: .WebFeedIconDidBecomeAvailable, object: nil) - NotificationCenter.default.addObserver(forName: UIApplication.willEnterForegroundNotification, object: nil, queue: .main, using: { _ in - self.updateNotificationSettings() - }) + NotificationCenter.default.addObserver(self, selector: #selector(updateNotificationSettings), name: UIApplication.willEnterForegroundNotification, object: nil) + } override func viewDidAppear(_ animated: Bool) { @@ -197,6 +196,7 @@ extension WebFeedInspectorViewController: UITextFieldDelegate { extension WebFeedInspectorViewController { + @objc func updateNotificationSettings() { UNUserNotificationCenter.current().getNotificationSettings { (settings) in DispatchQueue.main.async { From 8d11ee6c8238e712661ce2f088c834d5517548f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kiel=20Gillard=20=F0=9F=A4=AA?= Date: Wed, 20 May 2020 12:22:34 +1000 Subject: [PATCH 3/6] Use the same API for removing feeds Feedly web does and side step potential encoding issues. Attempt to fix #1691. --- .../Account/Feedly/FeedlyAPICaller.swift | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/Frameworks/Account/Feedly/FeedlyAPICaller.swift b/Frameworks/Account/Feedly/FeedlyAPICaller.swift index d52c83bb8..3a42109ee 100644 --- a/Frameworks/Account/Feedly/FeedlyAPICaller.swift +++ b/Frameworks/Account/Feedly/FeedlyAPICaller.swift @@ -281,14 +281,8 @@ final class FeedlyAPICaller { } } - guard let encodedFeedId = encodeForURLPath(feedId) else { - return DispatchQueue.main.async { - completion(.failure(FeedlyAccountDelegateError.unexpectedResourceId(feedId))) - } - } - var components = baseUrlComponents - components.percentEncodedPath = "/v3/collections/\(encodedCollectionId)/feeds/\(encodedFeedId)" + components.percentEncodedPath = "/v3/collections/\(encodedCollectionId)/feeds/.mdelete" guard let url = components.url else { fatalError("\(components) does not produce a valid URL.") @@ -300,6 +294,19 @@ final class FeedlyAPICaller { request.addValue("application/json", forHTTPHeaderField: "Accept-Type") request.addValue("OAuth \(accessToken)", forHTTPHeaderField: HTTPRequestHeader.authorization) + do { + struct RemovableFeed: Encodable { + let id: String + } + let encoder = JSONEncoder() + let data = try encoder.encode([RemovableFeed(id: feedId)]) + request.httpBody = data + } catch { + return DispatchQueue.main.async { + completion(.failure(error)) + } + } + transport.send(request: request, resultType: [FeedlyFeed].self, dateDecoding: .millisecondsSince1970, keyDecoding: .convertFromSnakeCase) { result in switch result { case .success((let httpResponse, _)): From 93c8a8561307f98506680cdcfd5853c3a6ff22ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kiel=20Gillard=20=F0=9F=A4=AA?= Date: Tue, 19 May 2020 14:14:17 +1000 Subject: [PATCH 4/6] Update Feedly's sandbox OAuth client secret. --- .../Account/Feedly/OAuthAuthorizationClient+Feedly.swift | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Frameworks/Account/Feedly/OAuthAuthorizationClient+Feedly.swift b/Frameworks/Account/Feedly/OAuthAuthorizationClient+Feedly.swift index 420aa5ed5..ab311ca9f 100644 --- a/Frameworks/Account/Feedly/OAuthAuthorizationClient+Feedly.swift +++ b/Frameworks/Account/Feedly/OAuthAuthorizationClient+Feedly.swift @@ -25,11 +25,11 @@ extension OAuthAuthorizationClient { /// See https://developer.feedly.com/v3/sandbox/ for more information. /// The return value models public sandbox API values found at: /// https://groups.google.com/forum/#!topic/feedly-cloud/WwQWMgDmOuw - /// They are due to expire on January 31 2020. + /// They are due to expire on May 31st 2020. /// Verify the sandbox URL host in the FeedlyAPICaller.API.baseUrlComponents method, too. return OAuthAuthorizationClient(id: "sandbox", redirectUri: "urn:ietf:wg:oauth:2.0:oob", state: nil, - secret: "nZmS4bqxgRQkdPks") + secret: "4ZfZ5DvqmJ8vKgMj") } } From e5a7706bb7deb3ea24a851ec98b78bc11516008e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kiel=20Gillard=20=F0=9F=A4=AA?= Date: Wed, 20 May 2020 11:13:53 +1000 Subject: [PATCH 5/6] Asychronously renew OAuth access tokens as needed for any 401 Unauthorized response from Feedly and automatically retry the request. Fixes #1859 --- .../Account/Feedly/FeedlyAPICaller.swift | 86 +++++++++++++++---- .../Feedly/FeedlyAccountDelegate.swift | 54 ++++++++++-- 2 files changed, 116 insertions(+), 24 deletions(-) diff --git a/Frameworks/Account/Feedly/FeedlyAPICaller.swift b/Frameworks/Account/Feedly/FeedlyAPICaller.swift index 3a42109ee..9b1421df7 100644 --- a/Frameworks/Account/Feedly/FeedlyAPICaller.swift +++ b/Frameworks/Account/Feedly/FeedlyAPICaller.swift @@ -9,6 +9,12 @@ import Foundation import RSWeb +protocol FeedlyAPICallerDelegate: class { + /// 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. + func reauthorizeFeedlyAPICaller(_ caller: FeedlyAPICaller, completionHandler: @escaping (Bool) -> ()) +} + final class FeedlyAPICaller { enum API { @@ -47,6 +53,8 @@ final class FeedlyAPICaller { self.baseUrlComponents = api.baseUrlComponents } + weak var delegate: FeedlyAPICallerDelegate? + var credentials: Credentials? var server: String? { @@ -69,6 +77,54 @@ final class FeedlyAPICaller { isSuspended = false } + 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 + assert(Thread.isMainThread) + + 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) + } + default: + completion(result) + } + } + } + } + func importOpml(_ opmlData: Data, completion: @escaping (Result) -> ()) { guard !isSuspended else { return DispatchQueue.main.async { @@ -95,7 +151,7 @@ final class FeedlyAPICaller { request.addValue("OAuth \(accessToken)", forHTTPHeaderField: HTTPRequestHeader.authorization) request.httpBody = opmlData - transport.send(request: request, resultType: String.self, dateDecoding: .millisecondsSince1970, keyDecoding: .convertFromSnakeCase) { result in + send(request: request, resultType: String.self, dateDecoding: .millisecondsSince1970, keyDecoding: .convertFromSnakeCase) { result in switch result { case .success(let (httpResponse, _)): if httpResponse.statusCode == 200 { @@ -147,7 +203,7 @@ final class FeedlyAPICaller { } } - transport.send(request: request, resultType: [FeedlyCollection].self, dateDecoding: .millisecondsSince1970, keyDecoding: .convertFromSnakeCase) { result in + send(request: request, resultType: [FeedlyCollection].self, dateDecoding: .millisecondsSince1970, keyDecoding: .convertFromSnakeCase) { result in switch result { case .success(let (httpResponse, collections)): if httpResponse.statusCode == 200, let collection = collections?.first { @@ -200,7 +256,7 @@ final class FeedlyAPICaller { } } - transport.send(request: request, resultType: [FeedlyCollection].self, dateDecoding: .millisecondsSince1970, keyDecoding: .convertFromSnakeCase) { result in + send(request: request, resultType: [FeedlyCollection].self, dateDecoding: .millisecondsSince1970, keyDecoding: .convertFromSnakeCase) { result in switch result { case .success(let (httpResponse, collections)): if httpResponse.statusCode == 200, let collection = collections?.first { @@ -248,7 +304,7 @@ final class FeedlyAPICaller { request.addValue("application/json", forHTTPHeaderField: "Accept-Type") request.addValue("OAuth \(accessToken)", forHTTPHeaderField: HTTPRequestHeader.authorization) - transport.send(request: request, resultType: String.self, dateDecoding: .millisecondsSince1970, keyDecoding: .convertFromSnakeCase) { result in + send(request: request, resultType: String.self, dateDecoding: .millisecondsSince1970, keyDecoding: .convertFromSnakeCase) { result in switch result { case .success(let (httpResponse, _)): if httpResponse.statusCode == 200 { @@ -307,7 +363,7 @@ final class FeedlyAPICaller { } } - transport.send(request: request, resultType: [FeedlyFeed].self, dateDecoding: .millisecondsSince1970, keyDecoding: .convertFromSnakeCase) { result in + send(request: request, resultType: [FeedlyFeed].self, dateDecoding: .millisecondsSince1970, keyDecoding: .convertFromSnakeCase) { result in switch result { case .success((let httpResponse, _)): if httpResponse.statusCode == 200 { @@ -369,7 +425,7 @@ extension FeedlyAPICaller: FeedlyAddFeedToCollectionService { } } - transport.send(request: request, resultType: [FeedlyFeed].self, dateDecoding: .millisecondsSince1970, keyDecoding: .convertFromSnakeCase) { result in + send(request: request, resultType: [FeedlyFeed].self, dateDecoding: .millisecondsSince1970, keyDecoding: .convertFromSnakeCase) { result in switch result { case .success((_, let collectionFeeds)): if let feeds = collectionFeeds { @@ -435,7 +491,7 @@ extension FeedlyAPICaller: OAuthAuthorizationCodeGrantRequesting { return } - transport.send(request: request, resultType: AccessTokenResponse.self, keyDecoding: .convertFromSnakeCase) { result in + send(request: request, resultType: AccessTokenResponse.self, keyDecoding: .convertFromSnakeCase) { result in switch result { case .success(let (_, tokenResponse)): if let response = tokenResponse { @@ -482,7 +538,7 @@ extension FeedlyAPICaller: OAuthAcessTokenRefreshRequesting { return } - transport.send(request: request, resultType: AccessTokenResponse.self, keyDecoding: .convertFromSnakeCase) { result in + send(request: request, resultType: AccessTokenResponse.self, keyDecoding: .convertFromSnakeCase) { result in switch result { case .success(let (_, tokenResponse)): if let response = tokenResponse { @@ -523,7 +579,7 @@ extension FeedlyAPICaller: FeedlyGetCollectionsService { request.addValue("application/json", forHTTPHeaderField: "Accept-Type") request.addValue("OAuth \(accessToken)", forHTTPHeaderField: HTTPRequestHeader.authorization) - transport.send(request: request, resultType: [FeedlyCollection].self, dateDecoding: .millisecondsSince1970, keyDecoding: .convertFromSnakeCase) { result in + send(request: request, resultType: [FeedlyCollection].self, dateDecoding: .millisecondsSince1970, keyDecoding: .convertFromSnakeCase) { result in switch result { case .success(let (_, collections)): if let response = collections { @@ -591,7 +647,7 @@ extension FeedlyAPICaller: FeedlyGetStreamContentsService { request.addValue("application/json", forHTTPHeaderField: "Accept-Type") request.addValue("OAuth \(accessToken)", forHTTPHeaderField: HTTPRequestHeader.authorization) - transport.send(request: request, resultType: FeedlyStream.self, dateDecoding: .millisecondsSince1970, keyDecoding: .convertFromSnakeCase) { result in + send(request: request, resultType: FeedlyStream.self, dateDecoding: .millisecondsSince1970, keyDecoding: .convertFromSnakeCase) { result in switch result { case .success(let (_, collections)): if let response = collections { @@ -659,7 +715,7 @@ extension FeedlyAPICaller: FeedlyGetStreamIdsService { request.addValue("application/json", forHTTPHeaderField: "Accept-Type") request.addValue("OAuth \(accessToken)", forHTTPHeaderField: HTTPRequestHeader.authorization) - transport.send(request: request, resultType: FeedlyStreamIds.self, dateDecoding: .millisecondsSince1970, keyDecoding: .convertFromSnakeCase) { result in + send(request: request, resultType: FeedlyStreamIds.self, dateDecoding: .millisecondsSince1970, keyDecoding: .convertFromSnakeCase) { result in switch result { case .success(let (_, collections)): if let response = collections { @@ -714,7 +770,7 @@ extension FeedlyAPICaller: FeedlyGetEntriesService { request.addValue("application/json", forHTTPHeaderField: "Accept-Type") request.addValue("OAuth \(accessToken)", forHTTPHeaderField: HTTPRequestHeader.authorization) - transport.send(request: request, resultType: [FeedlyEntry].self, dateDecoding: .millisecondsSince1970, keyDecoding: .convertFromSnakeCase) { result in + send(request: request, resultType: [FeedlyEntry].self, dateDecoding: .millisecondsSince1970, keyDecoding: .convertFromSnakeCase) { result in switch result { case .success(let (_, entries)): if let response = entries { @@ -773,7 +829,7 @@ extension FeedlyAPICaller: FeedlyMarkArticlesService { } } - transport.send(request: request, resultType: String.self, dateDecoding: .millisecondsSince1970, keyDecoding: .convertFromSnakeCase) { result in + send(request: request, resultType: String.self, dateDecoding: .millisecondsSince1970, keyDecoding: .convertFromSnakeCase) { result in switch result { case .success(let (httpResponse, _)): if httpResponse.statusCode == 200 { @@ -817,7 +873,7 @@ extension FeedlyAPICaller: FeedlySearchService { request.addValue("application/json", forHTTPHeaderField: HTTPRequestHeader.contentType) request.addValue("application/json", forHTTPHeaderField: "Accept-Type") - transport.send(request: request, resultType: FeedlyFeedsSearchResponse.self, dateDecoding: .millisecondsSince1970, keyDecoding: .convertFromSnakeCase) { result in + send(request: request, resultType: FeedlyFeedsSearchResponse.self, dateDecoding: .millisecondsSince1970, keyDecoding: .convertFromSnakeCase) { result in switch result { case .success(let (_, searchResponse)): if let response = searchResponse { @@ -859,7 +915,7 @@ extension FeedlyAPICaller: FeedlyLogoutService { request.addValue("application/json", forHTTPHeaderField: "Accept-Type") request.addValue("OAuth \(accessToken)", forHTTPHeaderField: HTTPRequestHeader.authorization) - transport.send(request: request, resultType: String.self, dateDecoding: .millisecondsSince1970, keyDecoding: .convertFromSnakeCase) { result in + send(request: request, resultType: String.self, dateDecoding: .millisecondsSince1970, keyDecoding: .convertFromSnakeCase) { result in switch result { case .success(let (httpResponse, _)): if httpResponse.statusCode == 200 { diff --git a/Frameworks/Account/Feedly/FeedlyAccountDelegate.swift b/Frameworks/Account/Feedly/FeedlyAccountDelegate.swift index 184ea576b..1f5c341ff 100644 --- a/Frameworks/Account/Feedly/FeedlyAccountDelegate.swift +++ b/Frameworks/Account/Feedly/FeedlyAccountDelegate.swift @@ -37,12 +37,14 @@ final class FeedlyAccountDelegate: AccountDelegate { var credentials: Credentials? { didSet { + #if DEBUG // https://developer.feedly.com/v3/developer/ if let devToken = ProcessInfo.processInfo.environment["FEEDLY_DEV_ACCESS_TOKEN"], !devToken.isEmpty { caller.credentials = Credentials(type: .oauthAccessToken, username: "Developer", secret: devToken) - } else { - caller.credentials = credentials + return } + #endif + caller.credentials = credentials } } @@ -52,6 +54,10 @@ final class FeedlyAccountDelegate: AccountDelegate { var refreshProgress = DownloadProgress(numberOfTasks: 0) + /// Set on `accountDidInitialize` for the purposes of refreshing OAuth tokens when they expire. + /// See the implementation for `FeedlyAPICallerDelegate`. + private weak var initializedAccount: Account? + internal let caller: FeedlyAPICaller private let log = OSLog(subsystem: Bundle.main.bundleIdentifier!, category: "Feedly") @@ -91,6 +97,8 @@ final class FeedlyAccountDelegate: AccountDelegate { let databaseFilePath = (dataFolder as NSString).appendingPathComponent("Sync.sqlite3") self.database = SyncDatabase(databaseFilePath: databaseFilePath) self.oauthAuthorizationClient = api.oauthAuthorizationClient + + self.caller.delegate = self } // MARK: Account API @@ -112,17 +120,10 @@ final class FeedlyAccountDelegate: AccountDelegate { let log = self.log - let refreshAccessToken = FeedlyRefreshAccessTokenOperation(account: account, service: self, oauthClient: oauthAuthorizationClient, refreshDate: Date(), log: log) - refreshAccessToken.downloadProgress = refreshProgress - operationQueue.add(refreshAccessToken) - let syncAllOperation = FeedlySyncAllOperation(account: account, feedlyUserId: credentials.username, caller: caller, database: database, lastSuccessfulFetchStartDate: accountMetadata?.lastArticleFetchStartTime, downloadProgress: refreshProgress, log: log) syncAllOperation.downloadProgress = refreshProgress - // Ensure the sync uses the latest credential. - syncAllOperation.addDependency(refreshAccessToken) - let date = Date() syncAllOperation.syncCompletionHandler = { [weak self] result in if case .success = result { @@ -500,6 +501,7 @@ final class FeedlyAccountDelegate: AccountDelegate { } func accountDidInitialize(_ account: Account) { + initializedAccount = account credentials = try? account.retrieveCredentials(type: .oauthAccessToken) } @@ -533,3 +535,37 @@ final class FeedlyAccountDelegate: AccountDelegate { caller.resume() } } + +extension FeedlyAccountDelegate: FeedlyAPICallerDelegate { + + func reauthorizeFeedlyAPICaller(_ caller: FeedlyAPICaller, completionHandler: @escaping (Bool) -> ()) { + guard let account = initializedAccount else { + completionHandler(false) + return + } + + /// 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 + } + } + + let refreshAccessToken = FeedlyRefreshAccessTokenOperation(account: account, service: self, oauthClient: oauthAuthorizationClient, refreshDate: Date(), 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) + } + + MainThreadOperationQueue.shared.add(refreshAccessToken) + } +} From 133544b748649a1da8fd73fd6acd8d1751d859de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kiel=20Gillard=20=F0=9F=A4=AA?= Date: Wed, 20 May 2020 11:18:09 +1000 Subject: [PATCH 6/6] Revert "Add lastCredentialRenewTime and honour it in FeedlyRefreshAccessTokenOperation" This reverts commit 8973adbdbdc9cb85cf9c3f1b0a1e3a6a3816f899. --- Frameworks/Account/AccountMetadata.swift | 11 ---------- .../Feedly/FeedlyAccountDelegate.swift | 2 +- .../FeedlyRefreshAccessTokenOperation.swift | 22 +------------------ 3 files changed, 2 insertions(+), 33 deletions(-) diff --git a/Frameworks/Account/AccountMetadata.swift b/Frameworks/Account/AccountMetadata.swift index 9f5845a64..eaeb71215 100644 --- a/Frameworks/Account/AccountMetadata.swift +++ b/Frameworks/Account/AccountMetadata.swift @@ -23,7 +23,6 @@ final class AccountMetadata: Codable { case lastArticleFetchStartTime = "lastArticleFetch" case lastArticleFetchEndTime case endpointURL - case lastCredentialRenewTime = "lastCredentialRenewTime" case performedApril2020RetentionPolicyChange } @@ -82,16 +81,6 @@ final class AccountMetadata: Codable { } } } - - /// The last moment an account successfully renewed its credentials, or `nil` if no such moment exists. - /// An account delegate can use this value to decide when to next ask the service provider to renew credentials. - var lastCredentialRenewTime: Date? { - didSet { - if lastCredentialRenewTime != oldValue { - valueDidChange(.lastCredentialRenewTime) - } - } - } var performedApril2020RetentionPolicyChange: Bool? { didSet { diff --git a/Frameworks/Account/Feedly/FeedlyAccountDelegate.swift b/Frameworks/Account/Feedly/FeedlyAccountDelegate.swift index 1f5c341ff..cccc52836 100644 --- a/Frameworks/Account/Feedly/FeedlyAccountDelegate.swift +++ b/Frameworks/Account/Feedly/FeedlyAccountDelegate.swift @@ -554,7 +554,7 @@ extension FeedlyAccountDelegate: FeedlyAPICallerDelegate { } } - let refreshAccessToken = FeedlyRefreshAccessTokenOperation(account: account, service: self, oauthClient: oauthAuthorizationClient, refreshDate: Date(), log: log) + 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`. diff --git a/Frameworks/Account/Feedly/Operations/FeedlyRefreshAccessTokenOperation.swift b/Frameworks/Account/Feedly/Operations/FeedlyRefreshAccessTokenOperation.swift index b4878dc49..d1c68d970 100644 --- a/Frameworks/Account/Feedly/Operations/FeedlyRefreshAccessTokenOperation.swift +++ b/Frameworks/Account/Feedly/Operations/FeedlyRefreshAccessTokenOperation.swift @@ -17,32 +17,14 @@ final class FeedlyRefreshAccessTokenOperation: FeedlyOperation { let account: Account let log: OSLog - /// The moment the refresh is being requested. The token will refresh only if the account's `lastCredentialRenewTime` is not on the same day as this moment. When nil, the operation will always refresh the token. - let refreshDate: Date? - - init(account: Account, service: OAuthAccessTokenRefreshing, oauthClient: OAuthAuthorizationClient, refreshDate: Date?, log: OSLog) { + init(account: Account, service: OAuthAccessTokenRefreshing, oauthClient: OAuthAuthorizationClient, log: OSLog) { self.oauthClient = oauthClient self.service = service self.account = account - self.refreshDate = refreshDate self.log = log } override func run() { - // Only refresh the token if these dates are not on the same day. - let shouldRefresh: Bool = { - guard let date = refreshDate, let lastRenewDate = account.metadata.lastCredentialRenewTime else { - return true - } - return !Calendar.current.isDate(lastRenewDate, equalTo: date, toGranularity: .day) - }() - - guard shouldRefresh else { - os_log(.debug, log: log, "Skipping access token renewal.") - didFinish() - return - } - let refreshToken: Credentials do { @@ -82,8 +64,6 @@ final class FeedlyRefreshAccessTokenOperation: FeedlyOperation { // Now store the access token because we want the account delegate to use it. try account.storeCredentials(grant.accessToken) - account.metadata.lastCredentialRenewTime = Date() - didFinish() } catch { didFinish(with: error)