Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

[Bug 785850] Limit votes on answers and questions #1217

Closed
wants to merge 2 commits into from

3 participants

Mike Cooper ricky rosario James Socol
Mike Cooper
Collaborator

Limit votes on answers and questions to 10 votes per day of either type. Votes after the limit of 10 will be silently ignored. The limit is based on IP addresses for unauthenticated users, or user id for authenticated users. Authenticated users are not limited by IP at all.

The tests are kind of complicated, but they work. The alternative is somehow trying to make the rate limits different during tests, which I'm not really excited about.

r?

Mike Cooper mythmon [Bug 785850] Limit votes on answers and questions
Limit votes on answers and questions to 10 votes per day of either type.
Votes after the limit of 10 will be silently ignored. The limit is based
on IP addresses for unauthenticated users, or user id for authenticated
users. Authenticatd users are not limited by IP at all.
1a8c397
ricky rosario rlr commented on the diff
apps/questions/views.py
@@ -714,6 +715,7 @@ def unsolve(request, question_id, answer_id):
714 715
715 716 @require_POST
716 717 @csrf_exempt
  718 +@ratelimit(keys=user_or_ip, ip=False, rate='10/d')
4
ricky rosario Collaborator
rlr added a note

What does ip=False mean? Is that just overriden by user_or_ip?

Mike Cooper Collaborator
mythmon added a note

By default django-ratelimit will limit based on IP. ip=False disables that default, so it only uses user_or_ip.

James Socol
jsocol added a note

I'm thinking the next rev adds settings to set project defaults for these things.

Mike Cooper Collaborator
mythmon added a note

Ooh, that's a good idea!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
apps/sumo/utils.py
@@ -165,3 +165,18 @@ def truncated_json_dumps(obj, max_length, key, ensure_ascii=False):
165 165 "`max_length`.")
166 166 dupe[key] = dupe[key][:-diff]
167 167 return json.dumps(dupe, ensure_ascii=ensure_ascii)
  168 +
  169 +
  170 +def user_or_ip(request):
  171 + """Used for generating rate limiting keys. Returns IP address for
  172 + anonymous users, and pks for authenticated users.
  173 +
  174 + Examples
  175 + Anonymous: 'uip:127.0.0.1'
  176 + Authenticated: 'uip:17859'
  177 + """
  178 + if hasattr(request, 'user') and request.user.is_authenticated():
  179 + key = str(request.user.pk)
  180 + else:
  181 + key = request.META['REMOTE_ADDR']
4
ricky rosario Collaborator
rlr added a note

I'm pretty sure you want HTTP_X_FORWARDED_FOR here. At least in prod. So maybe you want to do something like:
request.META.get('HTTP_X_FORWARDED_FOR', request.META.get('request.REMOTE_ADDR')).

See https://support.mozilla.org/admin/env for full WSGI request details on prod, for example.

ricky rosario Collaborator
rlr added a note

The reason is that in prod, REMOTE_ADDR will have the load balancer IP address. Does this sound right @jsocol ^^ ?

Mike Cooper Collaborator
mythmon added a note

That matches what I see on that env page. I'll fix that.

James Socol
jsocol added a note

We don't want to trust X-Forwarded-For unless we're validating it. (Clients can set it, and it can be a list.) We haved SetRemoteAddressFromForwardedFor middleware somewhere. That never worked because it implemented (or tried to, anyway) a complicated algorithm that was supposed to work with our specific set up.

Let's talk to IT and see if we can trust X_CLUSTER_CLIENT_IP or X_FORWARDED_FOR. It's possible that the rules have changed since Dave first walked me through that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ricky rosario
Collaborator
rlr commented

I like the decorator!

Mike Cooper
Collaborator

James did the heavy lifting, writing django-ratelimit, and modifying it recently. I just stuck it in :)

Mike Cooper
Collaborator

feedback^

ricky rosario
Collaborator
rlr commented

LGTM. I used the Modify Headers addon to add a HTTP_X_FORWARDED_FOR to the request and try to trick the proxy but it passed on the right value to the app. R+

ricky rosario
Collaborator
rlr commented

DOH! I should've tested passing X_FORWARDED_FOR. That one keeps the value you pass in the request. But if I pass X_CLUSTER_CLIENT_IP it is overriden with the right value.

ricky rosario
Collaborator
rlr commented
ricky rosario rlr closed this
Mike Cooper mythmon deleted the branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 2 unique commits by 1 author.

Mar 20, 2013
Mike Cooper mythmon [Bug 785850] Limit votes on answers and questions
Limit votes on answers and questions to 10 votes per day of either type.
Votes after the limit of 10 will be silently ignored. The limit is based
on IP addresses for unauthenticated users, or user id for authenticated
users. Authenticatd users are not limited by IP at all.
1a8c397
Mike Cooper mythmon Use HTTP_X_FORWARDED_FOR if availiable. 1bc6b4c
This page is out of date. Refresh to see the latest.
117 apps/questions/tests/test_views.py
... ... @@ -1,3 +1,4 @@
  1 +import json
1 2 from datetime import datetime, timedelta
2 3
3 4 from django.conf import settings
@@ -10,7 +11,7 @@
10 11 from pyquery import PyQuery as pq
11 12
12 13 from products.tests import product
13   -from questions.models import Question
  14 +from questions.models import Question, QuestionVote, AnswerVote
14 15 from questions.tests import answer, question
15 16 from questions.views import parse_troubleshooting
16 17 from search.tests.test_es import ElasticTestCase
@@ -419,3 +420,117 @@ def sub_test(locale, *titles):
419 420 sub_test('pt-BR', 'pies?')
420 421 # de is not in AAQ_LOCALES, so should show en-US, but not pt-BR
421 422 sub_test('de', 'cupcakes?', 'donuts?', 'pastries?')
  423 +
  424 +
  425 +class TestRateLimiting(TestCase):
  426 +
  427 + def _check_question_vote(self, q, ignored):
  428 + """Try and vote on `q`. If `ignored` is false, assert the
  429 + request worked. If `ignored` is True, assert the request didn't
  430 + do anything."""
  431 + url = reverse('questions.vote', args=[q.id], locale='en-US')
  432 + votes = QuestionVote.objects.filter(question=q).count()
  433 +
  434 + res = self.client.post(url, HTTP_X_REQUESTED_WITH='XMLHttpRequest')
  435 + eq_(res.status_code, 200)
  436 +
  437 + data = json.loads(res.content)
  438 + eq_(data.get('ignored', False), ignored)
  439 +
  440 + if ignored:
  441 + eq_(QuestionVote.objects.filter(question=q).count(), votes)
  442 + else:
  443 + eq_(QuestionVote.objects.filter(question=q).count(), votes + 1)
  444 +
  445 + def _check_answer_vote(self, q, a, ignored):
  446 + """Try and vote on `a`. If `ignored` is false, assert the
  447 + request worked. If `ignored` is True, assert the request didn't
  448 + do anything."""
  449 + url = reverse('questions.answer_vote', args=[q.id, a.id],
  450 + locale='en-US')
  451 + votes = AnswerVote.objects.filter(answer=a).count()
  452 +
  453 + res = self.client.post(url, HTTP_X_REQUESTED_WITH='XMLHttpRequest')
  454 + eq_(res.status_code, 200)
  455 +
  456 + data = json.loads(res.content)
  457 + eq_(data.get('ignored', False), ignored)
  458 +
  459 + if ignored:
  460 + eq_(AnswerVote.objects.filter(answer=a).count(), votes)
  461 + else:
  462 + eq_(AnswerVote.objects.filter(answer=a).count(), votes + 1)
  463 +
  464 + def test_question_vote_limit(self):
  465 + """Test that an anonymous user's votes are ignored after 10
  466 + question votes."""
  467 + questions = [question(save=True) for _ in range(11)]
  468 +
  469 + # The rate limit is 10 per day. So make 10 requests. (0 through 9)
  470 + for i in range(10):
  471 + self._check_question_vote(questions[i], False)
  472 +
  473 + # Now make another, it should fail.
  474 + self._check_question_vote(questions[10], True)
  475 +
  476 + def test_answer_vote_limit(self):
  477 + """Test that an anonymous user's votes are ignored after 10
  478 + answer votes."""
  479 + q = question(save=True)
  480 + answers = [answer(question=q, save=True) for _ in range(11)]
  481 +
  482 + # The rate limit is 10 per day. So make 10 requests. (0 through 9)
  483 + for i in range(10):
  484 + self._check_answer_vote(q, answers[i], False)
  485 +
  486 + # Now make another, it should fail.
  487 + self._check_answer_vote(q, answers[10], True)
  488 +
  489 + def test_question_vote_logged_in(self):
  490 + """This exhausts the rate limit, then logs in, and exhausts it
  491 + again."""
  492 + questions = [question(save=True) for _ in range(11)]
  493 + u = user(password='testpass', save=True)
  494 +
  495 + # The rate limit is 10 per day. So make 10 requests. (0 through 9)
  496 + for i in range(10):
  497 + self._check_question_vote(questions[i], False)
  498 + # The rate limit has been hit, so this fails.
  499 + self._check_question_vote(questions[10], True)
  500 +
  501 + # Login.
  502 + self.client.login(username=u.username, password='testpass')
  503 + for i in range(10):
  504 + self._check_question_vote(questions[i], False)
  505 +
  506 + # Now the user has hit the rate limit too, so this should fail.
  507 + self._check_question_vote(questions[10], True)
  508 +
  509 + # Logging out out won't help
  510 + self.client.logout()
  511 + self._check_question_vote(questions[10], True)
  512 +
  513 + def test_answer_vote_logged_in(self):
  514 + """This exhausts the rate limit, then logs in, and exhausts it
  515 + again."""
  516 + q = question(save=True)
  517 + answers = [answer(question=q, save=True) for _ in range(12)]
  518 + u = user(password='testpass', save=True)
  519 +
  520 + # The rate limit is 10 per day. So make 10 requests. (0 through 9)
  521 + for i in range(10):
  522 + self._check_answer_vote(q, answers[i], False)
  523 + # The ratelimit has been hit, so the next request will fail.
  524 + self._check_answer_vote(q, answers[11], True)
  525 +
  526 + # Login.
  527 + self.client.login(username=u.username, password='testpass')
  528 + for i in range(10):
  529 + self._check_answer_vote(q, answers[i], False)
  530 +
  531 + # Now the user has hit the rate limit too, so this should fail.
  532 + self._check_answer_vote(q, answers[10], True)
  533 +
  534 + # Logging out out won't help
  535 + self.client.logout()
  536 + self._check_answer_vote(q, answers[11], True)
38 apps/questions/views.py
@@ -24,6 +24,7 @@
24 24 import jingo
25 25 import waffle
26 26 from mobility.decorators import mobile_template
  27 +from ratelimit.decorators import ratelimit
27 28 from session_csrf import anonymous_csrf
28 29 from statsd import statsd
29 30 from taggit.models import Tag
@@ -52,7 +53,7 @@
52 53 Sphilastic, F)
53 54 from sumo.helpers import urlparams
54 55 from sumo.urlresolvers import reverse
55   -from sumo.utils import paginate, simple_paginate, build_paged_url
  56 +from sumo.utils import paginate, simple_paginate, build_paged_url, user_or_ip
56 57 from tags.utils import add_existing_tag
57 58 from topics.models import Topic
58 59 from upload.models import ImageAttachment
@@ -714,6 +715,7 @@ def unsolve(request, question_id, answer_id):
714 715
715 716 @require_POST
716 717 @csrf_exempt
  718 +@ratelimit(keys=user_or_ip, ip=False, rate='10/d')
717 719 def question_vote(request, question_id):
718 720 """I have this problem too."""
719 721 question = get_object_or_404(Question, pk=question_id)
@@ -728,19 +730,20 @@ def question_vote(request, question_id):
728 730 else:
729 731 vote.anonymous_id = request.anonymous.anonymous_id
730 732
731   - vote.save()
  733 + if not request.limited:
  734 + vote.save()
732 735
733   - if 'referrer' in request.REQUEST:
734   - referrer = request.REQUEST.get('referrer')
735   - vote.add_metadata('referrer', referrer)
  736 + if 'referrer' in request.REQUEST:
  737 + referrer = request.REQUEST.get('referrer')
  738 + vote.add_metadata('referrer', referrer)
736 739
737   - if referrer == 'search' and 'query' in request.REQUEST:
738   - vote.add_metadata('query', request.REQUEST.get('query'))
  740 + if referrer == 'search' and 'query' in request.REQUEST:
  741 + vote.add_metadata('query', request.REQUEST.get('query'))
739 742
740   - ua = request.META.get('HTTP_USER_AGENT')
741   - if ua:
742   - vote.add_metadata('ua', ua[:1000]) # 1000 max_length
743   - statsd.incr('questions.votes.question')
  743 + ua = request.META.get('HTTP_USER_AGENT')
  744 + if ua:
  745 + vote.add_metadata('ua', ua[:1000]) # 1000 max_length
  746 + statsd.incr('questions.votes.question')
744 747
745 748 if request.is_ajax():
746 749 tmpl = 'questions/includes/question_vote_thanks.html'
@@ -748,18 +751,29 @@ def question_vote(request, question_id):
748 751 html = jingo.render_to_string(request, tmpl, {
749 752 'question': question,
750 753 'watch_form': form})
751   - return HttpResponse(json.dumps({'html': html}))
  754 +
  755 + return HttpResponse(json.dumps({
  756 + 'html': html,
  757 + 'ignored': request.limited
  758 + }))
752 759
753 760 return HttpResponseRedirect(question.get_absolute_url())
754 761
755 762
756 763 @csrf_exempt
  764 +@ratelimit(keys=user_or_ip, ip=False, rate='10/d')
757 765 def answer_vote(request, question_id, answer_id):
758 766 """Vote for Helpful/Not Helpful answers"""
759 767 answer = get_object_or_404(Answer, pk=answer_id, question=question_id)
760 768 if answer.question.is_locked:
761 769 raise PermissionDenied
762 770
  771 + if request.limited:
  772 + if request.is_ajax():
  773 + return HttpResponse(json.dumps({"ignored": True}))
  774 + else:
  775 + return HttpResponseRedirect(answer.get_absolute_url())
  776 +
763 777 if not answer.has_voted(request):
764 778 vote = AnswerVote(answer=answer)
765 779
16 apps/sumo/utils.py
@@ -165,3 +165,19 @@ def truncated_json_dumps(obj, max_length, key, ensure_ascii=False):
165 165 "`max_length`.")
166 166 dupe[key] = dupe[key][:-diff]
167 167 return json.dumps(dupe, ensure_ascii=ensure_ascii)
  168 +
  169 +
  170 +def user_or_ip(request):
  171 + """Used for generating rate limiting keys. Returns IP address for
  172 + anonymous users, and pks for authenticated users.
  173 +
  174 + Examples
  175 + Anonymous: 'uip:127.0.0.1'
  176 + Authenticated: 'uip:17859'
  177 + """
  178 + if hasattr(request, 'user') and request.user.is_authenticated():
  179 + key = str(request.user.pk)
  180 + else:
  181 + key = request.META.get('HTTP_X_FORWARDED_FOR',
  182 + request.META['REMOTE_ADDR'])
  183 + return 'uip:%s' % key
6 media/js/ajaxform.js
@@ -83,8 +83,10 @@
83 83 self.options.afterComplete();
84 84 }
85 85
86   - // Trigger a document event for others to listen for.
87   - $(document).trigger('vote', $.extend(data, {url: url}));
  86 + if (!response.ignored) {
  87 + // Trigger a document event for others to listen for.
  88 + $(document).trigger('vote', $.extend(data, {url: url}));
  89 + }
88 90 },
89 91 error: function() {
90 92 var msg = self.options.errorText;
9 media/js/ajaxvote.js
@@ -55,7 +55,8 @@ AjaxVote.prototype = {
55 55 success: function(response) {
56 56 if (response.survey) {
57 57 self.showSurvey(response.survey, $form.parent());
58   - } else {
  58 + }
  59 + if (response.message) {
59 60 self.showMessage(response.message, $btn, $form);
60 61 }
61 62 $btn.addClass('active');
@@ -63,8 +64,10 @@ AjaxVote.prototype = {
63 64 $form.removeClass('busy');
64 65 self.voted = true;
65 66
66   - // Trigger a document event for others to listen for.
67   - $(document).trigger('vote', $.extend(data, {url: url}));
  67 + if (!data.ignored) {
  68 + // Trigger a document event for others to listen for.
  69 + $(document).trigger('vote', $.extend(data, {url: url}));
  70 + }
68 71
69 72 // Hide other forms
70 73 self.$form.filter(function() { return this !== $form; }).remove();
6 media/js/questions.js
@@ -189,8 +189,10 @@
189 189 .find('.msg').text(response.message);
190 190 }
191 191
192   - // Trigger a document event for others to listen for.
193   - $(document).trigger('vote', $.extend(data, {url: url}));
  192 + if (!response.ignored) {
  193 + // Trigger a document event for others to listen for.
  194 + $(document).trigger('vote', $.extend(data, {url: url}));
  195 + }
194 196 },
195 197 error: function() {
196 198 var message = gettext("There was an error.");
2  vendor/src/django-ratelimit
... ... @@ -1 +1 @@
1   -Subproject commit af71842a0c8a4e1f2ca22a012bc067fff4ceeb81
  1 +Subproject commit 5cdf0434658d1abb0ec4771623b6971188b8d463

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.