Merge lp:~maxiberta/launchpad/named-auth-tokens-htaccess into lp:launchpad

[Maximiliano Bertacchini]

Add support for named auth tokens in ppa htpasswd creation.

Add support for named auth tokens in ppa htpasswd creation (including the removal of revoked named tokens).

[Colin Watson (cjwatson)]

review: Approve

[Maximiliano Bertacchini (maxiberta)]

Updated branch with fixes and improvements based on code review.

=== modified file 'lib/lp/archivepublisher/htaccess.py'
--- lib/lp/archivepublisher/htaccess.py	2013-06-20 05:50:00 +0000
+++ lib/lp/archivepublisher/htaccess.py	2016-07-14 16:06:58 +0000
@@ -58,43 +58,51 @@
 
     file = open(filename, "a")
     try:
-        for entry in users:
-            user, password, salt = entry
+        for user, password, salt in users:
             encrypted = crypt.crypt(password, salt)
             file.write("%s:%s\n" % (user, encrypted))
     finally:
         file.close()
 
 
-def htpasswd_credentials_for_archive(archive, tokens=None):
+def htpasswd_credentials_for_archive(archive):
     """Return credentials for an archive for use with write_htpasswd.
 
     :param archive: An `IArchive` (must be private)
-    :param tokens: Optional iterable of `IArchiveAuthToken`s.
     :return: Iterable of tuples with (user, password, salt) for use with
         write_htpasswd.
     """
     assert archive.private, "Archive %r must be private" % archive
 
-    if tokens is None:
-        tokens = IStore(ArchiveAuthToken).find(
-            (ArchiveAuthToken.person_id, ArchiveAuthToken.token),
-            ArchiveAuthToken.archive == archive,
-            ArchiveAuthToken.date_deactivated == None)
+    tokens = IStore(ArchiveAuthToken).find(
+        (ArchiveAuthToken.person_id, ArchiveAuthToken.name,
+            ArchiveAuthToken.token),
+        ArchiveAuthToken.archive == archive,
+        ArchiveAuthToken.date_deactivated == None)
     # We iterate tokens more than once - materialise it.
     tokens = list(tokens)
 
-    # The first .htpasswd entry is the buildd_secret.
-    yield (BUILDD_USER_NAME, archive.buildd_secret, BUILDD_USER_NAME[:2])
-
+    # Preload map with person ID to person name.
     person_ids = map(itemgetter(0), tokens)
     names = dict(
         IStore(Person).find(
             (Person.id, Person.name), Person.id.is_in(set(person_ids))))
-    # Combine the token list with the person list, sorting by person ID
-    # so the file can be compared later.
-    sorted_tokens = [(token[1], names[token[0]]) for token in sorted(tokens)]
-    # Iterate over tokens and write the appropriate htpasswd
-    # entries for them.
-    for token, person_name in sorted_tokens:
-        yield (person_name, token, person_name[:2])
+
+    # Format the user field by combining the token list with the person list
+    # (when token has person_id) or prepending a '+' (for named tokens).
+    output = []
+    for person_id, token_name, token in tokens:
+        if token_name:
+            # A named auth token.
+            output.append(('+' + token_name, token, token_name[:2]))
+        else:
+            # A subscription auth token.
+            output.append((names[person_id], token, names[person_id][:2]))
+
+    # The first .htpasswd entry is the buildd_secret.
+    yield (BUILDD_USER_NAME, archive.buildd_secret, BUILDD_USER_NAME[:2])
+
+    # Iterate over tokens and write the appropriate htpasswd entries for them.
+    # Sort by name/person ID so the file can be compared later.
+    for user, password, salt in sorted(output):
+        yield (user, password, salt)

=== modified file 'lib/lp/archivepublisher/scripts/generate_ppa_htaccess.py'
--- lib/lp/archivepublisher/scripts/generate_ppa_htaccess.py	2014-10-29 06:04:09 +0000
+++ lib/lp/archivepublisher/scripts/generate_ppa_htaccess.py	2016-07-14 16:06:58 +0000
@@ -89,8 +89,7 @@
         fd, temp_filename = tempfile.mkstemp(dir=pub_config.temproot)
         os.close(fd)
 
-        write_htpasswd(
-            temp_filename, htpasswd_credentials_for_archive(ppa))
+        write_htpasswd(temp_filename, htpasswd_credentials_for_archive(ppa))
 
         return temp_filename
 
@@ -176,6 +175,7 @@
         store = IStore(ArchiveSubscriber)
         valid_tokens = store.find(
             ArchiveAuthToken,
+            ArchiveAuthToken.name == None,
             ArchiveAuthToken.date_deactivated == None,
             ArchiveAuthToken.archive_id == ArchiveSubscriber.archive_id,
             ArchiveSubscriber.status == ArchiveSubscriberStatus.CURRENT,
@@ -186,6 +186,7 @@
         # all active tokens and valid tokens.
         all_active_tokens = store.find(
             ArchiveAuthToken,
+            ArchiveAuthToken.name == None,
             ArchiveAuthToken.date_deactivated == None)
 
         return all_active_tokens.difference(valid_tokens)
@@ -274,6 +275,22 @@
             *extra_expr)
         return new_ppa_tokens
 
+    def getDeactivatedNamedTokens(self, since=None):
+        """Return result set of named tokens deactivated since given time."""
+        now = datetime.now(pytz.UTC)
+
+        store = IStore(ArchiveAuthToken)
+        extra_expr = []
+        if since:
+            extra_expr = [ArchiveAuthToken.date_deactivated >= since]
+        tokens = store.find(
+            ArchiveAuthToken,
+            ArchiveAuthToken.name != None,
+            ArchiveAuthToken.date_deactivated != None,
+            ArchiveAuthToken.date_deactivated <= now,
+            *extra_expr)
+        return tokens
+
     def getNewPrivatePPAs(self, since=None):
         """Return the recently created private PPAs."""
         store = IStore(Archive)
@@ -294,6 +311,18 @@
 
         last_success = self.getTimeToSyncFrom()
 
+        # Include ppas with named tokens deactivated since last time we ran.
+        num_tokens = 0
+        for token in self.getDeactivatedNamedTokens(since=last_success):
+            affected_ppas.add(token.archive)
+            num_tokens += 1
+
+        new_ppa_count = len(affected_ppas)
+        self.logger.debug(
+            "%s deactivated named tokens since last run, %s PPAs affected"
+            % (num_tokens, new_ppa_count - current_ppa_count))
+        current_ppa_count = new_ppa_count
+
         # In addition to the ppas that are affected by deactivated
         # tokens, we also want to include any ppas that have tokens
         # created since the last time we ran.
@@ -316,7 +345,7 @@
 
         self.logger.debug('%s PPAs require updating' % new_ppa_count)
         for ppa in affected_ppas:
-            # If this PPA is blacklisted, do not touch it's htaccess/pwd
+            # If this PPA is blacklisted, do not touch its htaccess/pwd
             # files.
             blacklisted_ppa_names_for_owner = self.blacklist.get(
                 ppa.owner.name, [])

=== modified file 'lib/lp/archivepublisher/tests/test_generate_ppa_htaccess.py'
--- lib/lp/archivepublisher/tests/test_generate_ppa_htaccess.py	2016-01-26 15:47:37 +0000
+++ lib/lp/archivepublisher/tests/test_generate_ppa_htaccess.py	2016-07-14 16:06:58 +0000
@@ -32,6 +32,7 @@
 from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.teammembership import TeamMembershipStatus
 from lp.services.config import config
+from lp.services.features.testing import FeatureFixture
 from lp.services.log.logger import BufferLogger
 from lp.services.osutils import (
     ensure_directory_exists,
@@ -43,6 +44,7 @@
     ArchiveStatus,
     ArchiveSubscriberStatus,
     )
+from lp.soyuz.interfaces.archive import NAMED_AUTH_TOKEN_FEATURE_FLAG
 from lp.testing import TestCaseWithFactory
 from lp.testing.dbuser import (
     lp_dbuser,
@@ -72,6 +74,9 @@
         ubuntutest = getUtility(IDistributionSet)['ubuntutest']
         self.ppa.distribution = ubuntutest
 
+        # Enable named auth tokens.
+        self.useFixture(FeatureFixture({NAMED_AUTH_TOKEN_FEATURE_FLAG: u"on"}))
+
     def getScript(self, test_args=None):
         """Return a HtaccessTokenGenerator instance."""
         if test_args is None:
@@ -260,6 +265,11 @@
             all_persons, persons1, persons2, tokens) = data
         team1_person = persons1[0]
 
+        # Named tokens should be ignored for deactivation.
+        self.ppa.newNamedAuthToken(u"tokenname1")
+        named_token = self.ppa.newNamedAuthToken(u"tokenname2")
+        named_token.deactivate()
+
         # Initially, nothing is eligible for deactivation.
         script = self.getScript()
         script.deactivateInvalidTokens()
@@ -325,12 +335,9 @@
         sub2 = self.ppa.newSubscription(name16, self.ppa.owner)
         token1 = self.ppa.newAuthToken(name12)
         token2 = self.ppa.newAuthToken(name16)
+        token3 = self.ppa.newNamedAuthToken(u"tokenname3")
         self.layer.txn.commit()
-        subs = [sub1]
-        subs.append(sub2)
-        tokens = [token1]
-        tokens.append(token2)
-        return subs, tokens
+        return (sub1, sub2), (token1, token2, token3)
 
     def ensureNoFiles(self):
         """Ensure the .ht* files don't already exist."""
@@ -375,6 +382,36 @@
         os.remove(htaccess)
         os.remove(htpasswd)
 
+    def testBasicOperation_with_named_tokens(self):
+        """Invoke the actual script and make sure it generates some files."""
+        token1 = self.ppa.newNamedAuthToken(u"tokenname1")
+        token2 = self.ppa.newNamedAuthToken(u"tokenname2")
+        token3 = self.ppa.newNamedAuthToken(u"tokenname3")
+        token3.deactivate()
+
+        # Call the script and check that we have a .htaccess and a .htpasswd.
+        htaccess, htpasswd = self.ensureNoFiles()
+        script = self.getScript()
+        script.main()
+        self.assertThat([htaccess, htpasswd], AllMatch(FileExists()))
+        with open(htpasswd) as htpasswd_file:
+            contents = htpasswd_file.read()
+        self.assertIn('+' + token1.name, contents)
+        self.assertIn('+' + token2.name, contents)
+        self.assertNotIn('+' + token3.name, contents)
+
+        # Deactivate a named token and verify it is removed from .htpasswd.
+        token2.deactivate()
+        script.main()
+        self.assertThat([htaccess, htpasswd], AllMatch(FileExists()))
+        with open(htpasswd) as htpasswd_file:
+            contents = htpasswd_file.read()
+        self.assertIn('+' + token1.name, contents)
+        self.assertNotIn('+' + token2.name, contents)
+        self.assertNotIn('+' + token3.name, contents)
+        os.remove(htaccess)
+        os.remove(htpasswd)
+
     def _setupOptionsData(self):
         """Setup test data for option testing."""
         subs, tokens = self.setupDummyTokens()
@@ -598,14 +635,47 @@
         script = self.getScript()
         self.assertContentEqual(tokens[1:], script.getNewTokens())
 
+    def test_getDeactivatedNamedTokens_no_previous_run(self):
+        """All deactivated named tokens returned if there is no record
+        of previous run."""
+        last_start = datetime.now(pytz.UTC) - timedelta(seconds=90)
+        before_last_start = last_start - timedelta(seconds=30)
+
+        self.ppa.newNamedAuthToken(u"tokenname1")
+        token2 = self.ppa.newNamedAuthToken(u"tokenname2")
+        token2.deactivate()
+        token3 = self.ppa.newNamedAuthToken(u"tokenname3")
+        token3.date_deactivated = before_last_start
+
+        script = self.getScript()
+        self.assertContentEqual(
+            [token2, token3], script.getDeactivatedNamedTokens())
+
+    def test_getDeactivatedNamedTokens_only_those_since_last_run(self):
+        """Only named tokens deactivated since last run are returned."""
+        last_start = datetime.now(pytz.UTC) - timedelta(seconds=90)
+        before_last_start = last_start - timedelta(seconds=30)
+        tomorrow = datetime.now(pytz.UTC) + timedelta(days=1)
+
+        self.ppa.newNamedAuthToken(u"tokenname1")
+        token2 = self.ppa.newNamedAuthToken(u"tokenname2")
+        token2.deactivate()
+        token3 = self.ppa.newNamedAuthToken(u"tokenname3")
+        token3.date_deactivated = before_last_start
+        token4 = self.ppa.newNamedAuthToken(u"tokenname4")
+        token4.date_deactivated = tomorrow
+
+        script = self.getScript()
+        self.assertContentEqual(
+            [token2], script.getDeactivatedNamedTokens(last_start))
+
     def test_processes_PPAs_without_subscription(self):
         # A .htaccess file is written for Private PPAs even if they don't have
         # any subscriptions.
         htaccess, htpasswd = self.ensureNoFiles()
         transaction.commit()
 
-        # Call the script and check that we have a .htaccess and a
-        # .htpasswd.
+        # Call the script and check that we have a .htaccess and a .htpasswd.
         return_code, stdout, stderr = self.runScript()
         self.assertEqual(
             return_code, 0, "Got a bad return code of %s\nOutput:\n%s" %

=== modified file 'lib/lp/archivepublisher/tests/test_htaccess.py'
--- lib/lp/archivepublisher/tests/test_htaccess.py	2012-05-31 01:57:07 +0000
+++ lib/lp/archivepublisher/tests/test_htaccess.py	2016-07-14 16:06:58 +0000
@@ -15,6 +15,8 @@
     )
 from lp.registry.interfaces.distribution import IDistributionSet
 from lp.registry.interfaces.person import IPersonSet
+from lp.services.features.testing import FeatureFixture
+from lp.soyuz.interfaces.archive import NAMED_AUTH_TOKEN_FEATURE_FLAG
 from lp.testing import TestCaseWithFactory
 from lp.testing.layers import LaunchpadZopelessLayer
 
@@ -36,6 +38,9 @@
         ubuntutest = getUtility(IDistributionSet)['ubuntutest']
         self.ppa.distribution = ubuntutest
 
+        # Enable named auth tokens.
+        self.useFixture(FeatureFixture({NAMED_AUTH_TOKEN_FEATURE_FLAG: u"on"}))
+
     def test_write_htpasswd(self):
         """Test that writing the .htpasswd file works properly."""
         fd, filename = tempfile.mkstemp()
@@ -109,15 +114,20 @@
         self.ppa.newSubscription(name12, self.ppa.owner)
         self.ppa.newSubscription(name16, self.ppa.owner)
         first_created_token = self.ppa.newAuthToken(name16)
-        tokens = [
-            (token.person_id, token.token)
-            for token in [self.ppa.newAuthToken(name12), first_created_token]]
-
-        credentials = list(htpasswd_credentials_for_archive(self.ppa, tokens))
-
-        self.assertContentEqual(
+        second_created_token = self.ppa.newAuthToken(name12)
+        named_token_20 = self.ppa.newNamedAuthToken(u"name20", as_dict=False)
+        named_token_14 = self.ppa.newNamedAuthToken(u"name14", as_dict=False)
+        named_token_99 = self.ppa.newNamedAuthToken(u"name99", as_dict=False)
+        named_token_99.deactivate()
+
+        credentials = list(htpasswd_credentials_for_archive(self.ppa))
+
+        # Use assertEqual instead of assertContentEqual to verify order.
+        self.assertEqual(
             credentials, [
                 ("buildd", "geheim", "bu"),
-                ("name12", tokens[0][1], "na"),
-                ("name16", tokens[1][1], "na")
+                ("+name14", named_token_14.token, "na"),
+                ("+name20", named_token_20.token, "na"),
+                ("name12", second_created_token.token, "na"),
+                ("name16", first_created_token.token, "na"),
                 ])

=== modified file 'lib/lp/soyuz/interfaces/archive.py'
--- lib/lp/soyuz/interfaces/archive.py	2016-07-07 21:32:09 +0000
+++ lib/lp/soyuz/interfaces/archive.py	2016-07-14 16:06:58 +0000
@@ -2097,22 +2097,25 @@
         :param dependency: is an `IArchive` object.
         """
 
+    @call_with(as_dict=True)
     @operation_parameters(
         name=TextLine(title=_("Authorization token name"), required=True),
         token=TextLine(
             title=_("Optional secret for this named token"), required=False))
     @export_write_operation()
     @operation_for_version("devel")
-    def newNamedAuthToken(name, token=None):
+    def newNamedAuthToken(name, token=None, as_dict=False):
         """Create a new named authorization token.
 
         :param name: An identifier string for this token.
         :param token: Optional unicode text to use as the token. One will be
             generated if not given.
+        :param as_dict: Optional boolean, controls whether the return value is
+            a dictionary or a full object.
 
-        :return: A dictionary where the value of `token` is the secret and
-            the value of `archive_url` is the externally-usable archive URL
-            including basic auth.
+        :return: An `ArchiveAuthToken` object or a dictionary where the value
+            of `token` is the secret and the value of `archive_url` is the
+            externally-usable archive URL including basic auth.
         """
 
     @operation_parameters(

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2016-07-07 21:32:09 +0000
+++ lib/lp/soyuz/model/archive.py	2016-07-14 16:06:58 +0000
@@ -1952,7 +1952,7 @@
         IStore(ArchiveAuthToken).add(archive_auth_token)
         return archive_auth_token
 
-    def newNamedAuthToken(self, name, token=None):
+    def newNamedAuthToken(self, name, token=None, as_dict=False):
         """See `IArchive`."""
 
         if not getFeatureFlag(NAMED_AUTH_TOKEN_FEATURE_FLAG):
@@ -1980,7 +1980,10 @@
         archive_auth_token.name = name
         archive_auth_token.token = token
         IStore(ArchiveAuthToken).add(archive_auth_token)
-        return archive_auth_token.asDict()
+        if as_dict:
+            return archive_auth_token.asDict()
+        else:
+            return archive_auth_token
 
     def getNamedAuthToken(self, name):
         """See `IArchive`."""

=== modified file 'lib/lp/soyuz/tests/test_archive.py'
--- lib/lp/soyuz/tests/test_archive.py	2016-07-07 21:32:09 +0000
+++ lib/lp/soyuz/tests/test_archive.py	2016-07-14 16:06:58 +0000
@@ -1294,7 +1294,7 @@
         self.assertEqual(token.archive_url, url)
 
     def test_newNamedAuthToken_private_archive(self):
-        res = self.private_ppa.newNamedAuthToken(u"tokenname")
+        res = self.private_ppa.newNamedAuthToken(u"tokenname", as_dict=True)
         token = getUtility(IArchiveAuthTokenSet).getActiveNamedTokenForArchive(
             self.private_ppa, u"tokenname")
         self.assertIsNotNone(token)
@@ -1320,25 +1320,21 @@
             self.private_ppa.newNamedAuthToken, u"tokenname")
 
     def test_newNamedAuthToken_with_custom_secret(self):
-        self.private_ppa.newNamedAuthToken(u"tokenname", u"somesecret")
-        token = getUtility(IArchiveAuthTokenSet).getActiveNamedTokenForArchive(
-            self.private_ppa, u"tokenname")
-        self.assertEqual(u"somesecret", token.token)
+        token = self.private_ppa.newNamedAuthToken(u"tokenname", u"secret")
+        self.assertEqual(u"secret", token.token)
 
     def test_getNamedAuthToken_with_no_token(self):
         self.assertRaises(
             NotFoundError, self.private_ppa.getNamedAuthToken, u"tokenname")
 
     def test_getNamedAuthToken_with_token(self):
-        res = self.private_ppa.newNamedAuthToken(u"tokenname")
+        res = self.private_ppa.newNamedAuthToken(u"tokenname", as_dict=True)
         self.assertEqual(
             self.private_ppa.getNamedAuthToken(u"tokenname"),
             res)
 
     def test_revokeNamedAuthToken_with_token(self):
-        self.private_ppa.newNamedAuthToken(u"tokenname")
-        token = getUtility(IArchiveAuthTokenSet).getActiveNamedTokenForArchive(
-            self.private_ppa, u"tokenname")
+        token = self.private_ppa.newNamedAuthToken(u"tokenname")
         self.private_ppa.revokeNamedAuthToken(u"tokenname")
         self.assertIsNotNone(token.date_deactivated)
 
@@ -1353,8 +1349,8 @@
             NotFoundError, self.private_ppa.getNamedAuthToken, u"tokenname")
 
     def test_getNamedAuthTokens(self):
-        res1 = self.private_ppa.newNamedAuthToken(u"tokenname1")
-        res2 = self.private_ppa.newNamedAuthToken(u"tokenname2")
+        res1 = self.private_ppa.newNamedAuthToken(u"tokenname1", as_dict=True)
+        res2 = self.private_ppa.newNamedAuthToken(u"tokenname2", as_dict=True)
         self.private_ppa.newNamedAuthToken(u"tokenname3")
         self.private_ppa.revokeNamedAuthToken(u"tokenname3")
         self.assertContentEqual(