Allow developer users to generate Terraform plans

Developers are not permitted to lock the Terraform
state, so in order to run `terraform plan` they must
specify the `-lock=false` option:

`terraform plan -lock=false`

https://gitlab.com/gitlab-org/gitlab/-/merge_requests/33573

app/policies/project_policy.rb

@@ -341,6 +341,7 @@ class ProjectPolicy < BasePolicy
     enable :update_alert_management_alert
     enable :create_design
     enable :destroy_design
+    enable :read_terraform_state
   end
 
   rule { can?(:developer_access) & user_confirmed? }.policy do

app/services/terraform/remote_state_handler.rb

@@ -5,26 +5,17 @@ module Terraform
     include Gitlab::OptimisticLocking
 
     StateLockedError = Class.new(StandardError)
+    UnauthorizedError = Class.new(StandardError)
 
-    # rubocop: disable CodeReuse/ActiveRecord
     def find_with_lock
-      raise ArgumentError unless params[:name].present?
-
-      state = Terraform::State.find_by(project: project, name: params[:name])
-      raise ActiveRecord::RecordNotFound.new("Couldn't find state") unless state
-
-      retry_optimistic_lock(state) { |state| yield state } if state && block_given?
-      state
-    end
-    # rubocop: enable CodeReuse/ActiveRecord
-
-    def create_or_find!
-      raise ArgumentError unless params[:name].present?
-
-      Terraform::State.create_or_find_by(project: project, name: params[:name])
+      retrieve_with_lock(find_only: true) do |state|
+        yield state if block_given?
+      end
     end
 
     def handle_with_lock
+      raise UnauthorizedError unless can_modify_state?
+
       retrieve_with_lock do |state|
         raise StateLockedError unless lock_matches?(state)
 
@@ -36,6 +27,7 @@ module Terraform
 
     def lock!
       raise ArgumentError if params[:lock_id].blank?
+      raise UnauthorizedError unless can_modify_state?
 
       retrieve_with_lock do |state|
         raise StateLockedError if state.locked?
@@ -49,6 +41,8 @@ module Terraform
     end
 
     def unlock!
+      raise UnauthorizedError unless can_modify_state?
+
       retrieve_with_lock do |state|
         # force-unlock does not pass ID, so we ignore it if it is missing
         raise StateLockedError unless params[:lock_id].nil? || lock_matches?(state)
@@ -63,8 +57,21 @@ module Terraform
 
     private
 
-    def retrieve_with_lock
-      create_or_find!.tap { |state| retry_optimistic_lock(state) { |state| yield state } }
+    def retrieve_with_lock(find_only: false)
+      create_or_find!(find_only: find_only).tap { |state| retry_optimistic_lock(state) { |state| yield state } }
+    end
+
+    def create_or_find!(find_only:)
+      raise ArgumentError unless params[:name].present?
+
+      find_params = { project: project, name: params[:name] }
+
+      if find_only
+        Terraform::State.find_by(find_params) || # rubocop: disable CodeReuse/ActiveRecord
+          raise(ActiveRecord::RecordNotFound.new("Couldn't find state"))
+      else
+        Terraform::State.create_or_find_by(find_params)
+      end
     end
 
     def lock_matches?(state)
@@ -73,5 +80,9 @@ module Terraform
       ActiveSupport::SecurityUtils
         .secure_compare(state.lock_xid.to_s, params[:lock_id].to_s)
     end
+
+    def can_modify_state?
+      current_user.can?(:admin_terraform_state, project)
+    end
   end
 end

changelogs/unreleased/216785-terraform-plan-developer-access.yml

+---
+title: Allow developer role read-only access to Terraform state
+merge_request: 33573
+author:
+type: added

lib/api/terraform/state.rb

@@ -11,7 +11,7 @@ module API
 
       before do
         authenticate!
-        authorize! :admin_terraform_state, user_project
+        authorize! :read_terraform_state, user_project
       end
 
       params do
@@ -46,6 +46,8 @@ module API
           desc 'Add a new terraform state or update an existing one'
           route_setting :authentication, basic_auth_personal_access_token: true, job_token_allowed: :basic_auth
           post do
+            authorize! :admin_terraform_state, user_project
+
             data = request.body.read
             no_content! if data.empty?
 
@@ -59,6 +61,8 @@ module API
           desc 'Delete a terraform state of a certain name'
           route_setting :authentication, basic_auth_personal_access_token: true, job_token_allowed: :basic_auth
           delete do
+            authorize! :admin_terraform_state, user_project
+
             remote_state_handler.handle_with_lock do |state|
               state.destroy!
               status :ok
@@ -77,6 +81,8 @@ module API
             requires :Path, type: String, desc: 'Terraform path'
           end
           post '/lock' do
+            authorize! :admin_terraform_state, user_project
+
             status_code = :ok
             lock_info = {
               'Operation' => params[:Operation],
@@ -108,6 +114,8 @@ module API
             optional :ID, type: String, limit: 255, desc: 'Terraform state lock ID'
           end
           delete '/lock' do
+            authorize! :admin_terraform_state, user_project
+
             remote_state_handler.unlock!
             status :ok
           rescue ::Terraform::RemoteStateHandler::StateLockedError

spec/factories/terraform/state.rb

@@ -9,5 +9,11 @@ FactoryBot.define do
     trait :with_file do
       file { fixture_file_upload('spec/fixtures/terraform/terraform.tfstate', 'application/json') }
     end
+
+    trait :locked do
+      sequence(:lock_xid) { |n| "lock-#{n}" }
+      locked_at { Time.current }
+      locked_by_user { create(:user) }
+    end
   end
 end

spec/policies/project_policy_spec.rb

@@ -46,6 +46,7 @@ RSpec.describe ProjectPolicy do
       resolve_note create_container_image update_container_image destroy_container_image daily_statistics
       create_environment update_environment create_deployment update_deployment create_release update_release
       create_metrics_dashboard_annotation delete_metrics_dashboard_annotation update_metrics_dashboard_annotation
+      read_terraform_state
     ]
   end
 

spec/requests/api/terraform/state_spec.rb

@@ -59,10 +59,11 @@ RSpec.describe API::Terraform::State do
       context 'with developer permissions' do
         let(:current_user) { developer }
 
-        it 'returns forbidden if the user cannot access the state' do
+        it 'returns terraform state belonging to a project of given state name' do
           request
 
-          expect(response).to have_gitlab_http_status(:forbidden)
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(response.body).to eq(state.file.read)
         end
       end
     end
@@ -94,10 +95,11 @@ RSpec.describe API::Terraform::State do
       context 'with developer permissions' do
         let(:job) { create(:ci_build, project: project, user: developer) }
 
-        it 'returns forbidden if the user cannot access the state' do
+        it 'returns terraform state belonging to a project of given state name' do
           request
 
-          expect(response).to have_gitlab_http_status(:forbidden)
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(response.body).to eq(state.file.read)
         end
       end
     end
@@ -235,9 +237,43 @@ RSpec.describe API::Terraform::State do
 
       expect(response).to have_gitlab_http_status(:ok)
     end
+
+    context 'state is already locked' do
+      before do
+        state.update!(lock_xid: 'locked', locked_by_user: current_user)
+      end
+
+      it 'returns an error' do
+        request
+
+        expect(response).to have_gitlab_http_status(:conflict)
+      end
+    end
+
+    context 'user does not have permission to lock the state' do
+      let(:current_user) { developer }
+
+      it 'returns an error' do
+        request
+
+        expect(response).to have_gitlab_http_status(:forbidden)
+      end
+    end
   end
 
   describe 'DELETE /projects/:id/terraform/state/:name/lock' do
+    let(:params) do
+      {
+        ID: lock_id,
+        Version: '0.1',
+        Operation: 'OperationTypePlan',
+        Info: '',
+        Who: "#{current_user.username}",
+        Created: Time.now.utc.iso8601(6),
+        Path: ''
+      }
+    end
+
     before do
       state.lock_xid = '123-456'
       state.save!
@@ -246,7 +282,7 @@ RSpec.describe API::Terraform::State do
     subject(:request) { delete api("#{state_path}/lock"), headers: auth_header, params: params }
 
     context 'with the correct lock id' do
-      let(:params) { { ID: '123-456' } }
+      let(:lock_id) { '123-456' }
 
       it 'removes the terraform state lock' do
         request
@@ -266,7 +302,7 @@ RSpec.describe API::Terraform::State do
     end
 
     context 'with an incorrect lock id' do
-      let(:params) { { ID: '456-789' } }
+      let(:lock_id) { '456-789' }
 
       it 'returns an error' do
         request
@@ -276,7 +312,7 @@ RSpec.describe API::Terraform::State do
     end
 
     context 'with a longer than 255 character lock id' do
-      let(:params) { { ID: '0' * 256 } }
+      let(:lock_id) { '0' * 256 }
 
       it 'returns an error' do
         request
@@ -284,5 +320,16 @@ RSpec.describe API::Terraform::State do
         expect(response).to have_gitlab_http_status(:bad_request)
       end
     end
+
+    context 'user does not have permission to unlock the state' do
+      let(:lock_id) { '123-456' }
+      let(:current_user) { developer }
+
+      it 'returns an error' do
+        request
+
+        expect(response).to have_gitlab_http_status(:forbidden)
+      end
+    end
   end
 end

spec/services/terraform/remote_state_handler_spec.rb

@@ -4,7 +4,10 @@ require 'spec_helper'
 
 RSpec.describe Terraform::RemoteStateHandler do
   let_it_be(:project) { create(:project) }
-  let_it_be(:user) { create(:user) }
+  let_it_be(:developer) { create(:user, developer_projects: [project]) }
+  let_it_be(:maintainer) { create(:user, maintainer_projects: [project]) }
+
+  let_it_be(:user) { maintainer }
 
   describe '#find_with_lock' do
     context 'without a state name' do
@@ -34,33 +37,6 @@ RSpec.describe Terraform::RemoteStateHandler do
     end
   end
 
-  describe '#create_or_find!' do
-    it 'requires passing a state name' do
-      handler = described_class.new(project, user)
-
-      expect { handler.create_or_find! }.to raise_error(ArgumentError)
-    end
-
-    it 'allows to create states with same name in different projects' do
-      project_b =  create(:project)
-
-      state_a = described_class.new(project, user, name: 'my-state').create_or_find!
-      state_b = described_class.new(project_b, user, name: 'my-state').create_or_find!
-
-      expect(state_a).to be_persisted
-      expect(state_b).to be_persisted
-      expect(state_a.id).not_to eq state_b.id
-    end
-
-    it 'loads the same state upon subsequent call in the project scope' do
-      state_a = described_class.new(project, user, name: 'my-state').create_or_find!
-      state_b = described_class.new(project, user, name: 'my-state').create_or_find!
-
-      expect(state_a).to be_persisted
-      expect(state_a.id).to eq state_b.id
-    end
-  end
-
   context 'when state locking is not being used' do
     subject { described_class.new(project, user, name: 'my-state') }
 
@@ -74,7 +50,7 @@ RSpec.describe Terraform::RemoteStateHandler do
       end
 
       it 'returns the state object itself' do
-        state = subject.create_or_find!
+        state = subject.handle_with_lock
 
         expect(state.name).to eq 'my-state'
       end
@@ -89,10 +65,9 @@ RSpec.describe Terraform::RemoteStateHandler do
 
   context 'when using locking' do
     describe '#handle_with_lock' do
-      it 'handles a locked state using exclusive read lock' do
-        handler = described_class
-          .new(project, user, name: 'new-state', lock_id: 'abc-abc')
+      subject(:handler) { described_class.new(project, user, name: 'new-state', lock_id: 'abc-abc') }
 
+      it 'handles a locked state using exclusive read lock' do
         handler.lock!
 
         state = handler.handle_with_lock do |state|
@@ -101,20 +76,35 @@ RSpec.describe Terraform::RemoteStateHandler do
 
         expect(state.name).to eq 'new-name'
       end
-    end
 
-    it 'raises exception if lock has not been acquired before' do
-      handler = described_class
-        .new(project, user, name: 'new-state', lock_id: 'abc-abc')
+      it 'raises exception if lock has not been acquired before' do
+        expect { handler.handle_with_lock }
+          .to raise_error(described_class::StateLockedError)
+      end
+
+      context 'user does not have permission to modify state' do
+        let(:user) { developer }
 
-      expect { handler.handle_with_lock }
-        .to raise_error(described_class::StateLockedError)
+        it 'raises an exception' do
+          expect { handler.handle_with_lock }
+            .to raise_error(described_class::UnauthorizedError)
+        end
+      end
     end
 
     describe '#lock!' do
-      it 'allows to lock state if it does not exist yet' do
-        handler = described_class.new(project, user, name: 'new-state', lock_id: 'abc-abc')
+      let(:lock_id) { 'abc-abc' }
+
+      subject(:handler) do
+        described_class.new(
+          project,
+          user,
+          name: 'new-state',
+          lock_id: lock_id
+        )
+      end
 
+      it 'allows to lock state if it does not exist yet' do
         state = handler.lock!
 
         expect(state).to be_persisted
@@ -122,22 +112,61 @@ RSpec.describe Terraform::RemoteStateHandler do
       end
 
       it 'allows to lock state if it exists and is not locked' do
-        state = described_class.new(project, user, name: 'new-state').create_or_find!
-        handler = described_class.new(project, user, name: 'new-state', lock_id: 'abc-abc')
+        state = create(:terraform_state, project: project, name: 'new-state')
 
         handler.lock!
 
-        expect(state.reload.lock_xid).to eq 'abc-abc'
+        expect(state.reload.lock_xid).to eq lock_id
         expect(state).to be_locked
       end
 
       it 'raises an exception when trying to unlocked state locked by someone else' do
-        described_class.new(project, user, name: 'new-state', lock_id: 'abc-abc').lock!
-
-        handler = described_class.new(project, user, name: 'new-state', lock_id: '12a-23f')
+        described_class.new(project, user, name: 'new-state', lock_id: '12a-23f').lock!
 
         expect { handler.lock! }.to raise_error(described_class::StateLockedError)
       end
     end
+
+    describe '#unlock!' do
+      let(:lock_id) { 'abc-abc' }
+
+      subject(:handler) do
+        described_class.new(
+          project,
+          user,
+          name: 'new-state',
+          lock_id: lock_id
+        )
+      end
+
+      before do
+        create(:terraform_state, :locked, project: project, name: 'new-state', lock_xid: 'abc-abc')
+      end
+
+      it 'unlocks the state' do
+        state = handler.unlock!
+
+        expect(state.lock_xid).to be_nil
+      end
+
+      context 'with no lock ID (force-unlock)' do
+        let(:lock_id) { }
+
+        it 'unlocks the state' do
+          state = handler.unlock!
+
+          expect(state.lock_xid).to be_nil
+        end
+      end
+
+      context 'with different lock ID' do
+        let(:lock_id) { 'other' }
+
+        it 'raises an exception' do
+          expect { handler.unlock! }
+            .to raise_error(described_class::StateLockedError)
+        end
+      end
+    end
   end
 end