From 1e99a96dccb264067caf771a66db6f9d0d3028c6 Mon Sep 17 00:00:00 2001 From: Emilie Balland Date: Fri, 14 Sep 2018 13:48:48 +0200 Subject: [PATCH 1/2] client config: make state parameter optional The state parameter is recommended but not mandatory so make this configurable in case the server does not support it. By default this is set to true in order to prevent cross-site request forgery. There is a pending PR in the main repo https://github.com/p2/OAuth2/pull/284 --- Sources/Base/OAuth2Base.swift | 15 +++++++++------ Sources/Base/OAuth2ClientConfig.swift | 7 ++++++- Tests/FlowTests/OAuth2CodeGrantTests.swift | 22 ++++++++++++++++++++++ 3 files changed, 37 insertions(+), 7 deletions(-) diff --git a/Sources/Base/OAuth2Base.swift b/Sources/Base/OAuth2Base.swift index 54308523..b3edc293 100644 --- a/Sources/Base/OAuth2Base.swift +++ b/Sources/Base/OAuth2Base.swift @@ -417,12 +417,15 @@ open class OAuth2Base: OAuth2Securable { This method checks `state`, throws `OAuth2Error.missingState` or `OAuth2Error.invalidState`. Resets state if it matches. */ public final func assureMatchesState(_ params: OAuth2JSON) throws { - guard let state = params["state"] as? String, !state.isEmpty else { - throw OAuth2Error.missingState - } - logger?.trace("OAuth2", msg: "Checking state, got “\(state)”, expecting “\(context.state)”") - if !context.matchesState(state) { - throw OAuth2Error.invalidState + if let state = params["state"] as? String, !state.isEmpty { + logger?.trace("OAuth2", msg: "Checking state, got “\(state)”, expecting “\(context.state)”") + if !context.matchesState(state) { + throw OAuth2Error.invalidState + } + } else { + if !clientConfig.stateParameterOptional { + throw OAuth2Error.missingState + } } context.resetState() } diff --git a/Sources/Base/OAuth2ClientConfig.swift b/Sources/Base/OAuth2ClientConfig.swift index 2a128875..668b0e68 100644 --- a/Sources/Base/OAuth2ClientConfig.swift +++ b/Sources/Base/OAuth2ClientConfig.swift @@ -71,6 +71,9 @@ open class OAuth2ClientConfig { /// Add custom parameters to the authorization request. public var customParameters: [String: String]? = nil + + /// Whether the state parameter is optional + public var stateParameterOptional = false /// Most servers use UTF-8 encoding for Authorization headers, but that's not 100% true: make it configurable (see https://github.com/p2/OAuth2/issues/165). open var authStringEncoding = String.Encoding.utf8 @@ -134,7 +137,9 @@ open class OAuth2ClientConfig { if let params = settings["parameters"] as? OAuth2StringDict { customParameters = params } - + if let stateOptional = settings["state_parameter_optional"] as? Bool { + stateParameterOptional = stateOptional + } // access token options if let assume = settings["token_assume_unexpired"] as? Bool { accessTokenAssumeUnexpired = assume diff --git a/Tests/FlowTests/OAuth2CodeGrantTests.swift b/Tests/FlowTests/OAuth2CodeGrantTests.swift index c656de8f..7fd298ca 100644 --- a/Tests/FlowTests/OAuth2CodeGrantTests.swift +++ b/Tests/FlowTests/OAuth2CodeGrantTests.swift @@ -179,6 +179,28 @@ class OAuth2CodeGrantTests: XCTestCase { XCTAssertTrue(false, "Should not throw, but threw \(error)") } } + + func testRedirectURINoStateParameterAllowed() { + let settings: OAuth2JSON = [ + "client_id": "abc", + "client_secret": "xyz", + "authorize_uri": "https://auth.ful.io", + "token_uri": "https://token.ful.io", + "keychain": false, + "state_parameter_optional": true + ] + let oauth = OAuth2CodeGrant(settings: settings) + oauth.redirect = "oauth2://callback" + oauth.context.redirectURL = oauth.redirect + // parse no state + let redirect = URL(string: "oauth2://callback?code=C0D3")! + do { + _ = try oauth.validateRedirectURL(redirect) + } + catch let error { + XCTAssertTrue(false, "Must not end up here with \(error)") + } + } func testTokenRequest() { let oauth = OAuth2CodeGrant(settings: [ From 2530e8f08c3f3db9ccc1cfd7846a2d5d105daea0 Mon Sep 17 00:00:00 2001 From: Emilie Balland Date: Fri, 14 Sep 2018 14:46:28 +0200 Subject: [PATCH 2/2] never delete refresh token when refreshing Otherwise this may cause any error or loss of connectivity leading to refresh token deletion. There is a pending PR on the main repo https://github.com/p2/OAuth2/pull/281 Related to sensational/popscan-android#977 --- Sources/Flows/OAuth2.swift | 1 - 1 file changed, 1 deletion(-) diff --git a/Sources/Flows/OAuth2.swift b/Sources/Flows/OAuth2.swift index 70702664..025ae988 100644 --- a/Sources/Flows/OAuth2.swift +++ b/Sources/Flows/OAuth2.swift @@ -367,7 +367,6 @@ open class OAuth2: OAuth2Base { let data = try response.responseData() let json = try self.parseRefreshTokenResponseData(data) if response.response.statusCode >= 400 { - self.clientConfig.refreshToken = nil throw OAuth2Error.generic("Failed with status \(response.response.statusCode)") } self.logger?.debug("OAuth2", msg: "Did use refresh token for access token [\(nil != self.clientConfig.accessToken)]")