bug 1384269: Fix revocation when more than one signoff has already ha…

…ppened. (#356) r=nthomas

auslib/test/admin/views/test_rules.py

@@ -918,6 +918,7 @@ def setUp(self):
         dbo.rules.scheduled_changes.conditions.history.t.insert().execute(change_id=2, changed_by="bill", timestamp=6, sc_id=3, when=2000000, data_version=1)
         dbo.rules.scheduled_changes.conditions.history.t.insert().execute(change_id=3, changed_by="bill", timestamp=10, sc_id=3, when=2900000, data_version=2)
         dbo.rules.scheduled_changes.signoffs.t.insert().execute(sc_id=3, username="bill", role="releng")
+        dbo.rules.scheduled_changes.signoffs.t.insert().execute(sc_id=3, username="mary", role="relman")
 
         dbo.rules.scheduled_changes.t.insert().execute(
             sc_id=4, scheduled_by="bill", data_version=2, complete=True, base_rule_id=5, base_priority=80, base_version="3.3",
@@ -1007,7 +1008,7 @@ def testGetScheduledChanges(self):
                     "buildTarget": None, "alias": None, "product": None, "buildID": None, "locale": None, "osVersion": None,
                     "distribution": None, "fallbackMapping": None, "distVersion": None, "headerArchitecture": None, "comment": None,
                     "data_version": None, "instructionSet": None, "telemetry_product": None, "telemetry_channel": None, "telemetry_uptake": None,
-                    "change_type": "insert", "signoffs": {"bill": "releng"}, "required_signoffs": {"releng": 1},
+                    "change_type": "insert", "signoffs": {"bill": "releng", "mary": "relman"}, "required_signoffs": {"releng": 1},
                 },
                 {
                     "sc_id": 5, "when": 600000, "scheduled_by": "bill", "complete": False, "sc_data_version": 1, "rule_id": 4, "priority": None,
@@ -1064,7 +1065,7 @@ def testGetScheduledChangesWithCompleted(self):
                     "buildTarget": None, "alias": None, "product": None, "buildID": None, "locale": None, "osVersion": None, "memory": None,
                     "distribution": None, "fallbackMapping": None, "distVersion": None, "headerArchitecture": None, "comment": None,
                     "data_version": None, "instructionSet": None, "telemetry_product": None, "telemetry_channel": None, "telemetry_uptake": None,
-                    "change_type": "insert", "signoffs": {"bill": "releng"}, "required_signoffs": {"releng": 1},
+                    "change_type": "insert", "signoffs": {"bill": "releng", "mary": "relman"}, "required_signoffs": {"releng": 1},
                 },
                 {
                     "sc_id": 4, "when": 500000, "scheduled_by": "bill", "complete": True, "sc_data_version": 2, "rule_id": 5, "priority": 80,
@@ -1331,7 +1332,7 @@ def testUpdateScheduledChangeResetSignOffs(self):
         }
         rows = dbo.rules.scheduled_changes.signoffs.t.select().where(
             dbo.rules.scheduled_changes.signoffs.sc_id == 3).execute().fetchall()
-        self.assertEquals(len(rows), 1)
+        self.assertEquals(len(rows), 2)
         ret = self._post("/scheduled_changes/rules/3", data=data)
         self.assertEquals(ret.status_code, 200, ret.data)
 
@@ -1500,9 +1501,10 @@ def testSignoffASecondTimeWithSameRole(self):
         ret = self._post("/scheduled_changes/rules/3/signoffs", data=dict(role="releng"), username="bill")
         self.assertEquals(ret.status_code, 200, ret.data)
         r = dbo.rules.scheduled_changes.signoffs.t.select().where(dbo.rules.scheduled_changes.signoffs.sc_id == 3).execute().fetchall()
-        self.assertEquals(len(r), 1)
-        db_data = dict(r[0])
-        self.assertEquals(db_data, {"sc_id": 3, "username": "bill", "role": "releng"})
+        self.assertEquals(len(r), 2)
+        db_data = [dict(row) for row in r]
+        self.assertIn({"sc_id": 3, "username": "bill", "role": "releng"}, db_data)
+        self.assertIn({"sc_id": 3, "username": "mary", "role": "relman"}, db_data)
 
     def testSignoffWithSecondRole(self):
         ret = self._post("/scheduled_changes/rules/3/signoffs", data=dict(role="qa"), username="bill")
@@ -1512,8 +1514,16 @@ def testRevokeSignoff(self):
         ret = self._delete("/scheduled_changes/rules/3/signoffs", username="bill")
         self.assertEquals(ret.status_code, 200, ret.data)
         r = dbo.rules.scheduled_changes.signoffs.t.select().where(dbo.rules.scheduled_changes.signoffs.sc_id == 3).execute().fetchall()
-        self.assertEquals(len(r), 0)
+        self.assertEquals(len(r), 1)
+
+    def testRevokeOtherUsersSignoffAsAdmin(self):
+        # The username passed to _delete is the user performing the action.
+        # The username in the query string is the person who's signoff we want to revoke.
+        ret = self._delete("/scheduled_changes/rules/3/signoffs", username="bill", qs={"username": "mary"})
+        self.assertEquals(ret.status_code, 200, ret.data)
 
-    def testRevokeOtherUsersSignoff(self):
-        ret = self._delete("/scheduled_changes/rules/3/signoffs", username="bob")
+    def testCannotRevokeOtherUsersSignoffAsNormalUser(self):
+        # The username passed to _delete is the user performing the action.
+        # The username in the query string is the person who's signoff we want to revoke.
+        ret = self._delete("/scheduled_changes/rules/3/signoffs", username="julie", qs={"username": "mary"})
         self.assertEquals(ret.status_code, 403, ret.data)

auslib/web/admin/views/scheduled_changes.py

@@ -164,16 +164,17 @@ def _post(self, sc_id, transaction, changed_by):
 
     @requirelogin
     def _delete(self, sc_id, transaction, changed_by):
-        where = {"sc_id": sc_id}
-        signoff = self.signoffs_table.select(where, transaction)
-        if not signoff:
-            return Response(status=404, response="{} has no signoff to revoke".format(changed_by))
-
         form = Form(request.args)
         if not form.validate():
             self.log.warning("Bad input: %s", form.errors)
             return Response(status=400, response=json.dumps(form.errors))
 
+        username = request.args.get("username", changed_by)
+        where = {"sc_id": sc_id, "username": username}
+        signoff = self.signoffs_table.select(where, transaction)
+        if not signoff:
+            return Response(status=404, response="{} has no signoff to revoke".format(changed_by))
+
         self.signoffs_table.delete(where, changed_by=changed_by, transaction=transaction)
         return Response(status=200)