Merge lp:~wallyworld/launchpad/create-aag-privacy-transitions2-1009364 into lp:launchpad

[Ian Booth]

New findPeopleWithoutAccess service methods, ensure all subscribers are granted access to a bug when it becomes private.

== Implementation ==

This branch ensures that, with the legacy bug visibility triggers removed, when a bug transitions to private, any direct subscribers are granted access to the bug.

A new set of model methods was added - get/findPeopleWithoutAccess. For a given artifact and a list of people, it returns those who cannot see the artifact. This is used in the transitionToInformationType method on bug, to add AAGs from those subscribers who cannot see the bug.

A couple of issues were encountered along the way and fixed.

The IBugTaskSet.searchBugIds() method was returning duplicate ids in it's result. This was fixed by tweaking the bugtasksearch search_bugs() method.

There was duplicate code to disable triggers due to separate landings. I kept the one which was done as a test fixture since this is required for existing uses.

The code to check whether a bug is visible to a subscriber calls searchBugIds() which requires that the bug has a bugtask or else it returns false. The createBug() method added subscribers before creating the default bugtask and so public bugs were having AAG's created. I changed the order of the code to address this.

== Tests ==

Add tests to test_sharingservice and test_accesspolicy to test the new get/findPeopleWithoutAccess() methods.

Add a new test test_transition_to_private_grants_subscribers_access() to TestBugPrivateAndSecurityRelatedUpdatesMixin to check the core functionality of this mp.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/model/bug.py
  lib/lp/bugs/model/bugtasksearch.py
  lib/lp/bugs/model/tests/test_bug.py
  lib/lp/bugs/model/tests/test_bugtask.py
  lib/lp/code/model/branch.py
  lib/lp/registry/interfaces/accesspolicy.py
  lib/lp/registry/interfaces/sharingservice.py
  lib/lp/registry/model/accesspolicy.py
  lib/lp/registry/services/sharingservice.py
  lib/lp/registry/services/tests/test_sharingservice.py
  lib/lp/registry/tests/test_accesspolicy.py

[Ian Booth (wallyworld)]

I've moved the getPeopleWithoutAccess() code completely to the sharing service and fixed the implementation and enhanced the test to check that people with either APG or AAG are properly accounted for.

[William Grant (wgrant)]

Thanks for addressing my comments. I have a few new ones:

312 +from storm.expr import (

This should be in the third-party section, after the lazr imports.

371 + # Determine the pillars via which an access policy grant will give
372 + # access.
373 + affected_pillars = []
374 + if access_artifact.bug_id:
375 + affected_pillars = concrete_artifact.affected_pillars
376 + else:
377 + target_context = concrete_artifact.target.context
378 + if IPillar.providedBy(target_context):
379 + affected_pillars.append(target_context)

Can't you run getPeopleWithoutAccess after _reconcileAccess, so you can just use AccessPolicyArtifact rather than duplicating the pillar determination logic?

403 + AccessPolicyGrantFlat.abstract_artifact_id ==
404 + access_artifact.id,
405 + AccessPolicyGrantFlat.grantee_id ==
406 + TeamParticipation.teamID),

This looks like it'd be fine with AccessArtifactGrant. No need for AccessPolicyGrantFlat.

[Ian Booth (wallyworld)]

Thanks for the suggestions, I think everything is ok now. The test
still passes with the modified query.

[William Grant (wgrant)]

Thanks. One last trivial thing:

309 from lazr.restful.utils import (
310 get_current_web_service_request,
311 )
312 +
313 +from storm.expr import (

That blank line shouldn't be there.

review: Approve (code)