Redirect sections that don't have the enableLoggedOut flag to login page #30537
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jsnajdr
added
Framework
[Status] Needs Review
jsnajdr
requested review from
bluefuton,
ockham,
mattwiebe,
taggon,
Tug,
stephanethomas,
gwwar,
delputnam,
jessefriedman,
sirbrillig,
klimeryk,
seear and
a team
This was referenced Feb 1, 2019
|
I tested the Google My Business section as well as login, and everything seems to work fine. |
sirbrillig
approved these changes
Feb 1, 2019
|
reader routes all checked out |
|
Seeing the same oddities on the logged-in theme routes that I reported on #30543 |
jsnajdr
force-pushed
the
update/logged-out-redirect-to-login
branch
from
4ac1eb4
to
ae685f7
Compare
|
The Themes fixes were merged separately in #30543 and now this branch contains only all the other sections that use the framework-level logged-out handler. I'll keep this open for another 24 hours before merging, so that the mentioned folks have a chance to test their sections. |
Routes in sections that don't have the `enableLoggedOut` flag set to `true` should always redirect to login page when requested in a logged-out session. This patch removes the check-and-redirect code (which is broken anyway) from the `loggedOutMiddleware` function and moves it to the section loader handler. Which, until now, only loaded the section's webpack chunk. Now it also guards for unwanted (logged-out) visitors.
Instead of per-route checks, remove the `enableLoggedOut` flag from the section and let the framework (section loader) do the login redirect.
Instead of per-route checks, remove the `enableLoggedOut` flag from the section and let the framework (section loader) do the login redirect. Effectively reverts #27261 in favor of doing it The Right Way.
Instead of per-route checks, remove the `enableLoggedOut` flag from the section and let the framework (section loader) do the login redirect. Effectively reverts #25000 and replaces it with a better implementation.
Instead of per-route checks, remove the `enableLoggedOut` flag from the section and let the framework (section loader) do the login redirect.
Instead of per-route checks, remove the `enableLoggedOut` flag from the section and let the framework (section loader) do the login redirect. Effectively reverts #25741 and replaces it with a better implementation.
Redirect to login page is handled in framework if the flag is false.
jsnajdr
force-pushed
the
update/logged-out-redirect-to-login
branch
from
ae685f7
to
cb3e63e
Compare
|
Full e2e suite finished with one failure: when signing up for account only with no site through
I verified manually that the failed flow works on calypso.live -- I got the expected results. |
matticbot
removed
the
[Status] Needs Review
jsnajdr
added a commit
that referenced
this pull request
Aug 26, 2020
The section references the same module (`client/reader`) as the main `reader` section, and doesn't do anything useful. It was added in #25741 to support logged-out redirects, but these redirects were then implemented later at a framework level in #30537 and the Reader-specific redirect code was removed. After that, the section became an empty shell. Also moves the `/recommendations` redirect handler to `reader/search`, potentially removing the need to load any other section than `reader/search` when handling the route.
jsnajdr
added a commit
that referenced
this pull request
Aug 26, 2020
The section references the same module (`client/reader`) as the main `reader` section, and doesn't do anything useful. It was added in #25741 to support logged-out redirects, but these redirects were then implemented later at a framework level in #30537 and the Reader-specific redirect code was removed. After that, the section became an empty shell. Also moves the `/recommendations` redirect handler to `reader/search`, potentially removing the need to load any other section than `reader/search` when handling the route.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Provides the long-awaited framework-level fix for #23785. All sections that don't have the
enableLoggedOutflag redirect to login page when a request hits them in a logged-out session.The only two sections that still need to a custom per-route redirect are:
/themes: there is a mixture of logged-out and logged-in (with/:sitesuffix) routes there/help: redirects not only to login page, but to en.support.wordpress.com URLs for some routesRemoves per-route redirects in many sections, in favor of the framework-ish solution:
/me/purchases: reverts Purchases: add logged-out redirect to all paths #24435 by @sirbrillig/me/security: reverts Me: add logged-out redirect for me/security/* #27261 by @bluefuton/checkout: reverts Enable the /checkout/:domain/:product route for logged-out users #22993 by @seear and Checkout: redirect logged-out users to login page #24687 by @taggon/domain-connect: reverts Make Domain Connect authorize page work for logged out users. #25000 by @delputnam (and @klimeryk)/google-my-business: reverts Google My Business: Redirect logged out users to login page #23548 by @stephanethomas/reader: reverts Reader: redirect common paths when logged out #25741 by @bluefuton/stats: reverts/fixes Guided Tour: Redirect to Login page if logged out #25498 by @mattwiebeThe
/themesroute gets its own very special solution (cc @ockham) that I'm likely to land in a separate PR. But it's still one of commits here.How to test:
Verify that logged-out accesses to the sections mentioned above (especially when you're mentioned for that section) correcly redirect to login. Verify that routes that are supposed to work in logged-out mode (themes, help, all kinds of logins and signups) continue to work 💯