Some code review comments appliedfj-web-terminal-ci-build

9 files changed, 60 insertions, 57 deletions

diff --git a/app/controllers/projects/jobs_controller.rb b/app/controllers/projects/jobs_controller.rb
index 8b6da224ed7..0f8cb7d1975 100644
--- a/app/controllers/projects/jobs_controller.rb
+++ b/app/controllers/projects/jobs_controller.rb
@@ -157,7 +157,7 @@ class Projects::JobsController < Projects::ApplicationController
   end
 
   def authorize_use_build_terminal!
-    return access_denied! unless can?(current_user, :use_build_terminal, build)
+    return access_denied! unless can?(current_user, :create_build_terminal, build)
   end
 
   def verify_api_request!
diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 7cbd6a1124e..142c7a7015f 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -29,7 +29,8 @@ module Ci
     has_one :metadata, class_name: 'Ci::BuildMetadata'
     has_one :runner_session, class_name: 'Ci::BuildRunnerSession', validate: true
     delegate :timeout, to: :metadata, prefix: true, allow_nil: true
-    delegate :url, :certificate, :authorization, to: :runner_session, prefix: true, allow_nil: true
+    delegate :url, to: :runner_session, prefix: true, allow_nil: true
+    delegate :terminal_specification, to: :runner_session, allow_nil: true
     delegate :gitlab_deploy_token, to: :project
 
     ##
@@ -594,25 +595,6 @@ module Ci
       running? && runner_session_url.present?
     end
 
-    def terminal_specification
-      return {} unless runner_session_url.present?
-
-      headers = {}
-
-      if runner_session_authorization.present?
-        headers['Authorization'] = runner_session_authorization
-      end
-
-      # TODO: change url to "#{metadata_runner_session_url}/exec" once the runner
-      # has all the terminal server changes
-      {
-        subprotocols: ['terminal.gitlab.com'].freeze,
-        url: "#{runner_session_url}/exec".sub("https://", "wss://"),
-        headers: headers,
-        ca_pem: runner_session_certificate.presence
-      }
-    end
-
     private
 
     def update_artifacts_size
diff --git a/app/models/ci/build_runner_session.rb b/app/models/ci/build_runner_session.rb
index 91ec818ef11..c9b36cefad1 100644
--- a/app/models/ci/build_runner_session.rb
+++ b/app/models/ci/build_runner_session.rb
@@ -10,5 +10,16 @@ module Ci
 
     validates :build, presence: true
     validates :url, url: { protocols: %w(https) }
+
+    def terminal_specification
+      return {} unless url.present?
+
+      {
+        subprotocols: ['terminal.gitlab.com'].freeze,
+        url: "#{url}/exec".sub("https://", "wss://"),
+        headers: { Authorization: authorization.presence }.compact,
+        ca_pem: certificate.presence
+      }
+    end
   end
 end
diff --git a/app/policies/ci/build_policy.rb b/app/policies/ci/build_policy.rb
index 88e00c3b716..c85d755a506 100644
--- a/app/policies/ci/build_policy.rb
+++ b/app/policies/ci/build_policy.rb
@@ -34,6 +34,6 @@ module Ci
       enable :update_commit_status
     end
 
-    rule { can?(:update_build) & terminal }.enable :use_build_terminal
+    rule { can?(:update_build) & terminal }.enable :create_build_terminal
   end
 end
diff --git a/app/views/projects/jobs/_sidebar.html.haml b/app/views/projects/jobs/_sidebar.html.haml
index d23c78f0ae3..ddf187994ce 100644
--- a/app/views/projects/jobs/_sidebar.html.haml
+++ b/app/views/projects/jobs/_sidebar.html.haml
@@ -1,11 +1,8 @@
 %aside.right-sidebar.right-sidebar-expanded.build-sidebar.js-build-sidebar.js-right-sidebar{ data: { "offset-top" => "101", "spy" => "affix" } }
   .sidebar-container
     .blocks-container
-      .block
-        %strong.inline.prepend-top-8
-          = @build.name
-
-        - if can?(current_user, :use_build_terminal, @build)
+      - if can?(current_user, :create_build_terminal, @build)
+        .block
           = link_to terminal_project_job_path(@project, @build), class: 'terminal-button pull-right btn visible-md-block visible-lg-block', title: 'Terminal' do
             Terminal
 
diff --git a/spec/controllers/projects/jobs_controller_spec.rb b/spec/controllers/projects/jobs_controller_spec.rb
index c97b2745fdc..3e7833f5904 100644
--- a/spec/controllers/projects/jobs_controller_spec.rb
+++ b/spec/controllers/projects/jobs_controller_spec.rb
@@ -562,7 +562,7 @@ describe Projects::JobsController, :clean_gitlab_redis_shared_state do
 
     context 'when job exists' do
       context 'and it has a terminal' do
-        let!(:job) { create(:ci_build, :running, :with_runner_session_url, pipeline: pipeline) }
+        let!(:job) { create(:ci_build, :running, :with_runner_session, pipeline: pipeline) }
 
         it 'has a job' do
           get_terminal(id: job.id)
@@ -602,7 +602,7 @@ describe Projects::JobsController, :clean_gitlab_redis_shared_state do
   end
 
   describe 'GET #terminal_websocket_authorize' do
-    let!(:job) { create(:ci_build, :running, :with_runner_session_url, pipeline: pipeline) }
+    let!(:job) { create(:ci_build, :running, :with_runner_session, pipeline: pipeline) }
 
     before do
       project.add_developer(user)
diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb
index 2ab154c8499..83cb4750741 100644
--- a/spec/factories/ci/builds.rb
+++ b/spec/factories/ci/builds.rb
@@ -249,9 +249,9 @@ FactoryBot.define do
       failure_reason 2
     end
 
-    trait :with_runner_session_url do
-      after(:create) do |build|
-        build.create_runner_session(url: 'ws://localhost')
+    trait :with_runner_session do
+      after(:build) do |build|
+        build.build_runner_session(url: 'ws://localhost')
       end
     end
   end
diff --git a/spec/models/ci/build_runner_session_spec.rb b/spec/models/ci/build_runner_session_spec.rb
new file mode 100644
index 00000000000..7183957aa50
--- /dev/null
+++ b/spec/models/ci/build_runner_session_spec.rb
@@ -0,0 +1,36 @@
+require 'spec_helper'
+
+describe Ci::BuildRunnerSession, model: true do
+  let!(:build) { create(:ci_build, :with_runner_session) }
+
+  subject { build.runner_session }
+
+  it { is_expected.to belong_to(:build) }
+
+  it { is_expected.to validate_presence_of(:build) }
+  it { is_expected.to validate_presence_of(:url).with_message('must be a valid URL') }
+
+  describe '#terminal_specification' do
+    let(:terminal_specification) { subject.terminal_specification }
+
+    it 'returns empty hash if no url' do
+      subject.url = ''
+
+      expect(terminal_specification).to be_empty
+    end
+
+    context 'when url is present' do
+      it 'returns ca_pem nil if empty certificate' do
+        subject.certificate = ''
+
+        expect(terminal_specification[:ca_pem]).to be_nil
+      end
+
+      it 'adds Authorization header if authorization is present' do
+        subject.authorization = 'whatever'
+
+        expect(terminal_specification[:headers]).to include(Authorization: 'whatever')
+      end
+    end
+  end
+end
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index b315c37afd8..8fc6ea8a8db 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -19,6 +19,7 @@ describe Ci::Build do
   it { is_expected.to belong_to(:erased_by) }
   it { is_expected.to have_many(:deployments) }
   it { is_expected.to have_many(:trace_sections)}
+  it { is_expected.to have_one(:runner_session)}
   it { is_expected.to validate_presence_of(:ref) }
   it { is_expected.to respond_to(:has_trace?) }
   it { is_expected.to respond_to(:trace) }
@@ -46,7 +47,7 @@ describe Ci::Build do
     context 'when transitioning to any state from running' do
       it 'removes runner_session' do
         %w(success drop cancel).each do |event|
-          build = FactoryBot.create(:ci_build, :running, :with_runner_session_url, pipeline: pipeline)
+          build = FactoryBot.create(:ci_build, :running, :with_runner_session, pipeline: pipeline)
 
           build.fire_events!(event)
 
@@ -2650,28 +2651,4 @@ describe Ci::Build do
       end
     end
   end
-
-  describe '#terminal_specification' do
-    subject { build.terminal_specification }
-
-    it 'returns empty hash if no runner_session_url' do
-      expect(subject).to be_empty
-    end
-
-    context 'when runner_session_url is present' do
-      set(:build) { create(:ci_build, :with_runner_session_url, project: project, user: user) }
-
-      it 'returns ca_pem nil if empty runner_session_certificate' do
-        build.runner_session.certificate = ''
-
-        expect(subject[:ca_pem]).to be_nil
-      end
-
-      it 'adds Authorization header if runner_session_authorization is present' do
-        build.runner_session.authorization = 'whatever'
-
-        expect(subject[:headers]).to include('Authorization' => 'whatever')
-      end
-    end
-  end
 end