Skip to content

Issues/15266 fix preview url#15309

Merged
aerych merged 8 commits intodevelopfrom
issues/15266-fix-preview-url
Nov 25, 2020
Merged

Issues/15266 fix preview url#15309
aerych merged 8 commits intodevelopfrom
issues/15266-fix-preview-url

Conversation

@aerych
Copy link
Copy Markdown
Contributor

@aerych aerych commented Nov 16, 2020

Fixes #15266
Refs #14220

This PR changes the way the app handles previews to resolve the issue reported in #15266 and potentially mitigates the issue reported in #15296 but needs confirmation. The changes include:

  • Adding a convenience getter to the Blog model for checking if a site has a mapped domain.
  • Updates the PreviewNonceHandler to only add the preview argument when viewing non published posts, or published post that have remote changes, and to only unmap a mapped URL in these cases.
  • Updates the RequestAuthenticator to explicitly proxy requests through r-login for mapped domains, excepted when loading an unmapped URL. This change was necessary to view published and drafts posts consistently on sites marked "coming soon" that have mapped domains. I suspect this is the change that resolves 15296.

To test:
Perform the following tests

  • View a published post.
  • Make a change to a draft post and preview the change.
  • Load View Site and confirm you are logged in.

Using the following site configurations:

  • a simple wordpress.com site. (private and public)
  • a simple wordpress.com site with a custom domain. (private and public)
  • an atomic site
  • a self-hosted + jetpack site.

Ping me if you need a site to test with. I have examples of them all :)

There's a lot to cover with these checks so maybe a couple of folks could help out. @guarani , @diegoreymendez would y'all be able to take a peek?

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@aerych aerych added this to the 16.2 milestone Nov 16, 2020
@aerych aerych self-assigned this Nov 16, 2020
@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Nov 16, 2020

You can test the changes on this Pull Request by downloading it from AppCenter here with build number: 38302. IPA is available here. If you need access to this, you can ask a maintainer to add you.

@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Nov 16, 2020

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@jkmassel
Copy link
Copy Markdown
Contributor

👋 Howdy! We are freezing 16.2 today, so this PR is being pushed to 16.3. If you need it to be part of the 16.2 release, please merge it to the release/16.2 branch, then DM me – I'll be happy to cut a new beta release!

@jkmassel jkmassel modified the milestones: 16.2, 16.3 Nov 16, 2020
@guarani
Copy link
Copy Markdown
Contributor

guarani commented Nov 16, 2020

Updates the RequestAuthenticator to explicitly proxy requests through r-login for mapped domains, excepted when loading an unmapped URL. This change was necessary to view published and drafts posts consistently on sites marked "coming soon" that have mapped domains. I suspect this is the change that resolves 15296.

I can confirm that this does appear to fix #15296 💯

@guarani
Copy link
Copy Markdown
Contributor

guarani commented Nov 17, 2020

Hi @aerych, thanks a lot for the PR!

Here's the results of my testing. There are some issues that came up and while I haven't been able to dig into the errors individually, I hope this is still useful.

A question first: Should we also be checking the Reader? As far as I know it uses previews which might be affected by this change.


View Post Draft Preview View Site Comments
Simple WP.com site (Public) The web view is logged in, I can Like, but some links for Comments take me to a logged-out page in mobile Safari
Simple WP.com site (Private) Same as above
Simple WP.com site mapped domain (Public) ⚠️ while this worked on iPhone, I got a strange error on iPad and some iPhone tests (see details below)
Simple WP.com site mapped domain (Private) ⚠️ The web view displays "Loading..." in one section and never resolves in View Site
Atomic site (Public) ⚠️ ⚠️ ⚠️ I'm asked to login
Atomic site (Private) ? ? ? Not tested yet
Jetpack-connected self-hosted site (Public) ⚠️ ⚠️ ⚠️ I'm asked to login
Jetpack-connected self-hosted site (Private) ? ? ? Not tested yet

Simple WP.com site mapped domain (Public)

While this worked on iPhone, on an iPad I consistently got an "Invalid key" error:

See screenshot Screenshot showing Invalid Key error on iPad

Atomic site (Public)

See screenshot Screenshot showing login request on Atomic site

Jetpack-connected self-hosted site (Public)

See screenshot Screenshot showing login request on Jetpack-connected self-hosted site

Copy link
Copy Markdown
Contributor

@diegoreymendez diegoreymendez left a comment

Choose a reason for hiding this comment

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

I tested this following the instructions and found no issues.

I added a few comments to the code, which I'd suggest to at least consider. But since none of those changes really affect the functionality of this PR, I'm approving it.

@aerych
Copy link
Copy Markdown
Contributor Author

aerych commented Nov 17, 2020

Atomic site (Public)

For the Atomic site, being prompted to log in would be expected if the WordPress.com login feature was disabled. It's found in wp-admin under Jetpack > Settings > WordPress.com login.
Screen Shot 2020-11-17 at 10 26 30 AM
For newly minted Atomic sites this should be enabled by default. For atomic sites that have disabled the feature the login prompt is a fallback. Sorry. I should have mentioned this detail.

Jetpack-connected self-hosted site (Private)

AFAIK we don't really have private self-hosted sites (with or without Jetpack).

I'm looking into the other issues.

@aerych
Copy link
Copy Markdown
Contributor Author

aerych commented Nov 17, 2020

while this worked on iPhone, I got a strange error on iPad and some iPhone tests ... While this worked on iPhone, on an iPad I consistently got an "Invalid key" error:

Hmm... I can't reproduce from a clean install in the iPad Pro 9.7" iOS 14.2 simulator. If you can reproduce it would be helpful to see the actual value of the URL composed for the mapped wpcom auth. (The r-login URL.)

Jetpack-connected self-hosted site

Dug into this. From a fresh install, when view a post or check the view site link I start logged out. I'm guessing when testing earlier I'd already had an authed session :(. This is, at least, consistent with the experience in Calypso. I can log into my wpcom account, switch to my jetpack connected self-hosted site, view a post and see that I'm also logged out there.
Screen Shot 2020-11-17 at 1 18 00 PM
Happily, once I'm logged in to the in-app browser (choosing the remember me option) I remain logged in, even in subsequent launches of the app.
From what I've gathered, we have limited options in this scenario. From what I can tell r-login doesn't work for self-hosted sites, and we can't really rely on Jetpack having WordPress.com login enabled. I'm fine to match the experience in Calypso for now. Thoughts?

Should we also be checking the Reader?

Good point.
I did a cursory test by viewing some of my private wpcom sites and confirmed I was logged in and could see them. For other sites, options for liking, replying and sharing are more prominent in the reader so even if a user is not logged in these are things they have readily available, and is the experience we'd want to promote. I think it is reasonable to have the same expectations for viewing an in-app browser in the reader as using the view site feature. Maybe this is fine?

@guarani
Copy link
Copy Markdown
Contributor

guarani commented Nov 18, 2020

For the Atomic site, being prompted to log in would be expected if the WordPress.com login feature was disabled. It's found in wp-admin under Jetpack > Settings > WordPress.com login.

Thanks! I check this and WordPress.com login was ON, but I tried again and now it works :D, so I think we can consider this solved.

AFAIK we don't really have private self-hosted sites (with or without Jetpack).

TIL 👍

Hmm... I can't reproduce from a clean install in the iPad Pro 9.7" iOS 14.2 simulator. If you can reproduce it would be helpful to see the actual value of the URL composed for the mapped wpcom auth. (The r-login URL.)

Shared details on a DM.

From a fresh install, when view a post or check the view site link I start logged out. I'm guessing when testing earlier I'd already had an authed session :(. This is, at least, consistent with the experience in Calypso. I can log into my wpcom account, switch to my jetpack connected self-hosted site, view a post and see that I'm also logged out there.

Good point, I tested this by adding associating a new Jurassic Ninja site to my WP.com in a Chrome tab, and then logging into my same WP.com account in a different browser (to ensure no cookies were brought across) and indeed I'm not "logged in" when viewing a published post from that Jurassic Ninja site.

From what I've gathered, we have limited options in this scenario. From what I can tell r-login doesn't work for self-hosted sites, and we can't really rely on Jetpack having WordPress.com login enabled. I'm fine to match the experience in Calypso for now. Thoughts?

I agree, I'm not familiar with r-login but this seems to match the Calypso experience.

I did a cursory test by viewing some of my private wpcom sites and confirmed I was logged in and could see them. For other sites, options for liking, replying and sharing are more prominent in the reader so even if a user is not logged in these are things they have readily available, and is the experience we'd want to promote. I think it is reasonable to have the same expectations for viewing an in-app browser in the reader as using the view site feature. Maybe this is fine?

Yeah, I agree that the Visit-in-web-view feature of Reader should have the same functionality as the View Site option. I'll give it a check too in my next round of tests for this PR.

@aerych
Copy link
Copy Markdown
Contributor Author

aerych commented Nov 18, 2020

Thanks! I check this and WordPress.com login was ON, but I tried again and now it works :D, so I think we can consider this solved. [...] Shared details on a DM

I'm speculating that the different results we're seeing might be due to being proxied when testing. I've been testing unproxied, but noticed when I proxied I could reproduce the issue you saw on the iPad. Assuming our proxy was the issue, and since it shouldn't be a factor for others, maybe this is fine?

@guarani
Copy link
Copy Markdown
Contributor

guarani commented Nov 18, 2020

I'm speculating that the different results we're seeing might be due to being proxied when testing. I've been testing unproxied, but noticed when I proxied I could reproduce the issue you saw on the iPad. Assuming our proxy was the issue, and since it shouldn't be a factor for others, maybe this is fine?

Great idea, that seems to be it. Unproxied and it worked.

@aerych
Copy link
Copy Markdown
Contributor Author

aerych commented Nov 18, 2020

After chatting with @designsimply about it we'd like to target 16.2 for this branch. I'll re-target and confirm there are no issues.

Currently, we're double checking some behavior with a like button that never loads. We'll confirm if it's another regression or existing behavior, then decide to merge or not.

Copy link
Copy Markdown
Contributor

@guarani guarani left a comment

Choose a reason for hiding this comment

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

Testing build 37971 on an iPhone (unproxied), the key takeaways are:

  1. No web views worked in my tests for Jetpack-connected self-hosted sites (i.e. the web views showed the content but were not logged in)
  2. Visiting a private Simple WP.com site with a mapped domain, the Reader showed a login screen
  3. The Reader web view is often not logged in for comments and likes (it is often "fixed" by going to View Site first and then coming back to Reader)
  4. A "Loading..." message is often shown instead of the like button (and fails to resolve)
  5. In all tests of draft previews, the app (incorrectly) lets you submit a comment but then (correctly) rejects it with an error message

Questions:

  1. Are there any downsides to the frame-nonce query param being included in URLs when sharing links from inside the web views?
View Post Draft Preview View Site Reader
Simple WP.com site (Public) The UI incorrectly lets me write a comment (it's a draft, comments shouldn't be allowed) but attempting to submit the comment is correctly blocked
Simple WP.com site (Private) The UI incorrectly lets me write a comment (it's a draft, comments shouldn't be allowed) but attempting to submit the comment is correctly blocked
Simple WP.com site mapped domain (Public) The Like button takes me to mobile Safari where I'm shown a login page The UI incorrectly lets me write a comment (it's a draft, comments shouldn't be allowed) but attempting to submit the comment is correctly blocked - ⚠️ The web view is not logged in
- The Like button is shown but tapping it presents and dismisses a modal popup and the Like action is not completed
Simple WP.com site mapped domain (Private) A "Loading..." message is shown instead of the like button (it fails to load) The UI incorrectly lets me write a comment (it's a draft, comments shouldn't be allowed) but attempting to submit the comment is correctly blocked ❌ The web view is not logged in (going to View Site and coming back doesn't fix the issue)
Atomic site (Public) - The frame-nonce query param is included in URLs when sharing links from inside the web views
- The Like button takes me to mobile Safari where I'm shown a login page
The UI incorrectly lets me write a comment (it's a draft, comments shouldn't be allowed) but attempting to submit the comment is correctly blocked - ⚠️ On a fresh install, navigating straight to the Reader and visiting a public Atomic site, I get asked to log-in when I try to comment (this fixes itself after I go to Visit Site and return to the Reader)
- The Like button is shown but tapping it presents and dismisses a modal popup and the Like action is not completed
Atomic site (Private) - The frame-nonce query param is included in URLs when sharing links from inside the web views
- A "Loading..." message is shown instead of the like button (it fails to load)
The UI incorrectly lets me write a comment (it's a draft, comments shouldn't be allowed) but attempting to submit the comment is correctly blocked A "Loading..." message is shown instead of the like button (it fails to load) - ⚠️ On a fresh install, navigating straight to the Reader and visiting a private Atomic site results in a log-in prompt (this fixes itself after I go to Visit Site and return to the Reader)
- A "Loading..." message is shown instead of the like button (it fails to load)
Jetpack-connected self-hosted site (Public) - ⚠️ The web view is not logged in
- The frame-nonce query param is included in URLs when sharing links from inside the web views
⚠️ The web view is not logged in ⚠️ The web view is not logged in ⚠️ Can't add site to Reader

@aerych
Copy link
Copy Markdown
Contributor Author

aerych commented Nov 19, 2020

You're an amazing tester @guarani :)

Like button opens browser

  • I see the same behavior when testing with 15.9, so at least this isn't a regression. 🤔 I've looked at the issue a little and it's cause (and fix) isn't obvious to me. How about we tackle it in another issue.

Like button shows loading

  • In 15.9, on two different simple sites with mapped domains I was shown the "Protected Blog" screen. Guessing the loading issue for private sites is likely related to the browser opening issue for public sites. Since it's not a regression let's look at this one later also.

Reader logged out

  • The current behavior is the same as 15.9 from what I see. The Reader was not configuring an authenticator for it's browser. So the behavior would depend on if you had already obtained auth cookies or not. We can make a small change to improve this in this patch, but let's leave it for another issue to do a deeper treatment.

Draft Previews:

  • Comment form: It looks like this is the expected behavior and I can reproduce this in a desktop browser. I agree it's not ideal but I'm not sure there's anything we can do here.

Jetpack-connected self-hosted

  • The behavior is the same in 15.9 from what I can see, and consistent with the experience on the web. I'm not sure there is a lot we can do here as we can't rely on Jetpack's WordPress.com login module to be enabled, and we won't have credentials to authenticate with a password.

I've pushed changes for the reader auth (which won't be perfect but should help) and for the frame-nonce. One last peek?

@aerych aerych changed the base branch from develop to release/16.2 November 19, 2020 17:00
@guarani
Copy link
Copy Markdown
Contributor

guarani commented Nov 19, 2020

Like button opens browser

* I see the same behavior when testing with 15.9, so at least this isn't a regression. 🤔 I've looked at the issue a little and it's cause (and fix) isn't obvious to me. How about we tackle it in another issue.

The Like button works for me (tested in View Post on a public Simple WP.com site with a mapped domain) using 15.9.0.2 (the latest 15.9 build on TestFlight), maybe there's something we're doing slightly different.

See screen recording Scree recording showing Like button working in 15.9

It could still be considered a minor issue, let me know if you'd still prefer we tackle this is a separate issue (it looks similar to the existing issue #13431).

Like button shows loading

* In 15.9, on two different simple sites with mapped domains I was shown the "Protected Blog" screen. Guessing the loading issue for private sites is likely related to the browser opening issue for public sites.  Since it's not a regression let's look at this one later also.

Again this works for me on 15.9 (15.9.0.2), so I'm not sure if we're taking the exact same steps. I used a private Simple WP.com site with a mapped domain and was able to see the Like button load.

See screen recording Scree recording showing Like button loading in 15.9

Reader logged out

* The current behavior is the same as 15.9 from what I see.  The Reader was not configuring an authenticator for it's browser. So the behavior would depend on if you had already obtained auth cookies or not. We can make a small change to improve this in this patch, but let's leave it for another issue to do a deeper treatment.

I agree that this is not a regression, I created #15340 to track this.

Draft Previews:

* Comment form: It looks like this is the expected behavior and I can reproduce this in a desktop browser. I agree it's not ideal but I'm not sure there's anything we can do here.

Sounds good to me 👍

Jetpack-connected self-hosted

* The behavior is the same in 15.9 from what I can see, and consistent with the experience on the web. I'm not sure there is a lot we can do here as we can't rely on Jetpack's WordPress.com login module to be enabled, and we won't have credentials to authenticate with a password.

I created #15342 to track this.

I've pushed changes for the reader auth (which won't be perfect but should help) and for the frame-nonce. One last peek?

I'll test that shortly and reply back here.

@aerych
Copy link
Copy Markdown
Contributor Author

aerych commented Nov 19, 2020

15.9.0.2 (the latest 15.9 build on TestFlight), maybe there's something we're doing slightly different.

Ah! I'd checked out the release/15.9 branch and was testing with it in the simulator. There's once difference. :)

@guarani
Copy link
Copy Markdown
Contributor

guarani commented Nov 19, 2020

I've pushed changes for the reader auth (which won't be perfect but should help) and for the frame-nonce. One last peek?

Circling back to this, I can confirm that the frame nonce is no longer showing up on share links from web views 👍.

As we discussed offline, the issues mentioned in the Reader column in #15309 (review) are unchanged.

@aerych aerych requested a review from guarani November 24, 2020 15:51
@aerych
Copy link
Copy Markdown
Contributor Author

aerych commented Nov 24, 2020

After realizing that some of the different behaviors we were seeing while testing was due to whether we tested with the existing 15.9 internal beta, or with a debug build (device or simulator) I shared what we were seeing internally. See pbArwn-1p6-p2.

Props @koke for catching that the internal build of 15.9 was likely built against the iOS 13 sdk and that changes related to App Bound Domains and Intelligent Tracking Prevention starting in iOS 14 could explain what we were seeing as others had reported some similar difficulty with their apps.

To summarize: Our consensus is that ITP is the likely explanation for the behavior differences and we're content to go ahead and merge the changes in this branch. Even without the changes merged in 16.0, the previous behavior in 15.9 would have been impacted by ITP in iOS 14/ Xcode 12. With this in mind we're comfortable saying there's likely not a regression introduced in this patch.

That said, we'll merge this to develop targeting 16.3 for additional time to test and refine if needed.

cc @guarani

Copy link
Copy Markdown
Contributor

@guarani guarani left a comment

Choose a reason for hiding this comment

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

Thanks @aerych! After testing the latest build 38302 on an iPhone 11 (iOS 14), I think we can say that:

  • Simple sites without a mapped domain are working great 💯
  • The biggest issue with Simple sites that use a mapped domain is that if the site is private, they fail to load in the Reader's web view
  • Similarly, the biggest issue with Atomic sites are with private sites also, they too fail to load in the Reader's web view
  • All other issues seem lower priority (and marked are with a ⚠️ below)

I'm approving these changes, although this feels has gotten way more complex than first thought. Do you still think we should merge and then address the other issues separately?


View Post Draft Preview View Site Reader "Read the source post" from pingback notifications
Simple WP.com site (Public)
Simple WP.com site (Private) not applicable (pingbacks don't apply to private sites AFAIK)
Simple WP.com site mapped domain (Public) ⚠️ Unable to like posts ⚠️ Unable to like posts ⚠️ Unable to like or comment on this type of site ⚠️ Unable to like or comment on the source post
Simple WP.com site mapped domain (Private) ⚠️ The like button fails to load ⚠️ The like button fails to load ❌ Unable to access web view not applicable (pingbacks don't apply to private sites AFAIK)
Atomic site (Public) ⚠️ Unable to like or star ⚠️ Unable to like or star ⚠️ Unable to like or comment on this type of site Pingback doesn't work
Atomic site (Private) ⚠️ The like button fails to load ⚠️ The like button fails to load ❌ Unable to access web view not applicable (pingbacks don't apply to private sites AFAIK)
Jetpack-connected self-hosted site (Public) ⚠️ The web view is not logged in ⚠️ The web view is not logged in Reader doesn't work with Jurassic Ninja sites: Automattic/wp-calypso#47289 Pingback doesn't work

Breakdown

View Post and View Site

The different errors I've come across are:

  • Scenario "Unable to like posts": If I try to like a post, mobile Safari is opened to an unauthenticated web view.
  • Scenario "The like button fails to load": On private simple sites the text "Loading..." is shown instead of the Like button. On private Atomic sites both the Like button and the star button show "Loading..."
  • Scenario "Unable to like or star" (only affects public Atomic sites): Tapping the like or star buttons either opens and closes a modal web view OR opens mobile Safari
  • Scenario "The web view is not logged in": The main problem with this is that every time the user views their own site "View Site" or post "View Post", their viewing stats for that page or site get incremented

Reader

  • In the "Unable to like or comment on this type of site" scenario above, an unauthenticated web view is shown. There are no known workarounds for this.
  • In the "❌ Unable to access web view" scenario above, a "Protected Blog" screen is shown for private Simple sites and a "This site is private" screen is shown for private Atomic sites.
  • There appears to be an bug that prevents the Reader from following Jurassic Ninja sites (it might be a backend issue so I've linked to a Calypso issue there)

Pingback

  • After receiving a pingback notification, users might often wish to "like" or comment on the source post from within the web view — however, in the scenarios marked with a ⚠️ above, tapping on the "Read the source post" button shows an unauthenticated web view:

    See screen recording Screen recording showing how the pingback 'Read the source post' web view is not working

    Workaround: Go to the Reader, follow the source site (if not already following it), locate the source post and like or comment using the app's native like and comment functionality (i.e. not using the web view).

  • Pingbacks don't appear to work on Atomic and Jetpack sites, I'm not sure yet why (#15387).

@aerych
Copy link
Copy Markdown
Contributor Author

aerych commented Nov 25, 2020

Do you still think we should merge and then address the other issues separately?

I do. Even with the shortcomings remaining with likes, etc. identified above, this patch is still an improvement over the current situation. If we're correct in thinking the remaining issues are related to ITP changes in WebKit between iOS SDK versions, any additional improvements are likely to be larger in scope. It seems better to make incremental improvements now than delay until there is a comprehensive solution.

Thanks @guarani !!

@aerych aerych merged commit 76e00a5 into develop Nov 25, 2020
@aerych aerych deleted the issues/15266-fix-preview-url branch November 25, 2020 02:12
@aerych
Copy link
Copy Markdown
Contributor Author

aerych commented Nov 25, 2020

Opting to forgo any thing added to release notes given there are still things we want to address elsewhere.

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.

View link: sharing a link to the post displays a preview URL instead of the public one

4 participants