[00:00] wallyworld_: Because the team has access, a grant is not created for the member. [00:00] wallyworld_: So they only have access through the team. [00:00] When the team's access is revoked from +sharing, a REmoveGranteeSubscriptionsJob removes the team's subscription. [00:00] But doesn't remove the person's subscription, even though they no longer have access. [00:01] oh, from +sharing. the remove subscription for former members works when using the team page to revoke membership [00:02] so there could be a case to drop the RemoveGranteeSubscriptionsJob and just rely on the RemoveBugSubscriptionsJob [00:03] wallyworld_: I think they should indeed be merged. [00:03] but RemoveBugSubscriptionsJob would need to not require bug_ids. [00:03] probably, it will need to be looked at [00:04] the RemoveGranteeSubscriptionsJob was written first and then several pipes later the RemoveBugSubscriptionsJob came along [00:05] Ah, I see. [00:06] Perhaps we should just use RemoveGranteeSubscriptionsJob and have it for both branches and bugs [00:07] StevenK: that job was written to handle both [00:08] Then why does RemoveBugSubscriptionsJob exist? :-) [00:08] StevenK: the former removes access for an individual subscriber from the specified bug/branches for a specified pillar [00:09] the later removes subscriptions for any artifacts any person can no longer see [00:09] for any pillar [00:09] the former was written early on to work will the +sharing page [00:10] the latter was written to handle team membership revocation and evolved from there [00:10] The rules for both should be the same. [00:10] But one is information_type-wide [00:10] And the other is restricted to a set of bugs. [00:10] Both need to consider teamparticipation. [00:11] one is potentially more efficient because it is more constrained [00:11] so perhaps both are needed, but the remove gants one needs to include team membership [00:11] Why are both needed? [00:11] One has bugtaskflat.information_type = 3 [00:11] The other has bugtaskflat.bug IN (1, 2, 3, 4) [00:11] They should otherwise be identical, AFAICT [00:12] i did say "potentially" - i need to review the code [00:12] Yeah [00:12] the one from the +sharing page is constrained to that pillar so potentially may have less work to do in the db [00:13] bugtaskflat.bug IN (1, 2, 3, 4) will return far less rows that infotype = 3 [00:14] hence the downstream processing may be less [00:14] Er yeah, I mean (bugtaskflat.information_type = 3 and bugtaskflat.product = 1) or (bugtaskflat.bug IN (1, 2, 3, 4)) [00:14] The two types of constraints are required for efficiency. [00:14] still less rows [00:14] But the implementation can otherwise be identical. [00:14] Just one filters by infotype and pillar, the other by a predefined list of bug IDs. [00:15] yes, the implementations can likely be merged now that both have been written [00:15] but the different filtering should remain probably [00:15] Yup [00:38] wallyworld_: btw, bin/celeryd -l DEBUG -Q job --config lp.services.job.celeryconfig [00:38] Handy for testing stuff outside tests. [00:38] Without waiting 50s for the cronjob to start [00:39] cool, i did know that a couple of weeks ago but forgot [01:21] ahahaha http://www.leakedin.com/ [01:25] also leakedin.org :> [02:00] StevenK: what did we decide about have a public A stacked on public B but then making B userdata - A should also become userdata? [02:01] wallyworld_: Hm, I think I've swapped that out. Let me think. [02:01] ok. i have a test which does that and fails [02:02] it used to set private = true and the triggers would update everything [02:02] but setting info type = userdata fails since A remains public [02:05] wallyworld_: Right, so the information_type won't change, but we check if the stacked_on branches information_type is private in places where we were depending on transivitely_private. [02:06] StevenK: sure, we now check for branches "under" the one being changed, but the old transitively private stuff also went "up" the chain [02:07] Yes, we don't do that. [02:07] i think we need to [02:07] Why? [02:07] or else we will have public stacked on user data and will be in the same mess [02:07] as we had before transitively private [02:09] wallyworld_: It's less than practical to do that universally, and the cases where it's useful are less than common. [02:09] it's not impractical if we use triggers [02:09] wallyworld_: If you're changing a stacked-on branch from public to private, you should know you're already in strife. [02:09] Neither wgrant or I want to use triggers. [02:09] wallyworld_: It's still impractical. [02:10] why impractical? [02:10] wallyworld_: It's just that it's so unused nowadays that the impractical cases haven't shown up yet. [02:10] and if we don't fix this, users will shoot themselves in the foot [02:10] wallyworld_: Writing to a few thousand branches because of a write on one branch is silly. [02:10] that's what we do now with the triggers and it works fine [02:10] wallyworld_: Users have shot themselves in the foot in the past due to Launchpad staff misconfiguring branch visibility policies. [02:11] sinzui has since fixed them, and in the new model it's impossible. [02:11] so are you sating allowing a public branch to stack on a private branch is ok? [02:11] because the code as written allows that [02:11] wallyworld_: The normal footgun situation is handled here, as it was before. If you push up a new public branch stacked on a private branch, it will be overridden to private. [02:12] What's not handled is making an existing public stacked-on branch private. [02:12] wgrant: sure, but now the other way arpund [02:12] not [02:12] Now, we should probably warn in that case. [02:12] But it's going to be extremely rare. [02:12] Extremely. [02:12] you can make a stacked on branch private [02:12] the code allows it [02:12] Sure. [02:12] so people will do it [02:12] there's a test which does it [02:13] It's possible to do. [02:13] i think we are leaving a huge hole [02:13] +sharing will say "you have n private branches" [02:13] and don't understand why we are regressing [02:13] The public branch will probably have a warning say "wtf are you crazy" [02:13] But the situation should never occur again. [02:13] Unless someone forgets to set up private branches before they push their code. [02:14] should !+ won't [02:14] it will and can occur via the api i think (need to check) [02:14] I'm not saying that it's impossible to do. [02:14] I'm saying that it is very unlikely to happen. [02:14] the system should not allow people to do this [02:14] Unless someone deliberately does it. [02:14] unlikely != won't [02:14] well they may not realise [02:15] what's to stopme from changing my branch to private not realisng it is stacked on [02:15] Nothing. [02:15] But why would you do that? [02:15] what, change a public branch to private? [02:15] Yes. [02:15] why not? [02:15] Why? [02:16] you may want to do future work in secret [02:17] Anyway. Doing unbounded cascading writes is foolish, and this case is sufficiently harmless and so extraordinarily uncommon that I'm pretty sure it's not worth it. [02:17] it's not unbounded really [02:18] There may be some sense in adding a warning to the stacked branch. [02:18] It is unbounded. [02:18] not in practice [02:18] and it works just fine now [02:18] so we are causing a deliberate regresiion [02:18] Has it ever been used? [02:18] in tests [02:18] i have no way of knowing whether on prod [02:19] It wasn't used during the several week information_type transition on production. [02:19] ingrequent != won't ever [02:20] we should not allow the system to get into a bad state [02:20] launchpad_dogfood=# SELECT COUNT(*) FROM branch WHERE stacked_on = (SELECT id FROM branch WHERE unique_name = '~launchpad-pqm/launchpad/db-devel'); [02:20] and that's what we wil be doing in introducing this regression in functionality [02:20] count [02:20] ------- [02:20] 5018 [02:20] (1 row) [02:20] That's pretty unbounded. [02:20] wallyworld_: What is wrong with the state? [02:20] 5018 < infinity [02:21] public branch stacked on private branch doesn't make sense [02:21] It doesn't make sense, no. [02:21] and yet we will be allowing it [02:21] But why do we need to prevent it? [02:21] because it doesn't make sense [02:21] We should prevent things that are harmful. [02:21] and the whole reason transitively private was done was to stop this [02:21] Not quite [02:21] that's the only reason transitivelt private was done [02:22] transitively_private's main use case is handled by information_type [02:22] That case is the one where a branch is pushed as public, but is stacked on a private branch. [02:22] In that case the user is probably unknowingly making a mistake. [02:23] We need to handle that, because it happens often due to misconfiguration. [02:23] and the other case will also happen [02:23] The case that transitively_private handles now, but we are dropping with information_type, is where a branch and its stacked-on are public, and the stacked-on transitions to private. [02:23] In that case the branch owner is already aware of some information disclosure. [02:23] They're going to want to audit their project's sharing once they've fixed the immediate hole. [02:23] yes, and i totally disagree with that regresiion [02:24] it is insane we are allowing bad data from one direction and stopping it from the other [02:24] Hm? [02:24] We're not doing it to stop bad data. [02:24] We're doing it to stop people from a footgun that they can't know about. [02:25] and yet we will allow it still [02:25] No. [02:25] yes [02:25] change the stacked on to private and boom [02:25] Boom? What boom? [02:25] Nothing breaks. [02:25] Some stuff may be left public. [02:25] But it's not unknown. [02:25] what breaks is that people will want to work with the public branch but cant [02:26] Public->private of a stacked-on happens as a result of an incorrect configuration being discovered. [02:26] Ah! [02:26] because they can't see the stacked on private branch [02:26] Now that's something we can't solve. [02:26] They can't work with it either way. [02:26] If it's public, they can no longer work with it since it's stacked on a private branch. [02:26] If we make it private, they can longer work with it since it's private. [02:26] 2012-06-06 23:26:57 DEBUG2 [PopulateSourcePackagePublishingHistoryPackageUpload] Done. 231995 items in 337 iterations, 807.064977 seconds, average size 688.415397 (287.45639486/s) [02:26] So it's no more unusable this way. [02:26] yes, and so it should be marked private [02:26] wgrant: ^ [02:27] wallyworld_: Why? [02:27] so that people realise it's private. as i said, that why tp was introduced [02:27] wallyworld_: This is not a regression which makes those forgotten branches less usable. And this is not a regression which prevents an audit from working, since the other branches will clearly show up as public too. [02:27] wallyworld_: tp's main usecase is, as I said, to prevent the initial footgun case. [02:28] it is a regression which removes previously agreed useful behaviour [02:28] *Not* to handle this change later. [02:28] It's very expensive and probably not worthwhile behaviour. [02:28] and we will now be allowing the initial footgun again [02:28] wgrant: And racy. [02:28] No [02:28] How? [02:28] by allowing a stacked on branch to be marked private [02:28] We explicitly do handle the initial push case. [02:28] That's not the initial footgun. [02:29] it's a way to create the problem without people knowing [02:29] Making a branch private later is a very explicit operation. [02:29] and one which is easy to mess up [02:29] if you don't realise yor branch is stacked on [02:29] Sure. [02:29] Then let's warn in the UI and move on [02:29] And what is the fallout from it? [02:29] we just simply cannot alllw this [02:30] It is messy, but not a significant security issue. [02:30] ui != api [02:30] So a warning is probably sufficient. [02:30] it's a significant functional issue [02:30] In an extraordinarily rare situation. [02:30] Where someone has already broken shit. [02:30] since it inadvertantly stops peope working wit the public branch [02:30] Huh? [02:30] That's irrelevant. [02:30] not rare enough that tp wasn't needed [02:30] We'd just be advertantly stopping people from working with it. [02:30] wallyworld_: tp solves the footgun case. [02:30] This also solves the footgun case. [02:31] tp does more than that [02:31] tp's implementation happens to also solve the later transition case [02:31] we are deliberately regressing [02:31] This does not. It consciously does not. [02:31] Correct. [02:31] But it's not a substantial regression in any way. [02:31] not "happens", it was a key part of the implementation [02:31] it is a substantioal regrsiion [02:31] we must not allow users to do things whuch break the system [02:32] It does not break the system. [02:32] it breaks the ability for people other than themselves to work wit the stacked branches [02:32] I can make a public branch unusable in other ways. [02:32] eg. by deleting the repository from it. [02:32] so? [02:32] 2 wrongs doesn't make a right [02:33] And all it does is avert a tiny bit of user confusion. [02:33] Since if we follow tp, we just make the branch even less usable. [02:33] it does a lot more than that [02:33] Not more usable. [02:33] why is the branch less usable wit tp? [02:34] wallyworld_: Your complaint is that with information_type the branch is unusable by people who don't have access to the stacked-on branch. [02:34] without tp, the ui lies [02:34] wallyworld_: With tp the branch is just as unusable. [02:34] Sure. [02:34] But the UI can say "uh, your branch is stacked on a private one, but it's public. you're probably wrong" [02:34] but it the private branch could be several layers down [02:35] That's true. [02:35] and model code also needs to treat the branch like it is private [02:35] to exlcude it from search results etc [02:36] Why? [02:36] It doesn't break anything. [02:36] if i change a staced on branch to private, any publiv branche sstaked on it must not show up in search results [02:36] It just makes it a little confusing. [02:36] Why not? [02:36] https://pastebin.canonical.com/67615/ [02:36] because the stacked branch is to be considered private [02:36] that was the required tp functionality [02:36] when that work was speced out [02:37] canonical-payment-service and ztrustee were misconfigurations that sinzui fixed. information_type handles those identically, because it's the initial footgun case. libunity-webapps was a misconfiguration that required a full project audit afterwards. Everything else has less than 5 branches in this state. [02:37] Which proves that no large transition has ever made use of the tp ability that we're regressing on. [02:37] Except *possibly* libunity-webapps. [02:38] sure, but just because it's uncommon, we can't just unilaterally decide to regress [02:38] That paste covers every branch that has made use of tp, except for those where explicity_private was set afterwards. [02:38] wallyworld_: We're regressing on a feature that has never been used. [02:38] Not once. [02:39] what about libunity-webapps [02:39] wallyworld_: What if we don't consider the stacked branch private? [02:39] wallyworld_: What if we consider it misconfigured? [02:39] and it's not so much that it's used, it is designed to prevent inconsistent data [02:39] and confusion [02:39] libunity-webapps I shall not go into here, but it was a complete misconfiguration of a private project. [02:40] In the case of a misconfigured project like that one, you should be fully auditing +sharing anyway. [02:40] even if we consider it misconfisgured, why would we deliverately design out a saftely check to prevent the misvonfiguration in the first place? [02:41] wallyworld_: It's not proven useful even once, it requires unbounded write cascades, and it is non-trivial to implement with information_type. [02:41] it was useful because of the state lp was in befre it was implemented [02:42] The main usecase for tp is extremely useful. [02:42] there were a large number of public branches stacked on private [02:42] This one is not. [02:42] Sure. [02:42] and now we can have the all over again [02:42] No! [02:42] No! [02:42] yes [02:42] Only if a project changes their stacked-on branch without touching the rest. [02:43] why do you say no when we have already agreed it can happen? [02:43] In that case it's not a vulnerability. [02:43] It's a project being stupid. [02:43] but they may no realise their branch is stacked on [02:43] wallyworld_: As we've seen, the vast, vast majority of cases where it can happen is the initial push. [02:43] So we won't have a large number of public branches stacked on private. [02:43] it can happen anytime [02:43] We might have 1% of what we had before [02:43] wallyworld_: Technically it can. [02:44] Practically it won't unless the project owner is a moron. [02:44] and therefore it will [02:44] Is it a problem? [02:44] moron != careless [02:44] yes [02:44] otherwise tp would not have been approved to be resourced [02:44] Huh? [02:44] TP solved a very real problem [02:44] It was totally worth it [02:44] It also solved another problem that wasn't real. [02:44] We have hard data on that now. [02:45] it was rel [02:45] i can't recall the exact figures, but a garbo job was needed to clean up the data [02:45] That is entirely irrelevant to the nature of that data. [02:45] The volume and split are unrelated. [02:46] The data I just obtained from dogfood confirms my assumptions: that the case we're dropping is unused. [02:46] It's a case I was always suspicious about. [02:46] used now != used ever [02:46] and we are taking away a safety net [02:46] to prevent bad data [02:46] deliberately [02:47] OK [02:47] It's not bad data. [02:47] it is [02:47] because it causes confusion [02:47] and stuff to break [02:47] Preventing bad data is not itself a useful goal [02:47] huh? [02:47] The "bad data" has to have some negative effect [02:47] it does [02:47] The goal is to eliminate the negative effect. [02:47] Right [02:47] confusion, no longer being able to use the stacked branch [02:47] So let's stop talking about bad data, and start talking about the negative effects :) [02:48] Confusion: granted, we can fix that with messages on the branch page, as we do already if the stacked-on location is invalid. [02:48] or we could just prevent the bad data and not introduce a deliberate regresiion [02:48] we would need to drill down the entire stack [02:48] No longer being able to use the stacked branch: that's a false statement; you can't use it if it's private either. [02:49] but it's not pretending to be usable [02:49] So that goes back to confusion. [02:49] Which is very similar to the invalid stacked-on issue [02:49] For which we currently warn [02:49] With a warning on Branch:+index [02:49] we never used to warn [02:49] is that new [02:49] We've always warned. [02:50] Invalid meaning not a valid URL, or the URL points to something that doesn't exist or is outside Launchpad [02:50] Not meaning private [02:50] Yet. [02:50] nope, not in the case of public stacked on private afair [02:50] we used to just show the stacked branch as if it were public [02:50] 12:49:24 < wgrant> Which is very similar to the invalid stacked-on issue [02:50] 12:49:27 < wgrant> For which we currently warn [02:51] "is very similar to" [02:51] Not "is" [02:51] so it's new then [02:51] Hm? [02:51] That warning is not new. [02:51] Extending it to warn when the stacked-on is private would be new. [02:51] but we never used to warn people their public branch was stacked on a private one [02:51] No. [02:51] I never said we did. [02:52] I'm saying it's what we should do [02:52] hence my assertion it will be new [02:52] Sure, warning *on privacy violation* is new. [02:52] Warning on bad stacked-on in general is not. [02:52] So it's quite anomalous that we don't warn in the case of a privacy violation. [02:52] even recusive stacked on [02:52] ? [02:53] I'm not sure about that. [02:53] we may warn on bad stacked on, but perhaps not recusrively [02:53] i guess it comes down to in part philosophy - should software prevent a known bad situation [02:54] or should software regress deliberately [02:54] It's a tradeoff. [02:54] given the current implementation works fine, i'm not sure i see why we are doing this [02:54] We can maintain messy code of unbounded computational complexity that will probably never be usefully used. [02:54] wallyworld_: Are you done yelling at wgrant? :-) [02:54] Or we can not. [02:55] it's not messy [02:55] the algorithm is very simple [02:55] With transitively_private it's not messy. [02:55] With information_type it is slightly moreso. [02:55] Because it's not boolean. [02:55] so what i would like to see is an exception raised [02:55] Hahahaha [02:55] if someone attempts to change a stacked on branch to an invalid state [02:56] We can't do that every time. [02:56] why? [02:56] You may recall we discussed this for weeks. [02:56] The information_type of a stacked on branch could change directly. [02:56] stacking can change via an sftp client [02:56] Or it could change by its stacked-on branch changing. [02:56] Precisely. [02:56] sure, but i'm talking about public stacked on not public [02:56] So. [02:56] Listen to this case: [02:57] via sftp we can't detect sure [02:57] I have a stacking chain. A stacked on B stacked on C [02:57] but that's the case now with tp also [02:57] They're all public. [02:57] Now, the owner of B changes it to stack on D, a private branch. [02:57] What happens now? [02:57] We can't block that. [02:58] So A and B have to become private, or we have to let them be inconsistent. [02:58] Blocking is the ideal solution. [02:58] But it's not feasible. [02:58] So we have to either propagate or accept. [02:58] what happens now is A and B become private [02:58] Yes. [02:58] as they should [02:58] That is arguable. [02:59] the argument was had before tp was approved [02:59] And even if we decide that they should, it becomes is it worth it. [02:59] and it came out in favour of tp [02:59] Curtis agreed with my view on several calls, and I'm pretty sure you were there too :) [03:00] i'm not sure it was agreed to intorduce a deleibate regression [03:00] It may have been agreed during TP's design on the absence of useful data. [03:00] But we have useful data now. [03:00] It was. [03:00]