-
Notifications
You must be signed in to change notification settings - Fork 120
Improve authentication logic for authenticated web view #15164
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
Conversation
|
|
| let stores = ServiceLocator.stores | ||
| guard let site = stores.sessionManager.defaultSite, | ||
| stores.shouldAuthenticateAdminPage(for: site) else { | ||
| WebviewHelper.launch(url.absoluteString, with: self) | ||
| return | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using authenticated web view controller now for all product preview - the web view now can check to see if authentication is possible. If not, it would just open the original URL.
selanthiraiyan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, Huong!
The code LGTM and tests as expected. 🚀
I am sharing a few observations just for information.
- I think the 8th point in TC3 is not relevant and should be removed.
- With
WPS Hide Logininstalled I had to exit the web view during the login flow and try login again. I think this is expected as your mentioned in test instructions. - With
WPS Hide Logininstalled after logging in to the app, I had to logout of the web view before testing further to avoid false positives. This is expected as you mentioned in the test instructions.
| } else { | ||
| loadContent() | ||
| /// Authentication logic differs depending on the destination URL and the current site. | ||
| /// More information: https://wp.me/pe5sF9-3Si |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// More information: https://wp.me/pe5sF9-3Si | |
| /// More information: pe5sF9-3Si-p2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch! Fixed in e9d83df.
| guard isAuthenticationFailure else { | ||
| return false | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Should we rename isAuthenticationFailure into isAuthenticationInProgress? If I understand this correctly, we are checking whether the current url is related to authentication and return false if the url is related to log. This lets the web view proceed further with the navigation and authentication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe isAuthenticationFailure makes more sense here, as the web view fails to handle the authentication (POST request) and attempts to load the login page (the HTLM page) instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice tests. ❤️
|
Thank you Sharma for the review!
I removed this from the PR description ✅ |
Closes: #15147
Description
This PR implements the improvements suggested in pe5sF9-3QB-p2. Authenticated web view now handles auto authentication in the web view for the following cases:
Also, the web view now catches the authentication failures to navigate the user to the original URL in these cases.
Steps to reproduce
Important
wp-admin/admin.php?page=jetpack#/settings)TC1: WordPress.com authenticated with a wordpress.com supported link
TC2: WordPress.com authenticated with a Jetpack SSO site
TC3: WordPress.com authenticated with a Jetpack site without SSO
TC4: Site Credentials authenticated using web authorization
WPS Hide Loginplugin.TC5: Site Credentials authenticated natively
TC6: Site Credentials authenticated natively, but uses custom login URL now
WPS Hide Loginplugin.Testing information
Screenshots
N/A
RELEASE-NOTES.txtif necessary.Reviewer (or Author, in the case of optional code reviews):
Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement: