-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Themes: redirect logged-in-only routes to login page in logged-out sessions #30543
Conversation
https://hash-a3c158b1b194ddfa66db1c5cc41f3621b18115dc.calypso.live/theme/modern-business/example.blog -> redirects to https://hash-a3c158b1b194ddfa66db1c5cc41f3621b18115dc.calypso.live/theme/modern-business. I would have expected it to show the login screen. https://hash-a3c158b1b194ddfa66db1c5cc41f3621b18115dc.calypso.live/themes/example.blog just shows a white screen, but didn't pop up the login dialog. |
…ssions For theme uploads, there can be only one route definition for both logged-in and logged-out sessions. When logged out, the `redirectLoggedOut` handler will kick in and will redirect to login page. For other `/themes` routes, we redirect the logged-in-only ones (i.e., with `:site_id` suffix) to login page when hit in logged-out session.
a3c158b
to
7ebcd60
Compare
That makes much more sense than showing a blank page, but I changed it anyway in 749b434. Previously, the
Oops, that's a bug in my patch. I implemented a flawed logic where the following series of routes was tried to match:
I expected Fixed by having only one set of routes for both logged in and logged out:
There is always and optional If the optional Then But @ockham can you please review this PR for more gotchas like this? Themes are complicated 😄 |
d2ef315
to
6398811
Compare
🤦♂️ Another SSR bug, similar to #26944 (comment) Server rendered a placeholder markup for Fixed in 6398811 by forcing a full page reload after login redirect. |
|
||
next(); | ||
} | ||
|
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.
Bit of a style nit, but I kind of prefer to make route information as explicit as possible in route defs (i.e. router()
statements, in this case) -- rather than having a check if there really is a site_id
param by this middleware.
How about:
- Renaming
loggedOutRoutes
toroutesWithoutSiteFragments
- Renaming
loggedInRoutes
toroutesWithSiteFragments
, and removing the?
after the:site_id(${ siteId })
- In
if ( isLoggedIn ) { /*...*/ }
: Changingrouter( routesWithSiteFragments, /*...*/ )
torouter( [ ...routesWithoutSiteFragments, ...routesWithSiteFragments ], /*...*/ )
- In the
else
branch: Addrouter( routesWithSiteFragments, redirectLoggedOut );
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.
Hi @ockham! This suggestion is very similar to my original implementation (visible here, I force-pushed over that commit since then), but there is one gotcha:
if ( loggedIn ) {
router( [ ...routesWithoutSiteFragments, ...routesWithSiteFragments ] );
}
This doesn't work, because /themes/site.blog
is matched by the /themes/:vertical?
pattern rather than the /themes/:site_id
one. Because of their order. Putting routesWithSiteFragments
first should fix that. /themes/site.blog
and /themes/1234
will match the more restrictive site_id
pattern, while /themes/wedding
will fall through to the one with vertical
.
It's solvable, but I'd rather land this PR first, as it works correctly, and then improve in a followup PR. There are folks blocked by this bug and waiting for a fix. And there is a risk that the improvements won't be so straightforward and will require a few review/testing cycles to make them right.
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.
This works fine in my testing (I tried different permutations of logged-in/out, /themes
with and without site fragments and/or filters, and /theme
with and without site fragments).
I left one suggestion which I think would allow us to keep route defs a bit more declarative, but feel free to disregard.
Fixes regression introduced in #30543 where (because of a missing `return`) the page.js handler chain is not terminated when a redirect to login happens. I.e., `next` is called and the following handlers are executed. That can lead to a "flash of silly content" before the redirect or even crash.
…30686) Fixes regression introduced in #30543 where (because of a missing `return`) the page.js handler chain is not terminated when a redirect to login happens. I.e., `next` is called and the following handlers are executed. That can lead to a "flash of silly content" before the redirect or even crash.
For theme uploads, there can be only one route definition for both logged-in and logged-out sessions. When logged out, the
redirectLoggedOut
handler will kick in and will redirect to login page.For other
/themes
routes, we redirect the logged-in-only ones (i.e., with:site_id
suffix) to login page when hit in logged-out session.Spinoff from #30537, fixes the issue with Themes deep links not correctly redirecting to login page described in p1HpG7-6eg-p2.
How to test:
Verify that all
/themes
routes are still correctly rendered, both in server- and client-rendered versions, both logged in and logged out. Verify that the logged-in-only (with/:site_id
suffix) routes redirect to login when accessed in logged-out session.