Team scoped users are now able to delete a deployment.

[#117557245]

Signed-off-by: Danny Berger <dberger@pivotal.io>

bosh-director/db/migrations/director/20160414183654_set_teams_on_task.rb

@@ -0,0 +1,7 @@
+Sequel.migration do
+  change do
+    alter_table(:tasks) do
+      add_column(:teams, String)
+    end
+  end
+end

bosh-director/lib/bosh/director/api/controllers/tasks_controller.rb

@@ -16,9 +16,9 @@ def self.authorization(perm)
           type = params[:type]
           task = @task_manager.find_task(params[:id])
           if type == 'debug' || type == 'cpi' || !type
-            check_access_to_task(task, :admin)
+            @permission_authorizer.granted_or_raise(task, :admin, token_scopes)
           elsif type == 'event' || type == 'result' || type == 'none'
-            check_access_to_task(task, :read)
+            @permission_authorizer.granted_or_raise(task, :read, token_scopes)
           else
             raise UnauthorizedToAccessDeployment, "Unknown type #{type}"
           end
@@ -89,10 +89,7 @@ def self.authorization(perm)
 
       get '/:id', scope: :list_tasks do
         task = @task_manager.find_task(params[:id])
-        deployment_name = task.deployment_name
-        if deployment_name
-          check_access_to_deployment(deployment_name, :read)
-        elsif !@permission_authorizer.is_granted?(:director, :read, token_scopes)
+        if !@permission_authorizer.is_granted?(task, :read, token_scopes)
           raise UnauthorizedToAccessDeployment,
             'One of the following scopes is required to access this task: ' +
               @permission_authorizer.list_expected_scope(:director, :read, token_scopes).join(', ')
@@ -134,23 +131,6 @@ def self.authorization(perm)
 
       private
 
-      def check_access_to_task(task, scope)
-        if task.deployment_name
-          check_access_to_deployment(task.deployment_name, scope)
-        else
-          @permission_authorizer.granted_or_raise(:director, scope, token_scopes)
-        end
-      end
-
-      def check_access_to_deployment(deployment_name, scope)
-        begin
-          deployment = @deployment_manager.find_by_name(deployment_name)
-          @permission_authorizer.granted_or_raise(deployment, scope, token_scopes)
-        rescue DeploymentNotFound
-          @permission_authorizer.granted_or_raise(:director, :admin, token_scopes)
-        end
-      end
-
       def task_timeout?(task)
         # Some of the old task entries might not have the checkpoint_time
         unless task.checkpoint_time

bosh-director/lib/bosh/director/api/task_helper.rb

@@ -4,12 +4,15 @@ module Bosh::Director
   module Api
     class TaskHelper
       def create_task(username, type, description, deployment_name)
+        teams = deployment_name ? DeploymentManager.new.find_by_name(deployment_name).teams : nil
+
         task = Models::Task.create(:username => username,
                                    :type => type,
                                    :description => description,
                                    :state => :queued,
                                    :deployment_name => deployment_name,
                                    :timestamp => Time.now,
+                                   :teams => teams,
                                    :checkpoint_time => Time.now)
         log_dir = File.join(Config.base_dir, 'tasks', task.id.to_s)
         task_status_file = File.join(log_dir, 'debug')

bosh-director/lib/bosh/director/permission_authorizer.rb

@@ -18,7 +18,7 @@ def list_expected_scope(subject, permission, user_scopes)
       expected_scope = director_permissions[:admin]
 
       if subject.instance_of? Models::Deployment
-        expected_scope << deployment_team_scopes(subject, 'admin')
+        expected_scope << subject_team_scopes(subject, 'admin')
 
         if :admin == permission
           # already allowed with initial expected_scope
@@ -40,6 +40,16 @@ def list_expected_scope(subject, permission, user_scopes)
         else
           raise ArgumentError, "Unexpected permission for director: #{permission}"
         end
+      elsif subject.instance_of?(Models::Task)
+        expected_scope << subject_team_scopes(subject, 'admin')
+
+        if :admin == permission
+          # already allowed with initial expected_scope
+        elsif :read == permission
+          expected_scope << director_permissions[:read]
+        else
+          raise ArgumentError, "Unexpected permission for task: #{permission}"
+        end
       else
         raise ArgumentError, "Unexpected subject: #{subject}"
       end
@@ -62,8 +72,8 @@ def director_permissions
       }
     end
 
-    def deployment_team_scopes(deployment, permission)
-      permissions = deployment.teams.nil? ? [] : deployment.teams.split(',')
+    def subject_team_scopes(subject, permission)
+      permissions = subject.teams.nil? ? [] : subject.teams.split(',')
       permissions.map{ |team_name| "bosh.teams.#{team_name}.#{permission}" }
     end
 

bosh-director/spec/unit/api/controllers/disks_controller_spec.rb

@@ -27,6 +27,7 @@ module Api
       let(:orphaned_at) { Time.now.utc }
 
       before do
+        Models::Deployment.make(name: 'foo')
         App.new(config)
       end
 

bosh-director/spec/unit/api/controllers/tasks_controller_spec.rb

@@ -73,15 +73,15 @@ module Api
 
             context 'collection of tasks associated with different deployments' do
               before do
-                Models::Task.make(type: 'attach_disk', deployment_name: deployment_name_1)
+                Models::Task.make(type: 'attach_disk', deployment_name: deployment_name_1, teams: 'team-rocket,dev')
                 Models::Task.make(type: 'create_snapshot')
-                Models::Task.make(type: 'delete_deployment', deployment_name: deployment_name_1)
+                Models::Task.make(type: 'delete_deployment', deployment_name: deployment_name_1, teams: 'team-rocket,dev')
                 Models::Task.make(type: 'delete_release')
                 Models::Task.make(type: 'delete_snapshot')
                 Models::Task.make(type: 'delete_stemcell')
-                Models::Task.make(type: 'run_errand', deployment_name: deployment_name_2)
-                Models::Task.make(type: 'snapshot_deployment', deployment_name: deployment_name_1)
-                Models::Task.make(type: 'update_deployment', deployment_name: deployment_name_2)
+                Models::Task.make(type: 'run_errand', deployment_name: deployment_name_2, teams: 'team-rocket')
+                Models::Task.make(type: 'snapshot_deployment', deployment_name: deployment_name_1, teams: 'team-rocket,dev')
+                Models::Task.make(type: 'update_deployment', deployment_name: deployment_name_2, teams: 'team-rocket')
                 Models::Task.make(type: 'update_release')
                 Models::Task.make(type: 'update_stemcell')
               end
@@ -313,11 +313,9 @@ module Api
           context 'user has readonly access' do
             before(:each) { basic_authorize 'reader', 'reader' }
 
-            context "user has access to task's deployment" do
-              it 'provides access if accessing task' do
-                get "/#{task.id}"
-                expect(last_response.status).to eq(200)
-              end
+            it 'provides access if accessing task' do
+              get "/#{task.id}"
+              expect(last_response.status).to eq(200)
             end
           end
 
@@ -345,36 +343,17 @@ module Api
               expect(task_json['description']).to eq('fake-description')
             end
 
-            context "user has access to task's deployment" do
-              before do
-                Models::Deployment.make(:name => deployment_name_1,
-                  :teams => 'team-rocket,dev'
-                )
-              end
-              let(:task) { Models::Task.make(state: 'queued', deployment_name: deployment_name_1) }
+            context 'user has access to task' do
+              let(:task) { Models::Task.make(state: 'queued', deployment_name: deployment_name_1, teams: 'team-rocket,dev') }
 
               it 'returns task' do
                 get "/#{task.id}"
                 expect(last_response.status).to eq(200)
               end
             end
 
-            context "user does not have access to task's deployment" do
-              before do
-                Models::Deployment.make(:name => deployment_name_1,
-                  :teams => 'team-rocket'
-                )
-              end
-              let(:task) { Models::Task.make(state: 'queued', deployment_name: deployment_name_1) }
-
-              it 'returns task' do
-                get "/#{task.id}"
-                expect(last_response.status).to eq(200)
-              end
-            end
-
-            context "task's deployment got deleted" do
-              let(:task) { Models::Task.make(state: 'queued', deployment_name: 'removed') }
+            context 'user does not have access to task' do
+              let(:task) { Models::Task.make(state: 'queued', deployment_name: deployment_name_1, teams: 'team-rocket') }
 
               it 'returns task' do
                 get "/#{task.id}"
@@ -384,39 +363,24 @@ module Api
           end
 
           context 'user has team admin access' do
-            context "user doesn't have access to task's deployment" do
+            context "user doesn't have access to task" do
               before do
-                Models::Deployment.make(:name => deployment_name_1,
-                  :teams => 'team-rocket'
-                )
                 basic_authorize 'dev-team-member', 'dev-team-member'
               end
-              let(:task) { Models::Task.make(state: 'queued', deployment_name: deployment_name_1) }
-              it 'returns 401' do
-                get "/#{task.id}"
-                expect(last_response.status).to eq(401)
-              end
-            end
 
-            context 'when task has no deployment' do
-              before do
-                basic_authorize 'dev-team-member', 'dev-team-member'
-              end
-              let(:task) { Models::Task.make(state: 'queued') }
+              let(:task) { Models::Task.make(state: 'queued', deployment_name: deployment_name_1, :teams => 'team-rocket') }
               it 'returns 401' do
                 get "/#{task.id}"
                 expect(last_response.status).to eq(401)
               end
             end
 
-            context "user has access to task's deployment" do
+            context "user has access to task" do
               before do
-                Models::Deployment.make(:name => deployment_name_1,
-                  :teams => 'team-rocket,dev'
-                )
                 basic_authorize 'dev-team-member', 'dev-team-member'
               end
-              let(:task) { Models::Task.make(state: 'queued', deployment_name: deployment_name_1) }
+
+              let(:task) { Models::Task.make(state: 'queued', deployment_name: deployment_name_1, :teams => 'team-rocket,dev') }
               it 'returns 200' do
                 get "/#{task.id}"
                 expect(last_response.status).to eq(200)
@@ -542,18 +506,20 @@ module Api
               Models::Task.make(
                 type: :update_deployment,
                 state: :queued,
-                deployment_name: deployment_name_1
+                deployment_name: deployment_name_1,
+                teams: 'team-rocket,dev',
               )
             end
             let(:task_2) do
               Models::Task.make(
                 type: :update_deployment,
                 state: :queued,
-                deployment_name: deployment_name_2
+                deployment_name: deployment_name_2,
+                teams: 'team-rocket',
               )
             end
             let(:task_deleted) do
-              Models::Task.make(type: :update_deployment, state: :queued, deployment_name: 'deleted')
+              Models::Task.make(type: :update_deployment, state: :queued, deployment_name: 'deleted', )
             end
 
             let(:task_no_deployment) { Models::Task.make(type: :update_deployment, state: :queued) }

bosh-director/spec/unit/api/task_helper_spec.rb

@@ -9,13 +9,14 @@ module Bosh::Director
       let(:type) { 'type' }
       let(:description) { 'description' }
       let(:config) { Psych.load_file(asset('test-director-config.yml')) }
-      let(:deployment_name) { 'deployment name' }
+      let(:deployment_name) { 'deployment-name' }
       let(:task_remover) { instance_double('Bosh::Director::Api::TaskRemover') }
 
       before do
         Config.configure(config)
         Config.base_dir = tmpdir
         Config.max_tasks = 2
+        Models::Deployment.make(name: deployment_name, teams: 'security,spies')
         allow(Api::TaskRemover).to receive(:new).and_return(task_remover)
         allow(task_remover).to receive(:remove)
       end
@@ -46,6 +47,18 @@ module Bosh::Director
         expect(director_version_line).to match(/INFO .* Director Version: #{Bosh::Director::VERSION}/)
         expect(enqueuing_task_line).to match(/INFO .* Enqueuing task: #{task.id}/)
       end
+
+      it 'persists deployment teams on the task so that they can be referenced even when the deployment database record has been deleted' do
+        expect {
+          described_class.new.create_task('fake-user', type, description, deployment_name)
+        }.to change {
+          Models::Task.where(teams: 'security,spies').count
+        }.from(0).to(1)
+      end
+
+      it 'does not reference teams when task is not deployment-specific' do
+        expect(described_class.new.create_task('fake-user', type, description, nil).teams).to be_nil
+      end
     end
   end
 end

bosh-director/spec/unit/db/migrations/director/20160414183654_set_teams_on_task_spec.rb

@@ -0,0 +1,48 @@
+require 'db_spec_helper'
+
+module Bosh::Director
+  describe 'set_teams_on_task' do
+    let(:db) { DBSpecHelper.db }
+    let(:migration_file) { '20160414183654_set_teams_on_task.rb' }
+
+    before { DBSpecHelper.migrate_all_before(migration_file) }
+
+    it 'allows teams to optionally be added to tasks' do
+      db[:tasks] << {
+          id: 1,
+          state: 'finished',
+          type: 'something',
+          deployment_name: 'test-deployment',
+          timestamp: '2016-04-14 11:53:42',
+          description: 'delete_deployment',
+      }
+
+      DBSpecHelper.migrate(migration_file)
+
+      db[:tasks] << {
+          id: 2,
+          state: 'finished',
+          type: 'something',
+          deployment_name: 'other-deployment',
+          timestamp: '2016-04-14 11:53:42',
+          description: 'delete_deployment',
+          teams: 'dev,qa',
+      }
+
+      db[:tasks] << {
+          id: 3,
+          state: 'finished',
+          type: 'something',
+          deployment_name: 'other-deployment',
+          timestamp: '2016-04-14 11:53:42',
+          description: 'delete_deployment',
+      }
+
+      tasks = db[:tasks].all
+      expect(tasks.count).to eq(3)
+      expect(tasks[0][:teams]).to be_nil
+      expect(tasks[1][:teams]).to eq('dev,qa')
+      expect(tasks[2][:teams]).to be_nil
+    end
+  end
+end