[12:07] wgrant: I think I've got the webhook privacy code we discussed written, and I'm trying to write tests for negative cases, starting with the situation you suggested where the owner of a branch in a private project leaves the team granting them access to that private project and thus no longer has access to view a branch they own. I can't quite get the test to work, though, because the owner of a branch always has an artifact grant ... [12:07] ... which doesn't appear to be removed by their leaving the team. Could you explain how this is meant to work? [12:07] (the owner of a non-public branch, that is) [12:16] cjwatson: The owner doesn't have an artifact grant if the artifact grant is revoked. [12:17] Nor should they have an AAG by default if they already have an APG, though I believe there is a bug or two which can erroneously result in an AAG in some cases. [12:17] I think the only thing that would naturally revoke the owner's artifact grant would be unsubscribing from the branch. [12:17] Or direct revocation. [12:17] which is done by the sharing job, but it only does that if there isn't an artifact grant. [12:18] right, or I could do that in the test, I suppose. [12:18] Right, the normal situation is direct revocation from +sharing. [12:20] So that's certainly a good point about having an AAG by default - I wonder why that exists in my case [12:20] If you can reproduce it, then that is good. [12:21] I may have done a stupid thing, since I get a bit lost in the disclosure code. [12:21] The code is somewhat convoluted, but the concepts are relatively simple. [12:29] Do I need to commit after creating the project/team/branch to get Branch.access_{policy,grants} up to date or something? [12:31] I thought the triggers happened immediately after the INSERT statement and before any subsequent statement [12:32] Indeed, a flush is sufficient. [12:32] It is not just security adapter caching? [12:33] getVisibleArtifacts should bypass all that, shouldn't it? [12:33] I'm not explicitly flushing here [12:33] It should indeed. [12:33] Have you reviewed the queries and the database state? [12:34] -D and psql are handy [12:34] And LP_DEBUG_SQL=1 [12:34] (do remember to commit before psqling, though) [12:34] Yeah I'm trawling through LP_DEBUG_SQL_EXTRA=1 now [12:34] _EXTRA? My condolences. [12:35] I think possibly there is no AccessPolicyArtifact created before the point where getVisibleArtifacts is called [12:35] Oh, during branch creation? [12:35] That is a possibility. [12:35] Yes [12:36] Ah [12:36] Because _reconcileAccess is called only after subscribe, perhaps. [12:36] Is the getVisibleArtifacts call within subscribe? [12:37] It has been some years since I reviewed this code. [12:39] Right, it is. I think BranchNamespace.createBranch is calling those in the wrong order [12:39] I can't think why that would be. [12:40] So the APA is only created after subscribing the owner, which means that the initial subscription of the owner always thinks that the owner doesn't have a policy grant and therefore creates an artifact grant. [12:41] Reversing those makes my test work. [12:41] I don't quite recall the history here. [12:41] But I probably wrote that code. [12:41] I don't remember whether that might have been because it was syncing subscribers and AAGs during the migration. [12:41] So it was important that it was after subscribe. [12:44] r15357 seems to be where this was introduced, and doesn't appear to have any evidence of this. It's structurally more or less the same as today. [12:45] I don't have the code in front of me, will investigate in a bit. [12:45] But it sounds reasonable. [12:45] But branches weren't the first artifact to be managed this way; maybe bugs had something like that previously. [12:47] Do bugs need a similar fix? APA creation comes after subscribing the owner in that case too. [12:49] Blueprints don't have the same problem. [12:50] Hm. [12:50] What creates the AA? [12:51] Is it automatically created when subscribe() goes to create the AAG? [12:52] Both reconcile_access_for_artifact and SharingService.ensureAccessGrants do getUtility(IAccessArtifactSource).ensure(artifacts) [12:53] Branch.subscribe calls SharingService.ensureAccessGrants [13:05] Right. [13:08] from experience I don't think bugs have the same problem. [13:09] but they may accidentally get around it due to tasks [13:09] createBug likely calls createTask, possibly before subscribe [13:09] It does [13:09] and createTask is likely to _reconcileAccess or equivalent [13:10] Aha, yes, I hadn't noticed that [13:10] whoever duplicated that for bugs may well have just done it exactly the same way [13:10] er branches, not bugs [13:10] and it just happened to work for bugs due to the way target setting works [13:10] Would still be better to be explicit rather than having a basically useless and misleading _reconcileAccess at the end of bug creation [13:11] createTask must be before subscribe for notifications to work [13:11] so that would explain it [13:11] It is [13:13] I think I'll delete the explicit bug._reconcileAccess from BugSet.createBug and leave a comment [13:14] Because it's really more about the bug task since that's the thing that's setting up the association with the pillar [13:14] as long as there's no opportunity for privacy/target changes between createTask and _reconcileAccess, and createTask really calls fullblown _reconcileAccess, that sounds fine [13:15] it's createTask, subscribe, assignee, importance, milestone, _reconcileAccess; so no [13:15] yay [13:16] and yes, removeSecurityProxy(bug)._reconcileAccess() at the end of BugTaskSet.createManyTasks, which does most of the work for createTask [13:16] ugh, that's right [13:16] sorry, was roughly on the right track at least [13:16] * cjwatson kicks off a test run for -t bugs -t code and goes out to run errands [13:17] I'll work out specific tests once I've ensured it doesn't blow up the world [13:17] tests are good! [13:17] thanks for the help figuring this out [13:18] (it was making no sense whatsoever yesterday evening, and no wonder ...) [13:19] I wonder if we need a way to find all the incorrect AAGs. :-( [13:19] glad my vague recollections led you down the right path! [13:19] they're not incorrect, just excess. [13:19] well, they mean that people retain access after leaving the team that grants them access [13:20] so at least some of them are incorrect [13:21] ish [13:21] when you're kicking someone out of a project you should be checking +sharing anyway [13:21] for Canonical employees that's all handled automatically by the leaver process [13:22] I guess they have an explicit subscription too [13:22] the main intent of not creating an AAG was to avoid polluting +sharing [13:22] rather than automatic revocation [13:22] ok [13:22] I won't panic about it then [13:23] anyway, hopefully the test suite will be happy [13:23] Have a good weekend! [13:24] you too