Merge lp:~wgrant/launchpad/hardcoded-password into lp:launchpad

[William Grant]

[r=stevenk][no-qa] Disable basic auth outside the testrunner, and use 'test' as the password instead of looking things up in AccountPassword.

This branch drops most of the remaining password support in Launchpad. The major changes are in lp.services.webapp.authentication, where I've disabled basic authentication outside the testrunner, added several more paranoid is-testrunner checks, and replaced the password validation with a string match against 'test'.

[Steve Kowalik (stevenk)]

I'm a little nervous about this change, but having said that, as long as this passes ec2, I'm okay with it, since, as you say it won't get anywhere near production.

review: Approve (code)

=== modified file 'configs/testrunner/launchpad-lazr.conf'
--- configs/testrunner/launchpad-lazr.conf	2012-01-01 02:25:04 +0000
+++ configs/testrunner/launchpad-lazr.conf	2012-01-18 06:45:31 +0000
@@ -114,6 +114,7 @@
 max_scaling: 2
 
 [launchpad]
+basic_auth_password: test
 max_attachment_size: 1024
 geoip_database: /usr/share/GeoIP/GeoLiteCity.dat
 logparser_max_parsed_lines: 100000

=== modified file 'lib/lp/blueprints/browser/tests/test_sprint.py'
--- lib/lp/blueprints/browser/tests/test_sprint.py	2012-01-01 02:58:52 +0000
+++ lib/lp/blueprints/browser/tests/test_sprint.py	2012-01-18 06:45:31 +0000
@@ -26,4 +26,4 @@
                 True)
         Store.of(sprint).flush()
         Store.of(sprint).invalidate()
-        self.assertThat(sprint, BrowsesWithQueryLimit(15, sprint.owner))
+        self.assertThat(sprint, BrowsesWithQueryLimit(17, sprint.owner))

=== modified file 'lib/lp/bugs/scripts/bugexpire.py'
--- lib/lp/bugs/scripts/bugexpire.py	2012-01-01 02:58:52 +0000
+++ lib/lp/bugs/scripts/bugexpire.py	2012-01-18 06:45:31 +0000
@@ -120,8 +120,7 @@
         auth_utility = getUtility(IPlacelessAuthUtility)
         janitor_email = self.janitor.preferredemail.email
         setupInteraction(
-            auth_utility.getPrincipalByLogin(
-                janitor_email, want_password=False),
+            auth_utility.getPrincipalByLogin(janitor_email),
             login=janitor_email)
 
     def _logout(self):

=== modified file 'lib/lp/bugs/scripts/checkwatches/base.py'
--- lib/lp/bugs/scripts/checkwatches/base.py	2012-01-01 02:58:52 +0000
+++ lib/lp/bugs/scripts/checkwatches/base.py	2012-01-18 06:45:31 +0000
@@ -121,7 +121,7 @@
         self._login = login
         self._principal = (
             getUtility(IPlacelessAuthUtility).getPrincipalByLogin(
-                self._login, want_password=False))
+                self._login))
         self._transaction_manager = transaction_manager
         self.logger = logger
 

=== modified file 'lib/lp/services/config/schema-lazr.conf'
--- lib/lp/services/config/schema-lazr.conf	2012-01-06 11:08:30 +0000
+++ lib/lp/services/config/schema-lazr.conf	2012-01-18 06:45:31 +0000
@@ -894,6 +894,12 @@
 # datatype: boolean
 enable_test_openid_provider: False
 
+# The password to accept for all users for HTTP basic auth.
+# If None, HTTP basic auth is disabled
+# Obviously this should never be set in anything even vaguely
+# resembling a production enviornment.
+basic_auth_password: none
+
 # Assume the slave database is lagged if it takes more than this many
 # milliseconds to calculate this information from the Slony-I tables.
 # datatype: integer

=== modified file 'lib/lp/services/webapp/authentication.py'
--- lib/lp/services/webapp/authentication.py	2012-01-03 11:08:31 +0000
+++ lib/lp/services/webapp/authentication.py	2012-01-18 06:45:31 +0000
@@ -64,13 +64,21 @@
         self.nobody.__parent__ = self
 
     def _authenticateUsingBasicAuth(self, credentials, request):
+        # authenticate() only attempts basic auth if it's enabled. But
+        # recheck here, just in case. There is a single password for all
+        # users, so this must never get anywhere near production!
+        if (not config.launchpad.basic_auth_password
+            or config.launchpad.basic_auth_password.lower() == 'none'):
+            raise AssertionError(
+                "Attempted to use basic auth when it is disabled")
+
         login = credentials.getLogin()
         if login is not None:
             login_src = getUtility(IPlacelessLoginSource)
             principal = login_src.getPrincipalByLogin(login)
             if principal is not None and principal.person.is_valid_person:
                 password = credentials.getPassword()
-                if principal.validate(password):
+                if password == config.launchpad.basic_auth_password:
                     # We send a LoggedInEvent here, when the
                     # cookie auth below sends a PrincipalIdentified,
                     # as the login form is never visited for BasicAuth.
@@ -130,7 +138,8 @@
             # encoded properly. That's a client error, so we don't really
             # care, and we're done.
             raise Unauthorized("Bad Basic authentication.")
-        if credentials is not None and credentials.getLogin() is not None:
+        if (config.launchpad.basic_auth_password and credentials is not None
+            and credentials.getLogin() is not None):
             return self._authenticateUsingBasicAuth(credentials, request)
         else:
             # Hack to make us not even think of using a session if there
@@ -166,10 +175,9 @@
         utility = getUtility(IPlacelessLoginSource)
         return utility.getPrincipals(name)
 
-    def getPrincipalByLogin(self, login, want_password=True):
+    def getPrincipalByLogin(self, login):
         """See IAuthenticationService."""
-        utility = getUtility(IPlacelessLoginSource)
-        return utility.getPrincipalByLogin(login, want_password=want_password)
+        return getUtility(IPlacelessLoginSource).getPrincipalByLogin(login)
 
 
 class SSHADigestEncryptor:
@@ -251,16 +259,10 @@
 
     def getPrincipalByLogin(self, login,
                             access_level=AccessLevel.WRITE_PRIVATE,
-                            scope=None, want_password=True):
+                            scope=None):
         """Return a principal based on the account with the email address
         signified by "login".
 
-        :param want_password: If want_password is False, the pricipal
-        will have None for a password. Use this when trying to retrieve a
-        principal in contexts where we don't need the password and the
-        database connection does not have access to the Account or
-        AccountPassword tables.
-
         :return: None if there is no account with the given email address.
 
         The `access_level` can be used for further restricting the capability
@@ -279,27 +281,18 @@
         person = getUtility(IPersonSet).getByEmail(login)
         if person is None or person.account is None:
             return None
-        return self._principalForAccount(
-            person.account, access_level, scope, want_password)
+        return self._principalForAccount(person.account, access_level, scope)
 
-    def _principalForAccount(self, account, access_level, scope,
-                             want_password=True):
+    def _principalForAccount(self, account, access_level, scope):
         """Return a LaunchpadPrincipal for the given account.
 
         The LaunchpadPrincipal will also have the given access level and
         scope.
-
-        If want_password is True, the principal's password will be set to the
-        account's password.  Otherwise it's set to None.
         """
         naked_account = removeSecurityProxy(account)
-        if want_password:
-            password = naked_account.password
-        else:
-            password = None
         principal = LaunchpadPrincipal(
             naked_account.id, naked_account.displayname,
-            naked_account.displayname, account, password,
+            naked_account.displayname, account,
             access_level=access_level, scope=scope)
         principal.__parent__ = self
         return principal
@@ -315,7 +308,7 @@
 
     implements(ILaunchpadPrincipal)
 
-    def __init__(self, id, title, description, account, pwd=None,
+    def __init__(self, id, title, description, account,
                  access_level=AccessLevel.WRITE_PRIVATE, scope=None):
         self.id = id
         self.title = title
@@ -324,17 +317,10 @@
         self.scope = scope
         self.account = account
         self.person = IPerson(account, None)
-        self.__pwd = pwd
 
     def getLogin(self):
         return self.title
 
-    def validate(self, pw):
-        encryptor = getUtility(IPasswordEncryptor)
-        pw1 = (pw or '').strip()
-        pw2 = (self.__pwd or '').strip()
-        return encryptor.validate(pw1, pw2)
-
 
 # zope.app.apidoc expects our principals to be adaptable into IAnnotations, so
 # we use these dummy adapters here just to make that code not OOPS.

=== modified file 'lib/lp/services/webapp/interaction.py'
--- lib/lp/services/webapp/interaction.py	2011-12-24 17:49:30 +0000
+++ lib/lp/services/webapp/interaction.py	2012-01-18 06:45:31 +0000
@@ -143,7 +143,7 @@
         # IPersonSet.getByEmail() and since this is security wrapped, it needs
         # an interaction available.
         setupInteraction(authutil.unauthenticatedPrincipal())
-        principal = authutil.getPrincipalByLogin(email, want_password=False)
+        principal = authutil.getPrincipalByLogin(email)
         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")

=== modified file 'lib/lp/services/webapp/interfaces.py'
--- lib/lp/services/webapp/interfaces.py	2011-12-24 17:49:30 +0000
+++ lib/lp/services/webapp/interfaces.py	2012-01-18 06:45:31 +0000
@@ -494,11 +494,8 @@
     login name.
     """
 
-    def getPrincipalByLogin(login, want_password=True):
-        """Return a principal based on his login name.
-
-        The principal's password is set to None if want_password is False.
-        """
+    def getPrincipalByLogin(login):
+        """Return a principal based on his login name."""
 
 
 class IPlacelessLoginSource(IPrincipalSource):
@@ -507,15 +504,8 @@
     between the user id and login name.
     """
 
-    # want_password is temporary. Eventually we will have accounts
-    # without passwords at all, authenticated via other means such as external
-    # OpenID providers or SSL certificates. Principals having passwords
-    # doesn't really make sense.
-    def getPrincipalByLogin(login, want_password=True):
-        """Return a principal based on his login name.
-
-        If want_password is False, the principal's password is set to None.
-        """
+    def getPrincipalByLogin(login):
+        """Return a principal based on his login name."""
 
     def getPrincipals(name):
         """Not implemented.

=== modified file 'lib/lp/services/webapp/tests/test_authutility.py'
--- lib/lp/services/webapp/tests/test_authutility.py	2012-01-03 12:25:31 +0000
+++ lib/lp/services/webapp/tests/test_authutility.py	2012-01-18 06:45:31 +0000
@@ -4,8 +4,8 @@
 __metaclass__ = type
 
 import base64
-import unittest
 
+import testtools
 from zope.app.security.basicauthadapter import BasicAuthAdapter
 from zope.app.security.interfaces import ILoginPassword
 from zope.app.security.principalregistry import UnauthenticatedPrincipal
@@ -17,13 +17,13 @@
 from zope.publisher.interfaces.http import IHTTPCredentials
 
 from lp.registry.interfaces.person import IPerson
+from lp.services.config import config
 from lp.services.identity.interfaces.account import IAccount
 from lp.services.webapp.authentication import (
     LaunchpadPrincipal,
     PlacelessAuthUtility,
     )
 from lp.services.webapp.interfaces import (
-    IPasswordEncryptor,
     IPlacelessAuthUtility,
     IPlacelessLoginSource,
     )
@@ -39,14 +39,14 @@
     person = DummyPerson()
 
 
-Bruce = LaunchpadPrincipal(42, 'bruce', 'Bruce', DummyAccount(), 'bruce!')
+Bruce = LaunchpadPrincipal(42, 'bruce', 'Bruce', DummyAccount())
 Bruce.person = Bruce.account.person
 
 
 class DummyPlacelessLoginSource(object):
     implements(IPlacelessLoginSource)
 
-    def getPrincipalByLogin(self, id, want_password=True):
+    def getPrincipalByLogin(self, id):
         return Bruce
 
     getPrincipal = getPrincipalByLogin
@@ -55,18 +55,11 @@
         return [Bruce]
 
 
-class DummyPasswordEncryptor(object):
-    implements(IPasswordEncryptor)
-
-    def validate(self, plaintext, encrypted):
-        return plaintext == encrypted
-
-
-class TestPlacelessAuth(PlacelessSetup, unittest.TestCase):
+class TestPlacelessAuth(PlacelessSetup, testtools.TestCase):
 
     def setUp(self):
+        testtools.TestCase.setUp(self)
         PlacelessSetup.setUp(self)
-        ztapi.provideUtility(IPasswordEncryptor, DummyPasswordEncryptor())
         ztapi.provideUtility(IPlacelessLoginSource,
                              DummyPlacelessLoginSource())
         ztapi.provideUtility(IPlacelessAuthUtility, PlacelessAuthUtility())
@@ -74,10 +67,10 @@
             IHTTPCredentials, ILoginPassword, BasicAuthAdapter)
 
     def tearDown(self):
-        ztapi.unprovideUtility(IPasswordEncryptor)
         ztapi.unprovideUtility(IPlacelessLoginSource)
         ztapi.unprovideUtility(IPlacelessAuthUtility)
         PlacelessSetup.tearDown(self)
+        testtools.TestCase.tearDown(self)
 
     def _make(self, login, pwd):
         dict = {
@@ -87,11 +80,11 @@
         return getUtility(IPlacelessAuthUtility), request
 
     def test_authenticate_ok(self):
-        authsvc, request = self._make('bruce', 'bruce!')
+        authsvc, request = self._make('bruce', 'test')
         self.assertEqual(authsvc.authenticate(request), Bruce)
 
     def test_authenticate_notok(self):
-        authsvc, request = self._make('bruce', 'notbruce!')
+        authsvc, request = self._make('bruce', 'nottest')
         self.assertEqual(authsvc.authenticate(request), None)
 
     def test_unauthenticatedPrincipal(self):
@@ -100,18 +93,52 @@
                                 UnauthenticatedPrincipal))
 
     def test_unauthorized(self):
-        authsvc, request = self._make('bruce', 'bruce!')
+        authsvc, request = self._make('bruce', 'test')
         self.assertEqual(authsvc.unauthorized('bruce', request), None)
         self.assertEqual(request._response._status, 401)
 
+    def test_basic_auth_disabled(self):
+        # Basic auth uses a single password for every user, so it must
+        # never be used on production. authenticate() will skip basic
+        # auth unless it's enabled.
+        authsvc, request = self._make('bruce', 'test')
+        self.assertEqual(authsvc.authenticate(request), Bruce)
+        try:
+            config.push(
+                "no-basic", "[launchpad]\nbasic_auth_password: none")
+            self.assertEqual(authsvc.authenticate(request), None)
+        finally:
+            config.pop("no-basic")
+
+    def test_direct_basic_call_fails_when_disabled(self):
+        # Basic auth uses a single password for every user, so it must
+        # never be used on production. authenticate() won't call the
+        # underlying method unless it's enabled, but even if it somehow
+        # does it will fail.
+        authsvc, request = self._make('bruce', 'test')
+        credentials = ILoginPassword(request, None)
+        self.assertEqual(
+            authsvc._authenticateUsingBasicAuth(credentials, request), Bruce)
+        try:
+            config.push(
+                "no-basic", "[launchpad]\nbasic_auth_password: none")
+            exception = self.assertRaises(
+                AssertionError, authsvc._authenticateUsingBasicAuth,
+                credentials, request)
+            self.assertEquals(
+                "Attempted to use basic auth when it is disabled",
+                str(exception))
+        finally:
+            config.pop("no-basic")
+
     def test_getPrincipal(self):
-        authsvc, request = self._make('bruce', 'bruce!')
+        authsvc, request = self._make('bruce', 'test')
         self.assertEqual(authsvc.getPrincipal('bruce'), Bruce)
 
     def test_getPrincipals(self):
-        authsvc, request = self._make('bruce', 'bruce!')
+        authsvc, request = self._make('bruce', 'test')
         self.assertEqual(authsvc.getPrincipals('bruce'), [Bruce])
 
     def test_getPrincipalByLogin(self):
-        authsvc, request = self._make('bruce', 'bruce!')
+        authsvc, request = self._make('bruce', 'test')
         self.assertEqual(authsvc.getPrincipalByLogin('bruce'), Bruce)

=== modified file 'lib/lp/testing/pages.py'
--- lib/lp/testing/pages.py	2012-01-01 02:58:52 +0000
+++ lib/lp/testing/pages.py	2012-01-18 06:45:31 +0000
@@ -682,11 +682,8 @@
     """
     naked_user = removeSecurityProxy(user)
     email = naked_user.preferredemail.email
-    if hasattr(naked_user, '_password_cleartext_cached'):
-        password = naked_user._password_cleartext_cached
     logout()
-    return setupBrowser(
-        auth="Basic %s:%s" % (str(email), password))
+    return setupBrowser(auth="Basic %s:test" % str(email))
 
 
 def safe_canonical_url(*args, **kwargs):

=== modified file 'lib/lp/testing/yuixhr.py'
--- lib/lp/testing/yuixhr.py	2012-01-01 02:58:52 +0000
+++ lib/lp/testing/yuixhr.py	2012-01-18 06:45:31 +0000
@@ -111,7 +111,7 @@
     request = get_current_browser_request()
     assert request is not None, "We do not have a browser request."
     authutil = getUtility(IPlacelessAuthUtility)
-    principal = authutil.getPrincipalByLogin(email, want_password=False)
+    principal = authutil.getPrincipalByLogin(email)
     launchbag = getUtility(IOpenLaunchBag)
     launchbag.setLogin(email)
     logInPrincipal(request, principal, email)