mozilla :: #conduit

10 Aug 2017
13:07imaduemeMorning, going to merge dwalsh's 2 PR's here in an hour or so, wanted to give others a chance to look.
13:21* zalun updated "Lando / Autoland Plan" with some details about pingback security
13:34mcotedkl: maybe you could take a look? I don't know how familiar you are with the ui stuff but would be good with another set of eyes on it
13:38dklmcote: sure thing
13:54imaduemeCool read for everyone on the team I think (you might have seen it already): I don't have anything against github, just opened my eyes to larger scale projects. gps you'd probably love that article.
14:22mcotemight be a couple minutes late for the meeting
14:44dkldavidwalsh: to get at an env var in the php code $bmo_url = PhabricatorEnv::getEnvConfig('bmo_url');
14:45dklwe could store it in the container startup scripts i assume
14:45davidwalshRight, but do we now update the BMO Auth Delegation extension to force using that? Then we'd have it in 2 places
14:55dkldavidwalsh: example
14:56zalunI'm rejoining the room and I'm alone...
14:57mcotezalun: oh did you drop off? I just called the meeting over because no one had anything else to discuss :)
14:57mcotedidn't notice you had disconnected
14:57zalunahh - I got cut off and the meeting ended while rejoining - almost was checking the news
14:58zalunI'm ok with that
14:58zalunmcote: you were mentioning something about running wsgi in one thread one process
14:59imaduemeglob: About the transplant fetching the patch from s3. It makes sense to use s3 to store the static files, but, I was wondering your thoughts on transplant requesting it from Lando API which would then retrieve it directly from Phabricator. That would remove the s3 middleman. Any advantages of having that middleman?
14:59davidwalshdkl: So how are you using a user phid / header so I can check to see if the user has permission to view a bug?
15:06smacleodmcote: we need to upgrade phabricator. Is anyone assigned to verifying our stuff works on that?
15:07mcotewell we have a QA person who is trying to do that but can't since dev is down :)
15:09mcotedkl/davidwalsh: you're using the mozilla-conduit/demo to verify BMO-Phabricator integration, yes?
15:11davidwalshmcote: I've been using mozilla-conduit/bmo-extensions with all of my work because it's preset with the demo SQL file from dkl
15:11davidwalshAnd that's where he's also been doing his BMO work
15:12davidwalshdkl: Can I grab differential options for my auth delegation extension? I don't see how this would work
15:12mcotehm well despite what I just said about finishing up the BMO integration work, maybe you could try upgrading Phabricator and verifying that the extensions still work
15:14davidwalshI guess it appears so
15:18mcoteI mean, or we just upgrade it on dev now and see what happens
15:18mcotenot like we have real users yet
15:18mcoteand if it helps fix the repo problem, we can fix extensions in parallel if we have to
15:19mcotesmacleod: ^
15:19mcoteas in, commit whatever change we need to upgrade to the latest and then deploy that
15:19dklmcote: i vote do it on dev
15:19dklit's been down anyway
15:21dkldavidwalsh: i set it in the test plan build step under harbormaster.
15:21smacleodwe're kinda busy trying to diagnose wth is going on with the repo
15:21smacleodif someone could verify things locally that would be helpful, so we're not piling more broken on top of broken
15:22davidwalshdkl: You set phab-bot's or that individual user's? I need it, programmatically, to ensure the user has permissions to the specific bug
15:23smacleodmcote: ^
15:23mcotesmacleod: is a phab update going to help that? I'm confused where this is coming in
15:25smacleodmcote: it will help with related issues
15:25smacleodthings are broken for weird reasons. I'm diagnosing what went wrong with hg, and why pulls keep failing in a mysterious way
15:26smacleodphabricator does silly things hg - bad bad things
15:26smacleodI can't talk to them about that possibly being an issue unless we upgrade
15:26smacleodas they say "upgrade first"
15:26smacleodamong a bunch of other stuff
15:26mcotehm okay seems like more variables thrown in but I'll take your word on it
15:27smacleodthat's why I'm trying to avoid just upgrading
15:27smacleodbut we need to soon
15:27mcotedavidwalsh: could you handle that then? I'd ask dkl but I really want him to finish his stuff up before he goes on pto
15:27smacleodso I want someone to verify it
15:27mcoteactually i'd probably ask mars but he's also away
15:27* mcote notes to never plan a release of anything in august again
15:28smacleodmcote: nvm, ckolos is going to spin up an entirely new dev box
15:28smacleodwhile we diagnose on the old one
15:29mcotethat seems reasonable
15:29smacleodmcote: ckolos has an open pr upgrading phabricator - we need someone to review it
15:29ckolosthat one ^^
15:29mcotesomeone grab that please
15:29ckolosjust need someone to verify the hashes
15:30davidwalshSo stable is now e0970a7e3d196a7e7f6e99e8616b67f4a3635c3d, it seems
15:31davidwalshOK, so all of those hashes are correct
15:32dklit is called a credential and i am not sure how we would get it from outside of harbormaster
15:43dklckolos: i thought when we commit to mozilla-services/mozphab then gets rebuilt soon after, no
15:51ckolosthought so; that's a function of circle though
15:52ckolosdkl: looks like mozphab was last updated via my branch 2 days ago
15:53ckolosdkl: what are you seeing?
15:53dklckolos: ah docker hub shows by default the scanned tags which shows the last build 2 months ago.
15:54dklif you click unscanned tags at the bottom it shows the one from 2 days ago
15:54ckolosyeah, that's the thing that got me a couple of days ago
15:54* ckolos laughs madle
15:54ckoloser madly
15:54dkldo we not build the mozilla/phabext image automatically if mozilla/mozphab is pushed?
15:55dkli blame your spelling on the madness
15:55dkli misspell for no apparent reason
16:01ckolosdkl: we do not
16:01ckolosthat was left as a tbd
16:02dkldockerhub can do that for us but i do not have admin rights to the org
16:02ckolosif that's what we want to do, that wfm. I can make that change.
16:03dklwe do that for the bmo stuff. although that was with taskcluster. might be different now with circleci
16:05dklckolos: normallywouldnt matter but was wondering why my docs changes are showing up on my dev instance and it is becase phabext is not using the new mozphabyet
16:05ckolosmore likely I haven't done a new deploy with that container yet
16:07davidwalshRight now I'm building mozilla-conduit/demo with ckolos' new hashes, hopefully that lets me test everything
16:23dkldavidwalsh: ok so i figured out the config stuf
16:25dklwe drop that in extensions/bugzilla/config/ and it is picked up
16:25dklwe can add as many options as we want
16:27davidwalshdkl: Added here (, will do that once I get phabricator upgrade checked
16:28dkldavidwalsh: should be a wayfor the auth delegation stuff to just pull its values from the config stuff
16:28davidwalshdkl: Right; I'll remove the "protocol" and "hostname" options from the auth delegation config screen
16:28davidwalshAnd simply use the new config value
16:33dklmay be a good idea to add bugzilla.automation_user and bugzilla.automation_api_key as well
16:34dklthenyou can use it for the bug id verification
16:38davidwalshdkl: I can verify it exists but the api provides a different response code if the requesting user can see it; using God Mode user will be a problem
16:39davidwalshdkl: Didn't you update the BMO api to accept something other than the API key though? The phid or bmo user id?
16:48zalunfake transplant is working
17:01zalunusage in readme -
17:09dkldavidwalsh: yeah i added ability to check the http authorization header but only for the revisions call
17:09dkli did that as a workaround for the harbormaster test plan i use to notify bugzilla
17:09dkldavidwalsh: if you are writing new code, you are better off just using the api key directly
17:13davidwalshdkl: We aren't storing user API keys in phabricator anymore, so the only API key we'd have is automation's, so I dont' believe that allows us to look up the visibility of a bug to the user
17:19dklwe would need to create a new api endpoint on bmo that takes phab-bot login with the users bugzilla login and bug id as arguments.
17:20dklthen check if said user can see the bug and return the reult
17:20dklarg. result
17:20dklthe REST API for BMO requires use of API keys for users and we do not store them
17:22mcoteI wonder if we should switch our approach to be something initiated from BMO
17:22mcotewhen we go to apply the policy
17:22mcoterather than stopping the revision ahead of time
17:22mcotejust let it be posted, but then keep it to admin & author with a comment or such
17:23mcoteI don't really like the idea of having a REST API that returns info like that
17:29davidwalshdkl: Does my issue here make sense or am I overthinking things? (
17:34dklmake sense but have no idea what the issues would be having never done it
17:35davidwalshmcote: We're sort of between a rock and a hard place; storing BMO API keys in Phab seems sort of horrifying, using automation's without new endpoint doens't give us the revision creator's permission to see the bug, and adding a new endpoint isn't to your liking
17:35davidwalshTrying to think of other options
17:36mcoteI don't understand the second point
17:37mcoteif you are creating a policy based on the bug's visibility, can you not get the author's BMO id and see if they can see the bug? all from within the BMO extension?
17:37mcoteyou know the bug and the revision at this point
17:39davidwalshIt was suggested we use our automation user's API key to check if the bug is legit, which we can do, but using that one automation user API key for all cases wont tell us if the the revision poster has permissions to said bug, it would only tell us if the *automation user* has permissions to the bug
17:40davidwalshI presume /rest/bug uses the API key (tied to the user) to check if the user has permissions to a bug
17:40davidwalshBut if our automation user has God Mode permissions for BMO, BMO will always say "yes"
17:41davidwalshLike, if I could pass the user's BMO id to /rest/bug, instead of an API key, this wouldn't be a problem
17:41mcoteright I understand that
17:42mcotething is, you don't want J Random User to be able to check if, say, I have access to a particular bug
17:42davidwalshThus my rock and hard place comment
17:42mcoteso I'm not sure what's wrong with my proposal above
17:43mcoteit sounds like we can't do this on the phabricator side
17:43mcoteso we let the post the revision, restricted to admin & author as usual
17:43mcotethen when we update the policy in the BMO extension, we verify that the author can even see the bug before applying the policies
17:43mcoteactually before even posting the attachment
17:44mcoteso before posting the attachment, query the revision, get the author's BMO ID, and check if they have access to the bug
17:44mcoteif yes, go ahead
17:44mcoteif not, post a comment to the revision saying "you do not have access to the bug so only you can see this revision"
17:45mcoteof course we later need to post the attachment if the user gets CCed or assigned to the bug or such
17:45mcotethat can be an extra check before we start updating policies
17:45davidwalshThat would work, I'm just trying to nip the problem before it fails at harbourmaster
17:46davidwalshOh wow, /rest/bug doesn't require an API key
17:46mcoteno it doesn't; you're considered an anonymous user
17:46mcotethat'll only tell you if a bug is completely public or not
17:46mcoteremind me where harbourmaster falls into this again?
17:47davidwalshharbourmaster is the service that kicks off sending the revision data to BMO and sets policies; correct me if I'm wrong dkl
17:48davidwalshWhich is why I'm trying to nip a problem before it kicks off all of that stuff
17:50circleci-botSuccess: dklawren's build (#62; push) in mozilla-services/mozphab (master) --
17:51ckolosI'm going to do a bump commit to master on phabext, anyone mind?
17:52* ckolos adds blank line to README
17:52mcotedavidwalsh: right, okay, to me it sounds like there's nothing we can do to stop that
17:53mcotewell, I guess we could make this API dkl suggested, but lock it down to just that automation user
17:54mcotewould depend in part on what permissions that user already has. if it's already powerful I guess it doesn't matter; if it's a regular user account, I'd be somewhat hesitant to grant it more powers (though not completely against it)
17:57davidwalshActually...maybe dkl's code already doesn't allow revisions to be posted by people not allowed to see the bug...
17:58mcotethat would be funny
17:58mcoteckolos: do you want to have a meeting? don't want to distract you if I don't have to
18:00ckolosmcote: your call
18:00davidwalshSo there's no explicit check for that at the moment
18:00ckolosif you have things to discuss, let's do so
18:00ckolosotherwise, meh
18:00mcoteI don't
18:00mcoteaside from "what is this all going to be fixed" ;)
18:00mcotebut I assume you don't have the answer to that yet
18:01ckolosthat's what I'm working on now actually
18:01mcoteheh well yes I figured :)
18:01smacleodckolos: soooooo, pretty sure the filesystem is all our problems
18:01davidwalshYeah, looks like no user/group validation is going on before attachment creation
18:01ckolossmacleod: well that's disappointing
18:01smacleodckolos: "hg pull -u" ran ~ 200x as fast
18:01smacleodand just worked
18:02ckolos1:01 < ckolos> smacleod: well that&#39;s disappointing
18:02dkldavidwalsh: attachment creation on the bmo side for the phab revision?
18:03davidwalshdkl: Yes
18:03smacleodckolos: I&#39;m a little baffled at the difference, not sure where to go from here
18:03dkldont need it really as we create the attachment currently as the phab-bot user
18:03dklif you want to create the attachment as the phab user instead we would need to do additional checking
18:04dklright now we just have to make sure the phab-bot user is in all of the bmo groups that we sync
18:04davidwalshdkl: As part of not allowing a user to create a revision for a bug they can&#39;t see due to security, we should make this check
18:04ckolossmacleod: ewll, if what you said earlier is true, then it&#39;s not r/w traffic to do those stat() calls; thus the burst shares don&#39;t get used and everything is essentially a network operation-ish.
18:06ckolossmacleod: maybe the r/w blocksizes are too high on the nfs volume
18:06smacleodckolos: I&#39;m pretty noobish wrt nfs tbh
18:07ckolosstay that way. You&#39;ll feel cleaner.
18:08ckolos28% of historic traffic on that volume is getattr()
18:08ckolos22% is open_noat
18:11smacleodckolos: I&#39;ve shut down all my containers etc. I think I&#39;ve gone as far down this rabbit hole as I can think of at this point
18:11smacleodckolos: let me know if there is anything else I can do to help?
18:13ckolossmacleod: no, I think you&#39;ve been immensely helpful
18:13ckolossounds like a re-architecting might be in order
18:18davidwalshmcote: The more I think about this, the more I think I agree with dkl in creating a new endpoint; isolate it so *only* phab-bot&#39;s API key can use it; it retus a tiny JSON obj that says whether or not the user can see the bug
18:18davidwalshmcote: That way we don&#39;t have broken harbourmaster builds and dead revisions in phab
18:18dklfor the time being it&#39;s a decent fix
18:18davidwalshi.e. they cannot &quot;communicate&quot; with BMO in any way because their revision never hits our radar
18:19mcotedylan: thoughts? ^
18:22davidwalshsmacleod: You posted a few links to phab that were 502&#39;ing due to a bug number issue; how did you trigger that?
18:23smacleoddavidwalsh: huh? I don&#39;t remember what you&#39;re talkinga bout
18:24davidwalshsmacleod: When you first brought up the issue of us not validating bug number, you posted a few links with it, where I was getting 502 errors
18:24smacleoddavidwalsh: these links?:
18:25smacleoddavidwalsh: the 502 is because the dev server is down
18:25davidwalshsmacleod: Can you tell me what was showing at those endpoints before it was down?
18:25smacleoddavidwalsh: okay, so what happens is harbourmaster build plan runs to tell BMO to change the revisions permissions
18:26smacleodbut BMO says the bug doesn&#39;t exist, so the build plan fails
18:26smacleodand the revision is left in limbo with admin only
18:26davidwalshOK, same locally
18:26davidwalshJust making sure we&#39;re on the same page, I have all the info
18:26* smacleod nods
18:26smacleodya, so we need to prevent this from ever happening
18:26smacleodby preventing posting invalid bug revisions
18:26smacleodor have some sort of followup, notification, fix or something
18:27dylanspecial purpose API? that sounds reasonable
18:36mcoteokay that might be the easiest then
19:03mcotedavidwalsh: if you are going that route please create some cards
19:03davidwalshJust did
19:09dkldavidwalsh: in hind sight the BMO code that updates the perms on new revision (test plan), I should be adding a comment to the revision if the bug id doesn&#39;t exist or they cannot see it.
19:10dkldavidwalsh: but if we block invalid bug ids from the beginning that wont matter
19:10davidwalshdkl: Right, I want to prevent all of these problems up front
19:10davidwalsh&quot;Do it the right way&quot; imo
19:10davidwalshBut once we get time, coming back and adding a bit of extra validation inside your revision() endpoint may be good
19:21ckolosall: mozphab-dev is back up
19:21ckolosphabricator is chewing through the repo now
19:21mcoteawesome good stuff
19:38imadueme+1 to ckolos :)
19:38ckolosdid y&#39;all see the build failures for the phabext image?
19:39dklckolos: was that my pull request failing?
19:39ckolosno, b/c even my bump commit failed
19:39dkli might have failed to rebuild the map file
19:39ckolosThe library map is out of date
19:39dklah failing on master
19:47ckolosyeah, if the mozphab:latest is bumped, then that library map will need to be rebuilt
19:47ckolosnow I remember why we didn&#39;t wire up auto-builds.
21:10botond|homeThe dev instance of Phabricator started spamming me with email :)
21:21Pikei&#39;ve created an account on out of curiousity, and now found a ton of mail about landed bugs in my inbox. is that known fall-out or is creating an account on .dev. a stupid idea to begin with?
22:17dklckolos: so weird, somehow the new phabricator code on the mozilla/mozphab is out of sync with the phutil_map file we generate in the mozilla/phabext image so that explains the circleci error but I am not having luck updating the map file. will try some more tonight
22:17dklzalun: unless you can look at this before me. The new phabricator code has some new classes that are mssing from the map file we are using.
22:29davidwalshdkl|afk: Seeing that too. Additionally, if I /bin/bash into the phabricator container of bmo-extensions, and add PHP files, they don&#39;t autoload or update. Possibly the same issue?
23:18mcoteStill some missing pieces but it&#39;s a start
23:23mcoteHm trying to figure out how to rename it to mozilla-conduit...
11 Aug 2017
No messages
Last message: 11 days and 14 hours ago