[23:03] <cjwatson> wgrant: Have you had a chance to figure out how to QA https://bugs.launchpad.net/launchpad/+bug/1489674 ?
[23:03] <mup> Bug #1489674: BugNotificationBuilder.build crashes with Unauthorized on invisible private teams <403> <fallout> <qa-needstesting> <regression> <Launchpad itself:Fix Committed by wgrant> <https://launchpad.net/bugs/1489674>
[23:03] <wgrant> cjwatson: Oh yeah, I should try that.
[23:04] <wgrant> Let me see.
[23:05] <cjwatson> wgrant: Thanks
[23:05] <cjwatson> blr: Were you going to land https://code.launchpad.net/~blr/launchpad/diff-code-select-bug-1483925/+merge/268017 ?
[23:05] <blr> cjwatson: oh, forgot about that. Yes I should do
[23:06] <cjwatson> I figured that was probably buried in flu.
[23:06] <blr> thanks cjwatson
[23:36] <cjwatson> wgrant: A quick test suggests that there are various BaseMailer call sites that could trigger this too, for example modifying a branch in the web UI that has a subscription from an inaccessible private team, so I think my first job tomorrow is going to be figuring out a central fix for that.
[23:37] <cjwatson> Or as central as possible.
[23:41] <wgrant> cjwatson: Sounds reasonable. I don't know how central it can be, but it needs investigation.
[23:42] <cjwatson> Maybe BaseMailer needs to grow a rule that all uses of it need to be zopeless-style.  Fixing this "centrally" would still involve sprinkling rSP over all the various constructors.
[23:42] <wgrant> Zopeless-style?
[23:42] <cjwatson> Jobs, scripts, or whatever.  Not with a user interaction.
[23:42] <cjwatson> Whatever the correct term is.
[23:43] <wgrant> Ah, right. The defining feature is PermissiveSecurityPolicy.
[23:43] <wgrant> But that makes it rather difficult to use from the large number of email callsites that aren't presently asynchronous, unfortunately.
[23:43] <cjwatson> That's the one, thanks.
[23:44] <cjwatson> Making those async would not be too hard nowadays.
[23:44] <cjwatson> I'll have to see whether it's worth it.
[23:45] <cjwatson> I always thought sending mail directly from something like a page rendering operation was a bit odd, though.
[23:46] <wgrant> Oh, it would certainly be a positive change.
[23:46] <wgrant> The question is how much work it is to fix.
[23:47] <cjwatson> I could stick an assert getSecurityPolicy() == PermissiveSecurityPolicy into BaseMailer and see how much of the test suite explodes.
[23:48] <wgrant> All of +queue, for one.
[23:50] <cjwatson> From a quick scan I think the currently affected mailers are Branch, BranchMergeProposal, GitRepository, and PackageUpload.  All the rest are only used from jobs or scripts.
[23:51] <wgrant> oh right, answers is async nowadays, isn't it.
[23:51] <cjwatson> Answers doesn't use BaseMailer (yet).
[23:51] <wgrant> oh
[23:51] <cjwatson> But I think its mail notifications are indeed async.
[23:52] <cjwatson> Bugs notifications are mostly async, except for the send_bug_details_to_new_bug_subscribers case you ran into.
[23:53] <wgrant> If by "mostly async" you mean "lies"
[23:53] <wgrant> The footer is calculated in the webapp.
[23:53] <wgrant> Very, very expensively.
[23:53] <cjwatson> Right, but only in the new-bug case.
[23:53] <wgrant> Most bug edit timeouts go away if we make notification sending properly async.
[23:53] <cjwatson> Er, new-bug-subscriber.
[23:53] <wgrant> Nope.
[23:54] <wgrant> The webapp populates BugNotificationRecipient
[23:54] <cjwatson> Oh, yikes.
[23:54] <wgrant> So the POST request calculates the set of people who should be notified and inserts a row for each of them, including the rationale text and footer etc.
[23:55] <wgrant> The correct solution is of course a job that says "this change happened to the bug when it was in this state, now please populate bugnotification"
[23:56] <cjwatson> I'll have a look at the BaseMailer cases tomorrow at least.  Would like to spend finite time in this swamp, but draining a bit of it is a prereq for landing team-mail safely
[23:56] <cjwatson> (I strongly suspect)
[23:56] <cjwatson> Anyway, bed.  Night
[23:56] <wgrant> Night.
[23:56] <wgrant> You weren't even here today, were you?
[23:56] <cjwatson> Nope.  And mostly actually not, just nipping in briefly
[23:57] <wgrant> If you can make it all PermissiveSecurityPolicy then that indeed makes things substantially simpler.
[23:57] <cjwatson> Code has a suitable job framework already, so probably easy.  Soyuz might need a bit of fiddling.
[23:57] <wgrant> Yup
[23:58] <wgrant> Though there's actually already a bug about the Soyuz one.
[23:58] <wgrant> https://bugs.launchpad.net/launchpad/+bug/566339
[23:58] <mup> Bug #566339: Cannot accept package which would notify private email addresses <boobytrap> <email> <lp-soyuz> <notifications> <Launchpad itself:Triaged> <https://launchpad.net/bugs/566339>