Merge lp:~abentley/launchpad/transitive-confidential into lp:launchpad

[Aaron Bentley]

Forbid non-proprietary artifacts when transitioning product to Proprietary

= Summary =
Fix bug #1079785: public artifacts are permitted on confidential projects

== Proposed fix ==
Move checks from view to model, raise exceptions, include private non-proprietary types

== Pre-implementation notes ==
None

== LOC Rationale ==
Reduces LOC

== Implementation details ==
Revert 16258 and migrate code from getPublicWarning. Use early exit to separate on-change code from on-creation code.

Tweak to check for USERDATA and PRIVATESECURITY bugs.

Remove getPublicWarning and related code and tests.

== Tests ==
bin/test -t test_change_info_type_proprietary_check_artifacts -t test_change_info_type_proprietary_check_translations

== Demo and Q/A ==
Create a public product with other/proprietary license. Create a public branch, bug, blueprint. Enable translations. You should not be able to change its information type to proprietary until you've made all artifacts private and disabled translations.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/browser/product.py
  lib/lp/registry/browser/tests/test_product.py
  lib/lp/registry/model/product.py
  lib/lp/registry/tests/test_product.py

[Richard Harding (rharding)]

review: Approve

[Aaron Bentley (abentley)]

Further discussion with Curtis reveals that only Proprietary and Embargoed artefacts should be permitted when transitioning product to proprietary/embargoed. See bug.

review: Needs Fixing

[Abel Deuring (adeuring)]

Looks good to land.

I just have concern: If we assume that somebody creates a public project, adds public bugs, branches, specifications, links packages and enables translations.

If this person decides to make product proprietary, the user experience is terrible:

The first error is "sorry, no public specs allowed", so the user changes the specifications.

The next error then is "sorry, no public bugs allowed" and so on. It would be better to collect all "show stoppers" and tell the user immediately that they have to fix issues with bugs, branches, specs, translations and packages.

review: Approve (code)

[Aaron Bentley (abentley)]

I have a plan to show all validation error at form validation time, which I'll pursue in a follow-up branch.

[William Grant (wgrant)]

Translations must not exist at all, not just be disabled. Answers must also not exist.

There's also some arbitrary capitalisation here:

274 + raise CommercialSubscribersOnly(
275 + 'A valid commercial subscription is required for private'
276 + ' Projects.')

[Aaron Bentley (abentley)]

> Translations must not exist at all, not just be disabled. Answers must also
> not exist.

I know this, and I've already started work on addressing that. See bug #1082422 and branch lp:~abentley/launchpad/more-transition-checks. Please add any checks that I've missed to that bug.

> There's also some arbitrary capitalisation here:
>
> 274 + raise CommercialSubscribersOnly(
> 275 + 'A valid commercial subscription is required for private'
> 276 + ' Projects.')

True, but I only moved that code around.

=== modified file 'lib/lp/registry/browser/product.py'
--- lib/lp/registry/browser/product.py	2012-11-15 16:03:56 +0000
+++ lib/lp/registry/browser/product.py	2012-11-23 22:13:26 +0000
@@ -105,7 +105,6 @@
     InformationType,
     PROPRIETARY_INFORMATION_TYPES,
     PUBLIC_PROPRIETARY_INFORMATION_TYPES,
-    PUBLIC_INFORMATION_TYPES,
     ServiceUsage,
     )
 from lp.app.errors import NotFoundError
@@ -129,9 +128,6 @@
 from lp.blueprints.browser.specificationtarget import (
     HasSpecificationsMenuMixin,
     )
-from lp.blueprints.enums import (
-    SpecificationFilter,
-    )
 from lp.bugs.browser.bugtask import (
     BugTargetTraversalMixin,
     get_buglisting_search_filter_url,
@@ -1450,47 +1446,10 @@
 
     @action("Change", name='change')
     def change_action(self, action, data):
-        old_info_type = self.context.information_type
         try:
             self.updateContextFromData(data)
         except CannotChangeInformationType as e:
             self.setFieldError('information_type', str(e))
-        warning = self.getPublicWarning(old_info_type)
-        if warning is not None:
-            self.request.response.addWarningNotification(warning)
-
-    def getPublicWarning(self, old_info_type):
-        """Get warning if public artifacts might expose the product.
-
-        :param old_info_type: The former information type of the product.
-        """
-        if (old_info_type != InformationType.PUBLIC or
-            self.context.information_type == InformationType.PUBLIC):
-            return None
-        public_bugs = self.context.searchTasks(None,
-            information_type=PUBLIC_INFORMATION_TYPES,
-            omit_duplicates=False, omit_targeted=False)
-        public_artifacts = []
-        if not public_bugs.is_empty():
-            public_artifacts.append('bugs')
-        # Default returns all public branches.
-        public_branches = self.context.getBranches()
-        if not public_branches.is_empty():
-            public_artifacts.append('branches')
-        # All specs located by an ALL search are public.
-        public_specs = self.context.specifications(
-            None, filter=[SpecificationFilter.ALL])
-        if not public_specs.is_empty():
-            public_artifacts.append('blueprints')
-        if len(public_artifacts) == 0:
-            return None
-        elif len(public_artifacts) < 3:
-            list_phrase = ' and '.join(public_artifacts)
-        else:
-            list_phrase = 'bugs, branches, and blueprints'
-        return ('Some %s are public.  This project will not be fully '
-                '%s until they have been changed.' %
-                (list_phrase, self.context.information_type.title))
 
 
 class ProductValidationMixin:

=== modified file 'lib/lp/registry/browser/tests/test_product.py'
--- lib/lp/registry/browser/tests/test_product.py	2012-11-15 16:03:56 +0000
+++ lib/lp/registry/browser/tests/test_product.py	2012-11-23 22:13:26 +0000
@@ -25,7 +25,6 @@
 from lp.app.enums import (
     InformationType,
     PROPRIETARY_INFORMATION_TYPES,
-    PUBLIC_PROPRIETARY_INFORMATION_TYPES,
     ServiceUsage,
     )
 from lp.registry.browser.product import (
@@ -553,118 +552,6 @@
             self.assertEqual(
                 InformationType.PUBLIC, updated_product.information_type)
 
-    def test_warning_on_public_artifacts(self):
-        # When changing to proprietary, any public artifacts will trigger a
-        # warning.
-        self.useFixture(FeatureFixture(
-            {u'disclosure.private_projects.enabled': u'on'}))
-        owner = self.factory.makePerson()
-        product = self.factory.makeProduct(owner=owner)
-        bug = self.factory.makeBug(target=product)
-
-        def make_proprietary():
-            with person_logged_in(None):
-                browser = self.getViewBrowser(product, '+edit',
-                    user=product.owner)
-                info_type = browser.getControl(name='field.information_type')
-                info_type.value = ['PROPRIETARY']
-                browser.getControl('Change').click()
-            with person_logged_in(owner):
-                product.information_type = InformationType.PUBLIC
-            return browser
-        browser = make_proprietary()
-
-        def assertWarning(browser, text):
-            warning_tag = Tag('warning', 'div', text=text,
-                              attrs={'class': 'warning message'})
-            self.assertThat(browser.contents, HTMLContains(warning_tag))
-        assertWarning(
-            browser, 'Some bugs are public.  This project will not be fully '
-            'Proprietary until they have been changed.')
-        browser = make_proprietary()
-        with person_logged_in(bug.owner):
-            bug.transitionToInformationType(InformationType.PROPRIETARY,
-                                            bug.owner)
-        browser = make_proprietary()
-        self.assertThat(browser.contents, Not(HTMLContains(
-            Tag('warning', 'div', attrs={'class': 'warning message'}))))
-
-    def test_getPublicWarning_artifacts(self):
-        # The warning indicates what kinds of artifacts are public, in
-        # grammatical English.
-        owner = self.factory.makePerson()
-        product = self.factory.makeProduct(owner=owner)
-        bug = self.factory.makeBug(target=product)
-        view = create_initialized_view(product, '+edit')
-        with person_logged_in(owner):
-            product.information_type = InformationType.PROPRIETARY
-        warning = view.getPublicWarning(InformationType.PUBLIC)
-        self.assertEqual(
-            'Some bugs are public.  This project will not be fully '
-            'Proprietary until they have been changed.', warning)
-        spec = self.factory.makeSpecification(product=product)
-        warning = view.getPublicWarning(InformationType.PUBLIC)
-        self.assertEqual(
-            'Some bugs and blueprints are public.  This project will not be '
-            'fully Proprietary until they have been changed.', warning)
-        branch = self.factory.makeBranch(product=product, owner=owner,
-                information_type=InformationType.PUBLIC)
-        warning = view.getPublicWarning(InformationType.PUBLIC)
-        self.assertEqual(
-            'Some bugs, branches, and blueprints are public.  This project '
-            'will not be fully Proprietary until they have been changed.',
-            warning)
-        with person_logged_in(owner):
-            bug.transitionToInformationType(
-                InformationType.PROPRIETARY, owner)
-        warning = view.getPublicWarning(InformationType.PUBLIC)
-        self.assertEqual(
-            'Some branches and blueprints are public.  This project '
-            'will not be fully Proprietary until they have been changed.',
-            warning)
-        with person_logged_in(owner):
-            branch.transitionToInformationType(
-                InformationType.PROPRIETARY, owner)
-        warning = view.getPublicWarning(InformationType.PUBLIC)
-        self.assertEqual(
-            'Some blueprints are public.  This project will not be fully '
-            'Proprietary until they have been changed.', warning)
-        with person_logged_in(owner):
-            spec.transitionToInformationType(
-                InformationType.PROPRIETARY, owner)
-        self.assertIs(None, view.getPublicWarning(InformationType.PUBLIC))
-
-    def test_getPublicWarning_embargoed(self):
-        # The message reflects an Embargoed product type.
-        owner = self.factory.makePerson()
-        product = self.factory.makeProduct(owner=owner)
-        self.factory.makeBug(target=product)
-        with person_logged_in(owner):
-            product.information_type = InformationType.EMBARGOED
-            view = create_initialized_view(product, '+edit')
-        warning = view.getPublicWarning(InformationType.PUBLIC)
-        self.assertEqual(
-            'Some bugs are public.  This project will not be fully '
-            'Embargoed until they have been changed.', warning)
-
-    def test_getPublicWarning_transition(self):
-        # The warning is emitted only when transitioning from PUBLIC to a
-        # proprietary type.
-        owner = self.factory.makePerson()
-        product = self.factory.makeProduct(owner=owner)
-        self.factory.makeBug(target=product)
-        view = create_initialized_view(product, '+edit')
-        for from_type in PUBLIC_PROPRIETARY_INFORMATION_TYPES:
-            for to_type in PUBLIC_PROPRIETARY_INFORMATION_TYPES:
-                with person_logged_in(owner):
-                    product.information_type = to_type
-                warning = view.getPublicWarning(from_type)
-                if (to_type in PROPRIETARY_INFORMATION_TYPES and
-                    from_type == InformationType.PUBLIC):
-                    self.assertIsNot(None, warning)
-                else:
-                    self.assertIs(None, warning)
-
 
 class ProductSetReviewLicensesViewTestCase(TestCaseWithFactory):
     """Tests the ProductSetReviewLicensesView."""

=== modified file 'lib/lp/registry/model/product.py'
--- lib/lp/registry/model/product.py	2012-11-19 18:23:06 +0000
+++ lib/lp/registry/model/product.py	2012-11-23 22:13:26 +0000
@@ -111,6 +111,7 @@
     BUG_POLICY_DEFAULT_TYPES,
     )
 from lp.bugs.interfaces.bugtaskfilter import OrderedBugTask
+from lp.bugs.model.bug import Bug
 from lp.bugs.model.bugtarget import (
     BugTargetBase,
     OfficialBugTagTargetMixin,
@@ -460,27 +461,53 @@
         if value in PROPRIETARY_INFORMATION_TYPES:
             if self.answers_usage == ServiceUsage.LAUNCHPAD:
                 raise CannotChangeInformationType('Answers is enabled.')
+        if self._SO_creating or value not in PROPRIETARY_INFORMATION_TYPES:
+            return value
+        # Additional checks when transitioning an existing product to a
+        # proprietary type
+        # All specs located by an ALL search are public.
+        public_specs = self.specifications(
+            None, filter=[SpecificationFilter.ALL])
+        if not public_specs.is_empty():
+            # Unlike bugs and branches, specifications cannot be USERDATA or a
+            # security type.
+            raise CannotChangeInformationType(
+                'Some blueprints are public.')
+        non_proprietary_bugs = Store.of(self).find(Bug,
+            Not(Bug.information_type.is_in(PROPRIETARY_INFORMATION_TYPES)),
+            BugTask.bug == Bug.id, BugTask.product == self.id)
+        if not non_proprietary_bugs.is_empty():
+            raise CannotChangeInformationType(
+                'Some bugs are neither proprietary nor embargoed.')
+        # Default returns all public branches.
+        non_proprietary_branches = Store.of(self).find(
+            Branch, Branch.product == self.id,
+            Not(Branch.information_type.is_in(PROPRIETARY_INFORMATION_TYPES))
+        )
+        if not non_proprietary_branches.is_empty():
+            raise CannotChangeInformationType(
+                'Some branches are neither proprietary nor embargoed.')
+        if not self.packagings.is_empty():
+            raise CannotChangeInformationType('Some series are packaged.')
+        if self.translations_usage == ServiceUsage.LAUNCHPAD:
+            raise CannotChangeInformationType('Translations are enabled.')
         # Proprietary check works only after creation, because during
         # creation, has_commercial_subscription cannot give the right value
         # and triggers an inappropriate DB flush.
 
         # If you're changing the license, and setting a PROPRIETARY
-        # information type, yet you don't have a subscription you get one when
-        # the license is set.
+        # information type, yet you don't have a subscription, you get one
+        # when the license is set.
+
+        # Create the complimentary commercial subscription for the product.
+        self._ensure_complimentary_subscription()
 
         # If you have a commercial subscription, but it's not current, you
         # cannot set the information type to a PROPRIETARY type.
-        if not self._SO_creating and value in PROPRIETARY_INFORMATION_TYPES:
-            if not self.packagings.is_empty():
-                raise CannotChangeInformationType('Some series are packaged.')
-            # Create the complimentary commercial subscription for the product.
-            self._ensure_complimentary_subscription()
-
-            if not self.has_current_commercial_subscription:
-                raise CommercialSubscribersOnly(
-                    'A valid commercial subscription is required for private'
-                    ' Projects.')
-
+        if not self.has_current_commercial_subscription:
+            raise CommercialSubscribersOnly(
+                'A valid commercial subscription is required for private'
+                ' Projects.')
         return value
 
     _information_type = EnumCol(

=== modified file 'lib/lp/registry/tests/test_milestone.py'
--- lib/lp/registry/tests/test_milestone.py	2012-11-14 16:24:59 +0000
+++ lib/lp/registry/tests/test_milestone.py	2012-11-23 22:13:26 +0000
@@ -520,27 +520,6 @@
         self.assertNotIn(
             specification, list(milestone.getSpecifications(None)))
 
-    def test_bugtasks_milestone_privacy(self):
-        # Ensure bugtasks respects milestone privacy.
-        # This looks wrong, because the bugtask is actually public, and we
-        # don't normally hide bugtasks based on the visibility of their
-        # products.  But we're not trying to hide the bugtask.  We're hiding
-        # the fact that this bugtask is associated with a proprietary Product
-        # milestone.  We create a proprietary product because that's the only
-        # way to get a proprietary milestone.
-        milestone, target_milestone, owner = self.makeMixedMilestone()
-        with person_logged_in(owner):
-            product = target_milestone.product
-            product.information_type = InformationType.PUBLIC
-            product.setBugSharingPolicy(BugSharingPolicy.PUBLIC)
-            bugtask = self.factory.makeBugTask(
-                target=target_milestone.product)
-            product.information_type = InformationType.PROPRIETARY
-        with person_logged_in(bugtask.owner):
-            bugtask.transitionToMilestone(target_milestone, owner)
-        self.assertContentEqual([], milestone.bugtasks(None))
-        self.assertContentEqual([bugtask], milestone.bugtasks(owner))
-
     def test_milestones_with_deleted_workitems(self):
         # Deleted work items do not cause the specification to show up
         # in the milestone page.

=== modified file 'lib/lp/registry/tests/test_product.py'
--- lib/lp/registry/tests/test_product.py	2012-11-19 06:26:32 +0000
+++ lib/lp/registry/tests/test_product.py	2012-11-23 22:13:26 +0000
@@ -391,6 +391,63 @@
         expected = [InformationType.USERDATA, InformationType.PRIVATESECURITY]
         self.assertContentEqual(expected, [policy.type for policy in aps])
 
+    def test_change_info_type_proprietary_check_artifacts(self):
+        # Cannot change product information_type if any artifacts are public.
+        spec_policy = SpecificationSharingPolicy.PUBLIC_OR_PROPRIETARY
+        product = self.factory.makeProduct(
+            licenses=[License.OTHER_PROPRIETARY],
+            specification_sharing_policy=spec_policy,
+            bug_sharing_policy=BugSharingPolicy.PUBLIC_OR_PROPRIETARY,
+            branch_sharing_policy=BranchSharingPolicy.PUBLIC_OR_PROPRIETARY,
+        )
+        self.useContext(person_logged_in(product.owner))
+        spec = self.factory.makeSpecification(product=product)
+        for info_type in PROPRIETARY_INFORMATION_TYPES:
+            with ExpectedException(
+                CannotChangeInformationType,
+                'Some blueprints are public.'):
+                product.information_type = info_type
+        spec.transitionToInformationType(InformationType.PROPRIETARY,
+                                         product.owner)
+        bug = self.factory.makeBug(target=product)
+        for bug_info_type in FREE_INFORMATION_TYPES:
+            bug.transitionToInformationType(bug_info_type, product.owner)
+            for info_type in PROPRIETARY_INFORMATION_TYPES:
+                with ExpectedException(
+                    CannotChangeInformationType,
+                    'Some bugs are neither proprietary nor embargoed.'):
+                    product.information_type = info_type
+        bug.transitionToInformationType(InformationType.PROPRIETARY,
+                                        product.owner)
+        branch = self.factory.makeBranch(product=product)
+        for branch_info_type in FREE_INFORMATION_TYPES:
+            branch.transitionToInformationType(branch_info_type,
+                                               product.owner)
+            for info_type in PROPRIETARY_INFORMATION_TYPES:
+                with ExpectedException(
+                    CannotChangeInformationType,
+                    'Some branches are neither proprietary nor embargoed.'):
+                    product.information_type = info_type
+        branch.transitionToInformationType(InformationType.PROPRIETARY,
+                                           product.owner)
+        for info_type in PROPRIETARY_INFORMATION_TYPES:
+            product.information_type = info_type
+
+    def test_change_info_type_proprietary_check_translations(self):
+        product = self.factory.makeProduct(
+            licenses=[License.OTHER_PROPRIETARY])
+        with person_logged_in(product.owner):
+            for usage in ServiceUsage:
+                product.translations_usage = usage.value
+                for info_type in PROPRIETARY_INFORMATION_TYPES:
+                    if product.translations_usage == ServiceUsage.LAUNCHPAD:
+                        with ExpectedException(
+                            CannotChangeInformationType,
+                            'Translations are enabled.'):
+                            product.information_type = info_type
+                    else:
+                        product.information_type = info_type
+
     def test_change_info_type_proprietary_sets_policies(self):
         # Changing information type from public to proprietary sets the
         # appropriate policies

=== modified file 'lib/lp/translations/doc/translationsoverview.txt'
--- lib/lp/translations/doc/translationsoverview.txt	2012-10-24 13:52:59 +0000
+++ lib/lp/translations/doc/translationsoverview.txt	2012-11-23 22:13:26 +0000
@@ -180,8 +180,8 @@
 Private projects are never included.
 
     >>> from lp.app.enums import InformationType
-    >>> removeSecurityProxy(upstart).information_type = (
-    ...     InformationType.PROPRIETARY)
+    >>> upstart.translations_usage = ServiceUsage.NOT_APPLICABLE
+    >>> upstart.information_type = InformationType.PROPRIETARY
     >>> display_pillars(overview.getMostTranslatedPillars())
     alsa-utils: 22
     Evolution: 20