Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Authentication – Refresh, persistence & API #681

Open
wants to merge 45 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
45 commits
Select commit Hold shift + click to select a range
1f0adfa
Reanem OAuthClient to AuthentincationDataManager
mohannad-hassan Dec 26, 2024
af28e6e
Create AppAuthCaller to wrap AppAuth calls
mohannad-hassan Dec 26, 2024
17886fb
Refactor to use the new AppAuthCaller
mohannad-hassan Dec 26, 2024
6a2ee35
Create AppAuthOAuthClientTests in OAuthClient
mohannad-hassan Dec 27, 2024
a3f8cca
Wrap OIDAuthState within AuthenticationState
mohannad-hassan Dec 28, 2024
3f1af6b
Persist the tokens after logging in
mohannad-hassan Dec 28, 2024
a3a700d
Provide a way to restoreState for AuthentincationDataManager
mohannad-hassan Dec 30, 2024
b2614b7
Authenitcate rquests in AuthentincationDataManager
mohannad-hassan Dec 30, 2024
986408a
Rename AuthenticationState to AuthenticationData
mohannad-hassan Dec 30, 2024
f601c03
Make AuthentincationDataManager expose an authentication state
mohannad-hassan Dec 30, 2024
1637fc1
Persist updated state
mohannad-hassan Dec 30, 2024
b9b5598
Refresh authentication data manager on launch startup
mohannad-hassan Dec 30, 2024
008a93e
Rename AppAuthOAuthClient to AuthentincationDataManagerImpl
mohannad-hassan Dec 30, 2024
eaf968a
Organize errors and logs
mohannad-hassan Dec 31, 2024
458d7bb
checking in package resolution for AppAuth-iOS
mohannad-hassan Dec 31, 2024
9b71d61
Linting
mohannad-hassan Dec 31, 2024
b8aebcb
Remove configurations
mohannad-hassan Dec 31, 2024
a54ceed
Fix linting issues in LaunchStartup
mohannad-hassan Jan 1, 2025
9a4fd81
Documentation
mohannad-hassan Jan 1, 2025
c0f4c25
Rename some internal types for brevity
mohannad-hassan Jan 1, 2025
a5fa1a8
Rename OAuthClient package to AuthenticationClient
mohannad-hassan Jan 1, 2025
bc85910
Rename AuthentincationDataManager to AuthenticationClient
mohannad-hassan Jan 4, 2025
2fa50e6
Some cleanup in AuthenticationClientTests
mohannad-hassan Jan 4, 2025
90bd841
Revert changes in Features/AppStructureFeature -- Pending integration…
mohannad-hassan Jan 4, 2025
8049a8b
Fix a compilation issue
mohannad-hassan Jan 4, 2025
3cdc724
Convert AuthenticationData to be a protocol
mohannad-hassan Jan 5, 2025
f07289d
Provide configurations to AuthenticationClient on initialization
mohannad-hassan Jan 5, 2025
8f80afa
Some linting issues
mohannad-hassan Jan 5, 2025
4f7000a
Fix typo in Persistence's name
mohannad-hassan Jan 5, 2025
2731407
Fix compilation issues in AuthenticationClientTests
mohannad-hassan Jan 5, 2025
87d8060
Create OAuthService and AppAuthOAuthService
mohannad-hassan Jan 7, 2025
ef596f9
Refactor AuthentincationClientImpl to use the new structure of OAuth …
mohannad-hassan Jan 8, 2025
09a97f1
Remove OAuthCaller and AuthenticationData
mohannad-hassan Jan 8, 2025
5b96158
Refactor AuthenticationClient to assume configurations always set
mohannad-hassan Jan 8, 2025
a316eb1
Revise errors throwb by AppAuthOAuthService
mohannad-hassan Jan 8, 2025
ba45925
Capture some errors in AuthenticationClient
mohannad-hassan Jan 8, 2025
6bb436f
Convert AuthentincationClientImpl to an actor to avoid possible data …
mohannad-hassan Jan 9, 2025
bc1e8f9
Provide coverage for exceptional scenarios
mohannad-hassan Jan 10, 2025
2e2ebe2
PRovide some documentation to OAuthService
mohannad-hassan Jan 10, 2025
d5a4ecb
Move OAuthService to a Core package
mohannad-hassan Jan 10, 2025
d878504
Handle some exceptional scenarios for login in AuthentincationClientImpl
mohannad-hassan Jan 10, 2025
4cfe501
Linting
mohannad-hassan Jan 10, 2025
644cef1
Update AuthenticationClientConfiguration
mohannad-hassan Jan 10, 2025
7edc942
Update data if data already exists in Persistence
mohannad-hassan Jan 17, 2025
f813565
Linting
mohannad-hassan Jan 17, 2025
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
121 changes: 121 additions & 0 deletions Data/AuthenticationClient/Sources/AuthenticationData.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
//
// AuthenticationData.swift
// QuranEngine
//
// Created by Mohannad Hassan on 27/12/2024.
//

import AppAuth
import Combine
import Foundation
import VLogging

enum AuthenticationStateError: Error {
/// Throws when the refresh token operation fails. Assume that the user is not authenticated anymore.
case failedToRefreshTokens(Error?)

/// Failed to decode the persisted state back.
case decodingError(Error?)
}

/// A wrapper for the authentication state's data.
///
/// The abstraction is mainly for testing purposes. The API has been designed to be in conjunction
/// with the `AppAuth's OIDAuthState` class.
class AuthenticationData: NSObject, Codable {
/// Invokes subscribers when the state changes. Usually happens during refreshing tokens.
var stateChangedPublisher: AnyPublisher<Void, Never> { fatalError() }

var isAuthorized: Bool {
fatalError()
}

/// Returns fresh access token to be used for API requests.
///
/// - throws: `AuthenticationStateError.failedToRefreshTokens` if the
/// refresh operation fails for any reason.
func getFreshTokens() async throws -> String {
fatalError()
}

override init() { }

required init(from decoder: any Decoder) throws {
fatalError()
}
}
mohannad-hassan marked this conversation as resolved.
Show resolved Hide resolved

class AppAuthAuthenticationData: AuthenticationData {
private enum CodingKeys: String, CodingKey {
case state
}

private let stateChangedSubject: PassthroughSubject<Void, Never> = .init()
override var stateChangedPublisher: AnyPublisher<Void, Never> {
stateChangedSubject.eraseToAnyPublisher()
}

private var state: OIDAuthState {
didSet {
stateChangedSubject.send()
}
}

override var isAuthorized: Bool {
state.isAuthorized
}

init(state: OIDAuthState) {
self.state = state
super.init()
state.stateChangeDelegate = self
}

required init(from decoder: any Decoder) throws {
do {
let container = try decoder.container(keyedBy: CodingKeys.self)
let data = try container.decode(Data.self, forKey: .state)
guard let state = try NSKeyedUnarchiver.unarchivedObject(ofClass: OIDAuthState.self, from: data) else {
logger.error("Failed to decode OIDAuthState: Failed to unarchive data")
throw AuthenticationStateError.decodingError(nil)
}
self.state = state
} catch {
logger.error("Failed to decode OIDAuthState: \(error)")
throw AuthenticationStateError.decodingError(error)
}
super.init()
state.stateChangeDelegate = self
}

override func encode(to encoder: any Encoder) throws {
var container: KeyedEncodingContainer<CodingKeys> = encoder.container(keyedBy: CodingKeys.self)
let data = try NSKeyedArchiver.archivedData(withRootObject: state, requiringSecureCoding: true)
try container.encode(data, forKey: .state)
}

override func getFreshTokens() async throws -> String {
return try await withCheckedThrowingContinuation { continuation in
state.performAction { accessToken, clientID, error in
guard error == nil else {
logger.error("Failed to refresh tokens: \(error!)")
continuation.resume(throwing: AuthenticationStateError.failedToRefreshTokens(error))
return
}
guard let accessToken else {
logger.error("Failed to refresh tokens: No access token returned. An unexpected situation.")
continuation.resume(throwing: AuthenticationStateError.failedToRefreshTokens(nil))
return
}
continuation.resume(returning: accessToken)
}
}
}
}

extension AppAuthAuthenticationData: OIDAuthStateChangeDelegate {
func didChange(_ state: OIDAuthState) {
logger.info("OIDAuthState changed - isAuthorized: \(state.isAuthorized)")
stateChangedSubject.send()
}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
//
// OAuthClient.swift
// AuthentincationDataManager.swift
mohannad-hassan marked this conversation as resolved.
Show resolved Hide resolved
// QuranEngine
//
// Created by Mohannad Hassan on 19/12/2024.
Expand All @@ -10,8 +10,23 @@ import UIKit

public enum OAuthClientError: Error {
case oauthClientHasNotBeenSet
case errorFetchingConfiguration(Error?)
case errorAuthenticating(Error?)

/// Thrown when an operation, that needs authentication, is attempted wheile the client
mohannad-hassan marked this conversation as resolved.
Show resolved Hide resolved
/// hasn't been authenticated or if the client's access has been revoked.
case clientIsNotAuthenticated
}

public enum AuthenticationState: Equatable {
/// Authentication is not available. Any action dependent on authentication
/// (such as logging in or invoking user APIs) should not be attempted..
case notAvailable

/// No user is currently authenticated, or access has been revoked or is expired.
/// Logging in is availble and is required for further APIs.
case notAuthenticated

case authenticated
}

public struct OAuthAppConfiguration {
Expand All @@ -32,13 +47,21 @@ public struct OAuthAppConfiguration {

/// Handles the OAuth flow to Quran.com
///
/// Note that the connection relies on dicvoering the configuration from the issuer service.
public protocol OAuthClient {
/// Expected to be configuered with the host app's OAuth configuration before further operations are attempted.
public protocol AuthentincationDataManager {
/// Sets the app configuration to be used for authentication.
func set(appConfiguration: OAuthAppConfiguration)
Copy link
Collaborator

Choose a reason for hiding this comment

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

As discussed in the previous PR, please move this code to the class initializer. This ensures the class cannot exist without a configuration, making it less prone to invalid states and reducing the number of states the class needs to manage.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay then, I moved setting the configurations to the initialization. However, I'm still not comfortable to make it nullable.

As explained in the PR's description: The public state of the client needs to indicate two non-usable states (not-configured and not-authenticate), so I saw that it's better to unify access through a non-optional type for both of them.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The public state of the client needs to indicate two non-usable states (not-configured and not-authenticate)

Do you mind explaining why is that please?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Services using the client need to guard against the state of not being authenticated. They also need to guard against the case of the client not having been configured (if cloned by someone.) These are the two non-usable states of the client.

The client needs to expose the first state, so my first idea is to unify all states of the client (not-configured, non-authenticated & authenticated) through the authenticationState property. I favoured this over handling one state through nil checking and the other through a property.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To be honest, a lot of the vagueness here will be cleared once we get to integrating the new APIs along the normal flow.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In general, a client instance should be configured for a consumer to use it. If the consumer does not configure the client, no client will be available for use. A null client serves as a clear indication that the app is not configured for authentication. This approach aligns with best practices followed by many system APIs, where the API is configured during initialization to ensure it can be used effectively.

There are additional nuances to consider, such as handling invalid configurations. Ideally, verifying the configuration during initialization would be great, but I assume this isn't feasible due to the need for backend API requests. Therefore, late verification of the configuration is acceptable. However, when early validation is possible, we should prioritize it and not defer it.


/// Performs the login flow to Quran.com
///
/// - Parameter viewController: The view controller to be used as base for presenting the login flow.
/// - Returns: Nothing is returned for now. The client may return the profile infromation in the future.
func login(on viewController: UIViewController) async throws

/// Returns `true` if the client is authenticated.
func restoreState() async throws -> Bool
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure what this method does. Can you explain please?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I saw the implementation below and got what it does. I believe we should reconsider whether this needs to be a public API. Similar to how saving the auth token is treated as an implementation detail, retrieving the auth token from a persistence store should also remain an implementation detail, IMO.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Perhaps naming needs to be reconsidered. The purpose of this is to restore and reevaluate the status. The full picture of this API is to be used by sync logic.

But your intuition is on point: this part is still not clear. The main remaining point is synchronization especially on startup, as this API is intended to be the base for that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What I’ve observed in other auth libraries is that they typically provide functionality to check isAuthenticated and perform login, similar to your implementation. In addition to that they often include either an explicit start method or implicitly initiate the auth process during the initialization of the service/client class. So, that would be my recommendation.


func authenticate(request: URLRequest) async throws -> URLRequest

var authenticationState: AuthenticationState { get }
}
120 changes: 120 additions & 0 deletions Data/AuthenticationClient/Sources/AuthentincationDataManagerImpl.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
//
// AuthentincationDataManagerImpl.swift
// QuranEngine
//
// Created by Mohannad Hassan on 23/12/2024.
//

import AppAuth
import Combine
import Foundation
import UIKit
import VLogging

public final class AuthentincationDataManagerImpl: AuthentincationDataManager {
// MARK: Lifecycle

init(caller: OAuthCaller, persistance: Persistance) {
self.caller = caller
self.persistance = persistance
}

// MARK: Public

public var authenticationState: AuthenticationState {
guard appConfiguration != nil else {
return .notAvailable
}
return state?.isAuthorized == true ? .authenticated : .notAuthenticated
}

public func set(appConfiguration: OAuthAppConfiguration) {
self.appConfiguration = appConfiguration
}

public func login(on viewController: UIViewController) async throws {
do {
try persistance.clear()
logger.info("Cleared previous authentication state before login")
} catch {
// If persisting the new state works, this error should be of little concern.
logger.warning("Failed to clear previous authentication state before login: \(error)")
}

guard let configuration = appConfiguration else {
logger.error("login invoked without OAuth client configurations being set")
throw OAuthClientError.oauthClientHasNotBeenSet
}

let state = try await caller.login(using: configuration, on: viewController)
self.state = state
logger.info("login succeeded with state. isAuthorized: \(state.isAuthorized)")
persist(state: state)
}

public func restoreState() async throws -> Bool {
guard appConfiguration != nil else {
logger.error("restoreState invoked without OAuth client configurations being set")
throw OAuthClientError.oauthClientHasNotBeenSet
}
guard let state = try persistance.retrieve() else {
logger.info("No previous authentication state found")
return false
}
// TODO: Called for the side effects!
_ = try await state.getFreshTokens()
self.state = state
logger.info("Restored previous authentication state. isAuthorized: \(state.isAuthorized)")
return state.isAuthorized
}

public func authenticate(request: URLRequest) async throws -> URLRequest {
guard let configuration = appConfiguration else {
logger.error("authenticate invoked without OAuth client configurations being set")
throw OAuthClientError.oauthClientHasNotBeenSet
}
guard authenticationState == .authenticated, let state else {
logger.error("authenticate invoked without client being authenticated")
throw OAuthClientError.clientIsNotAuthenticated
}
let token = try await state.getFreshTokens()
var request = request
request.setValue(token, forHTTPHeaderField: "x-auth-token")
request.setValue(configuration.clientID, forHTTPHeaderField: "x-client-id")
return request
}

// MARK: Private

private let caller: OAuthCaller
private let persistance: Persistance

private var cancellables = Set<AnyCancellable>()

private var appConfiguration: OAuthAppConfiguration?

private var state: AuthenticationData? {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This property is not thread-safe and needs protection. I recommend converting this class into an actor to ensure thread safety.

didSet {
guard let state else { return }
state.stateChangedPublisher.sink { [weak self] _ in
self?.persist(state: state)
}.store(in: &cancellables)
mohannad-hassan marked this conversation as resolved.
Show resolved Hide resolved
}
}

private func persist(state: AuthenticationData) {
do {
try persistance.persist(state: state)
} catch {
// If this happens, the state will not nullified so to keep the current session usable
// for the user. As for now, no workaround is in hand.
logger.error("Failed to persist authentication state. No workaround in hand.: \(error)")
}
}
}

extension AuthentincationDataManagerImpl {
public convenience init() {
self.init(caller: AppAuthCaller(), persistance: KeychainPersistance())
}
}
Loading