Fix scoreboard tie resolving with 0-point chals (#577) (#578)

* Fix scoreboard tie resolving with 0-point chals
* Closes #577

CTFd/models.py

@@ -210,14 +210,14 @@ class Teams(db.Model):
             db.func.sum(Challenges.value).label('score'),
             db.func.max(Solves.id).label('id'),
             db.func.max(Solves.date).label('date')
-        ).join(Challenges).group_by(Solves.teamid)
+        ).join(Challenges).filter(Challenges.value != 0).group_by(Solves.teamid)
 
         awards = db.session.query(
             Awards.teamid.label('teamid'),
             db.func.sum(Awards.value).label('score'),
             db.func.max(Awards.id).label('id'),
             db.func.max(Awards.date).label('date')
-        ).group_by(Awards.teamid)
+        ).filter(Awards.value != 0).group_by(Awards.teamid)
 
         if not admin:
             freeze = Config.query.filter_by(key='freeze').first()

CTFd/scoreboard.py

@@ -9,19 +9,31 @@ scoreboard = Blueprint('scoreboard', __name__)
 
 
 def get_standings(admin=False, count=None):
+    """
+    Get team standings as a list of tuples containing team_id, team_name, and score e.g. [(team_id, team_name, score)].
+
+    Ties are broken by who reached a given score first based on the solve ID. Two users can have the same score but one
+    user will have a solve ID that is before the others. That user will be considered the tie-winner.
+
+    Challenges & Awards with a value of zero are filtered out of the calculations to avoid incorrect tie breaks.
+    """
     scores = db.session.query(
         Solves.teamid.label('teamid'),
         db.func.sum(Challenges.value).label('score'),
         db.func.max(Solves.id).label('id'),
         db.func.max(Solves.date).label('date')
-    ).join(Challenges).group_by(Solves.teamid)
+    ).join(Challenges)\
+        .filter(Challenges.value != 0)\
+        .group_by(Solves.teamid)
 
     awards = db.session.query(
         Awards.teamid.label('teamid'),
         db.func.sum(Awards.value).label('score'),
         db.func.max(Awards.id).label('id'),
         db.func.max(Awards.date).label('date')
-    ).group_by(Awards.teamid)
+    )\
+        .filter(Awards.value != 0)\
+        .group_by(Awards.teamid)
 
     """
     Filter out solves and awards that are before a specific time point.
@@ -44,7 +56,8 @@ def get_standings(admin=False, count=None):
         db.func.sum(results.columns.score).label('score'),
         db.func.max(results.columns.id).label('id'),
         db.func.max(results.columns.date).label('date')
-    ).group_by(results.columns.teamid).subquery()
+    ).group_by(results.columns.teamid)\
+        .subquery()
 
     """
     Admins can see scores for all users but the public cannot see banned users.

tests/user/test_scoreboard.py

@@ -78,3 +78,155 @@ def test_top_10():
         received = json.loads(response)
         assert saved == received
     destroy_ctfd(app)
+
+
+def test_scoring_logic():
+    """Test that scoring logic is correct"""
+    app = create_ctfd()
+    with app.app_context():
+        admin = login_as_user(app, name="admin", password="password")
+
+        register_user(app, name="user1", email="user1@ctfd.io", password="password")
+        client1 = login_as_user(app, name="user1", password="password")
+        register_user(app, name="user2", email="user2@ctfd.io", password="password")
+        client2 = login_as_user(app, name="user2", password="password")
+
+        chal1 = gen_challenge(app.db)
+        flag1 = gen_flag(app.db, chal=chal1.id, flag='flag')
+        chal1_id = chal1.id
+
+        chal2 = gen_challenge(app.db)
+        flag2 = gen_flag(app.db, chal=chal2.id, flag='flag')
+        chal2_id = chal2.id
+
+        # user1 solves chal1
+        with freeze_time("2017-10-3 03:21:34"):
+            with client1.session_transaction() as sess:
+                data = {
+                    "key": 'flag',
+                    "nonce": sess.get('nonce')
+                }
+                r = client1.post('/chal/{}'.format(chal1_id), data=data)
+
+        # user1 is now on top
+        scores = get_scores(admin)
+        assert scores[0]['team'] == 'user1'
+
+        # user2 solves chal1 and chal2
+        with freeze_time("2017-10-4 03:30:34"):
+            with client2.session_transaction() as sess:
+                # solve chal1
+                data = {
+                    "key": 'flag',
+                    "nonce": sess.get('nonce')
+                }
+                r = client2.post('/chal/{}'.format(chal1_id), data=data)
+                # solve chal2
+                data = {
+                    "key": 'flag',
+                    "nonce": sess.get('nonce')
+                }
+                r = client2.post('/chal/{}'.format(chal2_id), data=data)
+
+        # user2 is now on top
+        scores = get_scores(admin)
+        assert scores[0]['team'] == 'user2'
+
+        # user1 solves chal2
+        with freeze_time("2017-10-5 03:50:34"):
+            with client1.session_transaction() as sess:
+                data = {
+                    "key": 'flag',
+                    "nonce": sess.get('nonce')
+                }
+                r = client1.post('/chal/{}'.format(chal2_id), data=data)
+
+        # user2 should still be on top because they solved chal2 first
+        scores = get_scores(admin)
+        assert scores[0]['team'] == 'user2'
+    destroy_ctfd(app)
+
+
+def test_scoring_logic_with_zero_point_challenges():
+    """Test that scoring logic is correct with zero point challenges. Zero point challenges should not tie break"""
+    app = create_ctfd()
+    with app.app_context():
+        admin = login_as_user(app, name="admin", password="password")
+
+        register_user(app, name="user1", email="user1@ctfd.io", password="password")
+        client1 = login_as_user(app, name="user1", password="password")
+        register_user(app, name="user2", email="user2@ctfd.io", password="password")
+        client2 = login_as_user(app, name="user2", password="password")
+
+        chal1 = gen_challenge(app.db)
+        flag1 = gen_flag(app.db, chal=chal1.id, flag='flag')
+        chal1_id = chal1.id
+
+        chal2 = gen_challenge(app.db)
+        flag2 = gen_flag(app.db, chal=chal2.id, flag='flag')
+        chal2_id = chal2.id
+
+        # A 0 point challenge shouldn't influence the scoreboard (see #577)
+        chal0 = gen_challenge(app.db, value=0)
+        flag0 = gen_flag(app.db, chal=chal0.id, flag='flag')
+        chal0_id = chal0.id
+
+        # user1 solves chal1
+        with freeze_time("2017-10-3 03:21:34"):
+            with client1.session_transaction() as sess:
+                data = {
+                    "key": 'flag',
+                    "nonce": sess.get('nonce')
+                }
+                r = client1.post('/chal/{}'.format(chal1_id), data=data)
+
+        # user1 is now on top
+        scores = get_scores(admin)
+        assert scores[0]['team'] == 'user1'
+
+        # user2 solves chal1 and chal2
+        with freeze_time("2017-10-4 03:30:34"):
+            with client2.session_transaction() as sess:
+                # solve chal1
+                data = {
+                    "key": 'flag',
+                    "nonce": sess.get('nonce')
+                }
+                r = client2.post('/chal/{}'.format(chal1_id), data=data)
+                # solve chal2
+                data = {
+                    "key": 'flag',
+                    "nonce": sess.get('nonce')
+                }
+                r = client2.post('/chal/{}'.format(chal2_id), data=data)
+
+        # user2 is now on top
+        scores = get_scores(admin)
+        assert scores[0]['team'] == 'user2'
+
+        # user1 solves chal2
+        with freeze_time("2017-10-5 03:50:34"):
+            with client1.session_transaction() as sess:
+                data = {
+                    "key": 'flag',
+                    "nonce": sess.get('nonce')
+                }
+                r = client1.post('/chal/{}'.format(chal2_id), data=data)
+
+        # user2 should still be on top because they solved chal2 first
+        scores = get_scores(admin)
+        assert scores[0]['team'] == 'user2'
+
+        # user2 solves a 0 point challenge
+        with freeze_time("2017-10-5 03:55:34"):
+            with client2.session_transaction() as sess:
+                data = {
+                    "key": 'flag',
+                    "nonce": sess.get('nonce')
+                }
+                r = client2.post('/chal/{}'.format(chal0_id), data=data)
+
+        # user2 should still be on top because 0 point challenges should not tie break
+        scores = get_scores(admin)
+        assert scores[0]['team'] == 'user2'
+    destroy_ctfd(app)

tests/user/test_user_facing.py

@@ -320,73 +320,6 @@ def test_user_get_reset_password():
     destroy_ctfd(app)
 
 
-def test_scoring_logic():
-    """Test that scoring logic is correct"""
-    app = create_ctfd()
-    with app.app_context():
-        admin = login_as_user(app, name="admin", password="password")
-
-        register_user(app, name="user1", email="user1@ctfd.io", password="password")
-        client1 = login_as_user(app, name="user1", password="password")
-        register_user(app, name="user2", email="user2@ctfd.io", password="password")
-        client2 = login_as_user(app, name="user2", password="password")
-
-        chal1 = gen_challenge(app.db)
-        flag1 = gen_flag(app.db, chal=chal1.id, flag='flag')
-        chal1_id = chal1.id
-
-        chal2 = gen_challenge(app.db)
-        flag2 = gen_flag(app.db, chal=chal2.id, flag='flag')
-        chal2_id = chal2.id
-
-        # user1 solves chal1
-        with freeze_time("2017-10-3 03:21:34"):
-            with client1.session_transaction() as sess:
-                data = {
-                    "key": 'flag',
-                    "nonce": sess.get('nonce')
-                }
-                r = client1.post('/chal/{}'.format(chal1_id), data=data)
-
-        # user1 is now on top
-        scores = get_scores(admin)
-        assert scores[0]['team'] == 'user1'
-
-        # user2 solves chal1 and chal2
-        with freeze_time("2017-10-4 03:30:34"):
-            with client2.session_transaction() as sess:
-                # solve chal1
-                data = {
-                    "key": 'flag',
-                    "nonce": sess.get('nonce')
-                }
-                r = client2.post('/chal/{}'.format(chal1_id), data=data)
-                # solve chal2
-                data = {
-                    "key": 'flag',
-                    "nonce": sess.get('nonce')
-                }
-                r = client2.post('/chal/{}'.format(chal2_id), data=data)
-
-        # user2 is now on top
-        scores = get_scores(admin)
-        assert scores[0]['team'] == 'user2'
-
-        # user1 solves chal2
-        with freeze_time("2017-10-5 03:50:34"):
-            with client1.session_transaction() as sess:
-                data = {
-                    "key": 'flag',
-                    "nonce": sess.get('nonce')
-                }
-                r = client1.post('/chal/{}'.format(chal2_id), data=data)
-
-        # user2 should still be on top because they solved chal2 first
-        scores = get_scores(admin)
-        assert scores[0]['team'] == 'user2'
-    destroy_ctfd(app)
-
-
 def test_user_score_is_correct():
     '''Test that a user's score is correct'''
     app = create_ctfd()