Bubble errors up from token fetch #232
Conversation
This prevents connection errors and timeouts from trying to reauthenticate
|
Thanks, that's probably a good idea! I think it would be neater though if the callback would be: (comments inline) Would that be an ok change? |
Sources/Flows/OAuth2.swift
Outdated
| @@ -106,11 +106,16 @@ open class OAuth2: OAuth2Base { | |||
|
|
|||
| didAuthorizeOrFail = callback | |||
| logger?.debug("OAuth2", msg: "Starting authorization") | |||
| tryToObtainAccessTokenIfNeeded(params: params) { successParams in | |||
| tryToObtainAccessTokenIfNeeded(params: params) { successParams, error in | |||
| if let successParams = successParams { | |||
| self.didAuthorize(withParameters: successParams) | |||
| } | |||
| else { | |||
There was a problem hiding this comment.
Add else if let error = error here (without the case .nsError(_) part – I don't think that's needed).
Sources/Flows/OAuth2.swift
Outdated
| if let successParams = successParams { | ||
| self.didAuthorize(withParameters: successParams) | ||
| } | ||
| else { | ||
| self.logger?.debug("OAuth2", msg: "Error obtaining token \(String(describing: error))") |
There was a problem hiding this comment.
tryToObtainAccessTokenIfNeeded already logs the error, so this here will double-log the same error. I'd remove this one or use it to replace the less verbose one on line 194 (now 199) – could say "error refreshing token".
Appears we could have no error and no success params, so makes sense to check each separately
|
Comments addressed. Hopefully this makes more sense now 😄 |
Sources/Flows/OAuth2.swift
Outdated
| @@ -109,13 +109,11 @@ open class OAuth2: OAuth2Base { | |||
| tryToObtainAccessTokenIfNeeded(params: params) { successParams, error in | |||
| if let successParams = successParams { | |||
| self.didAuthorize(withParameters: successParams) | |||
| } | |||
Sources/Flows/OAuth2.swift
Outdated
| @@ -196,7 +194,7 @@ open class OAuth2: OAuth2Base { | |||
| } | |||
| else { | |||
| if let err = error { | |||
| self.logger?.debug("OAuth2", msg: "\(err)") | |||
| self.logger?.debug("OAuth2", msg: "Error refreshing token: \(err)") | |||
There was a problem hiding this comment.
Ug, sorry, Xcode defaults kicking in. In our internal projects, we've got swift format as a build phase and I get used to not checking formatting :)
|
Perfect, thanks!! |
|
Just downloaded the code... Did a forgetTokens to test my login flow. When I try to login using authorizeEmbedded I just get; "I don't have a refresh token, not trying to refresh" and no embedded login page. Rolledback to the previous version and everything worked again... |
|
Ah crap of course! @dhardiman can you add a check in OAuth2.swift line 197 and 199 to ignore the |
I'm not sure if this is a pull request you'll want, as I'm not sure whether it's entirely correct behaviour, but I've noticed if a refresh token request gets a networking error, timeout, connection failure etc, then the OAuth mechanism tries to request new credentials rather than just notifying the client of an error. In my app that led us to erroneously log the user out rather than just ignore the error and wait for better network conditions.