Fix wrong pages access level default

- Set access level in before_validation hook
- Add post migration for updating existing project_features

app/models/project_feature.rb

@@ -86,6 +86,8 @@ class ProjectFeature < ApplicationRecord
   default_value_for :wiki_access_level,           value: ENABLED, allows_nil: false
   default_value_for :repository_access_level,     value: ENABLED, allows_nil: false
 
+  default_value_for(:pages_access_level, allows_nil: false) { |feature| feature.project&.public? ? ENABLED : PRIVATE }
+
   def feature_available?(feature, user)
     # This feature might not be behind a feature flag at all, so default to true
     return false unless ::Feature.enabled?(feature, user, default_enabled: true)

db/post_migrate/20190703185326_fix_wrong_pages_access_level.rb

+# frozen_string_literal: true
+
+class FixWrongPagesAccessLevel < ActiveRecord::Migration[5.1]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  MIGRATION = 'FixPagesAccessLevel'
+  BATCH_SIZE = 20_000
+  BATCH_TIME = 2.minutes
+
+  disable_ddl_transaction!
+
+  class ProjectFeature < ActiveRecord::Base
+    include ::EachBatch
+
+    self.table_name = 'project_features'
+    self.inheritance_column = :_type_disabled
+  end
+
+  def up
+    queue_background_migration_jobs_by_range_at_intervals(
+      ProjectFeature,
+      MIGRATION,
+      BATCH_TIME,
+      batch_size: BATCH_SIZE)
+  end
+end

db/post_migrate/20190715114644_drop_project_features_pages_access_level_default.rb

+# frozen_string_literal: true
+
+class DropProjectFeaturesPagesAccessLevelDefault < ActiveRecord::Migration[5.1]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  ENABLED_VALUE = 20
+
+  def change
+    change_column_default :project_features, :pages_access_level, from: ENABLED_VALUE, to: nil
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2019_07_03_130053) do
+ActiveRecord::Schema.define(version: 2019_07_15_114644) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "pg_trgm"
@@ -2507,7 +2507,7 @@ ActiveRecord::Schema.define(version: 2019_07_03_130053) do
     t.datetime "created_at"
     t.datetime "updated_at"
     t.integer "repository_access_level", default: 20, null: false
-    t.integer "pages_access_level", default: 20, null: false
+    t.integer "pages_access_level", null: false
     t.index ["project_id"], name: "index_project_features_on_project_id", unique: true, using: :btree
   end
 

lib/gitlab/background_migration/fix_pages_access_level.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module BackgroundMigration
+    # corrects stored pages access level on db depending on project visibility
+    class FixPagesAccessLevel
+      # Copy routable here to avoid relying on application logic
+      module Routable
+        def build_full_path
+          if parent && path
+            parent.build_full_path + '/' + path
+          else
+            path
+          end
+        end
+      end
+
+      # Namespace
+      class Namespace < ApplicationRecord
+        self.table_name = 'namespaces'
+        self.inheritance_column = :_type_disabled
+
+        include Routable
+
+        belongs_to :parent, class_name: "Namespace"
+      end
+
+      # Project
+      class Project < ActiveRecord::Base
+        self.table_name = 'projects'
+        self.inheritance_column = :_type_disabled
+
+        include Routable
+
+        belongs_to :namespace
+        alias_method :parent, :namespace
+        alias_attribute :parent_id, :namespace_id
+
+        PRIVATE = 0
+        INTERNAL = 10
+        PUBLIC = 20
+
+        def pages_deployed?
+          Dir.exist?(public_pages_path)
+        end
+
+        def public_pages_path
+          File.join(pages_path, 'public')
+        end
+
+        def pages_path
+          # TODO: when we migrate Pages to work with new storage types, change here to use disk_path
+          File.join(Settings.pages.path, build_full_path)
+        end
+      end
+
+      # ProjectFeature
+      class ProjectFeature < ActiveRecord::Base
+        include ::EachBatch
+
+        self.table_name = 'project_features'
+
+        belongs_to :project
+
+        PRIVATE = 10
+        ENABLED = 20
+        PUBLIC  = 30
+      end
+
+      def perform(start_id, stop_id)
+        fix_public_access_level(start_id, stop_id)
+
+        make_internal_projects_public(start_id, stop_id)
+
+        fix_private_access_level(start_id, stop_id)
+      end
+
+      private
+
+      def access_control_is_enabled
+        @access_control_is_enabled = Gitlab.config.pages.access_control
+      end
+
+      # Public projects are allowed to have only enabled pages_access_level
+      # which is equivalent to public
+      def fix_public_access_level(start_id, stop_id)
+        project_features(start_id, stop_id, ProjectFeature::PUBLIC, Project::PUBLIC).each_batch do |features|
+          features.update_all(pages_access_level: ProjectFeature::ENABLED)
+        end
+      end
+
+      # If access control is disabled and project has pages deployed
+      # project will become unavailable when access control will become enabled
+      # we make these projects public to avoid negative surprise to user
+      def make_internal_projects_public(start_id, stop_id)
+        return if access_control_is_enabled
+
+        project_features(start_id, stop_id, ProjectFeature::ENABLED, Project::INTERNAL).find_each do |project_feature|
+          next unless project_feature.project.pages_deployed?
+
+          project_feature.update(pages_access_level: ProjectFeature::PUBLIC)
+        end
+      end
+
+      # Private projects are not allowed to have enabled access level, only `private` and `public`
+      # If access control is enabled, these projects currently behave as if the have `private` pages_access_level
+      # if access control is disabled, these projects currently behave as if the have `public` pages_access_level
+      # so we preserve this behaviour for projects with pages already deployed
+      # for project without pages we always set `private` access_level
+      def fix_private_access_level(start_id, stop_id)
+        project_features(start_id, stop_id, ProjectFeature::ENABLED, Project::PRIVATE).find_each do |project_feature|
+          if access_control_is_enabled
+            project_feature.update!(pages_access_level: ProjectFeature::PRIVATE)
+          else
+            fixed_access_level = project_feature.project.pages_deployed? ? ProjectFeature::PUBLIC : ProjectFeature::PRIVATE
+            project_feature.update!(pages_access_level: fixed_access_level)
+          end
+        end
+      end
+
+      def project_features(start_id, stop_id, pages_access_level, project_visibility_level)
+        ProjectFeature.where(id: start_id..stop_id).joins(:project)
+          .where(pages_access_level: pages_access_level)
+          .where(projects: { visibility_level: project_visibility_level })
+      end
+    end
+  end
+end

spec/migrations/fix_wrong_pages_access_level_spec.rb

+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20190703185326_fix_wrong_pages_access_level.rb')
+
+describe FixWrongPagesAccessLevel, :migration, :sidekiq, schema: 20190628185004 do
+  using RSpec::Parameterized::TableSyntax
+
+  let(:migration_class) { described_class::MIGRATION }
+  let(:migration_name)  { migration_class.to_s.demodulize }
+
+  project_class = ::Gitlab::BackgroundMigration::FixPagesAccessLevel::Project
+  feature_class = ::Gitlab::BackgroundMigration::FixPagesAccessLevel::ProjectFeature
+
+  let(:namespaces_table) { table(:namespaces) }
+  let(:projects_table) { table(:projects) }
+  let(:features_table) { table(:project_features) }
+
+  let(:subgroup) do
+    root_group = namespaces_table.create(path: "group", name: "group")
+    namespaces_table.create!(path: "subgroup", name: "group", parent_id: root_group.id)
+  end
+
+  def create_project_feature(path, project_visibility, pages_access_level)
+    project = projects_table.create!(path: path, visibility_level: project_visibility,
+                                     namespace_id: subgroup.id)
+    features_table.create!(project_id: project.id, pages_access_level: pages_access_level)
+  end
+
+  it 'correctly schedules background migrations' do
+    Sidekiq::Testing.fake! do
+      Timecop.freeze do
+        first_id = create_project_feature("project1", project_class::PRIVATE, feature_class::PRIVATE).id
+        last_id = create_project_feature("project2", project_class::PRIVATE, feature_class::PUBLIC).id
+
+        migrate!
+
+        expect(migration_name).to be_scheduled_delayed_migration(2.minutes, first_id, last_id)
+        expect(BackgroundMigrationWorker.jobs.size).to eq(1)
+      end
+    end
+  end
+
+  def expect_migration
+    expect do
+      perform_enqueued_jobs do
+        migrate!
+      end
+    end
+  end
+
+  where(:project_visibility, :pages_access_level, :access_control_is_enabled,
+        :pages_deployed, :resulting_pages_access_level) do
+    # update settings for public projects regardless of access_control being enabled
+    project_class::PUBLIC | feature_class::PUBLIC | true | true  | feature_class::ENABLED
+    project_class::PUBLIC | feature_class::PUBLIC | false | true | feature_class::ENABLED
+    # don't update public level for private and internal projects
+    project_class::PRIVATE | feature_class::PUBLIC  | true | true | feature_class::PUBLIC
+    project_class::INTERNAL | feature_class::PUBLIC | true | true | feature_class::PUBLIC
+
+    # if access control is disabled but pages are deployed we make them public
+    project_class::INTERNAL | feature_class::ENABLED | false | true  | feature_class::PUBLIC
+    # don't change anything if one of the conditions is not satisfied
+    project_class::INTERNAL | feature_class::ENABLED | true  | true  | feature_class::ENABLED
+    project_class::INTERNAL | feature_class::ENABLED | true  | false | feature_class::ENABLED
+
+    # private projects
+    # if access control is enabled update pages_access_level to private regardless of deployment
+    project_class::PRIVATE | feature_class::ENABLED | true  | true  | feature_class::PRIVATE
+    project_class::PRIVATE | feature_class::ENABLED | true  | false | feature_class::PRIVATE
+    # if access control is disabled and pages are deployed update pages_access_level to public
+    project_class::PRIVATE | feature_class::ENABLED | false | true  | feature_class::PUBLIC
+    # if access control is disabled but pages aren't deployed update pages_access_level to private
+    project_class::PRIVATE | feature_class::ENABLED | false | false | feature_class::PRIVATE
+  end
+
+  with_them do
+    let!(:project_feature) do
+      create_project_feature("projectpath", project_visibility, pages_access_level)
+    end
+
+    before do
+      tested_path = File.join(Settings.pages.path, "group/subgroup/projectpath", "public")
+      allow(Dir).to receive(:exist?).with(tested_path).and_return(pages_deployed)
+
+      stub_pages_setting(access_control: access_control_is_enabled)
+    end
+
+    it "sets proper pages_access_level" do
+      expect(project_feature.reload.pages_access_level).to eq(pages_access_level)
+
+      perform_enqueued_jobs do
+        migrate!
+      end
+
+      expect(project_feature.reload.pages_access_level).to eq(resulting_pages_access_level)
+    end
+  end
+end

spec/models/project_feature_spec.rb

@@ -150,4 +150,32 @@ describe ProjectFeature do
       end
     end
   end
+
+  describe 'default pages access level' do
+    subject { project.project_feature.pages_access_level }
+
+    before do
+      # project factory overrides all values in project_feature after creation
+      project.project_feature.destroy!
+      project.build_project_feature.save!
+    end
+
+    context 'when new project is private' do
+      let(:project) { create(:project, :private) }
+
+      it { is_expected.to eq(ProjectFeature::PRIVATE) }
+    end
+
+    context 'when new project is internal' do
+      let(:project) { create(:project, :internal) }
+
+      it { is_expected.to eq(ProjectFeature::PRIVATE) }
+    end
+
+    context 'when new project is public' do
+      let(:project) { create(:project, :public) }
+
+      it { is_expected.to eq(ProjectFeature::ENABLED) }
+    end
+  end
 end

spec/support/helpers/stub_configuration.rb

@@ -65,6 +65,10 @@ module StubConfiguration
     allow(Gitlab.config.artifacts).to receive_messages(to_settings(messages))
   end
 
+  def stub_pages_setting(messages)
+    allow(Gitlab.config.pages).to receive_messages(to_settings(messages))
+  end
+
   def stub_storage_settings(messages)
     messages.deep_stringify_keys!