Skip to content
Snippets Groups Projects
Commit 5f426d5b authored by Michael Becker's avatar Michael Becker Committed by Bojan Marjanovic
Browse files

Add new jsonb column to store settings for default branch protection

Currently, the options for default branch protection at the instance
and group level lag behind, and are not as fine-grained, as the
options available from the Protected Branches feature

The branch defaults lag behind because the current implementation uses
an integer column to store the default settings, which becomes
difficult to expand as new options and finer detail controls are added
to the protected branches feature

To offer better support of protected branches features on the default
branch, we can:

  1. use a jsonb column rather than an integer
  2. update the settings API to accept a payload the matches
     the protected branches API
  3. Pass those settings into the ProtecteBrancheService

This commit performs step 1, by adding a new `jsonb` column

Changelog: added
parent 77b79ac2
No related branches found
No related tags found
1 merge request!120139Add new jsonb column to store settings for default branch protection
Showing
with 237 additions and 3 deletions
......@@ -55,6 +55,9 @@ class ApplicationSetting < MainClusterwide::ApplicationRecord
archive_builds_in_seconds: 'Archive job value'
}.freeze
# matches the size set in the database constraint
DEFAULT_BRANCH_PROTECTIONS_DEFAULT_MAX_SIZE = 1.kilobyte
enum whats_new_variant: { all_tiers: 0, current_tier: 1, disabled: 2 }, _prefix: true
enum email_confirmation_setting: { off: 0, soft: 1, hard: 2 }, _prefix: true
......@@ -110,6 +113,7 @@ def self.kroki_formats_attributes
attribute :id, default: 1
attribute :repository_storages_weighted, default: -> { {} }
attribute :kroki_formats, default: -> { {} }
attribute :default_branch_protection_defaults, default: -> { {} }
chronic_duration_attr_writer :archive_builds_in_human_readable, :archive_builds_in_seconds
......@@ -117,6 +121,9 @@ def self.kroki_formats_attributes
chronic_duration_attr :group_runner_token_expiration_interval_human_readable, :group_runner_token_expiration_interval
chronic_duration_attr :project_runner_token_expiration_interval_human_readable, :project_runner_token_expiration_interval
validates :default_branch_protection_defaults, json_schema: { filename: 'default_branch_protection_defaults' }
validates :default_branch_protection_defaults, bytesize: { maximum: -> { DEFAULT_BRANCH_PROTECTIONS_DEFAULT_MAX_SIZE } }
validates :grafana_url,
system_hook_url: ADDRESSABLE_URL_VALIDATION_OPTIONS.merge({
blocked_message: "is blocked: %{exception_message}. #{GRAFANA_URL_ERROR_MESSAGE}"
......
......@@ -12,8 +12,12 @@ class NamespaceSetting < ApplicationRecord
enum jobs_to_be_done: { basics: 0, move_repository: 1, code_storage: 2, exploring: 3, ci: 4, other: 5 }, _suffix: true
enum enabled_git_access_protocol: { all: 0, ssh: 1, http: 2 }, _suffix: true
attribute :default_branch_protection_defaults, default: -> { {} }
validates :enabled_git_access_protocol, inclusion: { in: enabled_git_access_protocols.keys }
validates :code_suggestions, allow_nil: false, inclusion: { in: [true, false] }
validates :default_branch_protection_defaults, json_schema: { filename: 'default_branch_protection_defaults' }
validates :default_branch_protection_defaults, bytesize: { maximum: -> { DEFAULT_BRANCH_PROTECTIONS_DEFAULT_MAX_SIZE } }
validate :allow_mfa_for_group
validate :allow_resource_access_token_creation_for_group
......@@ -43,6 +47,9 @@ class NamespaceSetting < ApplicationRecord
project_runner_token_expiration_interval
].freeze
# matches the size set in the database constraint
DEFAULT_BRANCH_PROTECTIONS_DEFAULT_MAX_SIZE = 1.kilobyte
self.primary_key = :namespace_id
def self.allowed_namespace_settings_params
......
{
"$schema": "http://json-schema.org/draft-07/schema#",
"description": "Default settings for default branch protection",
"type": "object",
"properties": {
"name": {
"type": "string"
},
"allow_force_push": {
"type": "boolean"
},
"allowed_to_merge": {
"type": "array",
"items": {
"oneOf": [
{
"type": "object",
"additionalProperties": false,
"properties": {
"group_id": {
"type": "integer"
}
}
},
{
"type": "object",
"additionalProperties": false,
"properties": {
"access_level": {
"type": "integer"
}
}
}
]
}
},
"allowed_to_push": {
"type": "array",
"items": {
"oneOf": [
{
"type": "object",
"additionalProperties": false,
"properties": {
"group_id": {
"type": "integer"
}
}
},
{
"type": "object",
"additionalProperties": false,
"properties": {
"access_level": {
"type": "integer"
}
}
}
]
}
},
"code_owner_approval_required": {
"type": "boolean"
},
"merge_access_level": {
"type": "integer"
},
"push_access_level": {
"type": "integer"
},
"unprotect_access_level": {
"type": "integer"
}
},
"additionalProperties": false
}
# frozen_string_literal: true
class AddDefaultBranchProtectionsJsonToApplicationSettings < Gitlab::Database::Migration[2.1]
def change
add_column :application_settings, :default_branch_protection_defaults, :jsonb, null: false, default: {}
end
end
# frozen_string_literal: true
class AddDefaultBranchProtectionsJsonToNamespaceSettings < Gitlab::Database::Migration[2.1]
def change
add_column :namespace_settings, :default_branch_protection_defaults, :jsonb, null: false, default: {}
end
end
# frozen_string_literal: true
class AddSizeConstraintToNamespaceSettingsJson < Gitlab::Database::Migration[2.1]
CONSTRAINT_NAME = 'default_branch_protection_defaults_size_constraint'
disable_ddl_transaction!
def up
add_check_constraint :namespace_settings, 'octet_length(default_branch_protection_defaults::text) <= 1024',
CONSTRAINT_NAME, validate: false
end
def down
remove_check_constraint :namespace_settings, CONSTRAINT_NAME
end
end
# frozen_string_literal: true
class AddSizeConstraintToApplicationSettingsJson < Gitlab::Database::Migration[2.1]
CONSTRAINT_NAME = 'default_branch_protection_defaults_size_constraint'
disable_ddl_transaction!
def up
add_check_constraint :application_settings, 'octet_length(default_branch_protection_defaults::text) <= 1024',
CONSTRAINT_NAME, validate: false
end
def down
remove_check_constraint :application_settings, CONSTRAINT_NAME
end
end
490dc775459b89701c9531c7d10698734e47bd22a17aca07df12f8ac5a6d38e9
\ No newline at end of file
dd1cf7175a283e60af315372d96ea6836aff4e678830ab9b9d8e3367dc351c92
\ No newline at end of file
afe649b351bee4ec7152d465a73c35e4356d947c25fec727db2b4e38f268da41
\ No newline at end of file
df3b650081b827ab465aa0bf35b30a8fcb81a0c3609caecfa30e71d71761c809
\ No newline at end of file
......@@ -11850,6 +11850,7 @@ CREATE TABLE application_settings (
instance_level_code_suggestions_enabled boolean DEFAULT false NOT NULL,
delete_unconfirmed_users boolean DEFAULT false NOT NULL,
unconfirmed_users_delete_after_days integer DEFAULT 7 NOT NULL,
default_branch_protection_defaults jsonb DEFAULT '{}'::jsonb NOT NULL,
CONSTRAINT app_settings_container_reg_cleanup_tags_max_list_size_positive CHECK ((container_registry_cleanup_tags_service_max_list_size >= 0)),
CONSTRAINT app_settings_container_registry_pre_import_tags_rate_positive CHECK ((container_registry_pre_import_tags_rate >= (0)::numeric)),
CONSTRAINT app_settings_dep_proxy_ttl_policies_worker_capacity_positive CHECK ((dependency_proxy_ttl_group_policy_worker_capacity >= 0)),
......@@ -18754,6 +18755,7 @@ CREATE TABLE namespace_settings (
code_suggestions boolean DEFAULT false NOT NULL,
experiment_features_enabled boolean DEFAULT false NOT NULL,
third_party_ai_features_enabled boolean DEFAULT true NOT NULL,
default_branch_protection_defaults jsonb DEFAULT '{}'::jsonb NOT NULL,
CONSTRAINT check_0ba93c78c7 CHECK ((char_length(default_branch_name) <= 255)),
CONSTRAINT namespace_settings_unique_project_download_limit_alertlist_size CHECK ((cardinality(unique_project_download_limit_alertlist) <= 100)),
CONSTRAINT namespace_settings_unique_project_download_limit_allowlist_size CHECK ((cardinality(unique_project_download_limit_allowlist) <= 100))
......@@ -27096,6 +27098,12 @@ ALTER TABLE ONLY dast_site_validations
ALTER TABLE ONLY dast_sites
ADD CONSTRAINT dast_sites_pkey PRIMARY KEY (id);
 
ALTER TABLE namespace_settings
ADD CONSTRAINT default_branch_protection_defaults_size_constraint CHECK ((octet_length((default_branch_protection_defaults)::text) <= 1024)) NOT VALID;
ALTER TABLE application_settings
ADD CONSTRAINT default_branch_protection_defaults_size_constraint CHECK ((octet_length((default_branch_protection_defaults)::text) <= 1024)) NOT VALID;
ALTER TABLE ONLY dependency_list_exports
ADD CONSTRAINT dependency_list_exports_pkey PRIMARY KEY (id);
 
......@@ -247,8 +247,7 @@
"Packages::Composer::Metadatum" => %w[composer_json],
"RawUsageData" => %w[payload], # Usage data payload changes often, we cannot use one schema
"Releases::Evidence" => %w[summary],
"Vulnerabilities::Finding::Evidence" => %w[data], # Validation work in progress
"EE::Gitlab::BackgroundMigration::FixSecurityScanStatuses::SecurityScan" => %w[info] # This is a migration model
"Vulnerabilities::Finding::Evidence" => %w[data] # Validation work in progress
}.freeze
# We are skipping GEO models for now as it adds up complexity
......@@ -258,8 +257,10 @@
next if models_by_table_name[hash["table_name"]].nil?
models_by_table_name[hash["table_name"]].each do |model|
jsonb_columns = [hash["column_name"]] - ignored_jsonb_columns(model.name)
# Skip migration models
next if model.name.include?('Gitlab::BackgroundMigration')
jsonb_columns = [hash["column_name"]] - ignored_jsonb_columns(model.name)
expect(model).to validate_jsonb_schema(jsonb_columns)
end
end
......
......@@ -23,6 +23,7 @@
it { expect(setting.id).to eq(1) }
it { expect(setting.repository_storages_weighted).to eq({}) }
it { expect(setting.kroki_formats).to eq({}) }
it { expect(setting.default_branch_protection_defaults).to eq({}) }
end
describe 'validations' do
......@@ -1228,6 +1229,25 @@ def expect_invalid
it { is_expected.to allow_value(*Gitlab::ColorSchemes.valid_ids).for(:default_syntax_highlighting_theme) }
it { is_expected.not_to allow_value(nil, 0, Gitlab::ColorSchemes.available_schemes.size + 1).for(:default_syntax_highlighting_theme) }
end
context 'default_branch_protections_defaults validations' do
let(:charset) { [*'a'..'z'] + [*0..9] }
let(:value) { Array.new(byte_size) { charset.sample }.join }
it { expect(described_class).to validate_jsonb_schema(['default_branch_protection_defaults']) }
context 'when json is more than 1kb' do
let(:byte_size) { 1.1.kilobytes }
it { is_expected.not_to allow_value({ name: value }).for(:default_branch_protection_defaults) }
end
context 'when json less than 1kb' do
let(:byte_size) { 0.5.kilobytes }
it { is_expected.to allow_value({ name: value }).for(:default_branch_protection_defaults) }
end
end
end
context 'restrict creating duplicates' do
......@@ -1498,6 +1518,26 @@ def expect_invalid
end
end
describe 'default_branch_protection_defaults' do
let(:defaults) { { name: 'main', push_access_level: 30, merge_access_level: 30, unprotect_access_level: 40 } }
it 'returns the value for default_branch_protection_defaults' do
subject.default_branch_protection_defaults = defaults
expect(subject.default_branch_protection_defaults['name']).to eq('main')
expect(subject.default_branch_protection_defaults['push_access_level']).to eq(30)
expect(subject.default_branch_protection_defaults['merge_access_level']).to eq(30)
expect(subject.default_branch_protection_defaults['unprotect_access_level']).to eq(40)
end
context 'when provided with content that does not match the JSON schema' do
# valid json
it { is_expected.to allow_value({ name: 'bar' }).for(:default_branch_protection_defaults) }
# invalid json
it { is_expected.not_to allow_value({ foo: 'bar' }).for(:default_branch_protection_defaults) }
end
end
describe '#static_objects_external_storage_auth_token=', :aggregate_failures do
subject { setting.static_objects_external_storage_auth_token = token }
......
......@@ -14,6 +14,12 @@
it { is_expected.to define_enum_for(:jobs_to_be_done).with_values([:basics, :move_repository, :code_storage, :exploring, :ci, :other]).with_suffix }
it { is_expected.to define_enum_for(:enabled_git_access_protocol).with_values([:all, :ssh, :http]).with_suffix }
describe 'default values' do
subject(:setting) { described_class.new }
it { expect(setting.default_branch_protection_defaults).to eq({}) }
end
describe "validations" do
it { is_expected.to validate_inclusion_of(:code_suggestions).in_array([true, false]) }
......@@ -138,6 +144,25 @@
end
end
end
context 'default_branch_protections_defaults validations' do
let(:charset) { [*'a'..'z'] + [*0..9] }
let(:value) { Array.new(byte_size) { charset.sample }.join }
it { expect(described_class).to validate_jsonb_schema(['default_branch_protection_defaults']) }
context 'when json is more than 1kb' do
let(:byte_size) { 1.1.kilobytes }
it { is_expected.not_to allow_value({ name: value }).for(:default_branch_protection_defaults) }
end
context 'when json less than 1kb' do
let(:byte_size) { 0.5.kilobytes }
it { is_expected.to allow_value({ name: value }).for(:default_branch_protection_defaults) }
end
end
end
describe '#prevent_sharing_groups_outside_hierarchy' do
......@@ -404,4 +429,24 @@
describe '#delayed_project_removal' do
it_behaves_like 'a cascading namespace setting boolean attribute', settings_attribute_name: :delayed_project_removal
end
describe 'default_branch_protection_defaults' do
let(:defaults) { { name: 'main', push_access_level: 30, merge_access_level: 30, unprotect_access_level: 40 } }
it 'returns the value for default_branch_protection_defaults' do
subject.default_branch_protection_defaults = defaults
expect(subject.default_branch_protection_defaults['name']).to eq('main')
expect(subject.default_branch_protection_defaults['push_access_level']).to eq(30)
expect(subject.default_branch_protection_defaults['merge_access_level']).to eq(30)
expect(subject.default_branch_protection_defaults['unprotect_access_level']).to eq(40)
end
context 'when provided with content that does not match the JSON schema' do
# valid json
it { is_expected.to allow_value({ name: 'bar' }).for(:default_branch_protection_defaults) }
# invalid json
it { is_expected.not_to allow_value({ foo: 'bar' }).for(:default_branch_protection_defaults) }
end
end
end
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment