Preserve deep-link when a guest hits a private-tree URL (#5388)#5409
Draft
jbh4x82 wants to merge 1 commit into
Draft
Preserve deep-link when a guest hits a private-tree URL (#5388)#5409jbh4x82 wants to merge 1 commit into
jbh4x82 wants to merge 1 commit into
Conversation
When an unauthenticated visitor follows a deep link into a private tree (e.g. an email link to /tree/<name>/individual/I1, or a custom-module tree URL), TreeService::all() hides the tree, so Router::process() falls through to NotFound, which redirects to the home page and discards the original URL. After signing in the visitor lands on the home page instead of the page they originally clicked. Redirect guests to the login page with the requested URL preserved - the same pattern AuthLoggedIn/AuthMember already use for auth-required routes. LoginPage and LoginAction both validate the URL with isLocalUrl(), so it is not an open redirect. Logged-in users keep the existing behaviour, and we never reveal whether a private tree exists. Refs fisharebest#5388
This file contains hidden or 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
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.
Fixes the deep-link loss described in #5388.
Problem
When an unauthenticated visitor follows a deep link into a private tree (e.g. an email link to
/tree/<name>/individual/I1, or any custom-module tree URL),TreeService::all()hides the tree from them, soRouter::process()reaches its{tree}fall-through and defers toNotFound, whichredirect()s to the home page. The original URL is discarded, so after signing in the visitor lands on the home page instead of the page they clicked. This breaks "click this link to see the post" workflows on sites with private trees.Why here, and not in
NotFoundAs you noted in #5388, rendering a 404 in place at the tree URL would be a dead-end (no valid tree → no genealogy menu). This change avoids that by redirecting guests to the login page carrying the requested URL, which is a normal navigable page.
Router::process()is also where the information actually is: we already know the matched route required{tree}and that the tree was not resolvable for this user, soNotFounditself stays stock.What it does
At the
{tree}-null branch:redirect(route(LoginPage::class, ['url' => (string) $request->getUri()])). This is the same patternAuthLoggedInandAuthMemberalready use for auth-required routes.LoginPagereads that URL andLoginActionredirects to it on success, both throughisLocalUrl(), so it is not an open redirect. It also avoids disclosing to a guest whether a private tree exists: a missing tree and a private-but-inaccessible tree are handled identically.NotFoundas before). Signing in again would not help, and we must not mislabel a genuinely missing tree as access-denied.Testing
main+php -lon this branch; behaviour confirmed on 2.2.6 / PHP 8.3.{tree}404s (typos, unknown routes) → unchanged.Happy to adjust. For example, if you'd prefer the logged-in-but-no-access case to render
HttpAccessDeniedExceptionrather than fall through, that's a small addition, though it needs distinguishing a private tree from a missing one (an extragedcom-table lookup), which is why I left it out here.