Property value from an asynchronous operation

Question

I have an app which has a session mechanism. When a user creates an account in the app initially, the API returns an access token, the expiry date of that access token and a refresh token which can be used to retrieve a new access token when the existing access token expires.

I have written this singleton class to store these session related data.

import Foundation
import KeychainAccess

class SessionManager {
    static let shared = SessionManager()

    /// Save the access token string in the keychain.
    /// Returns the access token string if available.
    var accessToken: String? {
        set {
            let keychain = Keychain(service: Bundle.main.bundleIdentifier!)
            if let token = newValue {
                keychain["AccessToken"] = token
            } else {
                keychain["AccessToken"] = nil
            }
        }
        get {
            let keychain = Keychain(service: Bundle.main.bundleIdentifier!)
            do {
                if
                    let accessToken = try keychain.getString("AccessToken"),
                    let expiryDate = expiryDate {
                    if expiryDate < Date() {
                        if let refreshToken = refreshToken {
                            return ApiClient.shared.refreshAccessToken(refreshToken: refreshToken)?.accessToken
                        }
                        return nil
                    } else {
                        return accessToken
                    }
                } else {
                    return nil
                }
            } catch {
                return nil
            }
        }
    }

    /// Save the refresh token in the keychain.
    /// Returns the refresh token string if available
    var refreshToken: String? {
        set {
            let keychain = Keychain(service: Bundle.main.bundleIdentifier!)
            if let token = newValue {
                keychain["RefreshToken"] = token
            } else {
                keychain["RefreshToken"] = nil
            }
        }
        get {
            let keychain = Keychain(service: Bundle.main.bundleIdentifier!)
            do {
                if let refreshToken = try keychain.getString("RefreshToken") {
                    return refreshToken
                } else {
                    return nil
                }
            } catch {
                return nil
            }
        }
    }

    /// Save the expiry date of access token in user defaults.
    /// Returns the expiry date of access token if available
    var expiryDate: Date? {
        set {
            if let date = newValue {
                UserDefaults.standard.set(date, forKey: "ExpiryDate")
            } else {
                UserDefaults.standard.removeObject(forKey: "ExpiryDate")
            }
        }
        get {
            if let expiryDate = UserDefaults.standard.object(forKey: "ExpiryDate") as? Date {
                return expiryDate
            } else {
                return nil
            }
        }
    }

}

The class has three computed properties; accessToken, refreshToken and expiryDate. I want to direct your attention to the accessToken property.

What I do here is whenever I retrieve this value in the app, I check if the access token is expired and if it is, I fire an API call with the refresh token to fetch a new access token. Now this is where things get a little hairy. I want to make the network call asynchronous. But since accessToken is a property, I cannot. So what I have done is, I'm making that API call synchronously. I'm using Alamofire in my project and I found this project called Alamofire-Synchronous that suits my need here. This is that method where the synchronous network call happens.

func refreshAccessToken(refreshToken: String) -> (accessToken: String, expiryDate: Date)? {
    let urlString = baseUrl + Endpoint.RefreshAccessToken

    let headers = [
        "Authorization": "Bearer \(refreshToken)"
    ]

    let response = Alamofire
        .request(urlString, method: .post, parameters: nil, encoding: JSONEncoding.default, headers: headers)
        .validate()
        .responseData()
    if let data = response.data {
        // parse dara and get retrieve accessToken and expiryDate

        // save them
        let sessionManager = SessionManager.shared
        sessionManager.accessToken = accessToken
        sessionManager.expiryDate = expiryDate

        return (accessToken, expiryDate)
    } else {
        if let httpError = response.result.error {
            print("HTTP Error: \(httpError)")
            return nil
        } else {
            return nil
        }
    }
}

This gets the job done. But I feel like having a synchronous network call is not a good idea. At the same time I don't just want to make this like an async function because I want to be able to get the accessToken value as a property anywhere from the app if needed. Is there a better, cleaner way to handle this?

Answer 1

In your accessToken property setter you've got the following statements:

set {
    let keychain = Keychain(service: Bundle.main.bundleIdentifier!)
    if let token = newValue {
        keychain["AccessToken"] = token
    } else {
        keychain["AccessToken"] = nil
    }
}

Since you're not using newValue for anything but to update the keychain, this can be reduced to:

set {
    let keychain = Keychain(service: Bundle.main.bundleIdentifier!)
    keychain["AccessToken"] = newValue
}

Same goes for your expiryDate getter:

get {
    if let expiryDate = UserDefaults.standard.object(forKey: "ExpiryDate") as? Date {
        return expiryDate
    } else {
        return nil
    }
}

Can be reduced to:

get {
    return UserDefaults.standard.object(forKey: "ExpiryDate") as? Date
}

Similarly, you can use try? as a shortcut if you want a variable to be set as nil if an error throws as you try to assign it, then you don't need the do-catch statements.

This is happening in a few places in your method, and in all the cases I see the alternative is both more concise, and more clear so I'd recommend taking that approach.

I also find the accessToken getter a bit hard to parse, and I'm not even 100% sure that it's functioning as expected.

if
    let accessToken = try keychain.getString("AccessToken"),
    let expiryDate = expiryDate {
    if expiryDate < Date() {
        if let refreshToken = refreshToken {
            return ApiClient.shared.refreshAccessToken(refreshToken: refreshToken)?.accessToken
        }
        return nil
    }
    else {
        return accessToken
    }
} else {
    return nil
}

Here's my take on it which breaks out preconditions with a guard statement, which has the same outcome if using if-let but is more expressive in terms of the purpose of the check.

Either way, guard or if-let, nested logic is greatly improved with a few brief comments to help guide readers through what's happening:

guard let accessToken = try? keychain.getString("AccessToken"),
    let expiryDate = expiryDate,
    let refreshToken = refreshToken
    else { return nil }

if expiryDate < Date() {
    // not expired, return refreshed access token
    return ApiClient.shared.refreshAccessToken(refreshToken: refreshToken)?.accessToken
}
else {
    // expired, return nil
    return nil
}

I agree with Gaurav that the synchronous request isn't a good idea (unless all of this is happening on a background thread). Maybe you could switch your approach for managing the accessToken from a computed property to using function that returns a closure:

// original 
var accessToken: String? { ... }

// alternative signatures
func getAccessToken() -> (String?) -> () { ... }
func setAccessToken(String) { ... }

You're also grabbing Keychain(service: Bundle.main.bundleIdentifier!) multiple times, so that might be a good candidate to set that as a property in the class. All of these changes combined your SessionManager class would look something like this:

import Foundation
import KeychainAccess

class SessionManager {
    static let shared = SessionManager()

    var keychain = Keychain(service: Bundle.main.bundleIdentifier!)

    /// Save the access token string in the keychain.
    func setAccessToken(token: String) {
        keychain["AccessToken"] = token
    }

    /// Returns the access token string if available.
    var accessToken: String? {

        guard let accessToken = try? keychain.getString("AccessToken"),
            let expiryDate = expiryDate,
            let refreshToken = refreshToken
            else { return nil }

        if expiryDate < Date() {
            // not expired, return refreshed access token
            return ApiClient.shared.refreshAccessToken(refreshToken: refreshToken)?.accessToken
        }
        else {
            // expired, return nil
            return nil
        }
    }

    /// Save the refresh token in the keychain.
    /// Returns the refresh token string if available
    var refreshToken: String? {
        set {
            keychain["RefreshToken"] = newValue
        }
        get {
            return try? keychain.getString("RefreshToken")
        }
    }

    /// Save the expiry date of access token in user defaults.
    /// Returns the expiry date of access token if available
    var expiryDate: Date? {
        set {
            guard let date = newValue else {
                UserDefaults.standard.removeObject(forKey: "ExpiryDate")
                return
            }

            UserDefaults.standard.set(date, forKey: "ExpiryDate")
        }
        get {
            return  UserDefaults.standard.object(forKey: "ExpiryDate") as? Date
        }
    }
}

Answer 2

First of all, including a network call in property getter is not a good idea. Instead of, I would recommend you to replace this code in accessToken getter.

let expiryDate = expiryDate {
                    if expiryDate < Date() {
                        if let refreshToken = refreshToken {
                            return ApiClient.shared.refreshAccessToken(refreshToken: refreshToken)?.accessToken
                        }
                        return nil
                    }

to

let expiryDate = expiryDate {
                    if expiryDate < Date() {
                        return nil
                    }

Before using accessToken check it for nil, if it is nil, refresh it using refreshToken. Convert ApiClient.shared.refreshAccessToken(refreshToken: refreshToken) to Asynchronous call.