Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 50 additions & 7 deletions Sources/GravatarUI/SwiftUI/OAuthSession/OAuthSession.swift
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import AuthenticationServices

public struct OAuthSession: Sendable {
private static let shared = OAuthSession()
static let shared = OAuthSession()
private var emailStorage = SessionEmailStorage()

private let storage: SecureStorage
private let authenticationSession: AuthenticationSession
Expand Down Expand Up @@ -54,25 +55,47 @@ public struct OAuthSession: Sendable {
try? storage.secret(with: email.rawValue)
}

@discardableResult
func retrieveAccessToken(with email: Email) async throws -> KeychainToken {
func retrieveAccessToken(with email: Email) async throws {
guard let secrets = await Configuration.shared.oauthSecrets else {
assertionFailure("Trying to retrieve access token without configuring oauth secrets.")
throw OAuthError.notConfigured
}

await emailStorage.save(email)
do {
let url = try oauthURL(with: email, secrets: secrets)
let callbackURL = try await authenticationSession.authenticate(using: url, callbackURLScheme: secrets.callbackScheme)
let tokenText = try tokenResponse(from: callbackURL).token
_ = await Self.handleCallback(callbackURL)
} catch {
throw OAuthError.from(error: error)
}
}

public static func handleCallback(_ callbackURL: URL) async -> Bool {
guard let email = await shared.emailStorage.restore() else { return false }

do {
let tokenText = try shared.tokenResponse(from: callbackURL).token
guard try await CheckTokenAuthorizationService().isToken(tokenText, authorizedFor: email) else {
throw OAuthError.loggedInWithWrongEmail(email: email.rawValue)
}
let newToken = KeychainToken(token: tokenText)
overrideToken(newToken, for: email)
return newToken
shared.overrideToken(newToken, for: email)
await shared.authenticationSession.cancel()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Just wondering why the manual cancel() is necessary? Doesn't the oauth session end itself once it's successful?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the callback URL comes from universal links, the oauth session never returns, so it's never a "success", and the OAuth web view won't dismiss.

As every real scenario will use https and universal link, it will always need this manual cancelling, so I think it safer to call it always, though is not necessary for our particular case of the demo app with the custom scheme exclusion.

Testing the demo app, it didn't seem to be a problem, so I decided to keep the logic simple.

Copy link
Contributor Author

@etoledom etoledom Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without the canceling, the web view stays like this without dismissing:

So the canceling is basically the simplest way I found to dismiss it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, interesting. Thanks!

postNotification(.authorizationFinished)
return true
} catch OAuthError.couldNotParseAccessCode {
return false // The URL was not a Gravatar callback URL with a token.
} catch {
throw OAuthError.from(error: error)
await shared.authenticationSession.cancel()
postNotification(.authorizationError, error: error)
return true
}
}

private static func postNotification(_ name: Notification.Name, error: Error? = nil) {
Task { @MainActor in
NotificationCenter.default.post(name: name, object: error)
}
}

Expand Down Expand Up @@ -216,6 +239,26 @@ extension [URLQueryItem] {

protocol AuthenticationSession: Sendable {
func authenticate(using url: URL, callbackURLScheme: String) async throws -> URL
func cancel() async
}

extension OldAuthenticationSession: AuthenticationSession {}

// Stores the email used for the current OAuth flow
private actor SessionEmailStorage {
var current: Email?

func save(_ email: Email) {
current = email
}

func restore() -> Email? {
let currentEmail = current
return currentEmail
}
}

extension Notification.Name {
static let authorizationFinished = Notification.Name("com.GravatarSDK.AuthorizationFinished")
static let authorizationError = Notification.Name("com.GravatarSDK.AuthorizationFinishedWithError")
}
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import SwiftUI

private struct OAuthSessionKey: EnvironmentKey {
static let defaultValue: OAuthSession = .init()
static let defaultValue: OAuthSession = .shared
}

extension EnvironmentValues {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,13 @@ final class WebAuthenticationPresentationContextProvider: NSObject, ASWebAuthent
}
}

struct OldAuthenticationSession: Sendable {
actor OldAuthenticationSession: Sendable {
let context = WebAuthenticationPresentationContextProvider()
var session: ASWebAuthenticationSession?

func authenticate(using url: URL, callbackURLScheme: String) async throws -> URL {
try await withCheckedThrowingContinuation { continuation in
let session = ASWebAuthenticationSession(url: url, callbackURLScheme: callbackURLScheme) { callbackURL, error in
session = ASWebAuthenticationSession(url: url, callbackURLScheme: callbackURLScheme) { callbackURL, error in
if let error {
continuation.resume(throwing: error)
} else if let callbackURL {
Expand All @@ -20,9 +21,16 @@ struct OldAuthenticationSession: Sendable {
}

Task { @MainActor in
session.presentationContextProvider = context
session.start()
await session?.presentationContextProvider = context
await session?.start()
}
}
}

nonisolated
func cancel() {
Task { @MainActor in
await session?.cancel()
}
}
}
23 changes: 19 additions & 4 deletions Sources/GravatarUI/SwiftUI/ProfileEditor/QuickEditor.swift
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,9 @@ struct QuickEditor<ImageEditor: ImageEditorView>: View {
self.avatarUpdatedHandler = avatarUpdatedHandler
}

let authorizationFinishedNotification = NotificationCenter.default.publisher(for: .authorizationFinished)
let authorizationErrorNotification = NotificationCenter.default.publisher(for: .authorizationError)

var body: some View {
NavigationView {
if let token {
Expand All @@ -63,6 +66,12 @@ struct QuickEditor<ImageEditor: ImageEditorView>: View {
noticeView()
.accumulateIntrinsicHeight()
}
}.onReceive(authorizationFinishedNotification) { _ in
onAuthenticationFinished()
}.onReceive(authorizationErrorNotification) { notification in
guard let error = notification.object as? OAuthError else { return }
oauthError = error
onAuthenticationFinished()
}
}

Expand Down Expand Up @@ -131,8 +140,7 @@ struct QuickEditor<ImageEditor: ImageEditorView>: View {
isAuthenticating = true
if !oauthSession.hasValidSession(with: email) {
do {
_ = try await oauthSession.retrieveAccessToken(with: email)
oauthError = nil
try await oauthSession.retrieveAccessToken(with: email)
} catch OAuthError.oauthResponseError(_, let code) where code == .canceledLogin {
// ignore the error if the user has cancelled the operation.
} catch let error as OAuthError {
Expand All @@ -141,9 +149,16 @@ struct QuickEditor<ImageEditor: ImageEditorView>: View {
oauthError = nil
}
}
fetchedToken = oauthSession.sessionToken(with: email)?.token
isAuthenticating = false
onAuthenticationFinished()
}
}

func onAuthenticationFinished() {
if let fetchedToken = oauthSession.sessionToken(with: email)?.token {
self.fetchedToken = fetchedToken
oauthError = nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WhenoauthSession.hasValidSession(with: email) returns false (in performAuthentication()), then oauthSession.sessionToken(with: email)?.token will return nil, and vice versa.

❓ Since we're already setting oauthEerrorl in performAuthentication() in cases when oauthSession.hasValidSesions(with: email) is false, does it makes sense to move this oauthError = nil there also when it's true?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the problem here, is that when the https flow is triggered (which will potentially be all the cases on production), the performAuthentication() function won't be canceled (to close the web view), and the flow is finished through the notifications, which will call onAuthenticationFinished(), also in the case of success. And in this case we might need oauthError = nil inside onAuthenticationFinished().

Maybe what you described will handle all cases properly, but I'd be extra careful testing every success and error case (also a success after an error screen), if there's any change here.

}
isAuthenticating = false
}
}

Expand Down
2 changes: 2 additions & 0 deletions Tests/GravatarUITests/OAuthSessionTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ final class OAuthSessionTests: XCTestCase {
}

class AuthenticationSessionMock: AuthenticationSession, @unchecked Sendable {
func cancel() async {}

let responseURL: URL

init(responseURL: URL) {
Expand Down