Balrog accepts scheduled rule changes with duplicate alias'

[bhearsum]

Alias' must be unique, yet the scheduled rule change endpoints accept the idea of a new rule being scheduled with an alias that already exists. I highly doubt these rules would successfully be enacted, but regardless we should not accept their scheduling in the first place.

[Stanzin7]

Hello @gabrielBusta @bhearsum, I am in the contribution phase for Outreachy. Can you please assign this task to me. Thanks )

[godplayer56]

Hey @bhearsum @gabrielBusta , I have some doubts regarding this issue -
If a new rule is being scheduled, no other aliases can schedule the same rule
Am I right about this?

[bhearsum]

Hello @gabrielBusta @bhearsum, I am in the contribution phase for Outreachy. Can you please assign this task to me. Thanks )

Hi, please feel free to work on the issue and open a PR when you feel it is ready.

[bhearsum]

Hey @bhearsum @gabrielBusta , I have some doubts regarding this issue - If a new rule is being scheduled, no other aliases can schedule the same rule Am I right about this?

Yes - only one rule can exist with the same alias at any given time. This works correctly for the rules table and endpoints, which reject duplicate alias'. The scheduled rules table does not have any constraints about unique alias' though (this is expected). We need some safeguards at the API level (https://github.com/mozilla-releng/balrog/blob/main/src/auslib/web/admin/views/rules.py#L188) to implement this.

[godplayer56]

Yes @bhearsum I think I got the fix. I am working on it

[bhearsum]

This is fixed by #3010. @michellemounde - I'm happy to help you work through your own fix for it as well, if you like, but if you'd prefer to focus on something else that's not yet fixed that's fine too.

[michellemounde]

@bhearsum I'll focus on something else

[jcristau]

Reopening because #3010 is being reverted due to causing #3048.

[godplayer56]

@jcristau Should I do some changes to my commit so as to fix this issue? or should I approach it differently?

[bhearsum]

@jcristau Should I do some changes to my commit so as to fix this issue? or should I approach it differently?

I can work with you on this. The first thing we should do is write some additional tests that reproduce these issues.

[bhearsum]

Just to summarize: I think the tests we have for /rules and /scheduled_changes/rules are fine. However, I neglected to notice that we weren't testing /rules/:id and /scheduled_changes/rules/:id when I reviewed the patch.

Here's a commit with a couple of tests as a starting point for this, which both currently fail for different reasons:

    def testPostRuleWithUnchangingAlias(self):
        """Verify that a rule with an alias can be updated, and that providing a value
        for alias that is the same as the current alias does not prevent this."""
    
        ret = self._post(
            "/rules/2",
            data=dict(
                alias="frodo",
                backgroundRate=50,
                data_version=1,
            ),
        )
        self.assertEqual(ret.status_code, 200, "Status Code: %d, Data: %s" % (ret.status_code, ret.get_data()))
        load = ret.get_json()
        self.assertEqual(load["new_data_version"], 2)
        r = dbo.rules.t.select().where(dbo.rules.rule_id == 1).execute().fetchall()
        self.assertEqual(len(r), 1)
>       self.assertEqual(r[0]["alias"], "frodo")
E       AssertionError: None != 'frodo'

tests/admin/views/test_rules.py:682: AssertionError
__ TestRuleScheduledChanges.testAddScheduledChangeExistingRuleUnchangingAlias __
[gw50] linux -- Python 3.9.18 /app/.tox/py39/bin/python

self = <tests.admin.views.test_rules.TestRuleScheduledChanges testMethod=testAddScheduledChangeExistingRuleUnchangingAlias>

    @mock.patch("time.time", mock.MagicMock(return_value=300))
    def testAddScheduledChangeExistingRuleUnchangingAlias(self):
        data = {
            "rule_id": 2,
            "telemetry_product": None,
            "telemetry_channel": None,
            "telemetry_uptake": None,
            "alias": "frodo",
            "backgroundRate": 50,
            "data_version": 1,
            "change_type": "update",
            "when": 1234567,
        }
        ret = self._post("/scheduled_changes/rules", data=data)
>       self.assertEqual(ret.status_code, 200, ret.get_data())
E       AssertionError: 400 != 200 : b'{"type": "about:blank", "title": "Bad Request", "detail": "Rule with alias exists", "status": 400}'

(The second is failing in the same way as reported in #3048, which is a good thing.)

We'll want some additional tests added here for the scheduled changes endpoint that utilizes other change_types, as well as tests that update an existing scheduled change. I expect this would look similar to

balrog/tests/admin/views/test_rules.py

Line 2286 in cbc1977

def testUpdateScheduledChange(self):

, but update rule_id 2 like the test I linked to.

[godplayer56]

Ok got it! I am in a crunch for a couple of days 😅 . I will try to add a test for this as soon as possible 😄