| 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:03 |
| wgrant | Let me see. | 23:04 |
| 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:05 |
| cjwatson | I figured that was probably buried in flu. | 23:06 |
| blr | thanks cjwatson | 23:06 |
| 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:36 |
| cjwatson | Or as central as possible. | 23:37 |
| wgrant | cjwatson: Sounds reasonable. I don't know how central it can be, but it needs investigation. | 23:41 |
| 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:42 |
| 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:43 |
| cjwatson | Making those async would not be too hard nowadays. | 23:44 |
| cjwatson | I'll have to see whether it's worth it. | 23:44 |
| cjwatson | I always thought sending mail directly from something like a page rendering operation was a bit odd, though. | 23:45 |
| wgrant | Oh, it would certainly be a positive change. | 23:46 |
| wgrant | The question is how much work it is to fix. | 23:46 |
| cjwatson | I could stick an assert getSecurityPolicy() == PermissiveSecurityPolicy into BaseMailer and see how much of the test suite explodes. | 23:47 |
| wgrant | All of +queue, for one. | 23:48 |
| 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:50 |
| 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:51 |
| cjwatson | Bugs notifications are mostly async, except for the send_bug_details_to_new_bug_subscribers case you ran into. | 23:52 |
| 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:53 |
| 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:54 |
| 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:55 |
| 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:56 |
| 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:57 |
| 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> | 23:58 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!