Skip to content

Commit

Permalink
Preventing authentication concurrency and queueing concurrent request
Browse files Browse the repository at this point in the history
  • Loading branch information
Thomas Roovers committed Jan 25, 2022
1 parent 6248fc3 commit d5b2bec
Show file tree
Hide file tree
Showing 3 changed files with 38 additions and 14 deletions.
34 changes: 24 additions & 10 deletions Sources/Core/Authentication/AuthenticationProvider.swift
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import CommonCrypto
class AuthenticationProvider {
private weak var client: Client!
private(set) var isAuthenticating = false

required init(client: Client) {
self.client = client
}
Expand Down Expand Up @@ -132,7 +132,7 @@ class AuthenticationProvider {
throw Error.missingClientAuthentication.set(request: request)
}

return sendOAuthRequest(grantType: grantType, parameters: parameters)
return try sendOAuthRequest(grantType: grantType, parameters: parameters)
.flatMap { [weak self] _ -> Single<Request> in
guard let self = self else {
return Single<Request>.never()
Expand All @@ -141,7 +141,15 @@ class AuthenticationProvider {
}
}

func sendOAuthRequest(grantType: OAuthenticationGrantType, parameters: Parameters? = nil) -> Single<Void> {
func sendOAuthRequest(grantType: OAuthenticationGrantType, parameters: Parameters? = nil) throws -> Single<Void> {

if isAuthenticating {
client.logger?.trace("Already authenticating, stopping the concurrent request")
throw Error.concurrentAuthentication
}

isAuthenticating = true

var parameters = parameters ?? [:]
parameters["grant_type"] = grantType.rawValue

Expand All @@ -161,16 +169,14 @@ class AuthenticationProvider {
"access_token": client.config.logging.maskTokens ? .halfMasked : .default,
"refresh_token": client.config.logging.maskTokens ? .halfMasked : .default,
])

}

isAuthenticating = true

return client.request(request).flatMap { [weak self, client] json in
let accessToken = try json.map(to: AccessToken.self)
accessToken.host = client?.authenticationHost ?? ""
accessToken.grantType = grantType
accessToken.store()

client?.logger?.debug("Store access-token: \(optionalDescription(accessToken))")
client?.authorizationGrantTypeSubject.onNext(accessToken.grantType)
self?.isAuthenticating = false
Expand All @@ -189,6 +195,7 @@ class AuthenticationProvider {
// So we can completely remove the access-token, since it is in no way able to revalidate.
client?.logger?.warning("Clearing access-token; invalid refresh-token")
client?.clearAccessToken()

throw Error.refreshTokenInvalidated
}
}
Expand Down Expand Up @@ -225,9 +232,7 @@ class AuthenticationProvider {

authURLString += "&code_challenge=\(codeChallenge)&code_challenge_method=S256"
}

print("authUrl: \(authURLString)")


guard let authURL = URL(string: authURLString) else {
observer(.failure(Error.invalidUrl))
return Disposables.create()
Expand Down Expand Up @@ -255,7 +260,11 @@ class AuthenticationProvider {
parameters["code_verifier"] = codeVerifier
}

return sendOAuthRequest(grantType: .authorizationCode, parameters: parameters)
do {
return try sendOAuthRequest(grantType: .authorizationCode, parameters: parameters)
} catch {
return Single<Void>.error(error)
}
}

private func _createCodeVerifier() -> String {
Expand Down Expand Up @@ -326,7 +335,12 @@ class AuthenticationProvider {

accessToken.invalidate()
return client.request(request)

} else if error == Error.concurrentAuthentication {
// Retry the request, which will probably add it to the queue because there's an authentication request pending
return client.request(request)
}

throw error
}
}
12 changes: 9 additions & 3 deletions Sources/Core/Base/Client.swift
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ open class Client: ReactiveCompatible {
///
/// - Returns: `Promise<JSON>`
open func request(_ request: Request) -> Single<JSON> {

// Strip slashes to form a valid urlString
guard let host = (request.host ?? config.host) else {
return Single<JSON>.error(Error.invalidRequest("Missing 'host'").set(request: request))
Expand Down Expand Up @@ -142,14 +143,14 @@ open class Client: ReactiveCompatible {
return self._request(newRequest)

// 3. If for some reason an error occurs, we check with the auth-provider if we need to retry
}.catch { [weak self, authProvider] error -> Single<JSON> in
}.catch { [weak self, authProvider, logger] error -> Single<JSON> in
self?.queue.removeFirst()
return try authProvider.recover(from: error, request: request)

// 4. If any other requests are queued, fire up the next one
}.flatMap { [queue] json -> Single<JSON> in
// When a request is finished, no matter if its succesful or not
// We try to clear th queue
// We try to clear the queue
if request.requiresOAuthentication {
queue.next()
}
Expand Down Expand Up @@ -298,7 +299,12 @@ open class Client: ReactiveCompatible {
"username": username,
"password": password
]
return authProvider.sendOAuthRequest(grantType: .password, parameters: parameters)

do {
return try authProvider.sendOAuthRequest(grantType: .password, parameters: parameters)
} catch {
return Single<Void>.error(error)
}
}

open func startAuthorizationFlow(scope: [String], redirectUri: String) -> Single<AuthorizationCodeRequest> {
Expand Down
6 changes: 5 additions & 1 deletion Sources/Core/Base/Error.swift
Original file line number Diff line number Diff line change
Expand Up @@ -54,10 +54,14 @@ public class Error: Swift.Error {
public static func unknown(_ json: JSON? = nil) -> Error {
return Error(code: 100, json: json)
}

public static func invalidRequest(_ message: String) -> Error {
return Error(code: 301, message: message)
}

public static var concurrentAuthentication: Error {
return Error(code: 401, message: "Concurrent authentication requests")
}

public static func underlying(_ error: Swift.Error, json: JSON? = nil) -> Error {
let apiError = Error(code: 601)
Expand Down

0 comments on commit d5b2bec

Please sign in to comment.