From 6796dcf27715f9a149387bb80e7a359208602f1f Mon Sep 17 00:00:00 2001 From: Vladimir Shushlin Date: Wed, 17 Jul 2019 12:56:58 +0000 Subject: [PATCH] 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 | 2 + ...0703185326_fix_wrong_pages_access_level.rb | 28 ++++ ...ect_features_pages_access_level_default.rb | 12 ++ db/schema.rb | 4 +- .../fix_pages_access_level.rb | 128 ++++++++++++++++++ .../fix_wrong_pages_access_level_spec.rb | 97 +++++++++++++ spec/models/project_feature_spec.rb | 28 ++++ spec/support/helpers/stub_configuration.rb | 4 + 8 files changed, 301 insertions(+), 2 deletions(-) create mode 100644 db/post_migrate/20190703185326_fix_wrong_pages_access_level.rb create mode 100644 db/post_migrate/20190715114644_drop_project_features_pages_access_level_default.rb create mode 100644 lib/gitlab/background_migration/fix_pages_access_level.rb create mode 100644 spec/migrations/fix_wrong_pages_access_level_spec.rb diff --git a/app/models/project_feature.rb b/app/models/project_feature.rb index 7ff06655de0..78e82955342 100644 --- a/app/models/project_feature.rb +++ b/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) diff --git a/db/post_migrate/20190703185326_fix_wrong_pages_access_level.rb b/db/post_migrate/20190703185326_fix_wrong_pages_access_level.rb new file mode 100644 index 00000000000..e5981956cf5 --- /dev/null +++ b/db/post_migrate/20190703185326_fix_wrong_pages_access_level.rb @@ -0,0 +1,28 @@ +# 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 diff --git a/db/post_migrate/20190715114644_drop_project_features_pages_access_level_default.rb b/db/post_migrate/20190715114644_drop_project_features_pages_access_level_default.rb new file mode 100644 index 00000000000..2fb0aa0f460 --- /dev/null +++ b/db/post_migrate/20190715114644_drop_project_features_pages_access_level_default.rb @@ -0,0 +1,12 @@ +# 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 diff --git a/db/schema.rb b/db/schema.rb index 644ca1fe970..f752211f2ec 100644 --- a/db/schema.rb +++ b/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 diff --git a/lib/gitlab/background_migration/fix_pages_access_level.rb b/lib/gitlab/background_migration/fix_pages_access_level.rb new file mode 100644 index 00000000000..0d49f3dd8c5 --- /dev/null +++ b/lib/gitlab/background_migration/fix_pages_access_level.rb @@ -0,0 +1,128 @@ +# 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 diff --git a/spec/migrations/fix_wrong_pages_access_level_spec.rb b/spec/migrations/fix_wrong_pages_access_level_spec.rb new file mode 100644 index 00000000000..75ac5d919b2 --- /dev/null +++ b/spec/migrations/fix_wrong_pages_access_level_spec.rb @@ -0,0 +1,97 @@ +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 diff --git a/spec/models/project_feature_spec.rb b/spec/models/project_feature_spec.rb index 50c9d5968ac..31e55bf6be6 100644 --- a/spec/models/project_feature_spec.rb +++ b/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 diff --git a/spec/support/helpers/stub_configuration.rb b/spec/support/helpers/stub_configuration.rb index c372a3f0e49..049702be1f6 100644 --- a/spec/support/helpers/stub_configuration.rb +++ b/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! -- GitLab