1459713 - Requests to Lando /revisions/D30 in an auth'd context triggering 500's

Status: RESOLVED FIXED

(Conduit :: Lando, defect)

Description

[Jonathan Claudius [:claudijd] (use NEEDINFO)]

I reached out to smacleod in #conduit-private ask perhaps why /revisions/D30 was triggering a 500 and it turns out he was already investigating.  Figured we could use this bug to sync up contexts from both of our views and see if there this is bug (maybe sec, not really sure yet).  At the very least, I think this behavior triggered some behavior that broke some of our expectations, so it's worth cataloging.

So, my testing strategy here was merely to see what sorts of behavior we could elicit by enumerating revision IDs in the /revisions/D{INSERT_INTEGER} pattern.

Basically...

/revisions/D1
/revisions/D2
/revisions/D3
...
/revisions/D499
/revisions/D500

Afterward, I looked through the results and found a series of 200/404's an a single 500 error on /revisions/D30.

After raising this with smacleod he noted that he had already reached out on #conduit asking the following "smacleod wonders who's viewing https://phabricator.services.mozilla.com/D30 in lando... and how the hell it's created by an unkown user".  Because of this, we're getting this bug going to understand it more and whether or not it's an issue worth pursuing.

Comment 1

[Jonathan Claudius [:claudijd] (use NEEDINFO)]

Please note that I was using https://lando.services.mozilla.com/revisions/D{INSERT_INTEGER} pattern here, and I haven't done anything on Phabricator yet.

Comment 2

[Jonathan Claudius [:claudijd] (use NEEDINFO)]

Useful context (note unknown User created this) https://phabricator.services.mozilla.com/D30

Comment 3

[Steven MacLeod [:smacleod]]

A little bit of extra context, sentry information for this request can be found here: https://sentry.prod.mozaws.net/operations/lando-api/issues/4311682/

The author phid the revision provides (which doesn't seem to point at a user) is 'PHID-USER-ssz7qxb7gumdfh3ihxd7'

Based on the bug activity for the bug tied to the revision (Bug 1236720) I'm guessing it was either rrelyea[1] or emaldona[2] who created the revision. Both of their phabricator accounts were created *after* this revision was, so probably one of them was deleted and then created a new account.

[1]https://phabricator.services.mozilla.com/p/rrelyea/
https://bugzilla.mozilla.org/user_profile?login=rrelyea%40redhat.com

[2]https://phabricator.services.mozilla.com/p/emaldona/
https://bugzilla.mozilla.org/user_profile?login=elio.maldonado.batiz%40gmail.com

Comment 4

[Steven MacLeod [:smacleod]]

(In reply to Steven MacLeod [:smacleod] from comment #3)
> [1]https://phabricator.services.mozilla.com/p/rrelyea/
> https://bugzilla.mozilla.org/user_profile?login=rrelyea%40redhat.com
> 
> [2]https://phabricator.services.mozilla.com/p/emaldona/
> https://bugzilla.mozilla.org/user_profile?login=elio.maldonado.batiz%40gmail.
> com

:rrelyea, :emaldona, did either of you create https://phabricator.services.mozilla.com/D30? Did you have your phabricator account deleted?

Comment 5

[Steven MacLeod [:smacleod]]

Alright, after talking to :ckolos it seems that :emaldona had their account deleted after they had created that revision. This is really an edge case and we should probably avoid deleting user accounts in the future.

Comment 6

[Phabricator Automation]

Attachment: revisions: anticipate deleted revision authors (Bug 1459713).

It's possible that a phabricator user account has been deleted, leaving
a user phid pointing at nothing. The revisions endpoint has been updated
to no longer break when the author of a revision has been deleted. The
code for generating reviewer information was already able to handle this
problem.

Comment 7

[Phabricator Automation]

Comment on attachment 8977061 [details]
revisions: anticipate deleted revision authors (Bug 1459713).

Israel Madueme [:imadueme] has approved the revision.

https://phabricator.services.mozilla.com/D1317

Comment 8

[Steven MacLeod [:smacleod]]

https://github.com/mozilla-conduit/lando-api/commit/4d09b498b5d4ef0cc5f60ff5050aa4781510cda6