| 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:07 |
| wgrant | cjwatson: The owner doesn't have an artifact grant if the artifact grant is revoked. | 12:16 |
| 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:17 |
| 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:18 |
| 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:20 |
| 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:21 |
| cjwatson | Do I need to commit after creating the project/team/branch to get Branch.access_{policy,grants} up to date or something? | 12:29 |
| cjwatson | I thought the triggers happened immediately after the INSERT statement and before any subsequent statement | 12:31 |
| wgrant | Indeed, a flush is sufficient. | 12:32 |
| wgrant | It is not just security adapter caching? | 12:32 |
| 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:33 |
| 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:34 |
| 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:35 |
| wgrant | Ah | 12:36 |
| wgrant | Because _reconcileAccess is called only after subscribe, perhaps. | 12:36 |
| wgrant | Is the getVisibleArtifacts call within subscribe? | 12:36 |
| wgrant | It has been some years since I reviewed this code. | 12:37 |
| 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:39 |
| 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:40 |
| 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:41 |
| 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:44 |
| 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:45 |
| cjwatson | Do bugs need a similar fix? APA creation comes after subscribing the owner in that case too. | 12:47 |
| cjwatson | Blueprints don't have the same problem. | 12:49 |
| wgrant | Hm. | 12:50 |
| wgrant | What creates the AA? | 12:50 |
| wgrant | Is it automatically created when subscribe() goes to create the AAG? | 12:51 |
| cjwatson | Both reconcile_access_for_artifact and SharingService.ensureAccessGrants do getUtility(IAccessArtifactSource).ensure(artifacts) | 12:52 |
| cjwatson | Branch.subscribe calls SharingService.ensureAccessGrants | 12:53 |
| wgrant | Right. | 13:05 |
| wgrant | from experience I don't think bugs have the same problem. | 13:08 |
| 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:09 |
| 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:10 |
| wgrant | createTask must be before subscribe for notifications to work | 13:11 |
| wgrant | so that would explain it | 13:11 |
| cjwatson | It is | 13:11 |
| cjwatson | I think I'll delete the explicit bug._reconcileAccess from BugSet.createBug and leave a comment | 13:13 |
| 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:14 |
| cjwatson | it's createTask, subscribe, assignee, importance, milestone, _reconcileAccess; so no | 13:15 |
| wgrant | yay | 13:15 |
| 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:16 | |
| 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:17 |
| cjwatson | (it was making no sense whatsoever yesterday evening, and no wonder ...) | 13:18 |
| 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:19 |
| cjwatson | so at least some of them are incorrect | 13:20 |
| 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:21 |
| 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:22 |
| wgrant | anyway, hopefully the test suite will be happy | 13:23 |
| wgrant | Have a good weekend! | 13:23 |
| cjwatson | you too | 13:24 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!