2
\$\begingroup\$

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?

\$\endgroup\$
1
\$\begingroup\$

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
        }
    }
}
\$\endgroup\$
0
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I did think about this at first. But I use this accessToken property in many places throughout the app. Checking for nil in each place would result in a lot of code duplication. That's why I didn't do it. \$\endgroup\$ – Isuru Jan 5 '17 at 13:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.