From 9681d9f9786772e3b83a27661444b0c4af0b54f0 Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Fri, 22 Nov 2024 15:30:54 -0400 Subject: [PATCH] fix: revert POST for document search requests (#8263) * Revert "fix: ensure csrf cookie for searches (#8260)" This reverts commit 622ded5d2b4db9d4bf948af0e1d0890edaa8bb33. * Revert "refactor: eliminate single-use helper (#8226)" This reverts commit 6608c9d530b62d10c88d637f949a00fb5fea4526. * Revert "feat: POST for document search requests (#8206)" This reverts commit b65a37b6e8f6cfe105cd89c61c7046fff9d21523. * test: add back test fix * refactor: eliminate single-use helper --- ietf/api/serializer.py | 1 - ietf/doc/tests.py | 155 ++++++--------------- ietf/doc/views_search.py | 120 ++++++---------- ietf/liaisons/forms.py | 1 - ietf/liaisons/widgets.py | 1 - ietf/templates/doc/search/search_form.html | 2 - 6 files changed, 84 insertions(+), 196 deletions(-) diff --git a/ietf/api/serializer.py b/ietf/api/serializer.py index ca34ea649e..27f194c5b5 100644 --- a/ietf/api/serializer.py +++ b/ietf/api/serializer.py @@ -146,7 +146,6 @@ def end_object(self, obj): field_value = None else: field_value = field - # Need QuerySetAny instead of QuerySet until django-stubs 5.0.1 if isinstance(field_value, QuerySetAny) or isinstance(field_value, list): self._current[name] = dict([ (rel.pk, self.expand_related(rel, name)) for rel in field_value ]) else: diff --git a/ietf/doc/tests.py b/ietf/doc/tests.py index abac10a5e9..f0c8e30626 100644 --- a/ietf/doc/tests.py +++ b/ietf/doc/tests.py @@ -73,163 +73,96 @@ class SearchTests(TestCase): - def test_search_handles_querystring_parameters(self): - """Search parameters via querystring should not actually search""" - url = urlreverse("ietf.doc.views_search.search") - r = self.client.get(url + "?name=some-document-name&oldDrafts=on") - # Check that we got a valid response and that the warning about query string parameters is shown. - self.assertContains( - r, - "Searching via the URL query string is no longer supported.", - status_code=200, - ) - # Check that the form was filled in correctly (not an exhaustive check, but different from the - # form defaults) - pq = PyQuery(r.content) - self.assertEqual( - pq("form#search_form input#id_name").attr("value"), - "some-document-name", - "The name field should be set in the SearchForm", - ) - self.assertEqual( - pq("form#search_form input#id_olddrafts").attr("checked"), - "checked", - "The old drafts checkbox should be selected in the SearchForm", - ) - self.assertIsNone( - pq("form#search_form input#id_rfcs").attr("checked"), - "The RFCs checkbox should not be selected in the SearchForm", - ) - self.assertIsNone( - pq("form#search_form input#id_activedrafts").attr("checked"), - "The active drafts checkbox should not be selected in the SearchForm", - ) - def test_search(self): - draft = WgDraftFactory( - name="draft-ietf-mars-test", - group=GroupFactory(acronym="mars", parent=Group.objects.get(acronym="farfut")), - authors=[PersonFactory()], - ad=PersonFactory(), - ) + + draft = WgDraftFactory(name='draft-ietf-mars-test',group=GroupFactory(acronym='mars',parent=Group.objects.get(acronym='farfut')),authors=[PersonFactory()],ad=PersonFactory()) rfc = WgRfcFactory() draft.set_state(State.objects.get(used=True, type="draft-iesg", slug="pub-req")) - old_draft = IndividualDraftFactory( - name="draft-foo-mars-test", - authors=[PersonFactory()], - title="Optimizing Martian Network Topologies", - ) + old_draft = IndividualDraftFactory(name='draft-foo-mars-test',authors=[PersonFactory()],title="Optimizing Martian Network Topologies") old_draft.set_state(State.objects.get(used=True, type="draft", slug="expired")) - - url = urlreverse("ietf.doc.views_search.search") - + + base_url = urlreverse('ietf.doc.views_search.search') + # only show form, no search yet - r = self.client.get(url) + r = self.client.get(base_url) self.assertEqual(r.status_code, 200) - + # no match - r = self.client.post(url, {"activedrafts": "on", "name": "thisisnotadocumentname"}) + r = self.client.get(base_url + "?activedrafts=on&name=thisisnotadocumentname") self.assertEqual(r.status_code, 200) self.assertContains(r, "No documents match") - - r = self.client.post(url, {"rfcs": "on", "name": "xyzzy"}) + + r = self.client.get(base_url + "?rfcs=on&name=xyzzy") self.assertEqual(r.status_code, 200) self.assertContains(r, "No documents match") - - r = self.client.post(url, {"olddrafts": "on", "name": "bar"}) + + r = self.client.get(base_url + "?olddrafts=on&name=bar") self.assertEqual(r.status_code, 200) self.assertContains(r, "No documents match") - - r = self.client.post(url, {"olddrafts": "on", "name": "foo"}) + + r = self.client.get(base_url + "?olddrafts=on&name=foo") self.assertEqual(r.status_code, 200) self.assertContains(r, "draft-foo-mars-test") - - r = self.client.post(url, {"olddrafts": "on", "name": "FoO"}) # mixed case + + r = self.client.get(base_url + "?olddrafts=on&name=FoO") # mixed case self.assertEqual(r.status_code, 200) self.assertContains(r, "draft-foo-mars-test") - + # find by RFC - r = self.client.post(url, {"rfcs": "on", "name": rfc.name}) + r = self.client.get(base_url + "?rfcs=on&name=%s" % rfc.name) self.assertEqual(r.status_code, 200) self.assertContains(r, rfc.title) - + # find by active/inactive - + draft.set_state(State.objects.get(type="draft", slug="active")) - r = self.client.post(url, {"activedrafts": "on", "name": draft.name}) + r = self.client.get(base_url + "?activedrafts=on&name=%s" % draft.name) self.assertEqual(r.status_code, 200) self.assertContains(r, draft.title) - + draft.set_state(State.objects.get(type="draft", slug="expired")) - r = self.client.post(url, {"olddrafts": "on", "name": draft.name}) + r = self.client.get(base_url + "?olddrafts=on&name=%s" % draft.name) self.assertEqual(r.status_code, 200) self.assertContains(r, draft.title) - + draft.set_state(State.objects.get(type="draft", slug="active")) - + # find by title - r = self.client.post(url, {"activedrafts": "on", "name": draft.title.split()[0]}) + r = self.client.get(base_url + "?activedrafts=on&name=%s" % draft.title.split()[0]) self.assertEqual(r.status_code, 200) self.assertContains(r, draft.title) - + # find by author - r = self.client.post( - url, - { - "activedrafts": "on", - "by": "author", - "author": draft.documentauthor_set.first().person.name_parts()[1], - }, - ) + r = self.client.get(base_url + "?activedrafts=on&by=author&author=%s" % draft.documentauthor_set.first().person.name_parts()[1]) self.assertEqual(r.status_code, 200) self.assertContains(r, draft.title) - + # find by group - r = self.client.post( - url, - {"activedrafts": "on", "by": "group", "group": draft.group.acronym}, - ) + r = self.client.get(base_url + "?activedrafts=on&by=group&group=%s" % draft.group.acronym) self.assertEqual(r.status_code, 200) self.assertContains(r, draft.title) - - r = self.client.post( - url, - {"activedrafts": "on", "by": "group", "group": draft.group.acronym.swapcase()}, - ) + + r = self.client.get(base_url + "?activedrafts=on&by=group&group=%s" % draft.group.acronym.swapcase()) self.assertEqual(r.status_code, 200) self.assertContains(r, draft.title) - + # find by area - r = self.client.post( - url, - {"activedrafts": "on", "by": "area", "area": draft.group.parent_id}, - ) + r = self.client.get(base_url + "?activedrafts=on&by=area&area=%s" % draft.group.parent_id) self.assertEqual(r.status_code, 200) self.assertContains(r, draft.title) - + # find by area - r = self.client.post( - url, - {"activedrafts": "on", "by": "area", "area": draft.group.parent_id}, - ) + r = self.client.get(base_url + "?activedrafts=on&by=area&area=%s" % draft.group.parent_id) self.assertEqual(r.status_code, 200) self.assertContains(r, draft.title) - + # find by AD - r = self.client.post(url, {"activedrafts": "on", "by": "ad", "ad": draft.ad_id}) + r = self.client.get(base_url + "?activedrafts=on&by=ad&ad=%s" % draft.ad_id) self.assertEqual(r.status_code, 200) self.assertContains(r, draft.title) - + # find by IESG state - r = self.client.post( - url, - { - "activedrafts": "on", - "by": "state", - "state": draft.get_state("draft-iesg").pk, - "substate": "", - }, - ) + r = self.client.get(base_url + "?activedrafts=on&by=state&state=%s&substate=" % draft.get_state("draft-iesg").pk) self.assertEqual(r.status_code, 200) self.assertContains(r, draft.title) @@ -238,15 +171,15 @@ def test_search_became_rfc(self): rfc = WgRfcFactory() draft.set_state(State.objects.get(type="draft", slug="rfc")) draft.relateddocument_set.create(relationship_id="became_rfc", target=rfc) - url = urlreverse("ietf.doc.views_search.search") + base_url = urlreverse('ietf.doc.views_search.search') # find by RFC - r = self.client.post(url, {"rfcs": "on", "name": rfc.name}) + r = self.client.get(base_url + f"?rfcs=on&name={rfc.name}") self.assertEqual(r.status_code, 200) self.assertContains(r, rfc.title) # find by draft - r = self.client.post(url, {"activedrafts": "on", "rfcs": "on", "name": draft.name}) + r = self.client.get(base_url + f"?activedrafts=on&rfcs=on&name={draft.name}") self.assertEqual(r.status_code, 200) self.assertContains(r, rfc.title) diff --git a/ietf/doc/views_search.py b/ietf/doc/views_search.py index f4ad247ff0..9e9b5e88dd 100644 --- a/ietf/doc/views_search.py +++ b/ietf/doc/views_search.py @@ -33,12 +33,11 @@ # (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. - +import hashlib +import json import re import datetime import copy -import hashlib -import json import operator from collections import defaultdict @@ -46,18 +45,16 @@ from django import forms from django.conf import settings -from django.contrib import messages from django.core.cache import cache, caches from django.urls import reverse as urlreverse from django.db.models import Q -from django.http import Http404, HttpResponseBadRequest, HttpResponse, HttpResponseRedirect +from django.http import Http404, HttpResponseBadRequest, HttpResponse, HttpResponseRedirect, QueryDict from django.shortcuts import render from django.utils import timezone from django.utils.html import strip_tags from django.utils.cache import _generate_cache_key # type: ignore from django.utils.text import slugify -from django.views.decorators.csrf import ensure_csrf_cookie -from django_stubs_ext import QuerySetAny + import debug # pyflakes:ignore @@ -149,29 +146,6 @@ def clean(self): q['irtfstate'] = None return q - def cache_key_fragment(self): - """Hash a bound form to get a value for use in a cache key - - Raises a ValueError if the form is not valid. - """ - def _serialize_value(val): - # Need QuerySetAny instead of QuerySet until django-stubs 5.0.1 - if isinstance(val, QuerySetAny): - return [item.pk for item in val] - else: - return getattr(val, "pk", val) # use pk if present, else value - - if not self.is_valid(): - raise ValueError(f"SearchForm invalid: {self.errors}") - contents = { - field_name: _serialize_value(field_value) - for field_name, field_value in self.cleaned_data.items() - if field_name != "sort" and field_value is not None - } - contents_json = json.dumps(contents, sort_keys=True) - return hashlib.sha512(contents_json.encode("utf-8")).hexdigest() - - def retrieve_search_results(form, all_types=False): """Takes a validated SearchForm and return the results.""" @@ -284,65 +258,51 @@ def retrieve_search_results(form, all_types=False): return docs -@ensure_csrf_cookie def search(request): - """Search for a draft""" - # defaults for results / meta - results = [] - meta = {"by": None, "searching": False} - - if request.method == "POST": - form = SearchForm(data=request.POST) - if form.is_valid(): - cache_key = f"doc:document:search:{form.cache_key_fragment()}" - cached_val = cache.get(cache_key) - if cached_val: - [results, meta] = cached_val - else: - results = retrieve_search_results(form) - results, meta = prepare_document_table( - request, results, form.cleaned_data - ) - cache.set( - cache_key, [results, meta] - ) # for settings.CACHE_MIDDLEWARE_SECONDS - log(f"Search results computed for {form.cleaned_data}") - meta["searching"] = True - else: - if request.GET: - # backwards compatibility - fill in the form - get_params = request.GET.copy() - if "activeDrafts" in request.GET: - get_params["activedrafts"] = request.GET["activeDrafts"] - if "oldDrafts" in request.GET: - get_params["olddrafts"] = request.GET["oldDrafts"] - if "subState" in request.GET: - get_params["substate"] = request.GET["subState"] - form = SearchForm(data=get_params) - messages.error( - request, - ( - "Searching via the URL query string is no longer supported. " - "The form below has been filled in with the parameters from your request. " - 'To execute your search, please click "Search".' - ), - ) + def _get_cache_key(params): + fields = set(SearchForm.base_fields) - {'sort'} + kwargs = dict([(k, v) for (k, v) in list(params.items()) if k in fields]) + key = "doc:document:search:" + hashlib.sha512(json.dumps(kwargs, sort_keys=True).encode('utf-8')).hexdigest() + return key + + if request.GET: + # backwards compatibility + get_params = request.GET.copy() + if 'activeDrafts' in request.GET: + get_params['activedrafts'] = request.GET['activeDrafts'] + if 'oldDrafts' in request.GET: + get_params['olddrafts'] = request.GET['oldDrafts'] + if 'subState' in request.GET: + get_params['substate'] = request.GET['subState'] + + form = SearchForm(get_params) + if not form.is_valid(): + return HttpResponseBadRequest("form not valid: %s" % form.errors) + + cache_key = _get_cache_key(get_params) + cached_val = cache.get(cache_key) + if cached_val: + [results, meta] = cached_val else: - form = SearchForm() + results = retrieve_search_results(form) + results, meta = prepare_document_table(request, results, get_params) + cache.set(cache_key, [results, meta]) # for settings.CACHE_MIDDLEWARE_SECONDS + log(f"Search results computed for {get_params}") + meta['searching'] = True + else: + form = SearchForm() + results = [] + meta = { 'by': None, 'searching': False } + get_params = QueryDict('') - return render( - request, - "doc/search/search.html", - context={"form": form, "docs": results, "meta": meta}, + return render(request, 'doc/search/search.html', { + 'form':form, 'docs':results, 'meta':meta, 'queryargs':get_params.urlencode() }, ) - -@ensure_csrf_cookie def frontpage(request): form = SearchForm() return render(request, 'doc/frontpage.html', {'form':form}) - def search_for_name(request, name): def find_unique(n): exact = Document.objects.filter(name__iexact=n).first() diff --git a/ietf/liaisons/forms.py b/ietf/liaisons/forms.py index a75028bf79..1d91041b25 100644 --- a/ietf/liaisons/forms.py +++ b/ietf/liaisons/forms.py @@ -203,7 +203,6 @@ def get_results(self): class CustomModelMultipleChoiceField(ModelMultipleChoiceField): '''If value is a QuerySet, return it as is (for use in widget.render)''' def prepare_value(self, value): - # Need QuerySetAny instead of QuerySet until django-stubs 5.0.1 if isinstance(value, QuerySetAny): return value if (hasattr(value, '__iter__') and diff --git a/ietf/liaisons/widgets.py b/ietf/liaisons/widgets.py index 3d4f2d13a5..74368e83f2 100644 --- a/ietf/liaisons/widgets.py +++ b/ietf/liaisons/widgets.py @@ -35,7 +35,6 @@ def render(self, name, value, **kwargs): html = '
' % name html += 'No files attached' html += '
' - # Need QuerySetAny instead of QuerySet until django-stubs 5.0.1 if value and isinstance(value, QuerySetAny): for attachment in value: html += '%s ' % (conditional_escape(attachment.document.get_href()), conditional_escape(attachment.document.title)) diff --git a/ietf/templates/doc/search/search_form.html b/ietf/templates/doc/search/search_form.html index 6c91894c8c..d4f463ec66 100644 --- a/ietf/templates/doc/search/search_form.html +++ b/ietf/templates/doc/search/search_form.html @@ -4,10 +4,8 @@ {% load widget_tweaks %} {% load ietf_filters %}
- {% csrf_token %}
{{ form.name|add_class:"form-control"|attr:"placeholder:Document name/title/RFC number"|attr:"aria-label:Document name/title/RFC number" }}