/srv/irclogs.ubuntu.com/2012/08/04/#launchpad-dev.txt

lifelesscjwatson: hi01:15
lifelesscjwatson: lazr.restful walks the declared attributes of its results01:16
lifelesscjwatson: catching permission errors and eliding those things, whatever they are.01:16
cjwatsonOK01:16
lifelesscjwatson: so for a collection, it materializes the batch, then iterates01:16
lifelessif 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
cjwatsonGot it, I think.  EntryResource.redacted_fields?01:18
lifelessPerson._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 art01:18
wgrantlifeless, cjwatson: Well, this is an... interesting... case01:21
wgrantTimes you don't want to filter stuff out:01:21
wgrant - auditing permissions01:21
lifelesscjwatson: So, I haven't read the plumbing in lazr.restful, but that sounds likely.01:21
lifelesswgrant: interacting w/public object -> discloses team.01:22
wgrantlifeless: Sure01:22
wgrantlifeless: this is an interaction with a public object01:22
lifelesswgrant: so if we add the rule, add it to the relevant core query, then join back on that.01:22
wgrantSo it discloses the team01:22
wgrantSo filtering based on privacy is wrong01:22
wgrantlifeless: no, that is impractical01:22
wgrantWe can't just add all 90 FKs to the privacy check01:22
wgrantHowever01:23
wgrantI suspect that private team linkage checks might forbid ArchivePermissions01:23
cjwatsonIs this a case where you should be forbidden from creating such APs in the first place?01:24
wgrantRight, I suspect it may already be01:24
wgrantAlthough the vocab is ValidPersonOrTeam, so maybe not01:24
wgrantlifeless: The private team security adapter is already very slow. We need to make it faster, not slower.01:25
cjwatsonI rather suspect there are none right now and we could (a) check that quickly (b) forbid it henceforth.01:25
wgrantWe eventually shouldn't forbid it, but it's the right thing to do now, until we have private team privacy sorted out01:26
lifelessso derived distros will want private teams01:27
cjwatsonI'm not entirely convinced that a public archive would ever want to have upload permissions associated with it for a private team.01:27
lifelessdo AP's apply to ppas ?01:27
wgrantlifeless: They do.01:27
lifelessand we have private teams with private ppas01:28
wgrantYes.01:28
wgrantBut they don't necessarily have APs01:28
wgrantFor PPAs, the archive owner has implicit access01:28
lifelessso I believe this invalidates any assertion about public being the only valid choice in the system01:28
cjwatsonI didn't assert that01:28
cjwatsonWould the owner of a private PPA want to grant upload permissions to a team whose membership they can't see?01:28
cjwatsonSurely not.01:28
wgrantcjwatson: They can see it01:29
lifelesscjwatson: no, but thats != not using private teams01:29
wgrantcjwatson: Because they're in the team01:29
cjwatsonIndeed01:29
lifelessor they own it01:29
cjwatsonHence showing that team => not a problem01:29
lifelessand the implicit visibility rule says that they should be able to see it afterwards anyway.01:29
cjwatsonI assert that if you can see the archive, you should be able to see the teams with permissions to upload to it01:29
lifelessso its fine, I think, to say that this API can force showing everything.01:29
wgrantcjwatson: The rule is that you can see the name, displayname and owner01:29
wgrantcjwatson: You can't see the membership01:29
lifelessbut we need the reflexive side to work as well.01:29
cjwatsonAnd that if that isn't true, we've made a mistake in allowing those permissions to be granted in the first place01:30
lifelesswhich 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
lifelesssure01:31
lifelessI think wgrant and I are riffing about how to make sure the reflexive side works, more than anything.01:31
wgrantAnd we have strongly disagreed on the approach in the past :)01:31
lifelessthat, and that the embedded team object in the API result needs to be appropriately neutered.01:31
wgrantlifeless: The team shouldn't be embedded, I don't think01:32
lifelesswgrant: think of it as a challenge to your ingenuity01:32
wgrantIt'll include a link01:32
wgrantAnd should only check LimitedView for that01:32
lifelesswgrant: I'd like to be confident of that01:32
wgrantlifeless: Well01:32
lifelesswgrant: a test, even if thrown away afterwards, would do that.01:32
wgrantThe way you become confident is to only grant LimitedView01:32
wgrantNot View01:32
lifelesswgrant: sure, +101:32
cjwatsonSo, 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 issue01:34
wgrantWell01:35
lifelesshistory tells us :>01:35
wgrantAs long as you preload the people themselves, it won't be a performance issue unless there are private teams.01:35
cjwatsonIf we're not filtering, then presumably we at least need to preload01:35
cjwatsonRight01:35
lifelessthere is a 'Person' eager loader which does the right thing for teams too01:35
wgrant(Since the privacy check on any person other than a private team is a trivial inline attribute access)01:35
lifelessso using that + injecting LimitedView onto everything should be sufficient01:35
cjwatsoni.e. precache_permission_for_objects?01:37
wgrantRight01:38
cjwatsonwgrant: 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
cjwatsonIt's kind of a hack around structsubs being insane, but ...01:50
wgrantcjwatson: "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
cjwatsonThe current implementation is sufficiently recursive that I couldn't see a way to improve it without ripping much of it to shreds02:00
wgrantYeah02:01
wgrantIt's reasonably likely that the refactoring that we require will make it substantially less bad.02:01
wgrantBut it probably won't be reliably good enough for your uses.02:01
cjwatsonSeeing as the list of bugs to close can be arbitrarily long, indeed02:02
wgrantRight02:02
wgrantWe're only really interested in optimising for the single bug case.02:03
wgrantAnd that's all that's feasible to do at this stage without massive refactoring.02:03
cjwatsonIf 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 there02:03
wgrantcjwatson: Why's distroseries nullable/02:04
wgrantYou could interpret that to mean upload_distroseries02:04
wgrantBut that seems pointless.02:05
cjwatsoncf. close_bugs_for_sourcepackagerelease02:05
cjwatsonIt's upload_distroseries in my current imp02:05
cjwatsonBecause otherwise I had to pass either a DSP or an SP, or whatever the objects are02:05
wgrantcjwatson: 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
wgrantSPR.upload_{distroseries,archive} are abominations. We should restrict their callsites as much as possible.02:08
cjwatsonAh, well that's possible yes02:08
cjwatsonIt's just close_bugs_for_queue_item, and that could use queue_item.distroseries02:08
cjwatsonNo idea why it doesn't02:08
wgrantRight02:09
wgrantIt's only the initial non-copy PackageUpload stuff which should be using upload_* at all02:09
wgrantEverything else should use it from a publication02:09
cjwatsonAnd "upload_distroseries" in processaccepted is actually SPPH.distroseries, not .upload_distroseries02:09
cjwatsonThe naming lies02:09
wgrantSo keeping the upload_blah in queue.py wins for everyone02:09
wgrantYeah, because when the function was originally written it was always the upload distroseries.02:09
cjwatsonSo yes, that can and should be non-nullable.02:10
wgrantGreat02:11
wgrantLimiting the propagation of mistakes from 2005 is good :)02:11
wgrantI shall give you a number.02:11

Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!