From 4ae7f065ef5d11d44c84c6eca4619054b56bd0f4 Mon Sep 17 00:00:00 2001 From: Pinar Olguc Date: Tue, 8 Oct 2024 13:58:54 +0300 Subject: [PATCH] Prevent the session expired error state from changing on tap (#466) Co-authored-by: etoledom --- .../AvatarPicker/AvatarPickerView.swift | 95 +++++++++++++++---- .../AvatarPicker/AvatarPickerViewModel.swift | 2 +- .../SwiftUI/OAuthSession/Keychain.swift | 12 +-- .../SwiftUI/OAuthSession/KeychainToken.swift | 25 +++++ .../SwiftUI/OAuthSession/OAuthSession.swift | 31 ++++-- .../SwiftUI/ProfileEditor/QuickEditor.swift | 14 ++- .../GravatarUI/SwiftUI/View+Additions.swift | 6 +- 7 files changed, 142 insertions(+), 43 deletions(-) create mode 100644 Sources/GravatarUI/SwiftUI/OAuthSession/KeychainToken.swift diff --git a/Sources/GravatarUI/SwiftUI/AvatarPicker/AvatarPickerView.swift b/Sources/GravatarUI/SwiftUI/AvatarPicker/AvatarPickerView.swift index 77d28320..c1159e38 100644 --- a/Sources/GravatarUI/SwiftUI/AvatarPicker/AvatarPickerView.swift +++ b/Sources/GravatarUI/SwiftUI/AvatarPicker/AvatarPickerView.swift @@ -10,18 +10,66 @@ struct AvatarPickerView: View { @Environment(\.verticalSizeClass) var verticalSizeClass @Environment(\.horizontalSizeClass) var horizontalSizeClass - @StateObject var model: AvatarPickerViewModel + // Declare "@StateObject"s as private to prevent setting them from a + // memberwise initializer, which can conflict with the storage + // management that SwiftUI provides. + // https://developer.apple.com/documentation/swiftui/stateobject + @StateObject private var model: AvatarPickerViewModel @Binding var isPresented: Bool + @Binding var authToken: String? @State private var safariURL: URL? @State private var uploadError: FailedUploadInfo? @State private var isUploadErrorDialogPresented: Bool = false - var contentLayoutProvider: AvatarPickerContentLayoutProviding = AvatarPickerContentLayoutType.vertical + var contentLayoutProvider: AvatarPickerContentLayoutProviding var customImageEditor: ImageEditorBlock? var tokenErrorHandler: (() -> Void)? var avatarUpdatedHandler: (() -> Void)? + init( + email: Email, + authToken: Binding, + isPresented: Binding, + contentLayoutProvider: AvatarPickerContentLayoutProviding = AvatarPickerContentLayoutType.vertical, + customImageEditor: ImageEditorBlock? = nil as NoCustomEditorBlock?, + tokenErrorHandler: (() -> Void)? = nil, + avatarUpdatedHandler: (() -> Void)? = nil + ) { + self._isPresented = isPresented + self.contentLayoutProvider = contentLayoutProvider + self.customImageEditor = customImageEditor + self.tokenErrorHandler = tokenErrorHandler + self.avatarUpdatedHandler = avatarUpdatedHandler + self._authToken = authToken + self._model = StateObject(wrappedValue: AvatarPickerViewModel(email: email, authToken: authToken.wrappedValue)) + } + + fileprivate init( + avatarImageModels: [AvatarImageModel], + selectedImageID: String? = nil, + profileModel: ProfileSummaryModel? = nil, + isPresented: Binding, + contentLayoutProvider: AvatarPickerContentLayoutProviding = AvatarPickerContentLayoutType.vertical, + customImageEditor: ImageEditorBlock? = nil as NoCustomEditorBlock?, + tokenErrorHandler: (() -> Void)? = nil, + avatarUpdatedHandler: (() -> Void)? = nil + ) { + self._isPresented = isPresented + self.contentLayoutProvider = contentLayoutProvider + self.customImageEditor = customImageEditor + self.tokenErrorHandler = tokenErrorHandler + self.avatarUpdatedHandler = avatarUpdatedHandler + self._authToken = .constant(nil) + self._model = StateObject( + wrappedValue: AvatarPickerViewModel( + avatarImageModels: avatarImageModels, + selectedImageID: selectedImageID, + profileModel: profileModel + ) + ) + } + public var body: some View { ZStack { VStack(spacing: 0) { @@ -87,6 +135,9 @@ struct AvatarPickerView: View { SafariView(url: url) .edgesIgnoringSafeArea(.all) } + .onChange(of: authToken ?? "") { newValue in + model.update(authToken: newValue) + } } private func header() -> some View { @@ -507,30 +558,34 @@ private enum AvatarPicker { } } - let model = AvatarPickerViewModel( - avatarImageModels: [ - .init(id: "0", source: .local(image: UIImage()), state: .loading), - .init(id: "1", source: .remote(url: "https://gravatar.com/userimage/110207384/aa5f129a2ec75162cee9a1f0c472356a.jpeg?size=256")), - .init(id: "2", source: .remote(url: "https://gravatar.com/userimage/110207384/db73834576b01b69dd8da1e29877ca07.jpeg?size=256")), - .init(id: "3", source: .remote(url: "https://gravatar.com/userimage/110207384/3f7095bf2580265d1801d128c6410016.jpeg?size=256")), - .init(id: "4", source: .remote(url: "https://gravatar.com/userimage/110207384/fbbd335e57862e19267679f19b4f9db8.jpeg?size=256")), - .init(id: "5", source: .remote(url: "https://gravatar.com/userimage/110207384/96c6950d6d8ce8dd1177a77fe738101e.jpeg?size=256")), - .init(id: "6", source: .remote(url: "https://gravatar.com/userimage/110207384/4a4f9385b0a6fa5c00342557a098f480.jpeg?size=256")), - .init(id: "7", source: .local(image: UIImage()), state: .error(supportsRetry: true, errorMessage: "Something went wrong.")), - .init(id: "8", source: .local(image: UIImage()), state: .error(supportsRetry: false, errorMessage: "Something went wrong.")), - ], - selectedImageID: "5", - profileModel: PreviewModel() + let avatarImageModels: [AvatarImageModel] = [ + .init(id: "0", source: .local(image: UIImage()), state: .loading), + .init(id: "1", source: .remote(url: "https://gravatar.com/userimage/110207384/aa5f129a2ec75162cee9a1f0c472356a.jpeg?size=256")), + .init(id: "2", source: .remote(url: "https://gravatar.com/userimage/110207384/db73834576b01b69dd8da1e29877ca07.jpeg?size=256")), + .init(id: "3", source: .remote(url: "https://gravatar.com/userimage/110207384/3f7095bf2580265d1801d128c6410016.jpeg?size=256")), + .init(id: "4", source: .remote(url: "https://gravatar.com/userimage/110207384/fbbd335e57862e19267679f19b4f9db8.jpeg?size=256")), + .init(id: "5", source: .remote(url: "https://gravatar.com/userimage/110207384/96c6950d6d8ce8dd1177a77fe738101e.jpeg?size=256")), + .init(id: "6", source: .remote(url: "https://gravatar.com/userimage/110207384/4a4f9385b0a6fa5c00342557a098f480.jpeg?size=256")), + .init(id: "7", source: .local(image: UIImage()), state: .error(supportsRetry: true, errorMessage: "Something went wrong.")), + .init(id: "8", source: .local(image: UIImage()), state: .error(supportsRetry: false, errorMessage: "Something went wrong.")), + ] + let selectedImageID = "5" + let profileModel = PreviewModel() + + return AvatarPickerView( + avatarImageModels: avatarImageModels, + selectedImageID: selectedImageID, + profileModel: profileModel, + isPresented: .constant(true), + contentLayoutProvider: AvatarPickerContentLayoutType.horizontal ) - - return AvatarPickerView(model: model, isPresented: .constant(true), contentLayoutProvider: AvatarPickerContentLayoutType.horizontal) } #Preview("Empty elements") { - AvatarPickerView(model: .init(avatarImageModels: [], profileModel: nil), isPresented: .constant(true)) + AvatarPickerView(avatarImageModels: [], profileModel: nil, isPresented: .constant(true)) } #Preview("Load from network") { /// Enter valid email and auth token. - AvatarPickerView(model: .init(email: .init(""), authToken: ""), isPresented: .constant(true)) + AvatarPickerView(email: .init(""), authToken: .constant(""), isPresented: .constant(true)) } diff --git a/Sources/GravatarUI/SwiftUI/AvatarPicker/AvatarPickerViewModel.swift b/Sources/GravatarUI/SwiftUI/AvatarPicker/AvatarPickerViewModel.swift index e49e6e4b..6f3f9b5f 100644 --- a/Sources/GravatarUI/SwiftUI/AvatarPicker/AvatarPickerViewModel.swift +++ b/Sources/GravatarUI/SwiftUI/AvatarPicker/AvatarPickerViewModel.swift @@ -47,7 +47,7 @@ class AvatarPickerViewModel: ObservableObject { @Published var profileModel: AvatarPickerProfileView.Model? @ObservedObject var toastManager: ToastManager = .init() - init(email: Email, authToken: String) { + init(email: Email, authToken: String?) { self.email = email avatarIdentifier = .email(email) self.authToken = authToken diff --git a/Sources/GravatarUI/SwiftUI/OAuthSession/Keychain.swift b/Sources/GravatarUI/SwiftUI/OAuthSession/Keychain.swift index 2fe1ae30..be91e812 100644 --- a/Sources/GravatarUI/SwiftUI/OAuthSession/Keychain.swift +++ b/Sources/GravatarUI/SwiftUI/OAuthSession/Keychain.swift @@ -1,9 +1,9 @@ import Foundation protocol SecureStorage: Sendable { - func setSecret(_ secret: String, for key: String) throws + func setSecret(_ secret: KeychainToken, for key: String) throws func deleteSecret(with key: String) throws - func secret(with key: String) throws -> String? + func secret(with key: String) throws -> KeychainToken? } struct Keychain: SecureStorage { @@ -13,8 +13,8 @@ struct Keychain: SecureStorage { case unhandledError(status: OSStatus, message: String?) } - func setSecret(_ secret: String, for key: String) throws { - guard let tokenData = secret.data(using: .utf8) else { + func setSecret(_ secret: KeychainToken, for key: String) throws { + guard let tokenData = secret.data else { throw KeychainError.cannotConvertSecretIntoData } @@ -31,7 +31,7 @@ struct Keychain: SecureStorage { } } - func secret(with key: String) throws -> String? { + func secret(with key: String) throws -> KeychainToken? { let query: [String: Any] = [ kSecClass as String: kSecClassGenericPassword, kSecMatchLimit as String: kSecMatchLimitOne, @@ -51,7 +51,7 @@ struct Keychain: SecureStorage { guard let existingItem = item as? [String: Any], let secretData = existingItem[kSecValueData as String] as? Data, - let secret = String(data: secretData, encoding: .utf8) + let secret = KeychainToken(data: secretData) else { throw KeychainError.unexpectedSecretData } diff --git a/Sources/GravatarUI/SwiftUI/OAuthSession/KeychainToken.swift b/Sources/GravatarUI/SwiftUI/OAuthSession/KeychainToken.swift new file mode 100644 index 00000000..c34e511d --- /dev/null +++ b/Sources/GravatarUI/SwiftUI/OAuthSession/KeychainToken.swift @@ -0,0 +1,25 @@ +import Foundation + +struct KeychainToken: Codable { + let token: String + var isExpired: Bool = false + + init?(data: Data) { + let decoder = JSONDecoder() + do { + let decodedToken = try decoder.decode(KeychainToken.self, from: data) + self = decodedToken + } catch { + return nil + } + } + + init(token: String) { + self.token = token + } + + var data: Data? { + let encoder = JSONEncoder() + return try? encoder.encode(self) + } +} diff --git a/Sources/GravatarUI/SwiftUI/OAuthSession/OAuthSession.swift b/Sources/GravatarUI/SwiftUI/OAuthSession/OAuthSession.swift index d7e46ae9..593df22b 100644 --- a/Sources/GravatarUI/SwiftUI/OAuthSession/OAuthSession.swift +++ b/Sources/GravatarUI/SwiftUI/OAuthSession/OAuthSession.swift @@ -23,16 +23,34 @@ public struct OAuthSession: Sendable { (try? storage.secret(with: email.rawValue) ?? nil) != nil } + func hasValidSession(with email: Email) -> Bool { + guard let token = try? storage.secret(with: email.rawValue) else { + return false + } + return !token.isExpired + } + + func markSessionAsExpired(with email: Email) { + guard var token = sessionToken(with: email), !token.isExpired else { return } + token.isExpired = true + overrideToken(token, for: email) + } + + func overrideToken(_ token: KeychainToken, for email: Email) { + deleteSession(with: email) + try? storage.setSecret(token, for: email.rawValue) + } + public func deleteSession(with email: Email) { try? storage.deleteSecret(with: email.rawValue) } - func sessionToken(with email: Email) -> String? { + func sessionToken(with email: Email) -> KeychainToken? { try? storage.secret(with: email.rawValue) } @discardableResult - func retrieveAccessToken(with email: Email) async throws -> String { + func retrieveAccessToken(with email: Email) async throws -> KeychainToken { guard let secrets = await Configuration.shared.oauthSecrets else { assertionFailure("Trying to retrieve access token without configuring oauth secrets.") throw OAuthError.notConfigured @@ -41,12 +59,13 @@ public struct OAuthSession: Sendable { do { let url = try oauthURL(with: email, secrets: secrets) let callbackURL = try await authenticationSession.authenticate(using: url, callbackURLScheme: secrets.callbackScheme) - let token = try tokenResponse(from: callbackURL).token - guard try await CheckTokenAuthorizationService().isToken(token, authorizedFor: email) else { + let tokenText = try tokenResponse(from: callbackURL).token + guard try await CheckTokenAuthorizationService().isToken(tokenText, authorizedFor: email) else { throw OAuthError.loggedInWithWrongEmail(email: email.rawValue) } - try storage.setSecret(token, for: email.rawValue) - return token + let newToken = KeychainToken(token: tokenText) + overrideToken(newToken, for: email) + return newToken } catch { throw OAuthError.from(error: error) } diff --git a/Sources/GravatarUI/SwiftUI/ProfileEditor/QuickEditor.swift b/Sources/GravatarUI/SwiftUI/ProfileEditor/QuickEditor.swift index cf187b8b..9cb41e4c 100644 --- a/Sources/GravatarUI/SwiftUI/ProfileEditor/QuickEditor.swift +++ b/Sources/GravatarUI/SwiftUI/ProfileEditor/QuickEditor.swift @@ -24,13 +24,12 @@ struct QuickEditor: View { fileprivate typealias Constants = QuickEditorConstants @Environment(\.oauthSession) private var oauthSession - @State var hasSession: Bool = false + @State var token: String? @State var scope: QuickEditorScopeType @State var isAuthenticating: Bool = true @State var oauthError: OAuthError? @Binding var isPresented: Bool let email: Email - let token: String? var customImageEditor: ImageEditorBlock? var contentLayoutProvider: AvatarPickerContentLayoutProviding var avatarUpdatedHandler: (() -> Void)? @@ -57,8 +56,6 @@ struct QuickEditor: View { NavigationView { if let token { editorView(with: token) - } else if hasSession, let token = oauthSession.sessionToken(with: email) { - editorView(with: token) } else { noticeView() .accumulateIntrinsicHeight() @@ -71,12 +68,13 @@ struct QuickEditor: View { switch scope { case .avatarPicker: AvatarPickerView( - model: .init(email: email, authToken: token), + email: email, + authToken: $token, isPresented: $isPresented, contentLayoutProvider: contentLayoutProvider, customImageEditor: customImageEditor, tokenErrorHandler: { - oauthSession.deleteSession(with: email) + oauthSession.markSessionAsExpired(with: email) performAuthentication() }, avatarUpdatedHandler: avatarUpdatedHandler @@ -128,7 +126,7 @@ struct QuickEditor: View { func performAuthentication() { Task { isAuthenticating = true - if !oauthSession.hasSession(with: email) { + if !oauthSession.hasValidSession(with: email) { do { _ = try await oauthSession.retrieveAccessToken(with: email) oauthError = nil @@ -140,7 +138,7 @@ struct QuickEditor: View { oauthError = nil } } - hasSession = oauthSession.hasSession(with: email) + token = oauthSession.sessionToken(with: email)?.token isAuthenticating = false } } diff --git a/Sources/GravatarUI/SwiftUI/View+Additions.swift b/Sources/GravatarUI/SwiftUI/View+Additions.swift index 1b8109f2..135cccff 100644 --- a/Sources/GravatarUI/SwiftUI/View+Additions.swift +++ b/Sources/GravatarUI/SwiftUI/View+Additions.swift @@ -20,7 +20,8 @@ extension View { avatarUpdatedHandler: (() -> Void)? = nil ) -> some View { let avatarPickerView = AvatarPickerView( - model: AvatarPickerViewModel(email: Email(email), authToken: authToken), + email: Email(email), + authToken: .constant(authToken), isPresented: isPresented, contentLayoutProvider: AvatarPickerContentLayoutType.vertical, customImageEditor: customImageEditor, @@ -40,7 +41,8 @@ extension View { avatarUpdatedHandler: (() -> Void)? = nil ) -> some View { let avatarPickerView = AvatarPickerView( - model: AvatarPickerViewModel(email: Email(email), authToken: authToken), + email: Email(email), + authToken: .constant(authToken), isPresented: isPresented, contentLayoutProvider: contentLayout, customImageEditor: customImageEditor,