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