| lifeless | cjwatson: hi | 01:15 |
|---|---|---|
| lifeless | cjwatson: lazr.restful walks the declared attributes of its results | 01:16 |
| lifeless | cjwatson: catching permission errors and eliding those things, whatever they are. | 01:16 |
| cjwatson | OK | 01:16 |
| lifeless | cjwatson: so for a collection, it materializes the batch, then iterates | 01:16 |
| lifeless | if you have objects that have permission checks which require db lookups, you end up calling whatever logic is needed for each object -> very slow. | 01:17 |
| cjwatson | Got it, I think. EntryResource.redacted_fields? | 01:18 |
| lifeless | Person._teamPrivacyQuery is likely what you want to use, however I'm stale on the detail; suggest you consult with purple (e.g. wgrant) about the current state of the art | 01:18 |
| wgrant | lifeless, cjwatson: Well, this is an... interesting... case | 01:21 |
| wgrant | Times you don't want to filter stuff out: | 01:21 |
| wgrant | - auditing permissions | 01:21 |
| lifeless | cjwatson: So, I haven't read the plumbing in lazr.restful, but that sounds likely. | 01:21 |
| lifeless | wgrant: interacting w/public object -> discloses team. | 01:22 |
| wgrant | lifeless: Sure | 01:22 |
| wgrant | lifeless: this is an interaction with a public object | 01:22 |
| lifeless | wgrant: so if we add the rule, add it to the relevant core query, then join back on that. | 01:22 |
| wgrant | So it discloses the team | 01:22 |
| wgrant | So filtering based on privacy is wrong | 01:22 |
| wgrant | lifeless: no, that is impractical | 01:22 |
| wgrant | We can't just add all 90 FKs to the privacy check | 01:22 |
| wgrant | However | 01:23 |
| wgrant | I suspect that private team linkage checks might forbid ArchivePermissions | 01:23 |
| cjwatson | Is this a case where you should be forbidden from creating such APs in the first place? | 01:24 |
| wgrant | Right, I suspect it may already be | 01:24 |
| wgrant | Although the vocab is ValidPersonOrTeam, so maybe not | 01:24 |
| wgrant | lifeless: The private team security adapter is already very slow. We need to make it faster, not slower. | 01:25 |
| cjwatson | I rather suspect there are none right now and we could (a) check that quickly (b) forbid it henceforth. | 01:25 |
| wgrant | We eventually shouldn't forbid it, but it's the right thing to do now, until we have private team privacy sorted out | 01:26 |
| lifeless | so derived distros will want private teams | 01:27 |
| cjwatson | I'm not entirely convinced that a public archive would ever want to have upload permissions associated with it for a private team. | 01:27 |
| lifeless | do AP's apply to ppas ? | 01:27 |
| wgrant | lifeless: They do. | 01:27 |
| lifeless | and we have private teams with private ppas | 01:28 |
| wgrant | Yes. | 01:28 |
| wgrant | But they don't necessarily have APs | 01:28 |
| wgrant | For PPAs, the archive owner has implicit access | 01:28 |
| lifeless | so I believe this invalidates any assertion about public being the only valid choice in the system | 01:28 |
| cjwatson | I didn't assert that | 01:28 |
| cjwatson | Would the owner of a private PPA want to grant upload permissions to a team whose membership they can't see? | 01:28 |
| cjwatson | Surely not. | 01:28 |
| wgrant | cjwatson: They can see it | 01:29 |
| lifeless | cjwatson: no, but thats != not using private teams | 01:29 |
| wgrant | cjwatson: Because they're in the team | 01:29 |
| cjwatson | Indeed | 01:29 |
| lifeless | or they own it | 01:29 |
| cjwatson | Hence showing that team => not a problem | 01:29 |
| lifeless | and the implicit visibility rule says that they should be able to see it afterwards anyway. | 01:29 |
| cjwatson | I assert that if you can see the archive, you should be able to see the teams with permissions to upload to it | 01:29 |
| lifeless | so its fine, I think, to say that this API can force showing everything. | 01:29 |
| wgrant | cjwatson: The rule is that you can see the name, displayname and owner | 01:29 |
| wgrant | cjwatson: You can't see the membership | 01:29 |
| lifeless | but we need the reflexive side to work as well. | 01:29 |
| cjwatson | And that if that isn't true, we've made a mistake in allowing those permissions to be granted in the first place | 01:30 |
| lifeless | which is to say that if you see a team in the API you need to be able to see the team directly as well (limitedview) | 01:30 |
| cjwatson | (Note this API is only usable by people who can see the archive) | 01:30 |
| lifeless | sure | 01:31 |
| lifeless | I think wgrant and I are riffing about how to make sure the reflexive side works, more than anything. | 01:31 |
| wgrant | And we have strongly disagreed on the approach in the past :) | 01:31 |
| lifeless | that, and that the embedded team object in the API result needs to be appropriately neutered. | 01:31 |
| wgrant | lifeless: The team shouldn't be embedded, I don't think | 01:32 |
| lifeless | wgrant: think of it as a challenge to your ingenuity | 01:32 |
| wgrant | It'll include a link | 01:32 |
| wgrant | And should only check LimitedView for that | 01:32 |
| lifeless | wgrant: I'd like to be confident of that | 01:32 |
| wgrant | lifeless: Well | 01:32 |
| lifeless | wgrant: a test, even if thrown away afterwards, would do that. | 01:32 |
| wgrant | The way you become confident is to only grant LimitedView | 01:32 |
| wgrant | Not View | 01:32 |
| lifeless | wgrant: sure, +1 | 01:32 |
| cjwatson | So, aside from the angle of avoiding unwanted disclosure or not, lifeless pointed out that not doing some kind of privacy check up front appears to be a performance issue | 01:34 |
| wgrant | Well | 01:35 |
| lifeless | history tells us :> | 01:35 |
| wgrant | As long as you preload the people themselves, it won't be a performance issue unless there are private teams. | 01:35 |
| cjwatson | If we're not filtering, then presumably we at least need to preload | 01:35 |
| cjwatson | Right | 01:35 |
| lifeless | there is a 'Person' eager loader which does the right thing for teams too | 01:35 |
| wgrant | (Since the privacy check on any person other than a private team is a trivial inline attribute access) | 01:35 |
| lifeless | so using that + injecting LimitedView onto everything should be sufficient | 01:35 |
| cjwatson | i.e. precache_permission_for_objects? | 01:37 |
| wgrant | Right | 01:38 |
| cjwatson | wgrant: Does my proposal in bug 745799 look awful? | 01:49 |
| _mup_ | Bug #745799: DistroSeries:+queue Timeout accepting packages (bug structural subscriptions) <timeout> <Launchpad itself:Triaged> < https://launchpad.net/bugs/745799 > | 01:49 |
| cjwatson | It's kind of a hack around structsubs being insane, but ... | 01:50 |
| wgrant | cjwatson: "awful"? Yes. "necessary anyway"? Probably. I need to look at related structsub performance issues with StevenK on Monday, but I suspect the fix will be sufficiently uninvasive that it doesn't fix your issue. | 01:58 |
| cjwatson | The current implementation is sufficiently recursive that I couldn't see a way to improve it without ripping much of it to shreds | 02:00 |
| wgrant | Yeah | 02:01 |
| wgrant | It's reasonably likely that the refactoring that we require will make it substantially less bad. | 02:01 |
| wgrant | But it probably won't be reliably good enough for your uses. | 02:01 |
| cjwatson | Seeing as the list of bugs to close can be arbitrarily long, indeed | 02:02 |
| wgrant | Right | 02:02 |
| wgrant | We're only really interested in optimising for the single bug case. | 02:03 |
| wgrant | And that's all that's feasible to do at this stage without massive refactoring. | 02:03 |
| cjwatson | If you'd feel like allocating me a DB patch number, I can start with moving the review process for this into merge proposals and see what people think there | 02:03 |
| wgrant | cjwatson: Why's distroseries nullable/ | 02:04 |
| wgrant | You could interpret that to mean upload_distroseries | 02:04 |
| wgrant | But that seems pointless. | 02:05 |
| cjwatson | cf. close_bugs_for_sourcepackagerelease | 02:05 |
| cjwatson | It's upload_distroseries in my current imp | 02:05 |
| cjwatson | Because otherwise I had to pass either a DSP or an SP, or whatever the objects are | 02:05 |
| wgrant | cjwatson: Sure, but I don't see much benefit in having the job use SPR.upload_distroseries, rather than forcing the job creators to always pass a series. | 02:07 |
| wgrant | SPR.upload_{distroseries,archive} are abominations. We should restrict their callsites as much as possible. | 02:08 |
| cjwatson | Ah, well that's possible yes | 02:08 |
| cjwatson | It's just close_bugs_for_queue_item, and that could use queue_item.distroseries | 02:08 |
| cjwatson | No idea why it doesn't | 02:08 |
| wgrant | Right | 02:09 |
| wgrant | It's only the initial non-copy PackageUpload stuff which should be using upload_* at all | 02:09 |
| wgrant | Everything else should use it from a publication | 02:09 |
| cjwatson | And "upload_distroseries" in processaccepted is actually SPPH.distroseries, not .upload_distroseries | 02:09 |
| cjwatson | The naming lies | 02:09 |
| wgrant | So keeping the upload_blah in queue.py wins for everyone | 02:09 |
| wgrant | Yeah, because when the function was originally written it was always the upload distroseries. | 02:09 |
| cjwatson | So yes, that can and should be non-nullable. | 02:10 |
| wgrant | Great | 02:11 |
| wgrant | Limiting the propagation of mistakes from 2005 is good :) | 02:11 |
| wgrant | I shall give you a number. | 02:11 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!