Merge lp:~flacoste/launchpad/bug-559128 into lp:launchpad

[Francis J. Lacoste]

Don't import lp.testing in production code. Refactor login and login_person helper into setupInteractionByEmail and setupInteractionForPerson that can be used in production code.

= Summary =

This branch should re-enable edge update (bug #559128) by fixing some tech-debt
(bug #285808)

The problem with the edge update is that subunit is now installed via a
package and that package isn't installed in production. You'd wonder why
subunit would be required for running Launchpad and that's exactly the point.

It turns out that some production code paths import from lp.testing because
they use the login and login_person helper (which was bug #285808).

== Proposed fix ==

Remove tech-debts

== Pre-implementation notes ==

Nothing special.

== Implementation details ==

I moved the useful part of login and login_person into two functions
setupInteractionByEmail and setupInteractionForPerson. Those lives alongside
setupInteraction in canonical.launchpad.webapp.interaction.

The login and login_person() contains the test-specific logic around these.

I updated all production call sites to use the one from
canonical.launchpad.webapp.interaction

Since execute_zcml_for_scripts also imported testing code, i wrapped that one
only if the layers module has been imported. I would have removed it entirely,
but thought that the safety hook was worth the relative ugliness.

== Tests ==

This branch is a pure refactoring. I did test that lp.testing wasn't imported
anymore in production paths by adding a raise RuntimeError in lp.testing, it's
not trigger when calling make run. (It is before this fix.)

== Demo and Q/A ==

Nothing special

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/canonical/launchpad/scripts/__init__.py
  lib/lp/services/scripts/base.py
  scripts/sourceforge-import.py
  lib/lp/code/xmlrpc/codehosting.py
  lib/lp/bugs/scripts/checkwatches/updater.py
  lib/lp/testing/__init__.py
  lib/canonical/launchpad/webapp/interaction.py
  scripts/bugzilla-import.py
  lib/lp/testing/_webservice.py
  lib/lp/testing/_login.py

== Pyflakes notices ==

lib/lp/testing/_login.py
    8: 'getUtility' imported but unused

I fixed that one on a follow-up commit.

--
Francis J. Lacoste
<email address hidden>

[Gary Poster (gary)]

Wow, very nice!

"Zopefull": nice word :-)

Very nice cleanups in the branch, such as correcting the imports from zope.security.

A niggle, but I found it a bit odd when ``setupInteractionForPerson`` called ``return setupInteractionByEmail(ANONYMOUS, participation)``. I wondered why I should call a function with the word "email" in it to get an interaction when there's no email involved. My preference would be to change setupInteraction to look like this:

def setupInteraction(principal, login=None, participation=None):
    """Sets up a new interaction with the given principal.

    The login gets added to the launch bag.

    You can optionally pass in a participation to be used. If no
    participation is given, a Participation is used.
    """
    # If principal is None, this method acts just like endInteraction.
    if principal is None:
        endInteraction()
        return

    # THIS IS THE NEW BIT
    if principal == ANONYMOUS:
        authutil = getUtility(IPlacelessAuthUtility)
        principal = authutil.unauthenticatedPrincipal()

    if participation is None:
        participation = Participation()
(... and so on, as is)

Then the line in `setupInteractionForPerson`` would read ``return setupInteraction(ANONYMOUS)``. That makes much more sense to me. I'd guess you'd also delete these lines from ``setupInteractionByEmail`` too, because they would be unnecessary:

119 + else:
120 + principal = authutil.unauthenticatedPrincipal()

As I said, that's a niggle, and I won't insist on it, but it did give me pause.

Looks like you caught all my XXXs. :-) Yay!

Thank you,

Gary

review: Approve

[Francis J. Lacoste (flacoste)]

On April 15, 2010, Gary Poster wrote:
> A niggle, but I found it a bit odd when ``setupInteractionForPerson``
> called ``return setupInteractionByEmail(ANONYMOUS, participation)``. I
> wondered why I should call a function with the word "email" in it to get
> an interaction when there's no email involved. My preference would be to
> change setupInteraction to look like this:
>
Well, I guess, I could call setupInteraction directly in that case. I'll do
this.

> def setupInteraction(principal, login=None, participation=None):
> """Sets up a new interaction with the given principal.

This changes the signature of the function. I'll pass on it since I don't feel
like chasing all the call sites :-)

Cheers

--
Francis J. Lacoste
<email address hidden>

[Francis J. Lacoste (flacoste)]

On April 15, 2010, Francis J. Lacoste wrote:
> On April 15, 2010, Gary Poster wrote:
> > A niggle, but I found it a bit odd when ``setupInteractionForPerson``
> > called ``return setupInteractionByEmail(ANONYMOUS, participation)``. I
> > wondered why I should call a function with the word "email" in it to get
> > an interaction when there's no email involved. My preference would be to
>
> > change setupInteraction to look like this:
> Well, I guess, I could call setupInteraction directly in that case. I'll do
> this.
>
> > def setupInteraction(principal, login=None, participation=None):
> > """Sets up a new interaction with the given principal.
>
> This changes the signature of the function. I'll pass on it since I don't
> feel like chasing all the call sites :-)
>
> Cheers

Disregard this, your suggestion makes perfect sense and I'll implement it.

/me shouldn't reply before the context restore has finished...

--
Francis J. Lacoste
<email address hidden>

=== modified file 'lib/canonical/launchpad/scripts/__init__.py'
--- lib/canonical/launchpad/scripts/__init__.py	2010-04-09 12:31:20 +0000
+++ lib/canonical/launchpad/scripts/__init__.py	2010-04-17 00:32:28 +0000
@@ -20,6 +20,7 @@
 
 import atexit
 import os
+import sys
 from textwrap import dedent
 import threading
 
@@ -32,6 +33,8 @@
 from zope.security.simplepolicies import PermissiveSecurityPolicy
 
 from canonical.launchpad.webapp.authorization import LaunchpadSecurityPolicy
+from canonical.launchpad.webapp.interaction import (
+    ANONYMOUS, setupInteractionByEmail)
 
 from canonical import lp
 from canonical.config import config
@@ -91,15 +94,18 @@
     application uses will be used. Otherwise everything protected by a
     permission is allowed, and everything else denied.
     """
-    # Prevent some cases of erroneous layer useage.
-    from canonical.testing import (
-            FunctionalLayer, BaseLayer, ZopelessLayer
-            )
-    assert not FunctionalLayer.isSetUp, \
-            'Setting up Zopeless CA when Zopefull CA is already running'
-    assert not BaseLayer.isSetUp or ZopelessLayer.isSetUp, """
-            execute_zcml_for_scripts should not be called from tests.
-            Instead, your test should use the Zopeless layer.
+
+    # When in testing mode, prevent some cases of erroneous layer usage.
+    # But we don't want to import that module in production usage, thus
+    # the conditional block.
+    if 'canonical.testing.layers' in sys.modules:
+        from canonical.testing.layers import (
+                FunctionalLayer, BaseLayer, ZopelessLayer)
+        assert not FunctionalLayer.isSetUp, \
+                'Setting up Zopeless CA when Zopefull CA is already running'
+        assert not BaseLayer.isSetUp or ZopelessLayer.isSetUp, """
+                execute_zcml_for_scripts should not be called from tests.
+                Instead, your test should use the Zopeless layer.
             """
 
     if config.instance_name == 'testrunner':
@@ -147,15 +153,7 @@
     # the proper API for having a principal / user running in scripts.
     # The script will have full permissions because of the
     # PermissiveSecurityPolicy set up in script.zcml.
-    # XXX gary 20-Oct-2008 bug 285808
-    # The wisdom of using a test fixture for production should be
-    # reconsidered.
-    from canonical.launchpad.ftests import login
-    # The Participation is used to specify that we do not want a
-    # LaunchpadTestRequest, which ftests normally use. shipit scripts, in
-    # particular, need to be careful, because of code in canonical_url.
-    from canonical.launchpad.webapp.interaction import Participation
-    login('launchpad.anonymous', Participation())
+    setupInteractionByEmail(ANONYMOUS)
 
 
 def db_options(parser):

=== modified file 'lib/canonical/launchpad/webapp/interaction.py'
--- lib/canonical/launchpad/webapp/interaction.py	2009-06-25 05:30:52 +0000
+++ lib/canonical/launchpad/webapp/interaction.py	2010-04-17 00:32:28 +0000
@@ -13,7 +13,20 @@
 from zope.security.management import (
     endInteraction, newInteraction, queryInteraction)
 
-from canonical.launchpad.webapp.interfaces import IOpenLaunchBag
+from canonical.launchpad.webapp.interfaces import (
+    IOpenLaunchBag, IPlacelessAuthUtility)
+
+
+__all__ = [
+    'ANONYMOUS',
+    'get_current_principal',
+    'setupInteraction',
+    'setupInteractionByEmail',
+    'setupInteractionForPerson',
+    ]
+
+
+ANONYMOUS = 'launchpad.anonymous'
 
 
 def get_current_principal():
@@ -40,6 +53,10 @@
         endInteraction()
         return
 
+    if principal == ANONYMOUS:
+        authutil = getUtility(IPlacelessAuthUtility)
+        principal = authutil.unauthenticatedPrincipal()
+
     if participation is None:
         participation = Participation()
 
@@ -61,7 +78,51 @@
         participation.principal = principal
 
 
+def setupInteractionByEmail(email, participation=None):
+    """Setup an interaction using an email.
+
+    If the ANONYMOUS constant is supplied as the email,
+    an interaction for the anonymous user will be used.
+
+    You can optionally pass in a participation to be used.  If no
+    participation is given, an empty participation is used.
+
+    If the participation provides IPublicationRequest, it must implement
+    setPrincipal(), otherwise it must allow setting its principal attribute.
+    """
+    authutil = getUtility(IPlacelessAuthUtility)
+
+    if email != ANONYMOUS:
+        # Create an anonymous interaction first because this calls
+        # IPersonSet.getByEmail() and since this is security wrapped, it needs
+        # an interaction available.
+        setupInteraction(authutil.unauthenticatedPrincipal())
+        principal = authutil.getPrincipalByLogin(email, want_password=False)
+        assert principal is not None, "Invalid login"
+        if principal.person is not None and principal.person.is_team:
+            raise AssertionError("Please do not try to login as a team")
+    else:
+        principal = authutil.unauthenticatedPrincipal()
+
+    if participation is None:
+        participation = Participation()
+
+    setupInteraction(principal, login=email, participation=participation)
+
+
+def setupInteractionForPerson(person, participation=None):
+    """Setup a participation for a person."""
+    from zope.security.proxy import removeSecurityProxy
+    if person is None:
+        return setupInteraction(ANONYMOUS, participation)
+    else:
+        # Bypass zope's security because IEmailAddress.email is not public.
+        naked_email = removeSecurityProxy(person.preferredemail)
+        return setupInteractionByEmail(naked_email.email, participation)
+
+
 class Participation:
+    """A very simple participation."""
     implements(IParticipation)
 
     interaction = None

=== modified file 'lib/lp/bugs/scripts/checkwatches/updater.py'
--- lib/lp/bugs/scripts/checkwatches/updater.py	2010-04-12 16:06:11 +0000
+++ lib/lp/bugs/scripts/checkwatches/updater.py	2010-04-17 00:32:28 +0000
@@ -37,6 +37,8 @@
 
 from zope.component import getUtility
 from zope.event import notify
+from zope.security.management import (
+    endInteraction, queryInteraction)
 
 from canonical.database.constants import UTC_NOW
 from canonical.database.sqlbase import flush_database_updates
@@ -54,9 +56,8 @@
     clear_request_started, get_request_start_time, set_request_started)
 from canonical.launchpad.webapp.errorlog import (
     ErrorReportingUtility, ScriptRequest)
+from canonical.launchpad.webapp.interaction import setupInteraction
 from canonical.launchpad.webapp.interfaces import IPlacelessAuthUtility
-from canonical.launchpad.webapp.interaction import (
-    setupInteraction, endInteraction, queryInteraction)
 from canonical.launchpad.webapp.publisher import canonical_url
 
 from lp.bugs import externalbugtracker

=== modified file 'lib/lp/code/xmlrpc/codehosting.py'
--- lib/lp/code/xmlrpc/codehosting.py	2010-02-24 04:24:01 +0000
+++ lib/lp/code/xmlrpc/codehosting.py	2010-04-17 00:32:28 +0000
@@ -21,8 +21,8 @@
 from zope.interface import implements
 from zope.security.interfaces import Unauthorized
 from zope.security.proxy import removeSecurityProxy
+from zope.security.management import endInteraction
 
-from canonical.launchpad.ftests import login_person, logout
 from lp.code.errors import UnknownBranchTypeError
 from lp.code.enums import BranchType
 from lp.code.interfaces.branch import BranchCreationException
@@ -40,11 +40,11 @@
 from canonical.launchpad.validators import LaunchpadValidationError
 from canonical.launchpad.webapp import LaunchpadXMLRPCView
 from canonical.launchpad.webapp.authorization import check_permission
+from canonical.launchpad.webapp.interaction import setupInteractionForPerson
 from canonical.launchpad.webapp.interfaces import (
     NameLookupFailed, NotFoundError)
 from canonical.launchpad.xmlrpc import faults
 from canonical.launchpad.xmlrpc.helpers import return_fault
-from canonical.launchpad.webapp.interaction import Participation
 
 
 UTC = pytz.timezone('UTC')
@@ -179,15 +179,11 @@
         requester = getUtility(IPersonSet).get(login_id)
     if requester is None:
         raise NotFoundError("No person with id %s." % login_id)
-    # XXX gary 21-Oct-2008 bug 285808
-    # We should reconsider using a ftest helper for production code.  For now,
-    # we explicitly keep the code from using a test request by using a basic
-    # participation.
-    login_person(requester, Participation())
+    setupInteractionForPerson(requester)
     try:
         return function(requester, *args, **kwargs)
     finally:
-        logout()
+        endInteraction()
 
 
 class BranchFileSystem(LaunchpadXMLRPCView):

=== modified file 'lib/lp/services/scripts/base.py'
--- lib/lp/services/scripts/base.py	2009-09-21 08:13:40 +0000
+++ lib/lp/services/scripts/base.py	2010-04-17 00:32:28 +0000
@@ -23,8 +23,9 @@
 from canonical.database.sqlbase import ISOLATION_LEVEL_DEFAULT
 from canonical.launchpad import scripts
 from canonical.launchpad.interfaces import IScriptActivitySet
+from canonical.launchpad.webapp.interaction import (
+    ANONYMOUS, setupInteractionByEmail)
 from canonical.lp import initZopeless
-from lp.testing import ANONYMOUS
 
 
 LOCK_PATH = "/var/lock/"
@@ -159,18 +160,7 @@
 
     def login(self, user):
         """Super-convenience method that avoids the import."""
-        # This import is actually quite expensive, and causes us to
-        # import circularly in pathological cases.
-        # XXX gary 20-Oct-2008 bug 285808
-        # The wisdom of using a test fixture for production should be
-        # reconsidered.
-        from canonical.launchpad.ftests import login
-        # The Participation is used to specify that we do not want a
-        # LaunchpadTestRequest, which ftests normally use.  shipit scripts,
-        # in particular, need to be careful, because of code in
-        # canonical_url.
-        from canonical.launchpad.webapp.interaction import Participation
-        login(user, Participation())
+        setupInteractionByEmail(user)
 
     #
     # Locking and running methods. Users only call these explicitly if

=== modified file 'lib/lp/testing/__init__.py'
--- lib/lp/testing/__init__.py	2010-04-15 14:38:43 +0000
+++ lib/lp/testing/__init__.py	2010-04-17 00:32:28 +0000
@@ -74,13 +74,14 @@
 
 from canonical.launchpad.webapp import errorlog
 from canonical.config import config
+from canonical.launchpad.webapp.interaction import ANONYMOUS
 from canonical.launchpad.webapp.interfaces import ILaunchBag
 from canonical.launchpad.windmill.testing import constants
 from lp.codehosting.vfs import branch_id_to_path, get_multi_server
 # Import the login and logout functions here as it is a much better
 # place to import them from in tests.
 from lp.testing._login import (
-    ANONYMOUS, is_logged_in, login, login_person, logout)
+    is_logged_in, login, login_person, logout)
 # canonical.launchpad.ftests expects test_tales to be imported from here.
 # XXX: JonathanLange 2010-01-01: Why?!
 from lp.testing._tales import test_tales

=== modified file 'lib/lp/testing/_login.py'
--- lib/lp/testing/_login.py	2009-06-25 04:06:00 +0000
+++ lib/lp/testing/_login.py	2010-04-17 00:32:28 +0000
@@ -5,10 +5,9 @@
 # pylint: disable-msg=W0602,W0603
 __metaclass__ = type
 
-from zope.component import getUtility
 from zope.security.management import endInteraction
-from canonical.launchpad.webapp.interfaces import IPlacelessAuthUtility
-from canonical.launchpad.webapp.interaction import setupInteraction
+from canonical.launchpad.webapp.interaction import (
+    setupInteractionByEmail, setupInteractionForPerson)
 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
 from canonical.launchpad.webapp.vhosts import allvhosts
 
@@ -16,18 +15,33 @@
     'login',
     'login_person',
     'logout',
-    'ANONYMOUS',
     'is_logged_in']
 
 
-ANONYMOUS = 'launchpad.anonymous'
-
 _logged_in = False
 
 def is_logged_in():
     global _logged_in
     return _logged_in
 
+
+def _test_login_impl(participation):
+    # Common implementation of the test login wrappers.
+    # It sets the global _logged_in flag and create a default
+    # participation if None was specified.
+    global _logged_in
+    _logged_in = True
+
+    if participation is None:
+        # we use the main site as the host name.  This is a guess, to make
+        # canonical_url produce a real-looking host name rather than
+        # 127.0.0.1.
+        participation = LaunchpadTestRequest(
+            environ={'HTTP_HOST': allvhosts.configs['mainsite'].hostname,
+                     'SERVER_URL': allvhosts.configs['mainsite'].rooturl})
+    return participation
+
+
 def login(email, participation=None):
     """Simulates a login, using the specified email.
 
@@ -40,42 +54,15 @@
     If the participation provides IPublicationRequest, it must implement
     setPrincipal(), otherwise it must allow setting its principal attribute.
     """
-    global _logged_in
-    _logged_in = True
-    authutil = getUtility(IPlacelessAuthUtility)
-
-    if email != ANONYMOUS:
-        # Create an anonymous interaction first because this calls
-        # IPersonSet.getByEmail() and since this is security wrapped, it needs
-        # an interaction available.
-        setupInteraction(authutil.unauthenticatedPrincipal())
-        principal = authutil.getPrincipalByLogin(email, want_password=False)
-        assert principal is not None, "Invalid login"
-        if principal.person is not None and principal.person.is_team:
-            raise AssertionError("Please do not try to login as a team")
-    else:
-        principal = authutil.unauthenticatedPrincipal()
-
-    if participation is None:
-        # we use the main site as the host name.  This is a guess, to make
-        # canonical_url produce a real-looking host name rather than
-        # 127.0.0.1.
-        participation = LaunchpadTestRequest(
-            environ={'HTTP_HOST': allvhosts.configs['mainsite'].hostname,
-                     'SERVER_URL': allvhosts.configs['mainsite'].rooturl})
-
-    setupInteraction(principal, login=email, participation=participation)
+
+    participation = _test_login_impl(participation)
+    setupInteractionByEmail(email, participation)
 
 
 def login_person(person, participation=None):
     """Login the person with their preferred email."""
-    from zope.security.proxy import removeSecurityProxy
-    if person is None:
-        return login(ANONYMOUS, participation)
-    else:
-        # Bypass zope's security because IEmailAddress.email is not public.
-        naked_email = removeSecurityProxy(person.preferredemail)
-        return login(naked_email.email, participation)
+    participation = _test_login_impl(participation)
+    setupInteractionForPerson(person, participation)
 
 
 def logout():

=== modified file 'lib/lp/testing/_webservice.py'
--- lib/lp/testing/_webservice.py	2010-03-30 09:18:53 +0000
+++ lib/lp/testing/_webservice.py	2010-04-17 00:32:28 +0000
@@ -15,11 +15,12 @@
 from launchpadlib.credentials import AccessToken, Credentials
 from launchpadlib.launchpad import Launchpad
 
+from canonical.launchpad.webapp.interaction import ANONYMOUS
 from canonical.launchpad.webapp.interfaces import OAuthPermission
 from canonical.launchpad.interfaces import (
     IOAuthConsumerSet, IPersonSet)
 
-from lp.testing._login import ANONYMOUS, login, logout
+from lp.testing._login import login, logout
 
 def oauth_access_token_for(consumer_name, person, permission, context=None):
     """Find or create an OAuth access token for the given person.

=== modified file 'scripts/bugzilla-import.py'
--- scripts/bugzilla-import.py	2010-02-16 15:25:52 +0000
+++ scripts/bugzilla-import.py	2010-04-17 00:32:28 +0000
@@ -15,8 +15,7 @@
 from canonical.lp import initZopeless
 from canonical.launchpad.scripts import (
     execute_zcml_for_scripts, logger_options, logger)
-from canonical.launchpad.ftests import login
-from canonical.launchpad.webapp.interaction import Participation
+from canonical.launchpad.webapp.interaction import setupInteractionByEmail
 
 from canonical.launchpad.scripts import bugzilla
 
@@ -81,11 +80,7 @@
 
     execute_zcml_for_scripts()
     ztm = initZopeless()
-    # XXX gary 21-Oct-2008 bug 285808
-    # We should reconsider using a ftest helper for production code.  For now,
-    # we explicitly keep the code from using a test request by using a basic
-    # participation.
-    login('[e-mail]', Participation())
+    setupInteractionByEmail('[e-mail]')
 
     db = make_connection(options)
     bz = bugzilla.Bugzilla(db)

=== modified file 'scripts/sourceforge-import.py'
--- scripts/sourceforge-import.py	2010-02-16 15:25:52 +0000
+++ scripts/sourceforge-import.py	2010-04-17 00:32:28 +0000
@@ -16,8 +16,7 @@
 from canonical.launchpad.interfaces import IProductSet
 from canonical.launchpad.scripts import (
     execute_zcml_for_scripts, logger_options, logger)
-from canonical.launchpad.ftests import login
-from canonical.launchpad.webapp.interaction import Participation
+from canonical.launchpad.webapp.interaction import setupInteractionByEmail
 
 from canonical.launchpad.scripts.sftracker import Tracker, TrackerImporter
 
@@ -52,11 +51,7 @@
 
     execute_zcml_for_scripts()
     ztm = initZopeless()
-    # XXX gary 21-Oct-2008 bug 285808
-    # We should reconsider using a ftest helper for production code.  For now,
-    # we explicitly keep the code from using a test request by using a basic
-    # participation.
-    login('[e-mail]', Participation())
+    setupInteractionByEmail('[e-mail]')
 
     product = getUtility(IProductSet).getByName(options.product)
     tracker = Tracker(options.dumpfile, options.dumpdir)