Handle https callback url #524
Conversation
|
 |
App Name |  Gravatar UIKit Prototype Build |
| Build Number | 1594 | |
| Version | 1.0 | |
| Bundle ID | com.automattic.gravatar-sdk-demo-uikit.prototype-build | |
| Commit | 6c24b1f | |
| App Center Build | Gravatar SDK Demo - UIKit #308 |
|
 |
App Name |  Gravatar SwiftUI Prototype Build |
| Build Number | 1594 | |
| Version | 1.0 | |
| Bundle ID | com.automattic.gravatar-sdk-demo-swiftui.prototype-build | |
| Commit | 6c24b1f | |
| App Center Build | Gravatar SDK Demo - SwiftUI #307 |
| func onAuthenticationFinished() { | ||
| if let fetchedToken = oauthSession.sessionToken(with: email)?.token { | ||
| self.fetchedToken = fetchedToken | ||
| oauthError = nil |
There was a problem hiding this comment.
WhenoauthSession.hasValidSession(with: email) returns false (in performAuthentication()), then oauthSession.sessionToken(with: email)?.token will return nil, and vice versa.
❓ Since we're already setting oauthEerrorl in performAuthentication() in cases when oauthSession.hasValidSesions(with: email) is false, does it makes sense to move this oauthError = nil there also when it's true?
There was a problem hiding this comment.
I think the problem here, is that when the https flow is triggered (which will potentially be all the cases on production), the performAuthentication() function won't be canceled (to close the web view), and the flow is finished through the notifications, which will call onAuthenticationFinished(), also in the case of success. And in this case we might need oauthError = nil inside onAuthenticationFinished().
Maybe what you described will handle all cases properly, but I'd be extra careful testing every success and error case (also a success after an error screen), if there's any change here.
| NotificationCenter.default.post(name: .authorizationError, object: error) | ||
| await shared.authenticationSession.cancel() |
There was a problem hiding this comment.
❓ Is it ok to send the notification before calling await ....cancel()
Could it be problematic for a recipient to receive that notification before await shared.authenticationSession.cancel() returns?
There was a problem hiding this comment.
Not sure if it could be problematic, but it does seem safer to make this change 👍
| overrideToken(newToken, for: email) | ||
| return newToken | ||
| shared.overrideToken(newToken, for: email) | ||
| await shared.authenticationSession.cancel() |
There was a problem hiding this comment.
Question: Just wondering why the manual cancel() is necessary? Doesn't the oauth session end itself once it's successful?
There was a problem hiding this comment.
When the callback URL comes from universal links, the oauth session never returns, so it's never a "success", and the OAuth web view won't dismiss.
As every real scenario will use https and universal link, it will always need this manual cancelling, so I think it safer to call it always, though is not necessary for our particular case of the demo app with the custom scheme exclusion.
Testing the demo app, it didn't seem to be a problem, so I decided to keep the logic simple.
There was a problem hiding this comment.
Without the canceling, the web view stays like this without dismissing:
So the canceling is basically the simplest way I found to dismiss it.
There was a problem hiding this comment.
I see, interesting. Thanks!
Closes #
Description
This PR is aimed to solve the problem where a oauth callback used has a https scheme.
Since WP.com OAuth only supports
https, custom schemes can not be used.Testing Steps
Pocket Casts PR for testing: Automattic/pocket-casts-ios#2267
Tests on both SDK and PocketCasts:
Login requirederror screen.Token expirederror.wrong emailerror.