Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

@W-16171409: [Android] Add QR Code Login Support in MSDK #2594

Conversation

JohnsonEricAtSalesforce
Copy link
Contributor

@JohnsonEricAtSalesforce JohnsonEricAtSalesforce commented Jul 22, 2024

🎸 Ready For Review! 🥁

This adds support for log in via log in QR codes generated with the UI Bridge API to MSDK. This completes a very detailed spike previously completed by @wmathurin and directly inherits from that codebase.

Before reviewing, I highly recommend reading this document and references in detail:
👉🏻 https://salesforce.quip.com/JXmvAwirhR3V

There's actually very little if any logic change from the spike code. What I did try to apply was my novice's eye to the topic so we can name and document the code such that it'll be a bit more approachable for those uninitiated to the UI Bridge API, how that is delivered via the QR code and the many touch points that has in MSDK's log in logic. I did the same in the companion pull request that introduces commented out content to our template guiding a developer through enabling this in a new app.

The companion template update is in forcedotcom/SalesforceMobileSDK-Templates#417

@JohnsonEricAtSalesforce
Copy link
Contributor Author

JohnsonEricAtSalesforce commented Jul 22, 2024

It's worth noting I did intend to include commits from the spike performed by @wmathurin. I rebased his branch on dev before adding my own commits. Plus, a squash will make this very tidy on dev when we're done while preserving his history here.

@mobilesdk-bot
Copy link

mobilesdk-bot commented Jul 22, 2024

1 Error
🚫 Tests have failed, see below for more information.
2 Warnings
⚠️ libs/SalesforceSDK/src/com/salesforce/androidsdk/ui/LoginActivity.kt#L209 - Using setJavaScriptEnabled can introduce XSS vulnerabilities into your application, review carefully
⚠️ libs/SalesforceSDK/src/com/salesforce/androidsdk/ui/OAuthWebviewHelper.kt#L215 - Using setJavaScriptEnabled can introduce XSS vulnerabilities into your application, review carefully

Tests:

Name Classname Time
testRestClientUnauthenticatedlientInfoAsync com.salesforce.androidsdk.rest.RestClientTest 10.236

Tests results for SalesforceSDK

Generated by 🚫 Danger

@JohnsonEricAtSalesforce JohnsonEricAtSalesforce force-pushed the feature/w-16171409_android-add-qr-code-login-support-in-msdk branch from e0c01af to f9ea0e5 Compare July 22, 2024 22:19
@JohnsonEricAtSalesforce
Copy link
Contributor Author

Hold merging this to dev and the 12.1.0 codebase until we confirm the iOS version will also make it in time for the release.

Copy link
Contributor

@brandonpage brandonpage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where the QR Code Login work should live isn't a blocker, but the User Agent Flow issue I mentioned is.

…ing login using front door URL and code verifier to override web server Vs. user agent flow)
@codecov-commenter
Copy link

codecov-commenter commented Sep 29, 2024

Codecov Report

Attention: Patch coverage is 16.98113% with 44 lines in your changes missing coverage. Please review.

Project coverage is 57.10%. Comparing base (301e0f4) to head (6f0974e).
Report is 72 commits behind head on dev.

Files with missing lines Patch % Lines
.../src/com/salesforce/androidsdk/ui/LoginActivity.kt 20.51% 27 Missing and 4 partials ⚠️
...com/salesforce/androidsdk/ui/OAuthWebviewHelper.kt 0.00% 13 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##                dev    #2594      +/-   ##
============================================
+ Coverage     51.41%   57.10%   +5.69%     
- Complexity     1789     2424     +635     
============================================
  Files           146      187      +41     
  Lines         12097    15204    +3107     
  Branches       1712     2135     +423     
============================================
+ Hits           6220     8683    +2463     
- Misses         5178     5615     +437     
- Partials        699      906     +207     
Flag Coverage Δ
MobileSync 81.68% <ø> (?)
SalesforceHybrid 55.56% <ø> (ø)
SalesforceReact 52.36% <ø> (ø)
SalesforceSDK 42.24% <16.98%> (-0.81%) ⬇️
SmartStore 78.30% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
.../salesforce/androidsdk/app/SalesforceSDKManager.kt 53.35% <100.00%> (+0.08%) ⬆️
...com/salesforce/androidsdk/ui/OAuthWebviewHelper.kt 10.28% <0.00%> (-0.15%) ⬇️
.../src/com/salesforce/androidsdk/ui/LoginActivity.kt 24.53% <20.51%> (-1.43%) ⬇️

... and 49 files with indirect coverage changes

…anion Member Visibility In `LoginActivity` )
…anion Member Visibility And Improve Naming In `OAuthWebViewHelper` )
@mobilesdk-bot
Copy link

1 Warning
⚠️ No Lint Results.

Tests results for SalesforceHybrid

Generated by 🚫 Danger

@mobilesdk-bot
Copy link

1 Error
🚫 Tests have failed, see below for more information.

Tests:

Name Classname Time
test[testCleanResyncGhosts] com.salesforce.androidsdk.reactnative.ReactMobileSyncTest 131.635

Tests results for SalesforceReact

Generated by 🚫 Danger

Copy link
Contributor

@brandonpage brandonpage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Though I think I would still like to see us extracting the login server from the QR Code URL like Wolf mentioned here.

… Door Bridge Parameters On Authorization Failure)
@mobilesdk-bot
Copy link

1 Warning
⚠️ No Lint Results.

Tests results for MobileSync

Generated by 🚫 Danger

@JohnsonEricAtSalesforce JohnsonEricAtSalesforce merged commit 1811fe5 into forcedotcom:dev Oct 2, 2024
4 of 9 checks passed
@JohnsonEricAtSalesforce JohnsonEricAtSalesforce deleted the feature/w-16171409_android-add-qr-code-login-support-in-msdk branch October 2, 2024 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants