From 908cfcef9e090de194ee883f5daa2dc33ef57a4e Mon Sep 17 00:00:00 2001 From: Ash McKenzie Date: Mon, 18 Mar 2019 13:28:27 +1100 Subject: [PATCH 1/3] Add new Secret visibility level Add Secret to the list of available visibility levels. Secret is essentially Public except you need to know the correct secret_word in order to access. --- .../javascripts/snippet/snippet_embed.js | 14 +- app/controllers/concerns/snippets_url.rb | 58 +++++ .../autocomplete_sources_controller.rb | 2 +- app/controllers/snippets_controller.rb | 16 +- app/finders/snippets_finder.rb | 2 + app/helpers/blob_helper.rb | 6 +- app/helpers/icons_helper.rb | 2 + app/helpers/snippets_helper.rb | 72 +++++- app/helpers/visibility_level_helper.rb | 6 +- app/models/personal_snippet.rb | 24 ++ app/models/project_snippet.rb | 1 + app/models/snippet.rb | 15 +- app/views/dashboard/snippets/index.html.haml | 2 +- app/views/projects/snippets/index.html.haml | 2 +- .../search/results/_snippet_blob.html.haml | 20 +- .../search/results/_snippet_title.html.haml | 5 +- app/views/shared/_visibility_radios.html.haml | 2 +- app/views/shared/snippets/_blob.html.haml | 3 +- app/views/shared/snippets/_header.html.haml | 2 +- .../snippets/_snippets_scope_menu.html.haml | 7 + app/views/snippets/show.html.haml | 2 +- .../unreleased/13235-secret-snippets.yml | 5 + .../20191025092748_add_secret_to_snippet.rb | 11 + db/schema.rb | 3 + .../gitlab/elastic/snippet_search_results.rb | 4 +- lib/api/entities.rb | 1 + lib/gitlab/import_export/import_export.yml | 3 + lib/gitlab/snippet_search_results.rb | 4 +- lib/gitlab/visibility_level.rb | 38 +++- locale/gitlab.pot | 6 +- spec/controllers/snippets_controller_spec.rb | 212 +++++++++++++++++- spec/factories/snippets.rb | 4 + spec/finders/snippets_finder_spec.rb | 131 ++++++----- spec/helpers/snippets_helper_spec.rb | 105 ++++++++- spec/helpers/visibility_level_helper_spec.rb | 31 +++ spec/lib/gitlab/visibility_level_spec.rb | 95 ++++++++ spec/models/personal_snippet_spec.rb | 111 +++++++++ spec/models/project_snippet_spec.rb | 22 ++ spec/models/snippet_spec.rb | 37 --- spec/requests/api/snippets_spec.rb | 73 +++++- 40 files changed, 969 insertions(+), 190 deletions(-) create mode 100644 app/controllers/concerns/snippets_url.rb create mode 100644 changelogs/unreleased/13235-secret-snippets.yml create mode 100644 db/migrate/20191025092748_add_secret_to_snippet.rb create mode 100644 spec/models/personal_snippet_spec.rb diff --git a/app/assets/javascripts/snippet/snippet_embed.js b/app/assets/javascripts/snippet/snippet_embed.js index 6606271c4fa7bd..8fefad448f842d 100644 --- a/app/assets/javascripts/snippet/snippet_embed.js +++ b/app/assets/javascripts/snippet/snippet_embed.js @@ -1,28 +1,30 @@ import { __ } from '~/locale'; +import { parseUrlPathname, parseUrl } from '../lib/utils/common_utils'; export default () => { const shareBtn = document.querySelector('.js-share-btn'); if (shareBtn) { - const { protocol, host, pathname } = window.location; - const embedBtn = document.querySelector('.js-embed-btn'); - const snippetUrlArea = document.querySelector('.js-snippet-url-area'); const embedAction = document.querySelector('.js-embed-action'); - const url = `${protocol}//${host + pathname}`; + const dataUrl = snippetUrlArea.getAttribute('data-url'); shareBtn.addEventListener('click', () => { shareBtn.classList.add('is-active'); embedBtn.classList.remove('is-active'); - snippetUrlArea.value = url; + snippetUrlArea.value = dataUrl; embedAction.innerText = __('Share'); }); embedBtn.addEventListener('click', () => { + const parser = parseUrl(dataUrl); + const url = `${parser.origin + parseUrlPathname(dataUrl)}`; + const params = parser.search; + const scriptTag = ``; + embedBtn.classList.add('is-active'); shareBtn.classList.remove('is-active'); - const scriptTag = ``; snippetUrlArea.value = scriptTag; embedAction.innerText = __('Embed'); }); diff --git a/app/controllers/concerns/snippets_url.rb b/app/controllers/concerns/snippets_url.rb new file mode 100644 index 00000000000000..3178d9937f9f6a --- /dev/null +++ b/app/controllers/concerns/snippets_url.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true + +module SnippetsUrl + extend ActiveSupport::Concern + + SNIPPETS_SECRET_KEYWORD = 'secret' + + private + + attr_reader :snippet + + def authorize_secret_snippet! + return unless snippet&.snippet? + + return render_404 unless snippet.valid_secret_token?(params[:token]) + + # current_user ? render_404 : authenticate_user! + end + + # def secrets_match?(secret) + # ActiveSupport::SecurityUtils.secure_compare(secret.to_s, snippet.secret_token) + # end + + # def ensure_complete_url + # redirect_to(complete_full_path.to_s) if redirect_to_complete_full_path? + # end + + # def redirect_to_complete_full_path? + # return unless snippet&.secret? + + # complete_full_path != current_full_path + # end + + # def complete_full_path + # @complete_full_path ||= begin + # path = current_full_path.clone + # secret_query = { SNIPPETS_SECRET_KEYWORD => snippet.secret_token } + # path.query = current_url_query_hash.merge(secret_query).to_query + # path + # end + # end + + # def current_full_path + # @current_full_path ||= begin + # path = URI.parse(current_url.path.chomp('/')) + # path.query = current_url_query_hash.to_query unless current_url_query_hash.empty? + # path + # end + # end + + # def current_url + # @current_url ||= URI.parse(request.original_url) + # end + + # def current_url_query_hash + # @current_url_query_hash ||= Rack::Utils.parse_nested_query(current_url.query) + # end +end diff --git a/app/controllers/projects/autocomplete_sources_controller.rb b/app/controllers/projects/autocomplete_sources_controller.rb index 605d70d440be4b..f91c256253fb0a 100644 --- a/app/controllers/projects/autocomplete_sources_controller.rb +++ b/app/controllers/projects/autocomplete_sources_controller.rb @@ -28,7 +28,7 @@ def commands end def snippets - render json: autocomplete_service.snippets + render json: autocomplete_service.snippets.to_json(only: [:id, :title]) end private diff --git a/app/controllers/snippets_controller.rb b/app/controllers/snippets_controller.rb index 5805d068e211ed..e2c67c2114fd93 100644 --- a/app/controllers/snippets_controller.rb +++ b/app/controllers/snippets_controller.rb @@ -5,6 +5,8 @@ class SnippetsController < ApplicationController include ToggleAwardEmoji include SpammableActions include SnippetsActions + include SnippetsHelper + # include SnippetsUrl include RendersBlob include PreviewMarkdown include PaginatedCollection @@ -18,6 +20,9 @@ class SnippetsController < ApplicationController # Allow read snippet before_action :authorize_read_snippet!, only: [:show, :raw] + # Ensure we're displaying the correct url, specifically for secret snippets + # before_action :ensure_complete_url, only: [:show, :raw] + # Allow modify snippet before_action :authorize_update_snippet!, only: [:edit, :update] @@ -119,17 +124,14 @@ def snippet alias_method :spammable, :snippet def spammable_path - snippet_path(@snippet) + reliable_snippet_path(@snippet) end def authorize_read_snippet! - return if can?(current_user, :read_personal_snippet, @snippet) + return if can?(current_user, :read_personal_snippet, snippet) + return if snippet&.secret? && snippet&.valid_secret_token?(params[:token]) - if current_user - render_404 - else - authenticate_user! - end + current_user ? render_404 : authenticate_user! end def authorize_update_snippet! diff --git a/app/finders/snippets_finder.rb b/app/finders/snippets_finder.rb index bd6b6190fb538d..fc063d7e5fc528 100644 --- a/app/finders/snippets_finder.rb +++ b/app/finders/snippets_finder.rb @@ -161,6 +161,8 @@ def visibility_from_scope case scope when 'are_private' Snippet::PRIVATE + when 'are_secret' + Snippet::SECRET when 'are_internal' Snippet::INTERNAL when 'are_public' diff --git a/app/helpers/blob_helper.rb b/app/helpers/blob_helper.rb index 5c24b0e1704e31..6c44a52d473cf6 100644 --- a/app/helpers/blob_helper.rb +++ b/app/helpers/blob_helper.rb @@ -141,11 +141,7 @@ def blob_raw_url(**kwargs) if @build && @entry raw_project_job_artifacts_url(@project, @build, path: @entry.path, **kwargs) elsif @snippet - if @snippet.project_id - raw_project_snippet_url(@project, @snippet, **kwargs) - else - raw_snippet_url(@snippet, **kwargs) - end + reliable_raw_snippet_url(@snippet) elsif @blob project_raw_url(@project, @id, **kwargs) end diff --git a/app/helpers/icons_helper.rb b/app/helpers/icons_helper.rb index 4f73270577fc60..ecf483c1233c9f 100644 --- a/app/helpers/icons_helper.rb +++ b/app/helpers/icons_helper.rb @@ -94,6 +94,8 @@ def visibility_level_icon(level, fw: true, options: {}) case level when Gitlab::VisibilityLevel::PRIVATE 'lock' + when Gitlab::VisibilityLevel::SECRET + 'user-secret' when Gitlab::VisibilityLevel::INTERNAL 'shield' else # Gitlab::VisibilityLevel::PUBLIC diff --git a/app/helpers/snippets_helper.rb b/app/helpers/snippets_helper.rb index 6ccc1fb2ed1ffb..19cec6ef4de887 100644 --- a/app/helpers/snippets_helper.rb +++ b/app/helpers/snippets_helper.rb @@ -11,22 +11,50 @@ def snippets_upload_path(snippet, user) end end - def reliable_snippet_path(snippet, opts = nil) - if snippet.project_id? - project_snippet_path(snippet.project, snippet, opts) - else - snippet_path(snippet, opts) + def reliable_snippet_path(snippet, opts = {}) + reliable_snippet_url(snippet, opts.merge(only_path: true)) + end + + def reliable_raw_snippet_path(snippet, opts = {}) + reliable_raw_snippet_url(snippet, opts.merge(only_path: true)) + end + + def reliable_snippet_url(snippet, opts = {}) + reliable_snippet_helper(snippet, opts) do |updated_opts| + if snippet.project_id? + project_snippet_url(snippet.project, snippet, nil, updated_opts) + else + snippet_url(snippet, nil, updated_opts) + end end end - def download_snippet_path(snippet) - if snippet.project_id - raw_project_snippet_path(@project, snippet, inline: false) - else - raw_snippet_path(snippet, inline: false) + def reliable_raw_snippet_url(snippet, opts = {}) + reliable_snippet_helper(snippet, opts) do |updated_opts| + if snippet.project_id? + raw_project_snippet_url(snippet.project, snippet, nil, updated_opts) + else + raw_snippet_url(snippet, nil, updated_opts) + end end end + def reliable_snippet_helper(snippet, opts) + opts[:token] = snippet.secret_token if snippet.secret? + opts[:only_path] = opts.fetch(:only_path, false) + + yield(opts) + end + + def download_raw_snippet_button(snippet) + link_to(icon('download'), reliable_raw_snippet_path(snippet, inline: false), target: '_blank', rel: 'noopener noreferrer', class: "btn btn-sm has-tooltip", title: 'Download', data: { container: 'body' }) + end + + def shareable_snippets_link(snippet) + url = reliable_snippet_url(snippet) + link_to(url, url, id: 'shareable_link_url', title: 'Open') + end + # Return the path of a snippets index for a user or for a project # # @returns String, path to snippet index @@ -114,8 +142,28 @@ def chunk_snippet(snippet, query, surrounding_lines = 3) { snippet_object: snippet, snippet_chunks: snippet_chunks } end - def snippet_embed - "" + def snippet_embed_url(snippet) + content_tag(:script, nil, src: reliable_snippet_url(snippet, format: :js, only_path: false)) + end + + def snippet_badge(snippet) + attrs = snippet_badge_attributes(snippet) + if attrs + css_class, text = attrs + tag.span(class: ['badge', 'badge-gray']) do + concat(tag.i(class: ['fa', css_class])) + concat(' ') + concat(_(text)) + end + end + end + + def snippet_badge_attributes(snippet) + if snippet.private? + ['fa-lock', 'private'] + elsif snippet.secret? + ['fa-user-secret', 'secret'] + end end def embedded_snippet_raw_button diff --git a/app/helpers/visibility_level_helper.rb b/app/helpers/visibility_level_helper.rb index a36de5dc548289..c71e3fb86e19ea 100644 --- a/app/helpers/visibility_level_helper.rb +++ b/app/helpers/visibility_level_helper.rb @@ -58,6 +58,8 @@ def snippet_visibility_level_description(level, snippet = nil) else _("The snippet is visible only to me.") end + when Gitlab::VisibilityLevel::SECRET + "The snippet can be accessed without any authentication, but is not searchable." when Gitlab::VisibilityLevel::INTERNAL _("The snippet is visible to any logged in user.") when Gitlab::VisibilityLevel::PUBLIC @@ -130,9 +132,7 @@ def project_visibility_icon_description(level) end def visibility_level_label(level) - # The visibility level can be: - # 'VisibilityLevel|Private', 'VisibilityLevel|Internal', 'VisibilityLevel|Public' - s_(Project.visibility_levels.key(level)) + s_(::Gitlab::VisibilityLevel.all_options.key(level)) end def restricted_visibility_levels(show_all = false) diff --git a/app/models/personal_snippet.rb b/app/models/personal_snippet.rb index 1b5be8698b152f..f0cc1ed46cfa55 100644 --- a/app/models/personal_snippet.rb +++ b/app/models/personal_snippet.rb @@ -2,4 +2,28 @@ class PersonalSnippet < Snippet include WithUploads + + before_save :update_secret_token + + def embeddable? + super || secret? + end + + alias_method :visibility_secret?, :secret? + def secret? + visibility_secret? && secret_token? + end + + private + + def update_secret_token + # secret? checks the visibility and also if the token exists + return if secret? + + # If the visibility is secret assign a random value, otherwise + # assign a nil value + self.secret_token = if visibility_secret? + SecureRandom.hex + end + end end diff --git a/app/models/project_snippet.rb b/app/models/project_snippet.rb index e732c1bd86fe62..a5ce96ffdee9a6 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 :visibility_level, exclusion: { in: [Gitlab::VisibilityLevel::SECRET] } end diff --git a/app/models/snippet.rb b/app/models/snippet.rb index 4010a3e2167220..efa7bea7fa68cd 100644 --- a/app/models/snippet.rb +++ b/app/models/snippet.rb @@ -46,11 +46,12 @@ def content_html_invalidated? length: { maximum: 255 } validates :content, presence: true - validates :visibility_level, inclusion: { in: Gitlab::VisibilityLevel.values } + validates :visibility_level, inclusion: { in: Gitlab::VisibilityLevel.all_values } # Scopes - scope :are_internal, -> { where(visibility_level: Snippet::INTERNAL) } scope :are_private, -> { where(visibility_level: Snippet::PRIVATE) } + scope :are_secret, -> { where(visibility_level: Snippet::SECRET) } + scope :are_internal, -> { where(visibility_level: Snippet::INTERNAL) } scope :are_public, -> { where(visibility_level: Snippet::PUBLIC) } scope :public_and_internal, -> { where(visibility_level: [Snippet::PUBLIC, Snippet::INTERNAL]) } scope :fresh, -> { order("created_at DESC") } @@ -63,6 +64,12 @@ 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_and_salt, + insecure_mode: true, + algorithm: 'aes-256-cbc' + def self.with_optional_visibility(value = nil) if value where(visibility_level: value) @@ -222,6 +229,10 @@ def to_ability_name model_name.singular end + def valid_secret_token?(secret) + ActiveSupport::SecurityUtils.secure_compare(secret.to_s, secret_token) + end + class << self # Searches for snippets with a matching title or file name. # diff --git a/app/views/dashboard/snippets/index.html.haml b/app/views/dashboard/snippets/index.html.haml index 2caa8e0cac4b6a..db6bb160c6b29c 100644 --- a/app/views/dashboard/snippets/index.html.haml +++ b/app/views/dashboard/snippets/index.html.haml @@ -4,7 +4,7 @@ = render 'dashboard/snippets_head' - if current_user.snippets.exists? - = render partial: 'snippets/snippets_scope_menu', locals: { include_private: true } + = render partial: 'snippets/snippets_scope_menu', locals: { include_private: true, include_secret: true } - if current_user.snippets.exists? = render partial: 'shared/snippets/list', locals: { link_project: true } diff --git a/app/views/projects/snippets/index.html.haml b/app/views/projects/snippets/index.html.haml index 7682d01a5a1e76..a0ceb88ee7d860 100644 --- a/app/views/projects/snippets/index.html.haml +++ b/app/views/projects/snippets/index.html.haml @@ -4,7 +4,7 @@ - if current_user .top-area - include_private = @project.team.member?(current_user) || current_user.admin? - = render partial: 'snippets/snippets_scope_menu', locals: { subject: @project, include_private: include_private } + = render partial: 'snippets/snippets_scope_menu', locals: { subject: @project, include_private: include_private, include_secret: false } - if can?(current_user, :create_project_snippet, @project) .nav-controls diff --git a/app/views/search/results/_snippet_blob.html.haml b/app/views/search/results/_snippet_blob.html.haml index f17dae0a94ccc6..267996d305ed4f 100644 --- a/app/views/search/results/_snippet_blob.html.haml +++ b/app/views/search/results/_snippet_blob.html.haml @@ -3,14 +3,20 @@ - snippet_chunks = snippet_blob[:snippet_chunks] .search-result-row - %span - = snippet.title - by - = link_to user_snippets_path(snippet.author) do - = image_tag avatar_icon_for_user(snippet.author), class: "avatar avatar-inline s16", alt: '' - = snippet.author_name - %span.light= time_ago_with_tooltip(snippet.created_at) %h4.snippet-title + = link_to reliable_snippet_path(snippet) do + = snippet.title + + .snippet-box.has-tooltip.inline.append-right-5.append-bottom-10{ title: snippet_visibility_level_description(snippet.visibility_level, snippet), data: { container: "body" } } + %span.sr-only + = visibility_level_label(snippet.visibility_level) + = visibility_level_icon(snippet.visibility_level, fw: false) + %span.creator + Authored + = time_ago_with_tooltip(snippet.created_at, placement: 'bottom', html_class: 'snippet_updated_ago') + by #{link_to_member(snippet.project, snippet.author, size: 24, author_class: "author item-title", avatar_class: "d-none d-sm-inline")} + = user_status(snippet.author) + - snippet_path = reliable_snippet_path(snippet) .file-holder .js-file-title.file-title diff --git a/app/views/search/results/_snippet_title.html.haml b/app/views/search/results/_snippet_title.html.haml index 1e01088d9e6936..7280146720efa8 100644 --- a/app/views/search/results/_snippet_title.html.haml +++ b/app/views/search/results/_snippet_title.html.haml @@ -2,10 +2,7 @@ %h4.snippet-title.term = link_to reliable_snippet_path(snippet_title) do = truncate(snippet_title.title, length: 60) - - if snippet_title.private? - %span.badge.badge-gray - %i.fa.fa-lock - = _("private") + = snippet_badge(snippet_title) %span.cgray.monospace.tiny.float-right.term = snippet_title.file_name diff --git a/app/views/shared/_visibility_radios.html.haml b/app/views/shared/_visibility_radios.html.haml index 80532c9187bd55..fcc8a2ab8a3eb2 100644 --- a/app/views/shared/_visibility_radios.html.haml +++ b/app/views/shared/_visibility_radios.html.haml @@ -1,4 +1,4 @@ -- Gitlab::VisibilityLevel.values.each do |level| +- Gitlab::VisibilityLevel.values_for(form_model).each do |level| - disallowed = disallowed_visibility_level?(form_model, level) - restricted = restricted_visibility_levels.include?(level) - next if disallowed || restricted diff --git a/app/views/shared/snippets/_blob.html.haml b/app/views/shared/snippets/_blob.html.haml index 2132fcbccc57e8..6a5e777706c868 100644 --- a/app/views/shared/snippets/_blob.html.haml +++ b/app/views/shared/snippets/_blob.html.haml @@ -8,7 +8,6 @@ .btn-group{ role: "group" }< = copy_blob_source_button(blob) = open_raw_blob_button(blob) - - = link_to icon('download'), download_snippet_path(@snippet), target: '_blank', class: "btn btn-sm has-tooltip", title: 'Download', data: { container: 'body' } + = download_raw_snippet_button(@snippet) = render 'projects/blob/content', blob: blob diff --git a/app/views/shared/snippets/_header.html.haml b/app/views/shared/snippets/_header.html.haml index 8d94a87a775189..fe99b5d15098d8 100644 --- a/app/views/shared/snippets/_header.html.haml +++ b/app/views/shared/snippets/_header.html.haml @@ -44,7 +44,7 @@ %li %button.js-share-btn.btn.btn-transparent{ type: 'button' } %strong.embed-toggle-list-item= _("Share") - %input.js-snippet-url-area.snippet-embed-input.form-control{ type: "text", autocomplete: 'off', value: snippet_embed } + %input.js-snippet-url-area.snippet-embed-input.form-control{ type: "text", autocomplete: 'off', data: { url: reliable_snippet_url(@snippet) }, value: snippet_embed_url(@snippet) } .input-group-append = clipboard_button(title: _('Copy'), class: 'js-clipboard-btn snippet-clipboard-btn btn btn-default', target: '.js-snippet-url-area') .clearfix diff --git a/app/views/snippets/_snippets_scope_menu.html.haml b/app/views/snippets/_snippets_scope_menu.html.haml index c312226dd6c987..1eb34842881294 100644 --- a/app/views/snippets/_snippets_scope_menu.html.haml +++ b/app/views/snippets/_snippets_scope_menu.html.haml @@ -24,6 +24,13 @@ %span.badge.badge-pill = subject.snippets.are_internal.count + - if include_secret + %li{ class: active_when(params[:scope] == "are_secret") } + = link_to subject_snippets_path(subject, scope: 'are_secret') do + Secret + %span.badge.badge-pill + = subject.snippets.are_secret.count + %li{ class: active_when(params[:scope] == "are_public") } = link_to subject_snippets_path(subject, scope: 'are_public') do = _("Public") diff --git a/app/views/snippets/show.html.haml b/app/views/snippets/show.html.haml index 36b4e00e8d51b4..afa564652583fb 100644 --- a/app/views/snippets/show.html.haml +++ b/app/views/snippets/show.html.haml @@ -10,7 +10,7 @@ %article.file-holder.snippet-file-content = render 'shared/snippets/blob' - .row-content-block.top-block.content-component-block + .row-content-block.top-block.content-component-block.append-bottom-10 = render 'award_emoji/awards_block', awardable: @snippet, inline: true #notes.limited-width-notes= render "shared/notes/notes_with_form", :autocomplete => false diff --git a/changelogs/unreleased/13235-secret-snippets.yml b/changelogs/unreleased/13235-secret-snippets.yml new file mode 100644 index 00000000000000..e2635abe6977da --- /dev/null +++ b/changelogs/unreleased/13235-secret-snippets.yml @@ -0,0 +1,5 @@ +--- +title: Support Secret Snippets +merge_request: 24042 +author: +type: changed diff --git a/db/migrate/20191025092748_add_secret_to_snippet.rb b/db/migrate/20191025092748_add_secret_to_snippet.rb new file mode 100644 index 00000000000000..4135302a2e0672 --- /dev/null +++ b/db/migrate/20191025092748_add_secret_to_snippet.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class AddSecretToSnippet < 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 + add_column :snippets, :encrypted_secret_token_salt, :string, limit: 255 + end +end diff --git a/db/schema.rb b/db/schema.rb index f6a445f3da6d7b..ac7c338c78bb93 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -3490,6 +3490,9 @@ 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.string "encrypted_secret_token_salt", limit: 255 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 diff --git a/ee/lib/gitlab/elastic/snippet_search_results.rb b/ee/lib/gitlab/elastic/snippet_search_results.rb index d146930dfed80a..56596992db5d55 100644 --- a/ee/lib/gitlab/elastic/snippet_search_results.rb +++ b/ee/lib/gitlab/elastic/snippet_search_results.rb @@ -8,9 +8,9 @@ def objects(scope, page = 1) case scope when 'snippet_titles' - eager_load(snippet_titles, page, eager: { project: [:route, :namespace] }) + eager_load(snippet_titles, page, eager: { project: [:route, :namespace], author: :status }) when 'snippet_blobs' - eager_load(snippet_blobs, page, eager: { project: [:route, :namespace] }) + eager_load(snippet_blobs, page, eager: { project: [:route, :namespace], author: :status }) end end diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 9faae4605270b3..c23199eda1699e 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -532,6 +532,7 @@ class PersonalSnippet < Snippet expose :raw_url do |snippet| Gitlab::UrlBuilder.build(snippet) + "/raw" end + expose :secret_token, if: -> (entity, _) { entity.secret? } end class IssuableEntity < Grape::Entity diff --git a/lib/gitlab/import_export/import_export.yml b/lib/gitlab/import_export/import_export.yml index 1aafe5804c00c4..0088e43b60d974 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 + - :encrypted_secret_token + - :encrypted_secret_token_iv + - :encrypted_secret_token_salt merge_request_diff: - :external_diff - :stored_externally diff --git a/lib/gitlab/snippet_search_results.rb b/lib/gitlab/snippet_search_results.rb index e955ccd35daed9..44ac0dd21dbe7e 100644 --- a/lib/gitlab/snippet_search_results.rb +++ b/lib/gitlab/snippet_search_results.rb @@ -53,11 +53,11 @@ def snippets # rubocop: enable CodeReuse/ActiveRecord def snippet_titles - snippets.search(query) + snippets.search(query).inc_relations_for_view end def snippet_blobs - snippets.search_code(query) + snippets.search_code(query).inc_relations_for_view end def default_scope diff --git a/lib/gitlab/visibility_level.rb b/lib/gitlab/visibility_level.rb index e2787744f09dd2..1d290687e7eb79 100644 --- a/lib/gitlab/visibility_level.rb +++ b/lib/gitlab/visibility_level.rb @@ -20,12 +20,11 @@ module VisibilityLevel end PRIVATE = 0 unless const_defined?(:PRIVATE) + SECRET = 5 unless const_defined?(:SECRET) INTERNAL = 10 unless const_defined?(:INTERNAL) PUBLIC = 20 unless const_defined?(:PUBLIC) class << self - delegate :values, to: :options - def levels_for_user(user = nil) return [PUBLIC] unless user @@ -38,8 +37,13 @@ def levels_for_user(user = nil) end end - def string_values - string_options.keys + def values_for(model) + case model + when PersonalSnippet + all_values + else + values + end end def options @@ -50,14 +54,36 @@ def options } end + def values + options.values + end + + def all_options + { + N_('VisibilityLevel|Private') => PRIVATE, + N_('VisibilityLevel|Secret') => SECRET, + N_('VisibilityLevel|Internal') => INTERNAL, + N_('VisibilityLevel|Public') => PUBLIC + } + end + + def all_values + all_options.values + end + def string_options { 'private' => PRIVATE, + 'secret' => SECRET, 'internal' => INTERNAL, 'public' => PUBLIC } end + def string_values + string_options.keys + end + def allowed_levels restricted_levels = Gitlab::CurrentSettings.restricted_visibility_levels @@ -127,6 +153,10 @@ def public? visibility_level_value == PUBLIC end + def secret? + visibility_level_value == SECRET + end + def visibility_level_value self[visibility_level_field] end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index dde99a8583ac34..cd9e0971653efc 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -18743,6 +18743,9 @@ msgstr "" msgid "VisibilityLevel|Public" msgstr "" +msgid "VisibilityLevel|Secret" +msgstr "" + msgid "VisibilityLevel|Unknown" msgstr "" @@ -20570,9 +20573,6 @@ msgid_plural "points" msgstr[0] "" msgstr[1] "" -msgid "private" -msgstr "" - msgid "private key does not match certificate." msgstr "" diff --git a/spec/controllers/snippets_controller_spec.rb b/spec/controllers/snippets_controller_spec.rb index e892c736c69f66..bebe9831cb29c7 100644 --- a/spec/controllers/snippets_controller_spec.rb +++ b/spec/controllers/snippets_controller_spec.rb @@ -5,9 +5,79 @@ describe SnippetsController do let(:user) { create(:user) } - describe 'GET #index' do - let(:user) { create(:user) } + shared_examples 'allow read access to secret snippets' do + let_it_be(:secret_snippet) { create(:personal_snippet, :secret) } + let(:author) { secret_snippet.author } + let(:snippet_params) { { id: secret_snippet.to_param } } + let(:secret_token) { secret_snippet.secret_token } + let(:params) { snippet_params } + + before do + sign_in(user) if user + + subject + end + + context 'when not signed in' do + let(:user) { nil } + + it 'redirects to the sign in page' do + expect(response).to redirect_to(new_user_session_path) + end + end + + context 'when user is not the author' do + context 'when the token is not present' do + it 'responds with status 404' do + expect(response).to have_gitlab_http_status(404) + end + end + context 'when the token is not valid' do + let(:params) { snippet_params.merge(token: 'foo') } + + it 'responds with status 404' do + expect(response).to have_gitlab_http_status(404) + end + end + + context 'when the token is valid' do + let(:params) { snippet_params.merge(token: secret_token) } + + it 'responds with status 200' do + expect(response).to have_gitlab_http_status(200) + end + end + end + + context 'when user is the author' do + let(:user) { author } + + context 'when the token is not present' do + it 'responds with status 200' do + expect(response).to have_gitlab_http_status(200) + end + end + + context 'when the token is not valid' do + let(:params) { snippet_params.merge(token: 'foo') } + + it 'responds with status 200' do + expect(response).to have_gitlab_http_status(200) + end + end + + context 'when the token is valid' do + let(:params) { snippet_params.merge(token: secret_token) } + + it 'responds with status 200' do + expect(response).to have_gitlab_http_status(200) + end + end + end + end + + describe 'GET #index' do context 'when username parameter is present' do it_behaves_like 'paginated collection' do let(:collection) { Snippet.all } @@ -65,6 +135,10 @@ end describe 'GET #show' do + it_behaves_like 'allow read access to secret snippets' do + subject { get :show, params: params } + end + context 'when the personal snippet is private' do let(:personal_snippet) { create(:personal_snippet, :private, author: user) } @@ -306,12 +380,59 @@ def create_snippet(snippet_params = {}, additional_params = {}) end end + describe 'GET #edit' do + context 'when the snippet is secret' do + let_it_be(:snippet) { create :personal_snippet, :secret } + let(:user) { snippet.author } + let(:params) { { id: snippet.id, token: snippet.secret_token } } + + subject { get :edit, params: params } + + before do + sign_in(user) if user + + subject + end + + context 'when the user is not signed in' do + let(:user) { nil } + + it 'redirects to the sign in page' do + expect(response).to redirect_to(new_user_session_path) + end + end + + context 'when the user not the author' do + let(:user) { create(:user) } + + it 'responds with 404' do + expect(response).to have_gitlab_http_status(404) + end + end + + context 'when the user is the author' do + it 'responds with 200' do + expect(response).to have_gitlab_http_status(200) + end + + context 'without the token param' do + let(:params) { { id: snippet.id } } + + it 'responds with 200' do + expect(response).to have_gitlab_http_status(200) + end + end + end + end + end + describe 'PUT #update' do let(:project) { create :project } - let(:snippet) { create :personal_snippet, author: user, project: project, visibility_level: visibility_level } + let(:snippet) { create :personal_snippet, visibility_level: visibility_level } + let(:user) { snippet.author } def update_snippet(snippet_params = {}, additional_params = {}) - sign_in(user) + sign_in(user) if user put :update, params: { id: snippet.id, @@ -425,6 +546,39 @@ def update_snippet(snippet_params = {}, additional_params = {}) end end end + + context 'when the snippet is secret' do + let(:visibility_level) { Snippet::SECRET } + + subject { update_snippet({ title: 'Foo' }, { token: snippet.secret_token }) } + + before do + subject + end + + context 'when the user is not authenticated' do + let(:user) { nil } + + it 'redirects to the sign in page' do + expect(response).to redirect_to(new_user_session_path) + end + end + + context 'when the user is not the author' do + let(:user) { create(:user) } + + it 'responds with 404' do + expect(response).to have_gitlab_http_status(404) + end + end + + context 'when the user is the author' do + it 'responds with 302' do + expect(response).to have_gitlab_http_status(302) + expect(snippet.title).to eq 'Foo' + end + end + end end describe 'POST #mark_as_spam' do @@ -451,6 +605,10 @@ def mark_as_spam end describe "GET #raw" do + it_behaves_like 'allow read access to secret snippets' do + subject { get :raw, params: params } + end + context 'when the personal snippet is private' do let(:personal_snippet) { create(:personal_snippet, :private, author: user) } @@ -634,4 +792,50 @@ def mark_as_spam expect(json_response.keys).to match_array(%w(body references)) end end + + describe "DELETE #destroy" do + context 'when the snippet is secret' do + let(:snippet) { create :personal_snippet, :secret } + let(:user) { snippet.author } + let(:params) { { id: snippet.id, token: snippet.secret_token } } + + subject { delete :destroy, params: params } + + before do + sign_in(user) if user + + subject + end + + context 'when the user is not signed in' do + let(:user) { nil } + + it 'redirects to the sign in page' do + expect(response).to redirect_to(new_user_session_path) + end + end + + context 'when the user not the author' do + let(:user) { create(:user) } + + it 'responds with 404' do + expect(response).to have_gitlab_http_status(404) + end + end + + context 'when the user is the author' do + it 'responds with 302' do + expect(response).to have_gitlab_http_status(302) + end + + context 'without the token param' do + let(:params) { { id: snippet.id } } + + it 'responds with 200' do + expect(response).to have_gitlab_http_status(302) + end + end + end + end + end end diff --git a/spec/factories/snippets.rb b/spec/factories/snippets.rb index ede071ae70c3bd..e399b918863a49 100644 --- a/spec/factories/snippets.rb +++ b/spec/factories/snippets.rb @@ -12,6 +12,10 @@ visibility_level { Snippet::PUBLIC } end + trait :secret do + visibility_level { Snippet::SECRET } + end + trait :internal do visibility_level { Snippet::INTERNAL } end diff --git a/spec/finders/snippets_finder_spec.rb b/spec/finders/snippets_finder_spec.rb index bcb762664f7627..6a1f5378966bbf 100644 --- a/spec/finders/snippets_finder_spec.rb +++ b/spec/finders/snippets_finder_spec.rb @@ -18,6 +18,7 @@ describe '#execute' do let_it_be(:user) { create(:user) } + let_it_be(:other_user) { create(:user) } let_it_be(:admin) { create(:admin) } let_it_be(:group) { create(:group, :public) } let_it_be(:project) { create(:project, :public, group: group) } @@ -25,6 +26,7 @@ let_it_be(:private_personal_snippet) { create(:personal_snippet, :private, author: user) } let_it_be(:internal_personal_snippet) { create(:personal_snippet, :internal, author: user) } let_it_be(:public_personal_snippet) { create(:personal_snippet, :public, author: user) } + let_it_be(:secret_personal_snippet) { create(:personal_snippet, :secret, author: user) } let_it_be(:private_project_snippet) { create(:project_snippet, :private, project: project) } let_it_be(:internal_project_snippet) { create(:project_snippet, :internal, project: project) } @@ -32,94 +34,93 @@ context 'filter by scope' do it "returns all snippets for 'all' scope" do - snippets = described_class.new(user, scope: :all).execute - - expect(snippets).to contain_exactly( + expect(find_snippets(:all)).to contain_exactly( private_personal_snippet, internal_personal_snippet, public_personal_snippet, - internal_project_snippet, public_project_snippet + internal_project_snippet, public_project_snippet, secret_personal_snippet ) end it "returns all snippets for 'are_private' scope" do - snippets = described_class.new(user, scope: :are_private).execute - - expect(snippets).to contain_exactly(private_personal_snippet) + expect(find_snippets(:are_private)).to contain_exactly(private_personal_snippet) end it "returns all snippets for 'are_internal' scope" do - snippets = described_class.new(user, scope: :are_internal).execute - - expect(snippets).to contain_exactly(internal_personal_snippet, internal_project_snippet) + expect(find_snippets(:are_internal)).to contain_exactly(internal_personal_snippet, internal_project_snippet) end it "returns all snippets for 'are_public' scope" do - snippets = described_class.new(user, scope: :are_public).execute + expect(find_snippets(:are_public)).to contain_exactly(public_personal_snippet, public_project_snippet) + end + + it "returns all snippets for 'are_secret' scope" do + expect(find_snippets(:are_secret)).to contain_exactly(secret_personal_snippet) + end + + context 'when the user it not the author' do + let(:user) { other_user } + + it "returns all snippets for 'all' scope" do + expect(find_snippets(:all)).to contain_exactly( + internal_personal_snippet, public_personal_snippet, + internal_project_snippet, public_project_snippet + ) + end + end - expect(snippets).to contain_exactly(public_personal_snippet, public_project_snippet) + def find_snippets(scope) + described_class.new(user, scope: scope).execute end end context 'filter by author' do it 'returns all public and internal snippets' do - snippets = described_class.new(create(:user), author: user).execute - - expect(snippets).to contain_exactly(internal_personal_snippet, public_personal_snippet) + expect(find_snippets(other_user)).to contain_exactly(internal_personal_snippet, public_personal_snippet) end it 'returns internal snippets' do - snippets = described_class.new(user, author: user, scope: :are_internal).execute - - expect(snippets).to contain_exactly(internal_personal_snippet) + expect(find_snippets(user, scope: :are_internal)).to contain_exactly(internal_personal_snippet) end it 'returns private snippets' do - snippets = described_class.new(user, author: user, scope: :are_private).execute - - expect(snippets).to contain_exactly(private_personal_snippet) + expect(find_snippets(user, scope: :are_private)).to contain_exactly(private_personal_snippet) end it 'returns public snippets' do - snippets = described_class.new(user, author: user, scope: :are_public).execute + expect(find_snippets(user, scope: :are_public)).to contain_exactly(public_personal_snippet) + end - expect(snippets).to contain_exactly(public_personal_snippet) + it 'returns secret snippets' do + expect(find_snippets(user, scope: :are_secret)).to contain_exactly(secret_personal_snippet) end it 'returns all snippets' do - snippets = described_class.new(user, author: user).execute - - expect(snippets).to contain_exactly(private_personal_snippet, internal_personal_snippet, public_personal_snippet) + expect(find_snippets(user)).to contain_exactly(private_personal_snippet, internal_personal_snippet, public_personal_snippet, secret_personal_snippet) end it 'returns only public snippets if unauthenticated user' do - snippets = described_class.new(nil, author: user).execute - - expect(snippets).to contain_exactly(public_personal_snippet) + expect(find_snippets(nil)).to contain_exactly(public_personal_snippet) end it 'returns all snippets for an admin' do - snippets = described_class.new(admin, author: user).execute + expect(find_snippets(admin)).to contain_exactly(private_personal_snippet, internal_personal_snippet, public_personal_snippet) + end - expect(snippets).to contain_exactly(private_personal_snippet, internal_personal_snippet, public_personal_snippet) + def find_snippets(search_user, scope: nil) + described_class.new(search_user, author: user, scope: scope).execute end end context 'project snippets' do it 'returns public personal and project snippets for unauthorized user' do - snippets = described_class.new(nil, project: project).execute - - expect(snippets).to contain_exactly(public_project_snippet) + expect(find_snippets(nil)).to contain_exactly(public_project_snippet) end it 'returns public and internal snippets for non project members' do - snippets = described_class.new(user, project: project).execute - - expect(snippets).to contain_exactly(internal_project_snippet, public_project_snippet) + expect(find_snippets(user)).to contain_exactly(internal_project_snippet, public_project_snippet) end it 'returns public snippets for non project members' do - snippets = described_class.new(user, project: project, scope: :are_public).execute - - expect(snippets).to contain_exactly(public_project_snippet) + expect(find_snippets(user, scope: :are_public)).to contain_exactly(public_project_snippet) end it 'returns internal snippets for non project members' do @@ -129,31 +130,23 @@ end it 'does not return private snippets for non project members' do - snippets = described_class.new(user, project: project, scope: :are_private).execute - - expect(snippets).to be_empty + expect(find_snippets(user, scope: :are_private)).to be_empty end it 'returns all snippets for project members' do project.add_developer(user) - snippets = described_class.new(user, project: project).execute - - expect(snippets).to contain_exactly(private_project_snippet, internal_project_snippet, public_project_snippet) + expect(find_snippets(user)).to contain_exactly(private_project_snippet, internal_project_snippet, public_project_snippet) end it 'returns private snippets for project members' do project.add_developer(user) - snippets = described_class.new(user, project: project, scope: :are_private).execute - - expect(snippets).to contain_exactly(private_project_snippet) + expect(find_snippets(user, scope: :are_private)).to contain_exactly(private_project_snippet) end it 'returns all snippets for an admin' do - snippets = described_class.new(admin, project: project).execute - - expect(snippets).to contain_exactly(private_project_snippet, internal_project_snippet, public_project_snippet) + expect(find_snippets(admin)).to contain_exactly(private_project_snippet, internal_project_snippet, public_project_snippet) end context 'filter by author' do @@ -175,30 +168,32 @@ ) end end + + def find_snippets(search_user, author: nil, scope: nil) + described_class.new(search_user, author: author, scope: scope, project: project).execute + end end context 'explore snippets' do it 'returns only public personal snippets for unauthenticated users' do - snippets = described_class.new(nil, explore: true).execute - - expect(snippets).to contain_exactly(public_personal_snippet) + expect(find_snippets(nil)).to contain_exactly(public_personal_snippet) end it 'also returns internal personal snippets for authenticated users' do - snippets = described_class.new(user, explore: true).execute - - expect(snippets).to contain_exactly( + expect(find_snippets(user)).to contain_exactly( internal_personal_snippet, public_personal_snippet ) end it 'returns all personal snippets for admins' do - snippets = described_class.new(admin, explore: true).execute - - expect(snippets).to contain_exactly( + expect(find_snippets(admin)).to contain_exactly( private_personal_snippet, internal_personal_snippet, public_personal_snippet ) end + + def find_snippets(search_user) + described_class.new(search_user, explore: true).execute + end end context 'when the user cannot read cross project' do @@ -208,7 +203,9 @@ end it 'returns only personal snippets when the user cannot read cross project' do - expect(described_class.new(user).execute).to contain_exactly(private_personal_snippet, internal_personal_snippet, public_personal_snippet) + expect(described_class.new(user).execute).to contain_exactly( + private_personal_snippet, internal_personal_snippet, public_personal_snippet, secret_personal_snippet + ) end end end @@ -220,6 +217,8 @@ let(:project) { create(:project) } let!(:snippet) { create(:project_snippet, :public, project: project) } + subject { described_class.new(user, project: project).execute } + before do project.add_maintainer(user) end @@ -232,17 +231,13 @@ it 'includes the result if the external service allows access' do external_service_allow_access(user, project) - results = described_class.new(user, project: project).execute - - expect(results).to contain_exactly(snippet) + expect(subject).to contain_exactly(snippet) end it 'does not include any results if the external service denies access' do external_service_deny_access(user, project) - results = described_class.new(user, project: project).execute - - expect(results).to be_empty + expect(subject).to be_empty end end end diff --git a/spec/helpers/snippets_helper_spec.rb b/spec/helpers/snippets_helper_spec.rb index 66c8d576a4c9c6..8527ffefc269bd 100644 --- a/spec/helpers/snippets_helper_spec.rb +++ b/spec/helpers/snippets_helper_spec.rb @@ -3,33 +3,126 @@ require 'spec_helper' describe SnippetsHelper do + include Gitlab::Routing include IconsHelper - describe '#embedded_snippet_raw_button' do + let_it_be(:public_personal_snippet) { create(:personal_snippet, :public) } + let_it_be(:secret_snippet) { create(:personal_snippet, :secret) } + let_it_be(:public_project_snippet) { create(:project_snippet, :public) } + + describe '.reliable_snippet_path' do + context 'personal snippets' do + context 'public' do + it 'gives a full path' do + expect(reliable_snippet_path(public_personal_snippet)).to eq("/snippets/#{public_personal_snippet.id}") + end + end + + context 'secret' do + it 'gives a full path, including secret word' do + expect(reliable_snippet_path(secret_snippet)).to match(%r{/snippets/#{secret_snippet.id}\?token=\w+}) + end + end + end + + context 'project snippets' do + it 'gives a full path' do + expect(reliable_snippet_path(public_project_snippet)).to eq("/#{public_project_snippet.project.full_path}/snippets/#{public_project_snippet.id}") + end + end + end + + describe '.reliable_snippet_url' do + context 'personal snippets' do + context 'public' do + it 'gives a full url' do + expect(reliable_snippet_url(public_personal_snippet)).to eq("http://test.host/snippets/#{public_personal_snippet.id}") + end + end + + context 'secret' do + it 'gives a full url, including secret word' do + expect(reliable_snippet_url(secret_snippet)).to match(%r{http://test.host/snippets/#{secret_snippet.id}\?token=\w+}) + end + end + end + + context 'project snippets' do + it 'gives a full url' do + expect(reliable_snippet_url(public_project_snippet)).to eq("http://test.host/#{public_project_snippet.project.full_path}/snippets/#{public_project_snippet.id}") + end + end + end + + describe '.shareable_snippets_link' do + context 'personal snippets' do + context 'public' do + it 'gives a full link' do + expect(reliable_snippet_url(public_personal_snippet)).to eq("http://test.host/snippets/#{public_personal_snippet.id}") + end + end + + context 'secret' do + it 'gives a full link, including secret word' do + expect(reliable_snippet_url(secret_snippet)).to match(%r{http://test.host/snippets/#{secret_snippet.id}\?token=\w+}) + end + end + end + + context 'project snippets' do + it 'gives a full link' do + expect(reliable_snippet_url(public_project_snippet)).to eq("http://test.host/#{public_project_snippet.project.full_path}/snippets/#{public_project_snippet.id}") + end + end + end + + describe '.embedded_snippet_raw_button' do it 'gives view raw button of embedded snippets for project snippets' do - @snippet = create(:project_snippet, :public) + @snippet = public_project_snippet expect(embedded_snippet_raw_button.to_s).to eq("#{external_snippet_icon('doc-code')}") end it 'gives view raw button of embedded snippets for personal snippets' do - @snippet = create(:personal_snippet, :public) + @snippet = public_personal_snippet expect(embedded_snippet_raw_button.to_s).to eq("#{external_snippet_icon('doc-code')}") end end - describe '#embedded_snippet_download_button' do + describe '.embedded_snippet_download_button' do it 'gives download button of embedded snippets for project snippets' do - @snippet = create(:project_snippet, :public) + @snippet = public_project_snippet expect(embedded_snippet_download_button.to_s).to eq("#{external_snippet_icon('download')}") end it 'gives download button of embedded snippets for personal snippets' do - @snippet = create(:personal_snippet, :public) + @snippet = public_personal_snippet expect(embedded_snippet_download_button.to_s).to eq("#{external_snippet_icon('download')}") end end + + describe '.snippet_embed_url' do + context 'personal snippets' do + context 'public' do + it 'gives a full link' do + expect(snippet_embed_url(public_personal_snippet)).to eq("") + end + end + + context 'secret' do + it 'gives a full link, including secret word' do + expect(snippet_embed_url(secret_snippet)).to eq("") + end + end + end + + context 'project snippets' do + it 'gives a full link' do + expect(snippet_embed_url(public_project_snippet)).to eq("") + end + end + end end diff --git a/spec/helpers/visibility_level_helper_spec.rb b/spec/helpers/visibility_level_helper_spec.rb index 1a176cfe965d27..b4ebbde442bc8e 100644 --- a/spec/helpers/visibility_level_helper_spec.rb +++ b/spec/helpers/visibility_level_helper_spec.rb @@ -79,6 +79,11 @@ .to eq "The snippet is visible only to project members." end + it 'describes visibility for secret snippets' do + expect(snippet_visibility_level_description(Gitlab::VisibilityLevel::SECRET, personal_snippet)) + .to eq "The snippet can be accessed without any authentication, but is not searchable." + end + it 'defaults to personal snippet' do expect(snippet_visibility_level_description(Gitlab::VisibilityLevel::PRIVATE)) .to eq "The snippet is visible only to me." @@ -229,4 +234,30 @@ it { is_expected.to eq(expected) } end end + + describe '.visibility_level_label' do + context 'PRIVATE' do + it 'returns Private' do + expect(visibility_level_label(Gitlab::VisibilityLevel::PRIVATE)).to eq('Private') + end + end + + context 'SECRET' do + it 'returns Secret' do + expect(visibility_level_label(Gitlab::VisibilityLevel::SECRET)).to eq('Secret') + end + end + + context 'INTERNAL' do + it 'returns Internal' do + expect(visibility_level_label(Gitlab::VisibilityLevel::INTERNAL)).to eq('Internal') + end + end + + context 'PUBLIC' do + it 'returns Public' do + expect(visibility_level_label(Gitlab::VisibilityLevel::PUBLIC)).to eq('Public') + end + end + end end diff --git a/spec/lib/gitlab/visibility_level_spec.rb b/spec/lib/gitlab/visibility_level_spec.rb index 75dc7d8e6d139f..4950636021d113 100644 --- a/spec/lib/gitlab/visibility_level_spec.rb +++ b/spec/lib/gitlab/visibility_level_spec.rb @@ -95,4 +95,99 @@ expect(described_class.valid_level?(described_class::PUBLIC)).to be_truthy end end + + describe '.values_for' do + context 'PersonalSnippet' do + it 'returns PRIVATE, SECERT, INTERNAL and PUBLIC' do + expect(described_class.values_for(PersonalSnippet.new)) + .to eq([ + Gitlab::VisibilityLevel::PRIVATE, + Gitlab::VisibilityLevel::SECRET, + Gitlab::VisibilityLevel::INTERNAL, + Gitlab::VisibilityLevel::PUBLIC + ]) + end + end + + context 'any other model' do + it 'returns PRIVATE, INTERNAL and PUBLIC' do + expect(described_class.values_for(Project.new)) + .to eq([ + Gitlab::VisibilityLevel::PRIVATE, + Gitlab::VisibilityLevel::INTERNAL, + Gitlab::VisibilityLevel::PUBLIC + ]) + end + end + end + + describe '.options' do + it 'returns a Hash of localized level name to const value mapping (excluding Secret)' do + expect(described_class.options) + .to eq( + 'VisibilityLevel|Private' => Gitlab::VisibilityLevel::PRIVATE, + 'VisibilityLevel|Internal' => Gitlab::VisibilityLevel::INTERNAL, + 'VisibilityLevel|Public' => Gitlab::VisibilityLevel::PUBLIC + ) + end + end + + describe '.values' do + it 'returns an Array of const values (excluding Secret)' do + expect(described_class.values) + .to eq([ + Gitlab::VisibilityLevel::PRIVATE, + Gitlab::VisibilityLevel::INTERNAL, + Gitlab::VisibilityLevel::PUBLIC + ]) + end + end + + describe '.all_options' do + it 'returns a Hash of localized level name to const value mapping (including Secret)' do + expect(described_class.all_options) + .to eq( + 'VisibilityLevel|Private' => Gitlab::VisibilityLevel::PRIVATE, + 'VisibilityLevel|Secret' => Gitlab::VisibilityLevel::SECRET, + 'VisibilityLevel|Internal' => Gitlab::VisibilityLevel::INTERNAL, + 'VisibilityLevel|Public' => Gitlab::VisibilityLevel::PUBLIC + ) + end + end + + describe '.all_values' do + it 'returns an Array of const values (including Secret)' do + expect(described_class.all_values) + .to eq([ + Gitlab::VisibilityLevel::PRIVATE, + Gitlab::VisibilityLevel::SECRET, + Gitlab::VisibilityLevel::INTERNAL, + Gitlab::VisibilityLevel::PUBLIC + ]) + end + end + + describe '.string_options' do + it 'returns a Hash of level name to const value mapping' do + expect(described_class.string_options) + .to eq( + 'private' => Gitlab::VisibilityLevel::PRIVATE, + 'secret' => Gitlab::VisibilityLevel::SECRET, + 'internal' => Gitlab::VisibilityLevel::INTERNAL, + 'public' => Gitlab::VisibilityLevel::PUBLIC + ) + end + end + + describe '.string_values' do + it 'returns an Array of const values (including Secret)' do + expect(described_class.string_values) + .to eq(%w{ + private + secret + internal + public + }) + end + end end diff --git a/spec/models/personal_snippet_spec.rb b/spec/models/personal_snippet_spec.rb new file mode 100644 index 00000000000000..a256a965a137aa --- /dev/null +++ b/spec/models/personal_snippet_spec.rb @@ -0,0 +1,111 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe PersonalSnippet do + describe '#update_secret_token' do + let(:snippet) { create(:personal_snippet, :public) } + + context 'visibility_level is NOT SECRET' do + it 'does not update the secret_token' do + expect(snippet.secret_token).to be_nil + end + end + + context 'visibility_level is SECRET' do + let(:snippet) { create(:personal_snippet, :secret) } + + it 'assigns a random hex value' do + expect(snippet.secret_token).not_to be_nil + end + + context 'when the visibility_level changes to any other level' do + it 'sets the secret_token to nil' do + snippet.visibility_level = Gitlab::VisibilityLevel::PUBLIC + snippet.save + + expect(snippet.secret_token).to be_nil + end + end + end + end + + describe '#visibility_secret?' do + let(:snippet) { create(:personal_snippet, :public) } + + context 'when snippet visibility is not Secret' do + it 'returns false' do + expect(snippet.visibility_secret?).to be_falsey + end + end + + context 'when snippet visibility is Secret' do + let(:snippet) { create(:personal_snippet, :secret) } + + it 'returns true' do + expect(snippet.visibility_secret?).to be_truthy + end + end + end + + describe '#secret?' do + let(:snippet) { create(:personal_snippet, :public) } + + context 'when snippet visibility is not Secret' do + it 'returns false' do + expect(snippet.secret?).to be_falsey + end + end + + context 'when snippet visibility is Secret' do + let(:snippet) { create(:personal_snippet, :secret) } + + it 'returns true' do + expect(snippet.secret?).to be_truthy + end + + context 'when secret_token is empty' do + let(:snippet) { create(:personal_snippet, :public) } + + it 'returns false' do + snippet.update_column(:visibility_level, Gitlab::VisibilityLevel::SECRET) + + expect(snippet.visibility_secret?).to be_truthy + expect(snippet.secret?).to be_falsey + end + end + end + end + + describe '#embeddable?' do + [ + { snippet: :public, embeddable: true, secret_token: nil }, + { snippet: :internal, embeddable: false, secret_token: nil }, + { snippet: :private, embeddable: false, secret_token: nil } + ].each do |combination| + it 'returns true when snippet is public' do + snippet = create(:personal_snippet, combination[:snippet], secret_token: combination[:secret_token]) + + expect(snippet.embeddable?).to eq(combination[:embeddable]) + end + end + + context 'when visibility_level is Secret' do + let(:snippet) { create(:personal_snippet, :secret) } + + it 'returns true' do + expect(snippet.embeddable?).to be_truthy + end + + context 'when secret_token is not present' do + let(:snippet) { create(:personal_snippet, :public) } + + it 'returns false' do + snippet.update_column(:visibility_level, Gitlab::VisibilityLevel::SECRET) + + expect(snippet.embeddable?).to be_falsey + end + end + end + end +end diff --git a/spec/models/project_snippet_spec.rb b/spec/models/project_snippet_spec.rb index e87b4f41f4da47..e7449ab6da1c41 100644 --- a/spec/models/project_snippet_spec.rb +++ b/spec/models/project_snippet_spec.rb @@ -9,5 +9,27 @@ describe "Validation" do it { is_expected.to validate_presence_of(:project) } + it { is_expected.to validate_exclusion_of(:visibility_level).in_array([Gitlab::VisibilityLevel::SECRET]) } + end + + describe '#embeddable?' do + [ + { project: :public, snippet: :public, embeddable: true }, + { project: :internal, snippet: :public, embeddable: false }, + { project: :private, snippet: :public, embeddable: false }, + { project: :public, snippet: :internal, embeddable: false }, + { project: :internal, snippet: :internal, embeddable: false }, + { project: :private, snippet: :internal, embeddable: false }, + { project: :public, snippet: :private, embeddable: false }, + { project: :internal, snippet: :private, embeddable: false }, + { project: :private, snippet: :private, embeddable: false } + ].each do |combination| + it 'only returns true when both project and snippet are public' do + project = create(:project, combination[:project]) + snippet = create(:project_snippet, combination[:snippet], project: project) + + expect(snippet.embeddable?).to eq(combination[:embeddable]) + end + end end end diff --git a/spec/models/snippet_spec.rb b/spec/models/snippet_spec.rb index f4dcbfbc190299..e4cc89318409f0 100644 --- a/spec/models/snippet_spec.rb +++ b/spec/models/snippet_spec.rb @@ -451,41 +451,4 @@ expect(blob.data).to eq(snippet.content) end end - - describe '#embeddable?' do - context 'project snippet' do - [ - { project: :public, snippet: :public, embeddable: true }, - { project: :internal, snippet: :public, embeddable: false }, - { project: :private, snippet: :public, embeddable: false }, - { project: :public, snippet: :internal, embeddable: false }, - { project: :internal, snippet: :internal, embeddable: false }, - { project: :private, snippet: :internal, embeddable: false }, - { project: :public, snippet: :private, embeddable: false }, - { project: :internal, snippet: :private, embeddable: false }, - { project: :private, snippet: :private, embeddable: false } - ].each do |combination| - it 'only returns true when both project and snippet are public' do - project = create(:project, combination[:project]) - snippet = create(:project_snippet, combination[:snippet], project: project) - - expect(snippet.embeddable?).to eq(combination[:embeddable]) - end - end - end - - context 'personal snippet' do - [ - { snippet: :public, embeddable: true }, - { snippet: :internal, embeddable: false }, - { snippet: :private, embeddable: false } - ].each do |combination| - it 'only returns true when snippet is public' do - snippet = create(:personal_snippet, combination[:snippet]) - - expect(snippet.embeddable?).to eq(combination[:embeddable]) - end - end - end - end end diff --git a/spec/requests/api/snippets_spec.rb b/spec/requests/api/snippets_spec.rb index 36d2a0d7ea7ca2..4ccce142ff8f9a 100644 --- a/spec/requests/api/snippets_spec.rb +++ b/spec/requests/api/snippets_spec.rb @@ -10,6 +10,7 @@ public_snippet = create(:personal_snippet, :public, author: user) private_snippet = create(:personal_snippet, :private, author: user) internal_snippet = create(:personal_snippet, :internal, author: user) + secret_snippet = create(:personal_snippet, :secret, author: user) get api("/snippets/", user) @@ -19,7 +20,8 @@ expect(json_response.map { |snippet| snippet['id']} ).to contain_exactly( public_snippet.id, internal_snippet.id, - private_snippet.id) + private_snippet.id, + secret_snippet.id) expect(json_response.last).to have_key('web_url') expect(json_response.last).to have_key('raw_url') expect(json_response.last).to have_key('visibility') @@ -27,6 +29,7 @@ it 'hides private snippets from regular user' do create(:personal_snippet, :private) + create(:personal_snippet, :secret) get api("/snippets/", user) @@ -38,6 +41,7 @@ it 'returns 404 for non-authenticated' do create(:personal_snippet, :internal) + create(:personal_snippet, :secret) get api("/snippets/") @@ -63,6 +67,7 @@ let!(:public_snippet) { create(:personal_snippet, :public, author: user) } let!(:private_snippet) { create(:personal_snippet, :private, author: user) } let!(:internal_snippet) { create(:personal_snippet, :internal, author: user) } + let!(:secret_snippet) { create(:personal_snippet, :secret, author: user) } let!(:public_snippet_other) { create(:personal_snippet, :public, author: other_user) } let!(:private_snippet_other) { create(:personal_snippet, :private, author: other_user) } let!(:internal_snippet_other) { create(:personal_snippet, :internal, author: other_user) } @@ -134,10 +139,63 @@ end describe 'GET /snippets/:id' do - set(:admin) { create(:user, :admin) } - set(:author) { create(:user) } - set(:private_snippet) { create(:personal_snippet, :private, author: author) } - set(:internal_snippet) { create(:personal_snippet, :internal, author: author) } + let_it_be(:admin) { create(:user, :admin) } + let_it_be(:author) { create(:user) } + let_it_be(:public_snippet) { create(:personal_snippet, :public, author: author) } + let_it_be(:private_snippet) { create(:personal_snippet, :private, author: author) } + let_it_be(:internal_snippet) { create(:personal_snippet, :internal, author: author) } + let_it_be(:secret_snippet) { create(:personal_snippet, :internal, author: author) } + + context 'attributes' do + subject { get api("/snippets/#{snippet.id}", author) } + + before do + subject + end + + shared_examples 'returns basic attributes' do + it do + expect(json_response['title']).to eq(snippet.title) + expect(json_response['description']).to eq(snippet.description) + expect(json_response['file_name']).to eq(snippet.file_name) + expect(json_response['visibility']).to eq(snippet.visibility) + expect(json_response['raw_url']).to eq(snippet.raw_url) + expect(json_response['web_url']).to eq(snippet.web_url) + end + end + + shared_examples 'does not include secret_token attribute' do + it do + expect(json_response).not_to include('secret_token') + end + end + + context 'public snippet' do + let(:snippet) { public_snippet } + + it_behaves_like 'does not include secret_token attribute' + end + + context 'private snippet' do + let(:snippet) { private_snippet } + + it_behaves_like 'does not include secret_token attribute' + end + + context 'internal snippet' do + let(:snippet) { internal_snippet } + + it_behaves_like 'does not include secret_token attribute' + end + + context 'secret snippet' do + let(:snippet) { secret_snippet } + + it 'includes secret_token attribute' do + expect(json_response['secret_token']).to eq(snippet.secret_token) + end + end + end it 'requires authentication' do get api("/snippets/#{private_snippet.id}", nil) @@ -149,11 +207,6 @@ get api("/snippets/#{private_snippet.id}", author) expect(response).to have_gitlab_http_status(200) - - expect(json_response['title']).to eq(private_snippet.title) - expect(json_response['description']).to eq(private_snippet.description) - expect(json_response['file_name']).to eq(private_snippet.file_name) - expect(json_response['visibility']).to eq(private_snippet.visibility) end it 'shows private snippets to an admin' do -- GitLab From 791903bdb2cc2b717810fb7b82cee368d822cf20 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Francisco=20Javier=20L=C3=B3pez?= Date: Mon, 4 Nov 2019 19:02:29 +0100 Subject: [PATCH 2/3] Simplifying MR --- app/controllers/concerns/snippets_url.rb | 58 ------------------------ app/controllers/snippets_controller.rb | 7 +-- 2 files changed, 1 insertion(+), 64 deletions(-) delete mode 100644 app/controllers/concerns/snippets_url.rb diff --git a/app/controllers/concerns/snippets_url.rb b/app/controllers/concerns/snippets_url.rb deleted file mode 100644 index 3178d9937f9f6a..00000000000000 --- a/app/controllers/concerns/snippets_url.rb +++ /dev/null @@ -1,58 +0,0 @@ -# frozen_string_literal: true - -module SnippetsUrl - extend ActiveSupport::Concern - - SNIPPETS_SECRET_KEYWORD = 'secret' - - private - - attr_reader :snippet - - def authorize_secret_snippet! - return unless snippet&.snippet? - - return render_404 unless snippet.valid_secret_token?(params[:token]) - - # current_user ? render_404 : authenticate_user! - end - - # def secrets_match?(secret) - # ActiveSupport::SecurityUtils.secure_compare(secret.to_s, snippet.secret_token) - # end - - # def ensure_complete_url - # redirect_to(complete_full_path.to_s) if redirect_to_complete_full_path? - # end - - # def redirect_to_complete_full_path? - # return unless snippet&.secret? - - # complete_full_path != current_full_path - # end - - # def complete_full_path - # @complete_full_path ||= begin - # path = current_full_path.clone - # secret_query = { SNIPPETS_SECRET_KEYWORD => snippet.secret_token } - # path.query = current_url_query_hash.merge(secret_query).to_query - # path - # end - # end - - # def current_full_path - # @current_full_path ||= begin - # path = URI.parse(current_url.path.chomp('/')) - # path.query = current_url_query_hash.to_query unless current_url_query_hash.empty? - # path - # end - # end - - # def current_url - # @current_url ||= URI.parse(request.original_url) - # end - - # def current_url_query_hash - # @current_url_query_hash ||= Rack::Utils.parse_nested_query(current_url.query) - # end -end diff --git a/app/controllers/snippets_controller.rb b/app/controllers/snippets_controller.rb index e2c67c2114fd93..55abcbd9197951 100644 --- a/app/controllers/snippets_controller.rb +++ b/app/controllers/snippets_controller.rb @@ -5,8 +5,6 @@ class SnippetsController < ApplicationController include ToggleAwardEmoji include SpammableActions include SnippetsActions - include SnippetsHelper - # include SnippetsUrl include RendersBlob include PreviewMarkdown include PaginatedCollection @@ -20,9 +18,6 @@ class SnippetsController < ApplicationController # Allow read snippet before_action :authorize_read_snippet!, only: [:show, :raw] - # Ensure we're displaying the correct url, specifically for secret snippets - # before_action :ensure_complete_url, only: [:show, :raw] - # Allow modify snippet before_action :authorize_update_snippet!, only: [:edit, :update] @@ -124,7 +119,7 @@ def snippet alias_method :spammable, :snippet def spammable_path - reliable_snippet_path(@snippet) + snippet_path(@snippet) end def authorize_read_snippet! -- GitLab From bcca1983f03983c55338131b2e7e57aaa72b7fb4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Francisco=20Javier=20L=C3=B3pez?= Date: Tue, 5 Nov 2019 12:11:40 +0100 Subject: [PATCH 3/3] Small changes --- app/helpers/snippets_helper.rb | 5 ----- app/helpers/visibility_level_helper.rb | 2 +- locale/gitlab.pot | 3 +++ spec/helpers/snippets_helper_spec.rb | 22 ---------------------- 4 files changed, 4 insertions(+), 28 deletions(-) diff --git a/app/helpers/snippets_helper.rb b/app/helpers/snippets_helper.rb index 19cec6ef4de887..ab2080386a1bdd 100644 --- a/app/helpers/snippets_helper.rb +++ b/app/helpers/snippets_helper.rb @@ -50,11 +50,6 @@ def download_raw_snippet_button(snippet) link_to(icon('download'), reliable_raw_snippet_path(snippet, inline: false), target: '_blank', rel: 'noopener noreferrer', class: "btn btn-sm has-tooltip", title: 'Download', data: { container: 'body' }) end - def shareable_snippets_link(snippet) - url = reliable_snippet_url(snippet) - link_to(url, url, id: 'shareable_link_url', title: 'Open') - end - # Return the path of a snippets index for a user or for a project # # @returns String, path to snippet index diff --git a/app/helpers/visibility_level_helper.rb b/app/helpers/visibility_level_helper.rb index c71e3fb86e19ea..f8615ae0aad2d7 100644 --- a/app/helpers/visibility_level_helper.rb +++ b/app/helpers/visibility_level_helper.rb @@ -59,7 +59,7 @@ def snippet_visibility_level_description(level, snippet = nil) _("The snippet is visible only to me.") end when Gitlab::VisibilityLevel::SECRET - "The snippet can be accessed without any authentication, but is not searchable." + _("The snippet can be accessed without any authentication, but is not searchable.") when Gitlab::VisibilityLevel::INTERNAL _("The snippet is visible to any logged in user.") when Gitlab::VisibilityLevel::PUBLIC diff --git a/locale/gitlab.pot b/locale/gitlab.pot index cd9e0971653efc..f6a7f5b1779488 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -16760,6 +16760,9 @@ msgstr "" msgid "The schedule time must be in the future!" msgstr "" +msgid "The snippet can be accessed without any authentication, but is not searchable." +msgstr "" + msgid "The snippet can be accessed without any authentication." msgstr "" diff --git a/spec/helpers/snippets_helper_spec.rb b/spec/helpers/snippets_helper_spec.rb index 8527ffefc269bd..0a4720c96f2468 100644 --- a/spec/helpers/snippets_helper_spec.rb +++ b/spec/helpers/snippets_helper_spec.rb @@ -54,28 +54,6 @@ end end - describe '.shareable_snippets_link' do - context 'personal snippets' do - context 'public' do - it 'gives a full link' do - expect(reliable_snippet_url(public_personal_snippet)).to eq("http://test.host/snippets/#{public_personal_snippet.id}") - end - end - - context 'secret' do - it 'gives a full link, including secret word' do - expect(reliable_snippet_url(secret_snippet)).to match(%r{http://test.host/snippets/#{secret_snippet.id}\?token=\w+}) - end - end - end - - context 'project snippets' do - it 'gives a full link' do - expect(reliable_snippet_url(public_project_snippet)).to eq("http://test.host/#{public_project_snippet.project.full_path}/snippets/#{public_project_snippet.id}") - end - end - end - describe '.embedded_snippet_raw_button' do it 'gives view raw button of embedded snippets for project snippets' do @snippet = public_project_snippet -- GitLab