Major rewrite/refactor of voting code

This is a rewrite of much of the code related to voting. Some of the
improvements include:

- Detangling the whole process of creating and processing votes
- Creating an actual Vote class to use instead of dealing with
  inconsistent ad-hoc voting data everywhere
- More consistency with naming and other similar things like vote
  directions (previously had True/False/None in some places,
  1/-1/0 in others, etc.)
- More flexible methods in determining and applying the effects of votes
- Improvement of modhash generation/validation
- Removing various obsolete/unnecessary code

Author: Deimos

r2/r2/controllers/api.py

@@ -22,6 +22,7 @@
 
 from r2.controllers.reddit_base import (
     cross_domain,
+    generate_modhash,
     is_trusted_origin,
     MinimalController,
     pagecache_policy,
@@ -51,7 +52,6 @@
     query_string,
     randstr,
     sanitize_url,
-    timeago,
     timefromnow,
     timeuntil,
     tup,
@@ -110,9 +110,11 @@
 from r2.controllers.ipn import generate_blob, update_blob
 from r2.lib.lock import TimeoutExpired
 from r2.lib.csrf import csrf_exempt
+from r2.lib.voting import cast_vote
 
 from r2.models import wiki
 from r2.models.recommend import AccountSRFeedback, FEEDBACK_ACTIONS
+from r2.models.vote import Vote
 from r2.lib.merge import ConflictException
 
 import csv
@@ -124,20 +126,6 @@
 import urllib
 import urllib2
 
-def reject_vote(thing):
-    voteword = request.params.get('dir')
-
-    if voteword == '1':
-        voteword = 'upvote'
-    elif voteword == '0':
-        voteword = '0-vote'
-    elif voteword == '-1':
-        voteword = 'downvote'
-
-    log_text ("rejected vote", "Rejected %s from %s (%s) on %s %s via %s" %
-              (voteword, c.user.name, request.ip, thing.__class__.__name__,
-               thing._id36, request.referer), "info")
-
 
 class ApiminimalController(MinimalController):
     """
@@ -547,7 +535,6 @@ def POST_submit(self, form, jquery, url, selftext, kind, title,
             sendreplies=sendreplies,
         )
 
-
         if not is_self:
             ban = is_banned_domain(url)
             if ban:
@@ -556,9 +543,6 @@ def POST_submit(self, form, jquery, url, selftext, kind, title,
                 hooks.get_hook('banned_domain.submit').call(item=l, url=url,
                                                             ban=ban)
 
-        queries.queue_vote(c.user, l, dir=True, ip=request.ip,
-            cheater=c.cheater)
-
         if sr.should_ratelimit(c.user, 'link'):
             VRatelimit.ratelimit(rate_user=True, rate_ip = True,
                                  prefix = "rate_submit_")
@@ -607,7 +591,7 @@ def _login(self, responder, user, rem = None):
         self.login(user, rem = rem)
 
         if request.params.get("hoist") != "cookie":
-            responder._send_data(modhash = user.modhash())
+            responder._send_data(modhash=generate_modhash())
             responder._send_data(cookie  = user.make_cookie())
         responder._send_data(need_https=feature.is_enabled("force_https"))
 
@@ -1882,8 +1866,7 @@ def POST_editusertext(self, form, jquery, item, text):
         if item._deleted:
             return abort(403, "forbidden")
 
-        if (item._date < timeago('3 minutes')
-            or (item._ups + item._downs > 2)):
+        if item._age > timedelta(minutes=3) or item.num_votes > 2:
             item.editted = c.start_time
 
         item.ignore_reports = False
@@ -2035,8 +2018,6 @@ def POST_comment(self, commentform, jquery, parent, comment):
         else:
             item, inbox_rel = Comment._new(c.user, link, parent_comment,
                                            comment, request.ip)
-            queries.queue_vote(c.user, item, dir=True, ip=request.ip,
-                cheater=c.cheater)
 
         if is_message:
             queries.new_message(item, inbox_rel)
@@ -2161,12 +2142,12 @@ def POST_share(self, shareform, jquery, share_to, message, link):
     @require_oauth2_scope("vote")
     @noresponse(VUser(),
                 VModhash(),
-                vote_info=VVotehash('vh'),
-                dir=VInt('dir', min=-1, max=1, docs={"dir":
-                         "vote direction. one of (1, 0, -1)"}),
+                direction=VInt("dir", min=-1, max=1,
+                    docs={"dir": "vote direction. one of (1, 0, -1)"}
+                ),
                 thing=VByName('id'))
     @api_doc(api_section.links_and_comments)
-    def POST_vote(self, dir, thing, vote_info):
+    def POST_vote(self, direction, thing):
         """Cast a vote on a thing.
 
         `id` should be the fullname of the Link or Comment to vote on.
@@ -2182,34 +2163,31 @@ def POST_vote(self, dir, thing, vote_info):
 
         """
 
-        user = c.user
-        store = True
-
         if not thing or thing._deleted:
             return self.abort404()
 
-        hooks.get_hook("vote.validate").call(thing=thing)
+        if not thing.is_votable:
+            abort(400, "That type of thing can't be voted on.")
 
-        if not isinstance(thing, (Link, Comment)):
-            return self.abort404()
+        hooks.get_hook("vote.validate").call(thing=thing)
 
         if isinstance(thing, Link) and promote.is_promo(thing):
             if not promote.is_promoted(thing):
                 return abort(400, "Bad Request")
 
-        if vote_info == 'rejected':
-            reject_vote(thing)
-            store = False
-
-        if thing._age > thing.subreddit_slow.archive_age:
-            store = False
-
-        dir = (True if dir > 0
-               else False if dir < 0
-               else None)
-
-        queries.queue_vote(user, thing, dir, request.ip, vote_info=vote_info,
-            store=store, cheater=c.cheater)
+        if thing.archived:
+            return abort(400,
+                "This thing is archived and may no longer be voted on")
+
+        # convert vote direction to enum value
+        if direction == 1:
+            direction = Vote.DIRECTIONS.up
+        elif direction == -1:
+            direction = Vote.DIRECTIONS.down
+        elif direction == 0:
+            direction = Vote.DIRECTIONS.unvote
+
+        cast_vote(c.user, thing, direction)
 
     @require_oauth2_scope("modconfig")
     @validatedForm(VSrModerator(perms='config'),

r2/r2/controllers/front.py

@@ -40,7 +40,6 @@
 from r2.lib.pages.things import hot_links_by_url_listing
 from r2.lib.pages import trafficpages
 from r2.lib.menus import *
-from r2.lib.admin_utils import check_cheating
 from r2.lib.csrf import csrf_exempt
 from r2.lib.utils import to36, sanitize_url, title_to_url
 from r2.lib.utils import query_string, UrlParser, url_links_builder
@@ -305,8 +304,6 @@ def GET_comments(
             infotext_class = 'locked-infobar'
             infotext_show_icon = True
 
-        check_cheating('comments')
-
         if not c.user.pref_num_comments:
             num = g.num_comments
         elif c.user_is_loggedin and (c.user.gold or sr.is_moderator(c.user)):
@@ -1169,7 +1166,6 @@ def GET_search(self, query, num, reverse, after, count, sort, recent,
                 content = subreddits
                 subreddits = None
 
-        check_cheating("search")
         res = SearchPage(_('search results'), query,
                          content=content,
                          subreddits=subreddits,

r2/r2/controllers/listingcontroller.py

@@ -42,7 +42,6 @@
 from r2.lib.db import queries
 from r2.lib.strings import Score
 from r2.lib.template_helpers import add_sr
-from r2.lib.admin_utils import check_cheating
 from r2.lib.csrf import csrf_exempt
 from r2.lib.utils import (
     extract_user_mentions,
@@ -239,7 +238,6 @@ def rightbox(self):
     @require_oauth2_scope("read")
     @base_listing
     def GET_listing(self, **env):
-        check_cheating('site')
         if self.can_send_referrer():
             c.referrer_policy = "always"
         return self.build_listing(**env)
@@ -927,8 +925,6 @@ def GET_listing(self, where, vuser, sort, time, show, **env):
             self.savedsr = sr
             self.savedcategory = category
 
-        check_cheating('user')
-
         self.vuser = vuser
         self.render_params = {'user' : vuser}
         c.profilepage = True
@@ -1692,7 +1688,6 @@ def GET_user_prefs(self, where, **kw):
             abort(404)
 
         kw['num'] = 0
-        check_cheating('site')
         return self.build_listing(**kw)
 
 
@@ -1774,7 +1769,6 @@ def GET_listing(self, where, user=None, **kw):
         if not self.paginated:
             kw['num'] = 0
 
-        check_cheating('site')
         return self.build_listing(**kw)
 
 

r2/r2/controllers/oauth2.py

@@ -89,7 +89,7 @@ def _error_response(self, state, redirect_uri, as_fragment=False):
             resp["error"] = "unauthorized_client"
         elif (errors.OAUTH2_ACCESS_DENIED, "authorize") in c.errors:
             resp["error"] = "access_denied"
-        elif (errors.BAD_HASH, None) in c.errors:
+        elif (errors.INVALID_MODHASH, None) in c.errors:
             resp["error"] = "access_denied"
         elif (errors.INVALID_OPTION, "response_type") in c.errors:
             resp["error"] = "unsupported_response_type"

r2/r2/controllers/post.py

@@ -109,9 +109,11 @@ def GET_quarantine(self, dest):
         request.environ['usable_error_content'] = errpage.render()
         self.abort403()
 
-    @validate(VModhash(fatal=False),
-              over18 = nop('over18'),
-              dest = VDestination(default = '/'))
+    @csrf_exempt
+    @validate(
+        over18=nop('over18'),
+        dest=VDestination(default='/'),
+    )
     def POST_over18(self, over18, dest):
         if over18 == 'yes':
             if c.user_is_loggedin and not c.errors:

r2/r2/controllers/reddit_base.py

@@ -231,9 +231,6 @@ def _subscribe(self, sr):
     def _unsubscribe(self, sr):
         pass
 
-    def valid_hash(self, hash):
-        return False
-
     def _commit(self):
         if self._dirty:
             for k, (oldv, newv) in self._dirties.iteritems():
@@ -724,6 +721,20 @@ def make_url_https(url):
     return new_url.unparse()
 
 
+def generate_modhash():
+    # OAuth clients should never receive a modhash of any kind as they could
+    # use it in a CSRF attack to bypass their permitted OAuth scopes
+    if c.oauth_user:
+        return None
+
+    modhash = hooks.get_hook("modhash.generate").call_until_return()
+    if modhash is not None:
+        return modhash
+
+    # if no plugins generate a modhash, just use the user name
+    return c.user.name
+
+
 def enforce_https():
     """Enforce policy for forced usage of HTTPS."""
 
@@ -1463,7 +1474,7 @@ def pre(self):
 
         if c.user_is_loggedin:
             self.set_up_user_context()
-            c.modhash = c.user.modhash()
+            c.modhash = generate_modhash()
             c.user_is_admin = maybe_admin and c.user.name in g.admins
             c.user_is_sponsor = c.user_is_admin or c.user.name in g.sponsors
             c.otp_cached = is_otpcookie_valid

r2/r2/lib/admin_utils.py

@@ -1401,8 +1401,6 @@ def perform_actions(self, item, data):
             new_comment.distinguished = "yes"
             new_comment.sendreplies = False
             new_comment._commit()
-            queries.queue_vote(ACCOUNT, new_comment, dir=True, ip=None,
-                send_event=False)
             queries.new_comment(new_comment, inbox_rel)
 
             g.stats.simple_event("automoderator.comment")

r2/r2/lib/automoderator.py

@@ -34,9 +34,6 @@
 def incr_counts(wrapped):
     pass
 
-def incr_sr_count(sr):
-    pass
-
 def get_link_counts(period = count_period):
     links = Link._query(Link.c._date >= utils.timeago(period),
                         limit=50, data = True)