Fix auth flow regressions.#2876
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #2876 +/- ##
============================================
+ Coverage 64.87% 64.92% +0.04%
- Complexity 2979 2982 +3
============================================
Files 223 223
Lines 17507 17508 +1
Branches 2497 2498 +1
============================================
+ Hits 11358 11367 +9
+ Misses 4937 4927 -10
- Partials 1212 1214 +2
🚀 New features to boost your workflow:
|
| // Launch the browser custom tab when applicable. | ||
| if (sdkManager.isBrowserLoginEnabled || singleServerCustomTabActivity) { | ||
| if ((sdkManager.isBrowserLoginEnabled || singleServerCustomTabActivity) | ||
| && !isUsingFrontDoorBridge) { |
There was a problem hiding this comment.
Could we add some comments for our future self?
There was a problem hiding this comment.
@wmathurin Hey, sorry I wrote a comment but forgot to push it. I was going to add:
Launch the browser custom tab when applicable.
Whether the User Agent or Web Server Flow is used for Front Door Bridge is determined
by the server, so because Adv Auth requires PKCE we cannot guarantee it is safe/allowed.
@JohnsonEricAtSalesforce Is the above statement accurate? And if so should we just be checking if frontdoorBridgeCodeVerifier is null?
Should we create a 14.0 WI to address this as we move to Adv Auth first? It could fall under that epic.
There was a problem hiding this comment.
Good question, @brandonpage. The isUsingFrontDoorBridge property is a computed check on frontDoorBridgeUrl.value != null, which is the definitive check for front door bridge URL status. The URL is set in both user agent flow and web server flow front door bridge URL cases, where the code verifier is only for web server flow.
It looks like you have a winner as I see it in the comment preview above.
JohnsonEricAtSalesforce
left a comment
JohnsonEricAtSalesforce
left a comment
There was a problem hiding this comment.
There's one possible change we might need.
| ) | ||
|
|
||
| viewModel.clearCookies() | ||
| viewModel.resetFrontDoorBridgeUrl() |
There was a problem hiding this comment.
@brandonpage , this reset was part of the QR Code Log In implementation. If I recall, it covered the scenario where a QR Code failed login and the user was returned to the login web view to with instructions to log in view the web view. If the front door URL is still present though, it acts as an override in that case.
I took a quick look at my fork's recently updated dev and it seems like that logic still needs this reset. Has anything else changed where we don't need this?
There was a problem hiding this comment.
@JohnsonEricAtSalesforce I called this out in the PR description. It is deleted here because it is also called at line 566. It doesn't need to be called twice, right?
There was a problem hiding this comment.
Gotcha. Thanks for the double clarification. That should be a good cleanup.
3 participants
LoginViewModel'soAuthConfigis set in to the migration config when generating the authorization path so code exchange succeedsisUsingFrontDoorBridgeto ensure CustomTab does not cover QR Code login.resetFrontDoorBridgeUrl.