From 29913816309c6f6387b20c8702bcc8e90ef3a984 Mon Sep 17 00:00:00 2001 From: Mayra Cabrera Date: Fri, 6 Apr 2018 09:30:21 -0500 Subject: [PATCH] Addresses database comments - Adds a default on expires_at datetime - Modifies deploy tokens views to handle default expires at value - Use datetime_with_timezone where possible - Remove unused scopes --- .../projects/settings/repository_controller.rb | 2 +- app/helpers/deploy_tokens_helper.rb | 12 ++++++++++-- app/models/deploy_token.rb | 10 ++++++---- ...container_registry_authentication_service.rb | 6 +++--- app/services/deploy_tokens/create_service.rb | 17 ++++++++++++++++- .../projects/deploy_tokens/_form.html.haml | 6 +++--- .../projects/deploy_tokens/_table.html.haml | 2 +- .../20180319190020_create_deploy_tokens.rb | 6 ++++-- ...180405142733_create_project_deploy_tokens.rb | 4 +--- db/schema.rb | 9 ++++----- .../settings/repository_settings_spec.rb | 1 + spec/models/project_deploy_token_spec.rb | 1 - 12 files changed, 50 insertions(+), 26 deletions(-) diff --git a/app/controllers/projects/settings/repository_controller.rb b/app/controllers/projects/settings/repository_controller.rb index 5dfd3af1d55..d8592233302 100644 --- a/app/controllers/projects/settings/repository_controller.rb +++ b/app/controllers/projects/settings/repository_controller.rb @@ -11,7 +11,7 @@ module Projects @new_deploy_token = DeployTokens::CreateService.new(@project, current_user, deploy_token_params).execute if @new_deploy_token.persisted? - flash.now[:notice] = 'Your new project deploy token has been created.' + flash.now[:notice] = s_('DeployTokens|Your new project deploy token has been created.') end render_show diff --git a/app/helpers/deploy_tokens_helper.rb b/app/helpers/deploy_tokens_helper.rb index c791b4e68b3..8496cc2e4b2 100644 --- a/app/helpers/deploy_tokens_helper.rb +++ b/app/helpers/deploy_tokens_helper.rb @@ -5,8 +5,16 @@ module DeployTokensHelper Rails.env.test? end - def container_registry_enabled? + def container_registry_enabled?(project) Gitlab.config.registry.enabled && - can?(current_user, :read_container_image, @project) + can?(current_user, :read_container_image, project) + end + + def expires_at_value(expires_at) + expires_at unless expires_at >= DeployToken::FUTURE_DATE + end + + def show_expire_at?(token) + token.expires? && token.expires_at != DeployToken::FUTURE_DATE end end diff --git a/app/models/deploy_token.rb b/app/models/deploy_token.rb index 11add42b2c5..688540dd67f 100644 --- a/app/models/deploy_token.rb +++ b/app/models/deploy_token.rb @@ -4,6 +4,7 @@ class DeployToken < ActiveRecord::Base add_authentication_token_field :token AVAILABLE_SCOPES = %i(read_repository read_registry).freeze + FUTURE_DATE = Date.new(3000 - 01 - 01) has_many :project_deploy_tokens, inverse_of: :deploy_token has_many :projects, through: :project_deploy_tokens @@ -13,9 +14,7 @@ class DeployToken < ActiveRecord::Base accepts_nested_attributes_for :project_deploy_tokens - scope :active, -> { where("revoked = false AND (expires_at >= NOW() OR expires_at IS NULL)") } - scope :read_repository, -> { where(read_repository: true) } - scope :read_registry, -> { where(read_registry: true) } + scope :active, -> { where("revoked = false AND expires_at >= NOW()") } def revoke! update!(revoked: true) @@ -26,7 +25,7 @@ class DeployToken < ActiveRecord::Base end def scopes - AVAILABLE_SCOPES.select { |token_scope| send("#{token_scope}") } # rubocop:disable GitlabSecurity/PublicSend + AVAILABLE_SCOPES.select { |token_scope| read_attribute(token_scope) } end def username @@ -37,6 +36,9 @@ class DeployToken < ActiveRecord::Base project == requested_project end + # This is temporal. Currently we limit DeployToken + # to a single project, later we're going to extend + # that to be for multiple projects and namespaces. def project projects.first end diff --git a/app/services/auth/container_registry_authentication_service.rb b/app/services/auth/container_registry_authentication_service.rb index 425ceb9c1a8..8f050072f74 100644 --- a/app/services/auth/container_registry_authentication_service.rb +++ b/app/services/auth/container_registry_authentication_service.rb @@ -124,8 +124,8 @@ module Auth end def can_user?(ability, project) - current_user.is_a?(User) && - can?(current_user, ability, project) + user = current_user.is_a?(User) ? current_user : nil + can?(user, ability, project) end def build_can_pull?(requested_project) @@ -143,7 +143,7 @@ module Auth def user_can_pull?(requested_project) has_authentication_ability?(:read_container_image) && - can?(current_user, :read_container_image, requested_project) + can_user?(:read_container_image, requested_project) end def deploy_token_can_pull?(requested_project) diff --git a/app/services/deploy_tokens/create_service.rb b/app/services/deploy_tokens/create_service.rb index a5d31dd9fa5..e623d94b444 100644 --- a/app/services/deploy_tokens/create_service.rb +++ b/app/services/deploy_tokens/create_service.rb @@ -1,7 +1,22 @@ module DeployTokens class CreateService < BaseService def execute - @project.deploy_tokens.build(params).tap(&:save) + @project.deploy_tokens.create(deploy_token_params) + end + + private + + def deploy_token_params + params[:expires_at] = expires_at_date + params + end + + def expires_at_date + params[:expires_at].present? ? default_expires_at : params[:expires_at] + end + + def default_expires_at + DeployToken::FUTURE_DATE end end end diff --git a/app/views/projects/deploy_tokens/_form.html.haml b/app/views/projects/deploy_tokens/_form.html.haml index a947013370d..4e1a796ade0 100644 --- a/app/views/projects/deploy_tokens/_form.html.haml +++ b/app/views/projects/deploy_tokens/_form.html.haml @@ -10,7 +10,7 @@ .form-group = f.label :expires_at, class: 'label-light' - = f.text_field :expires_at, class: 'datepicker form-control' + = f.text_field :expires_at, class: 'datepicker form-control', value: expires_at_value(token.expires_at) .form-group = f.label :scopes, class: 'label-light' @@ -18,8 +18,8 @@ = f.check_box :read_repository = label_tag ("deploy_token_read_repository"), 'read_repository' %span= s_('DeployTokens|Allows read-only access to the repository') - - - if container_registry_enabled? + + - if container_registry_enabled?(project) %fieldset = f.check_box :read_registry = label_tag ("deploy_token_read_registry"), 'read_registry' diff --git a/app/views/projects/deploy_tokens/_table.html.haml b/app/views/projects/deploy_tokens/_table.html.haml index 5013a9b250d..fe9bb1e724a 100644 --- a/app/views/projects/deploy_tokens/_table.html.haml +++ b/app/views/projects/deploy_tokens/_table.html.haml @@ -18,7 +18,7 @@ %td= token.username %td= token.created_at.to_date.to_s(:medium) %td - - if token.expires? + - if show_expire_at?(token) %span{ class: ('text-warning' if token.expires_soon?) } In #{distance_of_time_in_words_to_now(token.expires_at)} - else diff --git a/db/migrate/20180319190020_create_deploy_tokens.rb b/db/migrate/20180319190020_create_deploy_tokens.rb index 6a681ba79d6..b8f77f5ae17 100644 --- a/db/migrate/20180319190020_create_deploy_tokens.rb +++ b/db/migrate/20180319190020_create_deploy_tokens.rb @@ -7,11 +7,13 @@ class CreateDeployTokens < ActiveRecord::Migration t.boolean :read_repository, null: false, default: false t.boolean :read_registry, null: false, default: false - t.datetime :expires_at - t.timestamps null: false + t.datetime_with_timezone :expires_at, null: false, default: '3000-01-01' + t.datetime_with_timezone :created_at, null: false t.string :name, null: false t.string :token, index: { unique: true }, null: false + + t.index [:token, :expires_at], where: "(revoked IS FALSE)" end end end diff --git a/db/migrate/20180405142733_create_project_deploy_tokens.rb b/db/migrate/20180405142733_create_project_deploy_tokens.rb index adc4c526c9b..9d8f89243a8 100644 --- a/db/migrate/20180405142733_create_project_deploy_tokens.rb +++ b/db/migrate/20180405142733_create_project_deploy_tokens.rb @@ -1,13 +1,11 @@ class CreateProjectDeployTokens < ActiveRecord::Migration - include Gitlab::Database::MigrationHelpers - DOWNTIME = false def change create_table :project_deploy_tokens do |t| t.integer :project_id, null: false t.integer :deploy_token_id, null: false - t.timestamps null: false + t.datetime_with_timezone :created_at, null: false t.foreign_key :deploy_tokens, column: :deploy_token_id, on_delete: :cascade t.foreign_key :projects, column: :project_id, on_delete: :cascade diff --git a/db/schema.rb b/db/schema.rb index 0c8938b015d..b0174a4ed46 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -687,13 +687,13 @@ ActiveRecord::Schema.define(version: 20180405142733) do t.boolean "revoked", default: false t.boolean "read_repository", default: false, null: false t.boolean "read_registry", default: false, null: false - t.datetime "expires_at" - t.datetime "created_at", null: false - t.datetime "updated_at", null: false + t.datetime_with_timezone "expires_at", default: '3000-01-01 00:00:00', null: false + t.datetime_with_timezone "created_at", null: false t.string "name", null: false t.string "token", null: false end + add_index "deploy_tokens", ["token", "expires_at"], name: "index_deploy_tokens_on_token_and_expires_at", where: "(revoked IS FALSE)", using: :btree add_index "deploy_tokens", ["token"], name: "index_deploy_tokens_on_token", unique: true, using: :btree create_table "deployments", force: :cascade do |t| @@ -1446,8 +1446,7 @@ ActiveRecord::Schema.define(version: 20180405142733) do create_table "project_deploy_tokens", force: :cascade do |t| t.integer "project_id", null: false t.integer "deploy_token_id", null: false - t.datetime "created_at", null: false - t.datetime "updated_at", null: false + t.datetime_with_timezone "created_at", null: false end add_index "project_deploy_tokens", ["project_id", "deploy_token_id"], name: "index_project_deploy_tokens_on_project_id_and_deploy_token_id", unique: true, using: :btree diff --git a/spec/features/projects/settings/repository_settings_spec.rb b/spec/features/projects/settings/repository_settings_spec.rb index 7887178a3ed..2528c7f437d 100644 --- a/spec/features/projects/settings/repository_settings_spec.rb +++ b/spec/features/projects/settings/repository_settings_spec.rb @@ -94,6 +94,7 @@ feature 'Repository settings' do let!(:deploy_token) { deploy_token_project.deploy_token } before do + stub_container_registry_config(enabled: true) visit project_settings_repository_path(project) end diff --git a/spec/models/project_deploy_token_spec.rb b/spec/models/project_deploy_token_spec.rb index ccaed23f11a..9e2e40c2e8f 100644 --- a/spec/models/project_deploy_token_spec.rb +++ b/spec/models/project_deploy_token_spec.rb @@ -7,7 +7,6 @@ RSpec.describe ProjectDeployToken, type: :model do it { is_expected.to belong_to :project } it { is_expected.to belong_to :deploy_token } - it { is_expected.to accept_nested_attributes_for :deploy_token } it { is_expected.to validate_presence_of :deploy_token } it { is_expected.to validate_presence_of :project }