From b7b3c44381ba7275ed2d2e684194bf4435699b9f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Francisco=20Javier=20L=C3=B3pez?= Date: Tue, 12 Nov 2019 10:40:52 +0100 Subject: [PATCH] Added migrations for secret snippets For the new secret snippets feature we need to add a couple of migrations and also a new index to include the new fields. --- app/models/project_snippet.rb | 1 + app/models/snippet.rb | 29 ++++++++++++++----- .../snippets/_snippets_scope_menu.html.haml | 2 +- .../fj-secret-snippet-migrations.yml | 5 ++++ ...91025092748_add_secret_token_to_snippet.rb | 10 +++++++ .../20191105155113_add_secret_to_snippet.rb | 27 +++++++++++++++++ db/schema.rb | 5 +++- lib/gitlab/import_export/import_export.yml | 3 ++ spec/factories/snippets.rb | 5 ++++ spec/models/project_snippet_spec.rb | 1 + spec/models/snippet_spec.rb | 16 ++++++++++ 11 files changed, 95 insertions(+), 9 deletions(-) create mode 100644 changelogs/unreleased/fj-secret-snippet-migrations.yml create mode 100644 db/migrate/20191025092748_add_secret_token_to_snippet.rb create mode 100644 db/migrate/20191105155113_add_secret_to_snippet.rb diff --git a/app/models/project_snippet.rb b/app/models/project_snippet.rb index e732c1bd86fe62..ffb08e10f1f353 100644 --- a/app/models/project_snippet.rb +++ b/app/models/project_snippet.rb @@ -4,4 +4,5 @@ class ProjectSnippet < Snippet belongs_to :project validates :project, presence: true + validates :secret, inclusion: { in: [false] } end diff --git a/app/models/snippet.rb b/app/models/snippet.rb index 4010a3e2167220..51ab94e6f4ab83 100644 --- a/app/models/snippet.rb +++ b/app/models/snippet.rb @@ -51,8 +51,8 @@ def content_html_invalidated? # Scopes scope :are_internal, -> { where(visibility_level: Snippet::INTERNAL) } scope :are_private, -> { where(visibility_level: Snippet::PRIVATE) } - scope :are_public, -> { where(visibility_level: Snippet::PUBLIC) } - scope :public_and_internal, -> { where(visibility_level: [Snippet::PUBLIC, Snippet::INTERNAL]) } + scope :are_public, -> { public_only } + scope :are_secret, -> { public_only.where(secret: true) } scope :fresh, -> { order("created_at DESC") } scope :inc_author, -> { includes(:author) } scope :inc_relations_for_view, -> { includes(author: :status) } @@ -63,6 +63,11 @@ def content_html_invalidated? attr_spammable :title, spam_title: true attr_spammable :content, spam_description: true + attr_encrypted :secret_token, + key: Settings.attr_encrypted_db_key_base_truncated, + mode: :per_attribute_iv, + algorithm: 'aes-256-cbc' + def self.with_optional_visibility(value = nil) if value where(visibility_level: value) @@ -112,11 +117,8 @@ def self.for_project_with_user(project, user = nil) end def self.visible_to_or_authored_by(user) - where( - 'snippets.visibility_level IN (?) OR snippets.author_id = ?', - Gitlab::VisibilityLevel.levels_for_user(user), - user.id - ) + query = where(visibility_level: Gitlab::VisibilityLevel.levels_for_user(user)) + query.or(where(author_id: user.id)) end def self.reference_prefix @@ -222,6 +224,19 @@ def to_ability_name model_name.singular end + def valid_secret_token?(token) + return false unless token && secret_token + + ActiveSupport::SecurityUtils.secure_compare(token.to_s, secret_token.to_s) + end + + def as_json(options = {}) + options[:except] = Array.wrap(options[:except]) + options[:except] << :secret_token + + super + end + class << self # Searches for snippets with a matching title or file name. # diff --git a/app/views/snippets/_snippets_scope_menu.html.haml b/app/views/snippets/_snippets_scope_menu.html.haml index c312226dd6c987..cb59b11ca2b34f 100644 --- a/app/views/snippets/_snippets_scope_menu.html.haml +++ b/app/views/snippets/_snippets_scope_menu.html.haml @@ -9,7 +9,7 @@ - if include_private = subject.snippets.count - else - = subject.snippets.public_and_internal.count + = subject.snippets.public_and_internal_only.count - if include_private %li{ class: active_when(params[:scope] == "are_private") } diff --git a/changelogs/unreleased/fj-secret-snippet-migrations.yml b/changelogs/unreleased/fj-secret-snippet-migrations.yml new file mode 100644 index 00000000000000..67eefcbfd35189 --- /dev/null +++ b/changelogs/unreleased/fj-secret-snippet-migrations.yml @@ -0,0 +1,5 @@ +--- +title: Add migrations for secret snippets +merge_request: 19939 +author: +type: added diff --git a/db/migrate/20191025092748_add_secret_token_to_snippet.rb b/db/migrate/20191025092748_add_secret_token_to_snippet.rb new file mode 100644 index 00000000000000..0649f58d23e5b4 --- /dev/null +++ b/db/migrate/20191025092748_add_secret_token_to_snippet.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +class AddSecretTokenToSnippet < ActiveRecord::Migration[5.2] + DOWNTIME = false + + def change + add_column :snippets, :encrypted_secret_token, :string, limit: 255 + add_column :snippets, :encrypted_secret_token_iv, :string, limit: 255 + end +end diff --git a/db/migrate/20191105155113_add_secret_to_snippet.rb b/db/migrate/20191105155113_add_secret_to_snippet.rb new file mode 100644 index 00000000000000..ae514d484942f9 --- /dev/null +++ b/db/migrate/20191105155113_add_secret_to_snippet.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +class AddSecretToSnippet < ActiveRecord::Migration[5.2] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + unless column_exists?(:snippets, :secret) + add_column_with_default :snippets, :secret, :boolean, default: false + end + + add_concurrent_index :snippets, [:visibility_level, :secret] + remove_concurrent_index :snippets, :visibility_level + end + + def down + add_concurrent_index :snippets, :visibility_level + remove_concurrent_index :snippets, [:visibility_level, :secret] + + if column_exists?(:snippets, :secret) + remove_column :snippets, :secret + end + end +end diff --git a/db/schema.rb b/db/schema.rb index fd36599e1f9462..6d7b99a9f9fb45 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -3555,13 +3555,16 @@ t.integer "cached_markdown_version" t.text "description" t.text "description_html" + t.string "encrypted_secret_token", limit: 255 + t.string "encrypted_secret_token_iv", limit: 255 + t.boolean "secret", default: false, null: false t.index ["author_id"], name: "index_snippets_on_author_id" t.index ["content"], name: "index_snippets_on_content_trigram", opclass: :gin_trgm_ops, using: :gin t.index ["file_name"], name: "index_snippets_on_file_name_trigram", opclass: :gin_trgm_ops, using: :gin t.index ["project_id", "visibility_level"], name: "index_snippets_on_project_id_and_visibility_level" t.index ["title"], name: "index_snippets_on_title_trigram", opclass: :gin_trgm_ops, using: :gin t.index ["updated_at"], name: "index_snippets_on_updated_at" - t.index ["visibility_level"], name: "index_snippets_on_visibility_level" + t.index ["visibility_level", "secret"], name: "index_snippets_on_visibility_level_and_secret" end create_table "software_license_policies", id: :serial, force: :cascade do |t| diff --git a/lib/gitlab/import_export/import_export.yml b/lib/gitlab/import_export/import_export.yml index 1aafe5804c00c4..d491f45b0da035 100644 --- a/lib/gitlab/import_export/import_export.yml +++ b/lib/gitlab/import_export/import_export.yml @@ -163,6 +163,9 @@ excluded_attributes: - :identifier snippets: - :expired_at + - :secret + - :encrypted_secret_token + - :encrypted_secret_token_iv merge_request_diff: - :external_diff - :stored_externally diff --git a/spec/factories/snippets.rb b/spec/factories/snippets.rb index ede071ae70c3bd..5990ed7ffb0a3b 100644 --- a/spec/factories/snippets.rb +++ b/spec/factories/snippets.rb @@ -7,6 +7,7 @@ content { generate(:title) } description { generate(:title) } file_name { generate(:filename) } + secret { false } trait :public do visibility_level { Snippet::PUBLIC } @@ -27,5 +28,9 @@ end factory :personal_snippet, parent: :snippet, class: :PersonalSnippet do + trait :secret do + visibility_level { Snippet::PUBLIC } + secret { true } + end end end diff --git a/spec/models/project_snippet_spec.rb b/spec/models/project_snippet_spec.rb index 46025507cb5367..903671afb135bd 100644 --- a/spec/models/project_snippet_spec.rb +++ b/spec/models/project_snippet_spec.rb @@ -9,6 +9,7 @@ describe "Validation" do it { is_expected.to validate_presence_of(:project) } + it { is_expected.to validate_inclusion_of(:secret).in_array([false]) } end describe '#embeddable?' do diff --git a/spec/models/snippet_spec.rb b/spec/models/snippet_spec.rb index e4cc89318409f0..cfc0703af23ab9 100644 --- a/spec/models/snippet_spec.rb +++ b/spec/models/snippet_spec.rb @@ -451,4 +451,20 @@ expect(blob.data).to eq(snippet.content) end end + + describe '#to_json' do + let(:snippet) { build(:snippet) } + + it 'excludes secret_token from generated json' do + expect(JSON.parse(to_json).keys).not_to include("secret_token") + end + + it 'does not override existing exclude option value' do + expect(JSON.parse(to_json(except: [:id])).keys).not_to include("secret_token", "id") + end + + def to_json(params = {}) + snippet.to_json(params) + end + end end -- GitLab