1478898 - ensure bug-id validation works for revisions submitted with arc

Status: RESOLVED INVALID

(Conduit :: Phabricator, enhancement, P1)

Description

[:glob ✱]

in https://phabricator-dev.allizom.org/D555 i used arc to submit with bug id set to 1.

this should have been rejected at submission time.

Comment 1

[David Lawrence [:dkl]]

This is actually a false alarm. globs account is an admin on the bugzilla-dev server and the code is written that we skip bug id validation if the author is an admin. So it just let him use whatever bug id he chose as long as it was numeric.

But i will still fix the other bug where the feed daemon to handle invalid bug ids properly on its end. I am thinking that if an invalid bug is found by feed daemon, we leave the revision private, and add a comment to the revision stating that the bug id is invalid and let the author then change it themselves. It's really the only thing we can do. We cannot clear the bug id field and make it public and we also would not know which security policy to apply to it. We already do something similar when a bug is private to groups not preset in Phabricator projects.

Comment 2

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #1)
> This is actually a false alarm. globs account is an admin on the
> bugzilla-dev server and the code is written that we skip bug id validation
> if the author is an admin. So it just let him use whatever bug id he chose
> as long as it was numeric.

why does this override exist?  it sounds like a footgun that should be removed.

Comment 3

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› 🎈 from comment #2)
> why does this override exist?  it sounds like a footgun that should be
> removed.

https://github.com/mozilla-services/phabricator-extensions/blob/master/extensions/src/differential/customfield/DifferentialBugzillaBugIDValidator.php#L35-L46

Comment 4

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #3)
> (In reply to Byron Jones ‹:glob› 🎈 from comment #2)
> > why does this override exist?  it sounds like a footgun that should be
> > removed.
> 
> https://github.com/mozilla-services/phabricator-extensions/blob/master/
> extensions/src/differential/customfield/DifferentialBugzillaBugIDValidator.
> php#L35-L46

    // Check to see if the user is an admin; if so, don't validate bug existence
    // because the admin account may not have a BMO account ID associated with it
    // and we don't want to block admins from making revisions private
    // if BMO is down

ah..

> (In reply to David Lawrence [:dkl] from comment #1)
> > This is actually a false alarm. globs account is an admin on the
> > bugzilla-dev server

this override exists for phabricator admins, not bugzilla admins.


to mirror the setup on prod, i've created a glob-admin account on phab-dev and transferred my admin rights to that account.

Comment 5

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› 🎈 from comment #4)
> > (In reply to David Lawrence [:dkl] from comment #1)
> > > This is actually a false alarm. globs account is an admin on the
> > > bugzilla-dev server
> 
> this override exists for phabricator admins, not bugzilla admins.

Sorry that was a mis-type on my part. I meant phabricator-dev :(