[🚫DO NOT MERGE 🚫] Cimb 1 FA oauth flow #348
Conversation
guillaumedebavelaere
force-pushed
the
gd/cimb_oauth_1fa/157640549
branch
from
27b00dc
to
1616cec
Compare
guillaumedebavelaere
changed the title
guillaumedebavelaere
changed the title
guillaumedebavelaere
force-pushed
the
gd/cimb_oauth_1fa/157640549
branch
from
e5e32cc
to
1286aed
Compare
ExampleApp/AppDelegate.swift
Outdated
|
|
||
| private func handleExampleVerify1FA(verify1FACallbackUrl: URL, host: String) { | ||
| let VERIFY_1FA = "complete1FA" | ||
| let YAHOO_HOST = "completeYahoo1FA" |
There was a problem hiding this comment.
Why are there Yahoo changes as part of this?
There was a problem hiding this comment.
Yahoo has his own launcher (line 240).
There was a problem hiding this comment.
Right but is CIMB ever going to launch with Yahoo?
There was a problem hiding this comment.
No, I don't think so. I implemented the full feature because the trade service returns now a "complete1FA" closure.
In that case, we could ignore and do nothing here or return an error ? :
ExampleApp/AppDelegate.swift
Outdated
| if let navController = topViewController as? UINavigationController, | ||
| let navTopViewController = navController.topViewController { | ||
| topViewController = navTopViewController | ||
| } |
There was a problem hiding this comment.
Do we do this calculation somewhere else? Maybe we should create a function for it instead of duplicating
| _ onCancelSecurityQuestion: @escaping () -> Void | ||
| onVerifyUrl: @escaping ( | ||
| URL, | ||
| _ complete1FA: @escaping () -> Void |
There was a problem hiding this comment.
Indentation looks incorrect here. Also why was the onSecurityQuestion removed? Was that only used by CIMB?
There was a problem hiding this comment.
Yes security question was added specifically for CIMB
| guard let oAuthUrl = verifyOAuthURLResult.oAuthUrl() else { | ||
| onFailure( | ||
| TradeItErrorResult( | ||
| title: "Received empty OAuth verify popup URL" |
There was a problem hiding this comment.
This message will be displayed to the user. Should make it user friendly.
| onAnswerSecurityQuestion: answerSecurityQuestion, | ||
| onCancelSecurityQuestion: cancelSecurityQuestion | ||
| ) | ||
| onVerifyUrl: { url, complete1FA in |
There was a problem hiding this comment.
The 1FA naming is a little jarring - challenge seems a little clearer. This can totally wait for a follow up when we look to merge though.
There was a problem hiding this comment.
Maybe completeTradeChallenge because if you add just challenge1FA or completeChallenge to the SDK configuration then it's not clear what it's for.
| cancelOrderSecurityHandler() | ||
| } else { | ||
| return self.alertManager.showError( | ||
| TradeItErrorResult.error(withSystemMessage: "Canceling order could not be submitted please try again."), |
There was a problem hiding this comment.
This will show the default error message to the user. systemMessage is logged to console. Re-word to something like: Your cancel order request could not be submitted. Please try again.
| tradeSecurityHandler() | ||
| } else { | ||
| return self.alertManager.showError( | ||
| TradeItErrorResult.tradeError(withSystemMessage: "Trade could not be submitted please try again."), |
| originalViewController = topMostViewController.presentingViewController | ||
| } | ||
|
|
||
| if let tradeItYahooTradePreviewViewController = originalViewController?.childViewControllers.last as? TradeItYahooTradePreviewViewController { |
There was a problem hiding this comment.
Do these 1FA changes need to be made to Yahoo?
| @@ -240,7 +237,20 @@ class TradeItYahooTradePreviewViewController: TradeItYahooViewController, UITabl | |||
| } | |||
|
|
|||
| // MARK: Private | |||
|
|
|||
|
|
|||
| private var tradeSecurityHandler: (() -> Void)? | |||
There was a problem hiding this comment.
Do these 1FA changes need to be made to Yahoo?
There was a problem hiding this comment.
I fully implemented the feature, but we could ignore and do nothing here or return an error:
guillaumedebavelaere
force-pushed
the
gd/cimb_oauth_1fa/157640549
branch
from
1286aed
to
ddcaadc
Compare
guillaumedebavelaere
force-pushed
the
gd/cimb_oauth_1fa/157640549
branch
from
a47926d
to
ee55154
Compare
guillaumedebavelaere
force-pushed
the
gd/cimb_oauth_1fa/157640549
branch
from
ee55154
to
28fe7e7
Compare
No description provided.