Fix the encrypted token migration issue encountered on HEAD

This change ensures there is better messaging around the encrypted token migration, including a new phase to use for new installations, and fixes an issue encountered when running database migrations for new installations

data/database.py

@@ -378,7 +378,7 @@ def configure(config_object, testing=False):
                                                              real_for_update))
   db_concat_func.initialize(SCHEME_SPECIALIZED_CONCAT.get(parsed_write_uri.drivername,
                                                           function_concat))
-  db_encrypter.initialize(FieldEncrypter(config_object['DATABASE_SECRET_KEY']))
+  db_encrypter.initialize(FieldEncrypter(config_object.get('DATABASE_SECRET_KEY')))
 
   read_replicas = config_object.get('DB_READ_REPLICAS', None)
   is_read_only = config_object.get('REGISTRY_STATE', 'normal') == 'readonly'

data/encryption.py

@@ -59,11 +59,15 @@ class FieldEncrypter(object):
       and the application.
   """
   def __init__(self, secret_key, version='v0'):
-    self._secret_key = convert_secret_key(secret_key)
+    # NOTE: secret_key will be None when the system is being first initialized, so we allow that
+    # case here, but make sure to assert that it is *not* None below if any encryption is actually
+    # needed.
+    self._secret_key = convert_secret_key(secret_key) if secret_key is not None else None
     self._encryption_version = _VERSIONS[version]
 
   def encrypt_value(self, value, field_max_length=None):
     """ Encrypts the value using the current version of encryption. """
+    assert self._secret_key is not None
     encrypted_value = self._encryption_version.encrypt(self._secret_key, value, field_max_length)
     return '%s%s%s' % (self._encryption_version.prefix, _SEPARATOR, encrypted_value)
 
@@ -71,6 +75,7 @@ class FieldEncrypter(object):
     """ Decrypts the value, returning it. If the value cannot be decrypted
         raises a DecryptionFailureException.
     """
+    assert self._secret_key is not None
     if _SEPARATOR not in value:
       raise DecryptionFailureException('Invalid encrypted value')
 

data/migrations/tester.py

@@ -2,7 +2,7 @@ import json
 import logging
 import uuid
 
-from abc import ABCMeta, abstractmethod
+from abc import ABCMeta, abstractmethod, abstractproperty
 from datetime import datetime
 from six import add_metaclass
 
@@ -92,6 +92,10 @@ class MigrationTester(object):
   """
   TestDataType = DataTypes
 
+  @abstractproperty
+  def is_testing(self):
+    """ Returns whether we are currently under a migration test. """
+
   @abstractmethod
   def populate_table(self, table_name, fields):
     """ Called to populate a table with the given fields filled in with testing data. """
@@ -107,6 +111,10 @@ class NoopTester(MigrationTester):
 
 
 class PopulateTestDataTester(MigrationTester):
+  @property
+  def is_testing(self):
+    return True
+
   def populate_table(self, table_name, fields):
     columns = {field_name: field_type() for field_name, field_type in fields}
     field_name_vars = [':' + field_name for field_name, _ in fields]

data/migrations/versions/34c8ef052ec9_repo_mirror_columns.py

@@ -80,6 +80,8 @@ def upgrade(tables, tester, progress_reporter):
 
   op.add_column('repomirrorconfig', sa.Column('external_reference', sa.Text(), nullable=True))
 
+  from app import app
+  if app.config.get('SETUP_COMPLETE', False) or tester.is_testing:
     for repo_mirror in _iterate(RepoMirrorConfig, (RepoMirrorConfig.external_reference >> None)):
       repo = '%s/%s/%s' % (repo_mirror.external_registry, repo_mirror.external_namespace, repo_mirror.external_repository)
       logger.info('migrating %s' % repo)
@@ -109,6 +111,8 @@ def downgrade(tables, tester, progress_reporter):
   op.add_column('repomirrorconfig', sa.Column('external_namespace', sa.String(length=255), nullable=True))
   op.add_column('repomirrorconfig', sa.Column('external_repository', sa.String(length=255), nullable=True))
 
+  from app import app
+  if app.config.get('SETUP_COMPLETE', False):
     logger.info('Restoring columns from external_reference')
     for repo_mirror in _iterate(RepoMirrorConfig, (RepoMirrorConfig.external_registry >> None)):
       logger.info('Restoring %s' % repo_mirror.external_reference)

data/migrations/versions/703298a825c2_backfill_new_encrypted_fields.py

@@ -98,6 +98,8 @@ class OAuthApplication(BaseModel):
 def upgrade(tables, tester, progress_reporter):
   op = ProgressWrapper(original_op, progress_reporter)
 
+  from app import app
+  if app.config.get('SETUP_COMPLETE', False) or tester.is_testing:
     # Empty all access token names to fix the bug where we put the wrong name and code
     # in for some tokens.
     AccessToken.update(token_name=None).where(AccessToken.token_name >> None).execute()
@@ -271,10 +273,6 @@ def upgrade(tables, tester, progress_reporter):
 
 def downgrade(tables, tester, progress_reporter):
   op = ProgressWrapper(original_op, progress_reporter)
-  op.alter_column('accesstoken', 'code', nullable=False, existing_type=sa.String(length=255))       
-  op.alter_column('oauthaccesstoken', 'access_token', nullable=False, existing_type=sa.String(length=255))       
-  op.alter_column('oauthauthorizationcode', 'code', nullable=False, existing_type=sa.String(length=255))
-  op.alter_column('appspecificauthtoken', 'token_code', nullable=False, existing_type=sa.String(length=255))
 
   op.alter_column('accesstoken', 'token_name', nullable=True, existing_type=sa.String(length=255))       
   op.alter_column('accesstoken', 'token_code', nullable=True, existing_type=sa.String(length=255))       

data/migrations/versions/c059b952ed76_remove_unencrypted_fields_and_data.py

@@ -39,6 +39,8 @@ def upgrade(tables, tester, progress_reporter):
     # ### end Alembic commands ###
 
     # Overwrite all plaintext robot credentials.
+    from app import app
+    if app.config.get('SETUP_COMPLETE', False) or tester.is_testing:
         while True:
             try:
                 robot_account_token = RobotAccountToken.get(fully_migrated=False)

data/migrationutil.py

@@ -30,21 +30,35 @@ class NullDataMigration(DataMigration):
 
 class DefinedDataMigration(DataMigration):
   def __init__(self, name, env_var, phases):
+    assert phases
+
     self.name = name
     self.phases = {phase.name: phase for phase in phases}
 
+    # Add a synthetic phase for new installations that skips the entire migration.
+    self.phases['new-installation'] = phases[-1]._replace(name='new-installation',
+                                                          alembic_revision='head')
+
     phase_name = os.getenv(env_var)
     if phase_name is None:
-      msg = 'Missing env var `%s` for data migration `%s`' % (env_var, self.name)
+      msg = 'Missing env var `%s` for data migration `%s`. %s' % (env_var, self.name,
+                                                                  self._error_suffix)
       raise Exception(msg)
 
     current_phase = self.phases.get(phase_name)
     if current_phase is None:
-      msg = 'Unknown phase `%s` for data migration `%s`' % (phase_name, self.name)
+      msg = 'Unknown phase `%s` for data migration `%s`. %s' % (phase_name, self.name,
+                                                                self._error_suffix)
       raise Exception(msg)
 
     self.current_phase = current_phase
 
+  @property
+  def _error_suffix(self):
+    message = 'Available values for this migration: %s. ' % (self.phases.keys())
+    message += 'If this is a new installation, please use `new-installation`.'
+    return message
+
   @property
   def alembic_migration_revision(self):
     assert self.current_phase