diff options
41 files changed, 733 insertions, 52 deletions
diff --git a/app/assets/javascripts/dispatcher.js b/app/assets/javascripts/dispatcher.js index ffd8d494a40..81267c68cfc 100644 --- a/app/assets/javascripts/dispatcher.js +++ b/app/assets/javascripts/dispatcher.js @@ -396,6 +396,7 @@ import PerformanceBar from './performance_bar'; initSettingsPanels(); break; case 'projects:settings:ci_cd:show': + case 'groups:settings:ci_cd:show': new gl.ProjectVariables(); break; case 'ci:lints:create': diff --git a/app/controllers/groups/settings/ci_cd_controller.rb b/app/controllers/groups/settings/ci_cd_controller.rb new file mode 100644 index 00000000000..0142ad8278c --- /dev/null +++ b/app/controllers/groups/settings/ci_cd_controller.rb @@ -0,0 +1,24 @@ +module Groups + module Settings + class CiCdController < Groups::ApplicationController + before_action :authorize_admin_pipeline! + + def show + define_secret_variables + end + + private + + def define_secret_variables + @variable = Ci::GroupVariable.new(group: group) + .present(current_user: current_user) + @variables = group.variables.order_key_asc + .map { |variable| variable.present(current_user: current_user) } + end + + def authorize_admin_pipeline! + return render_404 unless can?(current_user, :admin_pipeline, group) + end + end + end +end diff --git a/app/controllers/groups/variables_controller.rb b/app/controllers/groups/variables_controller.rb new file mode 100644 index 00000000000..6fba9dab2a7 --- /dev/null +++ b/app/controllers/groups/variables_controller.rb @@ -0,0 +1,57 @@ +module Groups + class VariablesController < Groups::ApplicationController + before_action :variable, only: [:show, :update, :destroy] + before_action :authorize_admin_build! + + def index + redirect_to group_settings_ci_cd_path(group) + end + + def show + end + + def update + if variable.update(group_params) + redirect_to group_variables_path(group), + notice: 'Variable was successfully updated.' + else + render "show" + end + end + + def create + new_variable = Ci::GroupVariable.new(group_params) + + if new_variable.valid? && group.variables << new_variable + redirect_to group_settings_ci_cd_path(group), + notice: 'Variables were successfully updated.' + else + @variable = new_variable.present(current_user: current_user) + render "show" + end + end + + def destroy + variable.destroy + + redirect_to group_settings_ci_cd_path(group), + status: 302, + notice: 'Variable was successfully removed.' + end + + private + + def authorize_admin_build! + return render_404 unless can?(current_user, :admin_build, group) + end + + def group_params + params.require(:variable) + .permit([:key, :value, :protected]) + end + + def variable + @variable ||= group.variables.find(params[:id]).present(current_user: current_user) + end + end +end diff --git a/app/controllers/projects/settings/ci_cd_controller.rb b/app/controllers/projects/settings/ci_cd_controller.rb index 24fe78bc1bd..ea7ceb3eaa5 100644 --- a/app/controllers/projects/settings/ci_cd_controller.rb +++ b/app/controllers/projects/settings/ci_cd_controller.rb @@ -21,7 +21,10 @@ module Projects end def define_secret_variables - @variable = Ci::Variable.new + @variable = Ci::Variable.new(project: project) + .present(current_user: current_user) + @variables = project.variables.order_key_asc + .map { |variable| variable.present(current_user: current_user) } end def define_triggers_variables diff --git a/app/controllers/projects/variables_controller.rb b/app/controllers/projects/variables_controller.rb index 326d31ecec2..716e1347604 100644 --- a/app/controllers/projects/variables_controller.rb +++ b/app/controllers/projects/variables_controller.rb @@ -1,53 +1,50 @@ class Projects::VariablesController < Projects::ApplicationController + before_action :variable, only: [:show, :update, :destroy] before_action :authorize_admin_build! layout 'project_settings' def index - redirect_to project_settings_ci_cd_path(@project) + redirect_to namespace_project_settings_ci_cd_path(@project.namespace, @project) end def show - @variable = @project.variables.find(params[:id]) end def update - @variable = @project.variables.find(params[:id]) - - if @variable.update_attributes(variable_params) - redirect_to project_variables_path(project), notice: 'Variable was successfully updated.' + if @variable.update(project_params) + redirect_to namespace_project_variables_path(project.namespace, project), notice: 'Variable was successfully updated.' else - render action: "show" + render "show" end end def create - @variable = @project.variables.new(variable_params) + @variable = Ci::Variable.new(project_params) - if @variable.save - flash[:notice] = 'Variables were successfully updated.' - redirect_to project_settings_ci_cd_path(project) + if @variable.valid? && @project.variables << @variable + redirect_to namespace_project_settings_ci_cd_path(project.namespace, project), notice: 'Variables were successfully updated.' else render "show" end end def destroy - @key = @project.variables.find(params[:id]) - @key.destroy + variable.destroy - redirect_to project_settings_ci_cd_path(project), + redirect_to namespace_project_settings_ci_cd_path(project.namespace, project), status: 302, notice: 'Variable was successfully removed.' end private - def variable_params - params.require(:variable).permit(*variable_params_attributes) + def project_params + params.require(:variable) + .permit([:id, :key, :value, :protected, :_destroy]) end - def variable_params_attributes - %i[id key value protected _destroy] + def variable + @variable ||= project.variables.find(params[:id]).present(current_user: current_user) end end diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index ba2ecbf82a2..25e75a19f37 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -200,6 +200,7 @@ module Ci variables += project.deployment_variables if has_environment? variables += yaml_variables variables += user_variables + variables += project.group.secret_variables_for(ref, project).map(&:to_runner_variable) if project.group variables += secret_variables(environment: environment) variables += trigger_request.user_variables if trigger_request variables += persisted_environment_variables if environment diff --git a/app/models/ci/group_variable.rb b/app/models/ci/group_variable.rb new file mode 100644 index 00000000000..f64bc245a67 --- /dev/null +++ b/app/models/ci/group_variable.rb @@ -0,0 +1,13 @@ +module Ci + class GroupVariable < ActiveRecord::Base + extend Ci::Model + include HasVariable + include Presentable + + belongs_to :group + + validates :key, uniqueness: { scope: :group_id } + + scope :unprotected, -> { where(protected: false) } + end +end diff --git a/app/models/ci/variable.rb b/app/models/ci/variable.rb index 0b8d0ff881a..cf0fe04ddaf 100644 --- a/app/models/ci/variable.rb +++ b/app/models/ci/variable.rb @@ -2,6 +2,7 @@ module Ci class Variable < ActiveRecord::Base extend Ci::Model include HasVariable + include Presentable belongs_to :project diff --git a/app/models/group.rb b/app/models/group.rb index b93fce6100d..f625b8c250c 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -22,6 +22,7 @@ class Group < Namespace has_many :shared_projects, through: :project_group_links, source: :project has_many :notification_settings, dependent: :destroy, as: :source # rubocop:disable Cop/ActiveRecordDependent has_many :labels, class_name: 'GroupLabel' + has_many :variables, class_name: 'Ci::GroupVariable' validate :avatar_type, if: ->(user) { user.avatar.present? && user.avatar_changed? } validate :visibility_level_allowed_by_projects @@ -248,6 +249,13 @@ class Group < Namespace } end + def secret_variables_for(ref, project) + variables = [] + variables += parent.secret_variables_for(ref, project) if has_parent? + variables += project.protected_for?(ref) ? self.variables : self.variables.unprotected + variables + end + protected def update_two_factor_requirement diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb index dcb37416ca3..6defab75fce 100644 --- a/app/policies/group_policy.rb +++ b/app/policies/group_policy.rb @@ -31,6 +31,8 @@ class GroupPolicy < BasePolicy rule { master }.policy do enable :create_projects enable :admin_milestones + enable :admin_pipeline + enable :admin_build end rule { owner }.policy do diff --git a/app/presenters/ci/group_variable_presenter.rb b/app/presenters/ci/group_variable_presenter.rb new file mode 100644 index 00000000000..81fea106a5c --- /dev/null +++ b/app/presenters/ci/group_variable_presenter.rb @@ -0,0 +1,25 @@ +module Ci + class GroupVariablePresenter < Gitlab::View::Presenter::Delegated + presents :variable + + def placeholder + 'GROUP_VARIABLE' + end + + def form_path + if variable.persisted? + group_variable_path(group, variable) + else + group_variables_path(group) + end + end + + def edit_path + group_variable_path(group, variable) + end + + def delete_path + group_variable_path(group, variable) + end + end +end diff --git a/app/presenters/ci/variable_presenter.rb b/app/presenters/ci/variable_presenter.rb new file mode 100644 index 00000000000..3f14d972c50 --- /dev/null +++ b/app/presenters/ci/variable_presenter.rb @@ -0,0 +1,25 @@ +module Ci + class VariablePresenter < Gitlab::View::Presenter::Delegated + presents :variable + + def placeholder + 'PROJECT_VARIABLE' + end + + def form_path + if variable.persisted? + namespace_project_variable_path(project.namespace, project, variable) + else + namespace_project_variables_path(project.namespace, project) + end + end + + def edit_path + namespace_project_variable_path(project.namespace, project, variable) + end + + def delete_path + namespace_project_variable_path(project.namespace, project, variable) + end + end +end diff --git a/app/views/projects/variables/_content.html.haml b/app/views/ci/variables/_content.html.haml index 98f618ca3b8..98f618ca3b8 100644 --- a/app/views/projects/variables/_content.html.haml +++ b/app/views/ci/variables/_content.html.haml diff --git a/app/views/projects/variables/_form.html.haml b/app/views/ci/variables/_form.html.haml index 0a70a301cb4..eebd0955c80 100644 --- a/app/views/projects/variables/_form.html.haml +++ b/app/views/ci/variables/_form.html.haml @@ -1,12 +1,12 @@ -= form_for [@project.namespace.becomes(Namespace), @project, @variable] do |f| += form_for @variable, as: :variable, url: @variable.form_path do |f| = form_errors(@variable) .form-group = f.label :key, "Key", class: "label-light" - = f.text_field :key, class: "form-control", placeholder: "PROJECT_VARIABLE", required: true + = f.text_field :key, class: "form-control", placeholder: @variable.placeholder, required: true .form-group = f.label :value, "Value", class: "label-light" - = f.text_area :value, class: "form-control", placeholder: "PROJECT_VARIABLE" + = f.text_area :value, class: "form-control", placeholder: @variable.placeholder .form-group .checkbox = f.label :protected do diff --git a/app/views/projects/variables/_index.html.haml b/app/views/ci/variables/_index.html.haml index 5e6786f6698..007c2344b5a 100644 --- a/app/views/projects/variables/_index.html.haml +++ b/app/views/ci/variables/_index.html.haml @@ -1,16 +1,16 @@ .row.prepend-top-default.append-bottom-default .col-lg-4 - = render "projects/variables/content" + = render "ci/variables/content" .col-lg-8 %h5.prepend-top-0 Add a variable - = render "projects/variables/form", btn_text: "Add new variable" + = render "ci/variables/form", btn_text: "Add new variable" %hr %h5.prepend-top-0 - Your variables (#{@project.variables.size}) - - if @project.variables.empty? + Your variables (#{@variables.size}) + - if @variables.empty? %p.settings-message.text-center.append-bottom-0 No variables found, add one with the form above. - else - = render "projects/variables/table" + = render "ci/variables/table" %button.btn.btn-info.js-btn-toggle-reveal-values{ "data-status" => 'hidden' } Reveal Values diff --git a/app/views/ci/variables/_show.html.haml b/app/views/ci/variables/_show.html.haml new file mode 100644 index 00000000000..2bfb290629d --- /dev/null +++ b/app/views/ci/variables/_show.html.haml @@ -0,0 +1,9 @@ +- page_title "Variables" + +.row.prepend-top-default.append-bottom-default + .col-lg-3 + = render "ci/variables/content" + .col-lg-9 + %h5.prepend-top-0 + Update variable + = render "ci/variables/form", btn_text: "Save variable" diff --git a/app/views/projects/variables/_table.html.haml b/app/views/ci/variables/_table.html.haml index 4ce6a828812..71a0b56c4f4 100644 --- a/app/views/projects/variables/_table.html.haml +++ b/app/views/ci/variables/_table.html.haml @@ -11,18 +11,18 @@ %th Protected %th %tbody - - @project.variables.order_key_asc.each do |variable| + - @variables.each do |variable| - if variable.id? %tr %td.variable-key= variable.key %td.variable-value{ "data-value" => variable.value }****** %td.variable-protected= Gitlab::Utils.boolean_to_yes_no(variable.protected) %td.variable-menu - = link_to project_variable_path(@project, variable), class: "btn btn-transparent btn-variable-edit" do + = link_to variable.edit_path, class: "btn btn-transparent btn-variable-edit" do %span.sr-only Update = icon("pencil") - = link_to project_variable_path(@project, variable), class: "btn btn-transparent btn-variable-delete", method: :delete, data: { confirm: "Are you sure?" } do + = link_to variable.delete_path, class: "btn btn-transparent btn-variable-delete", method: :delete, data: { confirm: "Are you sure?" } do %span.sr-only Remove = icon("trash") diff --git a/app/views/groups/_settings_head.html.haml b/app/views/groups/_settings_head.html.haml index 2454e7355a7..623d233a46a 100644 --- a/app/views/groups/_settings_head.html.haml +++ b/app/views/groups/_settings_head.html.haml @@ -12,3 +12,8 @@ = link_to projects_group_path(@group), title: 'Projects' do %span Projects + + = nav_link(controller: :ci_cd) do + = link_to group_settings_ci_cd_path(@group), title: 'Pipelines' do + %span + Pipelines diff --git a/app/views/groups/settings/ci_cd/show.html.haml b/app/views/groups/settings/ci_cd/show.html.haml new file mode 100644 index 00000000000..bf36baf48ab --- /dev/null +++ b/app/views/groups/settings/ci_cd/show.html.haml @@ -0,0 +1,4 @@ +- page_title "Pipelines" += render "groups/settings_head" + += render 'ci/variables/index' diff --git a/app/views/groups/variables/show.html.haml b/app/views/groups/variables/show.html.haml new file mode 100644 index 00000000000..df533952b76 --- /dev/null +++ b/app/views/groups/variables/show.html.haml @@ -0,0 +1 @@ += render 'ci/variables/show' diff --git a/app/views/projects/settings/ci_cd/show.html.haml b/app/views/projects/settings/ci_cd/show.html.haml index 00ccc3ec41e..6afb38c5709 100644 --- a/app/views/projects/settings/ci_cd/show.html.haml +++ b/app/views/projects/settings/ci_cd/show.html.haml @@ -3,6 +3,6 @@ = render "projects/settings/head" = render 'projects/runners/index' -= render 'projects/variables/index' += render 'ci/variables/index' = render 'projects/triggers/index' = render 'projects/pipelines_settings/show' diff --git a/app/views/projects/variables/show.html.haml b/app/views/projects/variables/show.html.haml index 297a53ca98c..df533952b76 100644 --- a/app/views/projects/variables/show.html.haml +++ b/app/views/projects/variables/show.html.haml @@ -1,9 +1 @@ -- page_title "Variables" - -.row.prepend-top-default.append-bottom-default - .col-lg-3 - = render "content" - .col-lg-9 - %h5.prepend-top-0 - Update variable - = render "form", btn_text: "Save variable" += render 'ci/variables/show' diff --git a/changelogs/unreleased/feature-intermediate-12729-group-secret-variables.yml b/changelogs/unreleased/feature-intermediate-12729-group-secret-variables.yml new file mode 100644 index 00000000000..333895ffba9 --- /dev/null +++ b/changelogs/unreleased/feature-intermediate-12729-group-secret-variables.yml @@ -0,0 +1,4 @@ +--- +title: Add Group secret variables +merge_request: 12582 +author: diff --git a/config/routes/group.rb b/config/routes/group.rb index 11cdff55ed8..bd04ad51076 100644 --- a/config/routes/group.rb +++ b/config/routes/group.rb @@ -23,6 +23,12 @@ scope(path: 'groups/*group_id', resources :labels, except: [:show] do post :toggle_subscription, on: :member end + + namespace :settings do + resource :ci_cd, only: [:show], controller: 'ci_cd' + end + + resources :variables, only: [:index, :show, :update, :create, :destroy] end scope(path: 'groups/*id', diff --git a/db/migrate/20170525130346_create_group_variables_table.rb b/db/migrate/20170525130346_create_group_variables_table.rb new file mode 100644 index 00000000000..eaa38dbc40d --- /dev/null +++ b/db/migrate/20170525130346_create_group_variables_table.rb @@ -0,0 +1,23 @@ +class CreateGroupVariablesTable < ActiveRecord::Migration + DOWNTIME = false + + def up + create_table :ci_group_variables do |t| + t.string :key, null: false + t.text :value + t.text :encrypted_value + t.string :encrypted_value_salt + t.string :encrypted_value_iv + t.integer :group_id, null: false + t.boolean :protected, default: false, null: false + + t.timestamps_with_timezone null: false + end + + add_index :ci_group_variables, [:group_id, :key], unique: true + end + + def down + drop_table :ci_group_variables + end +end diff --git a/db/migrate/20170525130758_add_foreign_key_to_group_variables.rb b/db/migrate/20170525130758_add_foreign_key_to_group_variables.rb new file mode 100644 index 00000000000..0146235c5ba --- /dev/null +++ b/db/migrate/20170525130758_add_foreign_key_to_group_variables.rb @@ -0,0 +1,15 @@ +class AddForeignKeyToGroupVariables < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_concurrent_foreign_key :ci_group_variables, :namespaces, column: :group_id + end + + def down + remove_foreign_key :ci_group_variables, column: :group_id + end +end diff --git a/db/schema.rb b/db/schema.rb index a47a6ae9a98..f4d83a4dd9c 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -253,6 +253,20 @@ ActiveRecord::Schema.define(version: 20170703102400) do add_index "ci_builds", ["updated_at"], name: "index_ci_builds_on_updated_at", using: :btree add_index "ci_builds", ["user_id"], name: "index_ci_builds_on_user_id", using: :btree + create_table "ci_group_variables", force: :cascade do |t| + t.string "key", null: false + t.text "value" + t.text "encrypted_value" + t.string "encrypted_value_salt" + t.string "encrypted_value_iv" + t.integer "group_id", null: false + t.boolean "protected", default: false, null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + end + + add_index "ci_group_variables", ["group_id", "key"], name: "index_ci_group_variables_on_group_id_and_key", unique: true, using: :btree + create_table "ci_pipeline_schedules", force: :cascade do |t| t.string "description" t.string "ref" @@ -1550,6 +1564,7 @@ ActiveRecord::Schema.define(version: 20170703102400) do add_foreign_key "ci_builds", "ci_pipelines", column: "auto_canceled_by_id", name: "fk_a2141b1522", on_delete: :nullify add_foreign_key "ci_builds", "ci_stages", column: "stage_id", name: "fk_3a9eaa254d", on_delete: :cascade add_foreign_key "ci_builds", "projects", name: "fk_befce0568a", on_delete: :cascade + add_foreign_key "ci_group_variables", "namespaces", column: "group_id", name: "fk_33ae4d58d8", on_delete: :cascade add_foreign_key "ci_pipeline_schedules", "projects", name: "fk_8ead60fcc4", on_delete: :cascade add_foreign_key "ci_pipeline_schedules", "users", column: "owner_id", name: "fk_9ea99f58d2", on_delete: :nullify add_foreign_key "ci_pipelines", "ci_pipeline_schedules", column: "pipeline_schedule_id", name: "fk_3d34ab2e06", on_delete: :nullify diff --git a/doc/ci/variables/README.md b/doc/ci/variables/README.md index 3501aae75ec..ee3349d9526 100644 --- a/doc/ci/variables/README.md +++ b/doc/ci/variables/README.md @@ -10,7 +10,8 @@ The variables can be overwritten and they take precedence over each other in this order: 1. [Trigger variables][triggers] (take precedence over all) -1. [Secret variables](#secret-variables) or [protected secret variables](#protected-secret-variables) +1. [Project-level secret variables](#project-level-secret-variables) +1. [Group-level secret variables](#group-level-secret-variables) 1. YAML-defined [job-level variables](../yaml/README.md#job-variables) 1. YAML-defined [global variables](../yaml/README.md#variables) 1. [Deployment variables](#deployment-variables) @@ -138,7 +139,7 @@ script: - 'eval $LS_CMD' # will execute 'ls -al $TMP_DIR' ``` -## Secret variables +## Project-level secret variables >**Notes:** - This feature requires GitLab Runner 0.4.0 or higher. @@ -158,7 +159,8 @@ Secret variables can be added by going to your project's **Settings ➔ Pipelines**, then finding the section called **Secret variables**. -Once you set them, they will be available for all subsequent pipelines. +Once you set them, they will be available for all subsequent pipelines. You can also +[protect your variables](#protected-secret-variables). ### Protected secret variables @@ -176,6 +178,19 @@ Protected variables can be added by going to your project's Once you set them, they will be available for all subsequent pipelines. +## Group-level secret variables + +>**Notes:** +This feature requires GitLab 9.4 or higher. + +You can also define variables per Group. The essential functionality is exactly the +same with [project-level secret variables](#project-level-secret-variables). You +can also [protect your variables](#protected-secret-variables). + +Secret variables can be added by going to your group's +**Settings ➔ Pipelines**, then finding the section called +**Secret variables**. + ## Deployment variables >**Note:** diff --git a/lib/gitlab/path_regex.rb b/lib/gitlab/path_regex.rb index 10eb99fb461..f0d5e3f7bc0 100644 --- a/lib/gitlab/path_regex.rb +++ b/lib/gitlab/path_regex.rb @@ -129,6 +129,8 @@ module Gitlab pipeline_quota projects subgroups + settings + variables ].freeze ILLEGAL_PROJECT_PATH_WORDS = PROJECT_WILDCARD_ROUTES diff --git a/spec/controllers/groups/settings/ci_cd_controller_spec.rb b/spec/controllers/groups/settings/ci_cd_controller_spec.rb new file mode 100644 index 00000000000..2e0efb57c74 --- /dev/null +++ b/spec/controllers/groups/settings/ci_cd_controller_spec.rb @@ -0,0 +1,20 @@ +require 'spec_helper' + +describe Groups::Settings::CiCdController do + let(:group) { create(:group) } + let(:user) { create(:user) } + + before do + group.add_master(user) + sign_in(user) + end + + describe 'GET #show' do + it 'renders show with 200 status code' do + get :show, group_id: group + + expect(response).to have_http_status(200) + expect(response).to render_template(:show) + end + end +end diff --git a/spec/controllers/groups/variables_controller_spec.rb b/spec/controllers/groups/variables_controller_spec.rb new file mode 100644 index 00000000000..c11fe93ffca --- /dev/null +++ b/spec/controllers/groups/variables_controller_spec.rb @@ -0,0 +1,56 @@ +require 'spec_helper' + +describe Groups::VariablesController do + let(:group) { create(:group) } + let(:user) { create(:user) } + + before do + sign_in(user) + group.add_master(user) + end + + describe 'POST #create' do + context 'variable is valid' do + it 'shows a success flash message' do + post :create, group_id: group, variable: { key: "one", value: "two" } + + expect(flash[:notice]).to include 'Variables were successfully updated.' + expect(response).to redirect_to(group_settings_ci_cd_path(group)) + end + end + + context 'variable is invalid' do + it 'renders show' do + post :create, group_id: group, variable: { key: "..one", value: "two" } + + expect(response).to render_template("groups/variables/show") + end + end + end + + describe 'POST #update' do + let(:variable) { create(:ci_group_variable) } + + context 'updating a variable with valid characters' do + before do + group.variables << variable + end + + it 'shows a success flash message' do + post :update, group_id: group, + id: variable.id, variable: { key: variable.key, value: 'two' } + + expect(flash[:notice]).to include 'Variable was successfully updated.' + expect(response).to redirect_to(group_variables_path(group)) + end + + it 'renders the action #show if the variable key is invalid' do + post :update, group_id: group, + id: variable.id, variable: { key: '?', value: variable.value } + + expect(response).to have_http_status(200) + expect(response).to render_template :show + end + end + end +end diff --git a/spec/controllers/projects/variables_controller_spec.rb b/spec/controllers/projects/variables_controller_spec.rb index a0ecc756653..0304ea6f93c 100644 --- a/spec/controllers/projects/variables_controller_spec.rb +++ b/spec/controllers/projects/variables_controller_spec.rb @@ -21,7 +21,7 @@ describe Projects::VariablesController do end context 'variable is invalid' do - it 'shows an alert flash message' do + it 'renders show' do post :create, namespace_id: project.namespace.to_param, project_id: project, variable: { key: "..one", value: "two" } @@ -35,7 +35,6 @@ describe Projects::VariablesController do context 'updating a variable with valid characters' do before do - variable.project_id = project.id project.variables << variable end diff --git a/spec/factories/ci/group_variables.rb b/spec/factories/ci/group_variables.rb new file mode 100644 index 00000000000..565ced9eb1a --- /dev/null +++ b/spec/factories/ci/group_variables.rb @@ -0,0 +1,12 @@ +FactoryGirl.define do + factory :ci_group_variable, class: Ci::GroupVariable do + sequence(:key) { |n| "VARIABLE_#{n}" } + value 'VARIABLE_VALUE' + + trait(:protected) do + protected true + end + + group factory: :group + end +end diff --git a/spec/features/group_variables_spec.rb b/spec/features/group_variables_spec.rb new file mode 100644 index 00000000000..37814ba6238 --- /dev/null +++ b/spec/features/group_variables_spec.rb @@ -0,0 +1,78 @@ +require 'spec_helper' + +feature 'Group variables', js: true do + let(:user) { create(:user) } + let(:group) { create(:group) } + + background do + group.add_master(user) + gitlab_sign_in(user) + end + + context 'when user creates a new variable' do + background do + visit group_settings_ci_cd_path(group) + fill_in 'variable_key', with: 'AAA' + fill_in 'variable_value', with: 'AAA123' + find(:css, "#variable_protected").set(true) + click_on 'Add new variable' + end + + scenario 'user sees the created variable' do + page.within('.variables-table') do + expect(find(".variable-key")).to have_content('AAA') + expect(find(".variable-value")).to have_content('******') + expect(find(".variable-protected")).to have_content('Yes') + end + click_on 'Reveal Values' + page.within('.variables-table') do + expect(find(".variable-value")).to have_content('AAA123') + end + end + end + + context 'when user edits a variable' do + background do + create(:ci_group_variable, key: 'AAA', value: 'AAA123', protected: true, + group: group) + + visit group_settings_ci_cd_path(group) + + page.within('.variable-menu') do + click_on 'Update' + end + + fill_in 'variable_key', with: 'BBB' + fill_in 'variable_value', with: 'BBB123' + find(:css, "#variable_protected").set(false) + click_on 'Save variable' + end + + scenario 'user sees the updated variable' do + page.within('.variables-table') do + expect(find(".variable-key")).to have_content('BBB') + expect(find(".variable-value")).to have_content('******') + expect(find(".variable-protected")).to have_content('No') + end + end + end + + context 'when user deletes a variable' do + background do + create(:ci_group_variable, key: 'BBB', value: 'BBB123', protected: false, + group: group) + + visit group_settings_ci_cd_path(group) + + page.within('.variable-menu') do + page.accept_alert 'Are you sure?' do + click_on 'Remove' + end + end + end + + scenario 'user does not see the deleted variable' do + expect(page).to have_no_css('.variables-table') + end + end +end diff --git a/spec/features/variables_spec.rb b/spec/features/variables_spec.rb index 1a2dedf27eb..a7c24c7d718 100644 --- a/spec/features/variables_spec.rb +++ b/spec/features/variables_spec.rb @@ -82,7 +82,7 @@ describe 'Project variables', js: true do it 'deletes variable' do page.within('.variables-table') do - find('.btn-variable-delete').click + click_on 'Remove' end expect(page).not_to have_selector('variables-table') @@ -90,7 +90,7 @@ describe 'Project variables', js: true do it 'edits variable' do page.within('.variables-table') do - find('.btn-variable-edit').click + click_on 'Update' end expect(page).to have_content('Update variable') @@ -104,7 +104,7 @@ describe 'Project variables', js: true do it 'edits variable with empty value' do page.within('.variables-table') do - find('.btn-variable-edit').click + click_on 'Update' end expect(page).to have_content('Update variable') @@ -117,7 +117,7 @@ describe 'Project variables', js: true do it 'edits variable to be protected' do page.within('.variables-table') do - find('.btn-variable-edit').click + click_on 'Update' end expect(page).to have_content('Update variable') @@ -132,7 +132,7 @@ describe 'Project variables', js: true do project.variables.first.update(protected: true) page.within('.variables-table') do - find('.btn-variable-edit').click + click_on 'Update' end expect(page).to have_content('Update variable') diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 431fcda165d..cf6d356c524 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -1356,6 +1356,59 @@ describe Ci::Build, :models do end end + context 'when group secret variable is defined' do + let(:secret_variable) do + { key: 'SECRET_KEY', value: 'secret_value', public: false } + end + + let(:group) { create(:group, :access_requestable) } + + before do + build.project.update(group: group) + + create(:ci_group_variable, + secret_variable.slice(:key, :value).merge(group: group)) + end + + it { is_expected.to include(secret_variable) } + end + + context 'when group protected variable is defined' do + let(:protected_variable) do + { key: 'PROTECTED_KEY', value: 'protected_value', public: false } + end + + let(:group) { create(:group, :access_requestable) } + + before do + build.project.update(group: group) + + create(:ci_group_variable, + :protected, + protected_variable.slice(:key, :value).merge(group: group)) + end + + context 'when the branch is protected' do + before do + create(:protected_branch, project: build.project, name: build.ref) + end + + it { is_expected.to include(protected_variable) } + end + + context 'when the tag is protected' do + before do + create(:protected_tag, project: build.project, name: build.ref) + end + + it { is_expected.to include(protected_variable) } + end + + context 'when the ref is not protected' do + it { is_expected.not_to include(protected_variable) } + end + end + context 'when build is for triggers' do let(:trigger) { create(:ci_trigger, project: project) } let(:trigger_request) { create(:ci_trigger_request_with_variables, pipeline: pipeline, trigger: trigger) } diff --git a/spec/models/ci/group_variable_spec.rb b/spec/models/ci/group_variable_spec.rb new file mode 100644 index 00000000000..24b914face9 --- /dev/null +++ b/spec/models/ci/group_variable_spec.rb @@ -0,0 +1,31 @@ +require 'spec_helper' + +describe Ci::GroupVariable, models: true do + subject { build(:ci_group_variable) } + + it { is_expected.to include_module(HasVariable) } + it { is_expected.to include_module(Presentable) } + it { is_expected.to validate_uniqueness_of(:key).scoped_to(:group_id) } + + describe '.unprotected' do + subject { described_class.unprotected } + + context 'when variable is protected' do + before do + create(:ci_group_variable, :protected) + end + + it 'returns nothing' do + is_expected.to be_empty + end + end + + context 'when variable is not protected' do + let(:variable) { create(:ci_group_variable, protected: false) } + + it 'returns the variable' do + is_expected.to contain_exactly(variable) + end + end + end +end diff --git a/spec/models/ci/variable_spec.rb b/spec/models/ci/variable_spec.rb index 4ffbfa6c130..890ffaae494 100644 --- a/spec/models/ci/variable_spec.rb +++ b/spec/models/ci/variable_spec.rb @@ -3,10 +3,9 @@ require 'spec_helper' describe Ci::Variable, models: true do subject { build(:ci_variable) } - let(:secret_value) { 'secret' } - describe 'validations' do it { is_expected.to include_module(HasVariable) } + it { is_expected.to include_module(Presentable) } it { is_expected.to validate_uniqueness_of(:key).scoped_to(:project_id, :environment_scope) } end diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 4de1683b21c..dab33aa4418 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -13,6 +13,7 @@ describe Group, models: true do it { is_expected.to have_many(:shared_projects).through(:project_group_links) } it { is_expected.to have_many(:notification_settings).dependent(:destroy) } it { is_expected.to have_many(:labels).class_name('GroupLabel') } + it { is_expected.to have_many(:variables).class_name('Ci::GroupVariable') } it { is_expected.to have_many(:uploads).dependent(:destroy) } it { is_expected.to have_one(:chat_team) } @@ -418,4 +419,62 @@ describe Group, models: true do expect(calls).to eq 2 end end + + describe '#secret_variables_for' do + let(:project) { create(:empty_project, group: group) } + + let!(:secret_variable) do + create(:ci_group_variable, value: 'secret', group: group) + end + + let!(:protected_variable) do + create(:ci_group_variable, :protected, value: 'protected', group: group) + end + + subject { group.secret_variables_for('ref', project) } + + shared_examples 'ref is protected' do + it 'contains all the variables' do + is_expected.to contain_exactly(secret_variable, protected_variable) + end + end + + context 'when the ref is not protected' do + before do + stub_application_setting( + default_branch_protection: Gitlab::Access::PROTECTION_NONE) + end + + it 'contains only the secret variables' do + is_expected.to contain_exactly(secret_variable) + end + end + + context 'when the ref is a protected branch' do + before do + create(:protected_branch, name: 'ref', project: project) + end + + it_behaves_like 'ref is protected' + end + + context 'when the ref is a protected tag' do + before do + create(:protected_tag, name: 'ref', project: project) + end + + it_behaves_like 'ref is protected' + end + + context 'when group has a child' do + let!(:group_child) { create(:group, :access_requestable, parent: group) } + let!(:variable_child) { create(:ci_group_variable, group: group_child) } + + subject { group_child.secret_variables_for('ref', project) } + + it 'returns all variables belong to the group and parent groups' do + is_expected.to contain_exactly(secret_variable, protected_variable, variable_child) + end + end + end end diff --git a/spec/presenters/ci/group_variable_presenter_spec.rb b/spec/presenters/ci/group_variable_presenter_spec.rb new file mode 100644 index 00000000000..d404028405b --- /dev/null +++ b/spec/presenters/ci/group_variable_presenter_spec.rb @@ -0,0 +1,63 @@ +require 'spec_helper' + +describe Ci::GroupVariablePresenter do + include Gitlab::Routing.url_helpers + + let(:group) { create(:group) } + let(:variable) { create(:ci_group_variable, group: group) } + + subject(:presenter) do + described_class.new(variable) + end + + it 'inherits from Gitlab::View::Presenter::Delegated' do + expect(described_class.superclass).to eq(Gitlab::View::Presenter::Delegated) + end + + describe '#initialize' do + it 'takes a variable and optional params' do + expect { presenter }.not_to raise_error + end + + it 'exposes variable' do + expect(presenter.variable).to eq(variable) + end + + it 'forwards missing methods to variable' do + expect(presenter.key).to eq(variable.key) + end + end + + describe '#placeholder' do + subject { described_class.new(variable).placeholder } + + it { is_expected.to eq('GROUP_VARIABLE') } + end + + describe '#form_path' do + context 'when variable is persisted' do + subject { described_class.new(variable).form_path } + + it { is_expected.to eq(group_variable_path(group, variable)) } + end + + context 'when variable is not persisted' do + let(:variable) { build(:ci_group_variable, group: group) } + subject { described_class.new(variable).form_path } + + it { is_expected.to eq(group_variables_path(group)) } + end + end + + describe '#edit_path' do + subject { described_class.new(variable).edit_path } + + it { is_expected.to eq(group_variable_path(group, variable)) } + end + + describe '#delete_path' do + subject { described_class.new(variable).delete_path } + + it { is_expected.to eq(group_variable_path(group, variable)) } + end +end diff --git a/spec/presenters/ci/variable_presenter_spec.rb b/spec/presenters/ci/variable_presenter_spec.rb new file mode 100644 index 00000000000..3b615c4bf36 --- /dev/null +++ b/spec/presenters/ci/variable_presenter_spec.rb @@ -0,0 +1,63 @@ +require 'spec_helper' + +describe Ci::VariablePresenter do + include Gitlab::Routing.url_helpers + + let(:project) { create(:empty_project) } + let(:variable) { create(:ci_variable, project: project) } + + subject(:presenter) do + described_class.new(variable) + end + + it 'inherits from Gitlab::View::Presenter::Delegated' do + expect(described_class.superclass).to eq(Gitlab::View::Presenter::Delegated) + end + + describe '#initialize' do + it 'takes a variable and optional params' do + expect { presenter }.not_to raise_error + end + + it 'exposes variable' do + expect(presenter.variable).to eq(variable) + end + + it 'forwards missing methods to variable' do + expect(presenter.key).to eq(variable.key) + end + end + + describe '#placeholder' do + subject { described_class.new(variable).placeholder } + + it { is_expected.to eq('PROJECT_VARIABLE') } + end + + describe '#form_path' do + context 'when variable is persisted' do + subject { described_class.new(variable).form_path } + + it { is_expected.to eq(namespace_project_variable_path(project.namespace, project, variable)) } + end + + context 'when variable is not persisted' do + let(:variable) { build(:ci_variable, project: project) } + subject { described_class.new(variable).form_path } + + it { is_expected.to eq(namespace_project_variables_path(project.namespace, project)) } + end + end + + describe '#edit_path' do + subject { described_class.new(variable).edit_path } + + it { is_expected.to eq(namespace_project_variable_path(project.namespace, project, variable)) } + end + + describe '#delete_path' do + subject { described_class.new(variable).delete_path } + + it { is_expected.to eq(namespace_project_variable_path(project.namespace, project, variable)) } + end +end |