Detect cancellation of login #26
Conversation
erkenberg
changed the title
|
Thanks for the pull request! 🙇🏼♂️ At a glance, the code looks good. I'd like to do a more thorough review and run it to see some of the edge cases you mentioned, but I probably won't have a chance to do that until the weekend. |
|
Thanks for the quick feedback, let me know when you need any more information, looking forward to your review. Have a nice week! |
src/ios/OAuthPlugin.swift
Outdated
| @@ -37,6 +37,8 @@ class ASWebAuthenticationSessionOAuthSessionProvider : OAuthSessionProvider { | |||
| self.aswas = ASWebAuthenticationSession(url: endpoint, callbackURLScheme: callbackURLScheme, completionHandler: { (callBack:URL?, error:Error?) in | |||
| if let incomingUrl = callBack { | |||
| NotificationCenter.default.post(name: NSNotification.Name.CDVPluginHandleOpenURL, object: incomingUrl) | |||
| } else if let incomingUrl = error { | |||
There was a problem hiding this comment.
Reusing the incomingUrl variable name here doesn't make the most sense, something like else if let err = error would be better (and then use err as the object passed to NotificationCenter.
For background, error is a Error? which means it might be nil.
if let err = error is a Swift construct that evaluates truthy if error is non-nil, and assigns the non-nil value to err (essentially changing from Error? to Error).
There was a problem hiding this comment.
Ah, thanks for the explanation!
I adapted (and retestet) the commit accordingly.
src/ios/OAuthPlugin.swift
Outdated
| @@ -64,6 +66,8 @@ class SFAuthenticationSessionOAuthSessionProvider : OAuthSessionProvider { | |||
| self.sfas = SFAuthenticationSession(url: endpoint, callbackURLScheme: callbackScheme, completionHandler: { (callBack:URL?, error:Error?) in | |||
| if let incomingUrl = callBack { | |||
| NotificationCenter.default.post(name: NSNotification.Name.CDVPluginHandleOpenURL, object: incomingUrl) | |||
| } else if let incomingUrl = error { | |||
There was a problem hiding this comment.
Same here with reusing incomingUrl
src/ios/OAuthPlugin.swift
Outdated
| @@ -210,6 +219,10 @@ class OAuthPlugin : CDVPlugin, SFSafariViewControllerDelegate, ASWebAuthenticati | |||
| } | |||
|
|
|||
|
|
|||
| @objc internal func _handleError_(_ notification : NSNotification) { | |||
There was a problem hiding this comment.
This method doesn't need to end in an underscore, just _handleError(_ notification : NSNotification) is fine.
Yes, Swift has a lot of underscores in weird places 😛
src/android/OAuthPlugin.java
Outdated
| if(!loginSuccessful) { | ||
| this.webView.getEngine().evaluateJavascript("window.dispatchEvent(new MessageEvent('message', { data: 'oauth::{\"error\":\"cancelled\"}' }));", null); | ||
| } else { | ||
| // reset loginSuccessful to make sure it is correctly initialized on the next login attempt | ||
| loginSuccessful=false; |
There was a problem hiding this comment.
Super minor nit, but formatting-wise, I prefer if (statement) with spaces, and spaces around the equals sign in assignments.
There was a problem hiding this comment.
Definitely on your side, fixed it to be consistent with the rest of the code :)
erkenberg
force-pushed
the
feature/detect-oAuth-login-cancellation
branch
from
28922fa
to
06c1d57
Compare
erkenberg
force-pushed
the
feature/detect-oAuth-login-cancellation
branch
from
06c1d57
to
22ce32b
Compare
|
As said, the dialog closing issue ist not really an issue, more a question of what is expected. For Visualization:
If I click cancel in the top left, my code would trigger (as the login overlay is cancelled/closed) and I would send the However, if I click the cancel next to continue (so in the actual login website), the identity provider would handle that and send a redirect URL with an error. Therefore I didn't want to mix those in the plugin and just regularly parse/send this url/message to the actual application. |
|
Hi @dpogue, Are you going to merge this PR? We need that cancelation event as well. |
We had an issue similar to #20, where we needed to detect when the oAuth progress was cancelled by the user (thus the login overlay closes without getting a redirect from the identity providers endpoint, as the user never interacted with it).
Currently the plugins sends no message at all if this happens, therefore we couldn't detect this case.
We solved it by adding a special message that the plugin sends when this happens:
oauth:{"error":"cancelled"}.The only edge case that came up while testing is, that while closing the overlay via the close buttons included in the overlay itself returns the new message (instead of nothing), canceling via the actual dialog will still return the regular response from the identity provider (as before).
While this makes sense implementation wise, it might be a bit confusing when using it.
What do you think?
Tested without issues on Android 12 and iOS 15. Note that while I'm sufficiently familiar with Android, I don't know much about Swift, and the implementation for iOS was more based on testing and following the documentation for ASWebAuthenticationSession than actually understanding what exactly I'm doing (but seems to run just fine).