1896389 - Uplift approval granted by non-relman revision review

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: Phabricator Integration, defect)

Description

[Donal Meehan [:dmeehan]]

Scenario:

• This patch was targeting the rESRONEONEFIVE mozilla-esr115 repository
• The corresponding bug had a pending approval-mozilla-esr115?
• The patch was approved by someone outside the release-managers group

Expected results:

• The revision is approved in Phabricator and there is no further action

Actual results:

• The revision was approved in Phabricator and the uplift request was also marked as approved.

Comment 1

[Connor Sheehan [:sheehan]]

The Bugzilla attachment approval flags are set in BMO's phab-bot, and we do have some code that does a "check if this change is allowed" check here. I think either one of two things has happened here. Either:

• jcristau has permissions to change the flag, possibly due to his previous tenure in RelMan.
• The can_change_flag function only checks if someone has the permissions to set the flag to any value, ie it checks if they are capable of approval-mozilla-beta? and returns true in that case.

I expected BMO would disallow non-RelMan users from setting that flag to + or -, either in that checking function or later when the flag is changed with $attachment->set_flags. I think what we should do here is add some extra logic to only set the value to + or - when the flag_setter is a member of the #release-managers Phabricator group.

Comment 2

[Connor Sheehan [:sheehan]]

After some discussion, it looks like the permission necessary to set the flag is mozilla-next-drivers in BMO. Since jcristau is a member of this group, the check for can_change_flag passed and he was able to update the flag. We should add a check that the $flag_setter is a member of the Phabricator #release-managers project before allowing the flag state to be changed to + or -.

Comment 3

[BMO Github Automation]

Attachment: [mozilla-bteam/bmo] Bug 1896389 - Uplift approval granted by non-relman revision review (#2247)

Comment 4

[David Lawrence [:dkl]]

Merged
https://github.com/mozilla-bteam/bmo/commit/9f1ac397a96aabc1ecb83f17ff5ee5a660de4508
Will be in next deployment, probably May 28th.