Repository navigation
Conversation
e96538d to
9680405
Compare
nkissebe
left a comment
There was a problem hiding this comment.
Thanks for this. Keeping bots off the expensive listing queries is a real need, but I don't think a per-component "allow public search" boolean is the right tool, and as submitted it doesn't close the door it means to. I'll make the change described at the end and open it as a PR that supersedes this one.
The guard only covers one entry point per component.
- Resources: only
browseTaskis gated.browsetagsTask(the tag browser, on by default, so thereturn $this->browseTask()fallback never runs) and the AJAXbrowserTaskstill run keyword and tag searches for guests. - Tags: only
viewTask.feedTasktakes the same comma-separated list and returns the multi-tag result set as RSS. - The resources API
listendpoint (?search=) and the site-search plugins for publications and resources still answer keyword queries for guests.
Behaviour problems.
- A guest tag-filtered publications browse throws HTTP 410 Gone with a "you must log in" message instead of redirecting to login. 410 tells crawlers the URL is permanently gone even though it's valid for logged-in users and whenever the option is re-enabled.
- The tags check counts the raw exploded list, so
/tags/foo,or/tags/foo,fooor?addtag=Foolook like multi-tag searches and bounce a guest to login for a single tag. - The
QUERY_STRINGregex is redundant withRequest::getString('tag')and only differs for an empty?tag=, which then gets the 410. - The same login-redirect block is copied into three components (publications already has
_login()for this).
Proposal: make it a view access level, not a boolean. The hub already has view levels (Public, Registered, Special, plus whatever a hub defines) and com_projects already exposes a config field of type="accesslevel". So:
- each component gets a
browse_access(tags:search_access) field of type accesslevel, default Public, so nothing changes for hubs that don't touch it; - one helper on
Hubzero\Component\SiteController(requireViewLevel($level, $message)) does the check againstUser::getAuthorisedViewLevels(), sending guests to log in with a return URL and giving logged-in users without the level a 403; - it's enforced on every listing/search path: publications browse; resources browse, tag browser and AJAX browser; tags view and feed (on the sanitized, de-duplicated tag count); the resources API list search; and the two search plugins.
"Registered only" then just means picking that level, and a hub can pick a narrower one. The default stays public, which matters because this hub's robots.txt and nofollow links are what actually keep crawlers off these pages today.
|
|
||
| if (Request::getString('tag', '', 'request') || preg_match('/(?:^|[&;])(?:amp;)?tag=/i', $query)) | ||
| { | ||
| throw new Exception(Lang::txt('COM_PUBLICATIONS_SEARCH_LOGIN_REQUIRED'), 410); |
There was a problem hiding this comment.
410 Gone tells crawlers this URL is permanently dead, but it's valid for anyone logged in and whenever the option is re-enabled; and a guest gets an error page with no login link. The replacement sends guests to log in with a return URL, like the other guarded paths. (The QUERY_STRING regex above only differs from Request::getString('tag') for an empty ?tag=, which then lands here too.)
| $tgs[] = $addtag; | ||
| } | ||
|
|
||
| if (!$this->config->get('allow_public_search', 1) && count($tgs) > 1 && User::isGuest()) |
There was a problem hiding this comment.
count($tgs) > 1 is the raw exploded list, so /tags/foo, or /tags/foo,foo or ?addtag=Foo bounce a guest for what is really a single tag. The de-duplicated $added after the sanitize loop is the right thing to count. feedTask also needs the same check; it takes the same list and returns the multi-tag results as RSS.
|
Merged via #1938: your three commits carried as-is, plus the rework proposed in the review above. The per-component boolean became a view access level ( |
Searching Tags/Resources/Publications can be disabled for public users /bots