Merge pull request #3547 from okriuchykhin/ok_SCI_6042

Update/implement permission checks in the project related controllers [SCI-6042]

Gemfile

@@ -134,7 +134,6 @@ group :test do
   gem 'capybara'
   gem 'capybara-email'
   gem 'cucumber-rails', '~> 1.8', require: false
-  gem 'database_cleaner'
   gem 'json_matchers'
   gem 'selenium-webdriver'
   gem 'shoulda-matchers'

Gemfile.lock

@@ -236,7 +236,6 @@ GEM
       railties (>= 4.2, < 7)
     cucumber-tag_expressions (1.1.1)
     cucumber-wire (0.0.1)
-    database_cleaner (1.8.5)
     debug_inspector (1.0.0)
     deface (1.6.1)
       nokogiri (>= 1.6)

app/models/concerns/assignable.rb

@@ -12,7 +12,7 @@ module Assignable
       UserAssignment.create(
         user: created_by,
         assignable: self,
-        user_role: UserRole.find_by(name: 'Owner')
+        user_role: UserRole.find_by(name: I18n.t('user_roles.predefined.owner'))
       )
 
       UserAssignments::GenerateUserAssignmentsJob.perform_later(self, created_by)

app/models/project.rb

@@ -367,11 +367,11 @@ class Project < ApplicationRecord
   end
 
   def assign_project_ownership
-    UserAssignment.create(
+    UserAssignment.create!(
       user: created_by,
       assignable: self,
       assigned: :manually, # we set this to manually since was the user action to create the project
-      user_role: UserRole.find_by(name: 'Owner')
+      user_role: UserRole.find_by(name: I18n.t('user_roles.predefined.owner'))
     )
   end
 

app/models/project_member.rb

@@ -130,6 +130,6 @@ class ProjectMember
   def project_owners
     @project_owners ||= @project.user_assignments
                                 .includes(:user_role)
-                                .where(user_roles: { name: 'Owner' })
+                                .where(user_roles: { name: I18n.t('user_roles.predefined.owner') })
   end
 end

app/models/user_role.rb

@@ -84,7 +84,7 @@ class UserRole < ApplicationRecord
   end
 
   def owner?
-    name == 'Owner'
+    name == I18n.t('user_roles.predefined.owner')
   end
 
   private

app/models/user_team.rb

@@ -9,8 +9,8 @@ class UserTeam < ApplicationRecord
   belongs_to :assigned_by, foreign_key: 'assigned_by_id', class_name: 'User', optional: true
   belongs_to :team, inverse_of: :user_teams
 
-  before_destroy :destroy_associations
   after_create :assign_user_to_visible_projects
+  before_destroy :destroy_associations
 
   def role_str
     I18n.t("user_teams.enums.role.#{role}")

features/support/env.rb

@@ -54,31 +54,6 @@ Capybara.javascript_driver = :chrome
 #
 ActionController::Base.allow_rescue = false
 
-# Remove/comment out the lines below if your app doesn't have a database.
-# For some databases (like MongoDB and CouchDB) you may need to use :truncation instead.
-begin
-  require 'database_cleaner'
-  require 'database_cleaner/cucumber'
-  DatabaseCleaner.strategy = :truncation
-rescue NameError
-  raise "You need to add database_cleaner to your Gemfile (in the :test group) if you wish to use it."
-end
-
-# You may also want to configure DatabaseCleaner to use different strategies for certain features and scenarios.
-# See the DatabaseCleaner documentation for details. Example:
-#
-#   Before('@no-txn,@selenium,@culerity,@celerity,@javascript') do
-#     # { except: [:widgets] } may not do what you expect here
-#     # as Cucumber::Rails::Database.javascript_strategy overrides
-#     # this setting.
-#     DatabaseCleaner.strategy = :truncation
-#   end
-#
-#   Before('not @no-txn', 'not @selenium', 'not @culerity', 'not @celerity', 'not @javascript') do
-#     DatabaseCleaner.strategy = :transaction
-#   end
-#
-
 # Possible values are :truncation and :transaction
 # The :transaction strategy is faster, but might give you threading problems.
 # See https://github.com/cucumber/cucumber-rails/blob/master/features/choose_javascript_database_strategy.feature

spec/controllers/projects_controller_spec.rb

@@ -4,65 +4,11 @@ require 'rails_helper'
 
 describe ProjectsController, type: :controller do
   login_user
-  render_views
 
-  PROJECTS_CNT = 26
-  time = Time.new(2015, 8, 1, 14, 35, 0)
-  let!(:user) { User.first }
-  let!(:team) { create :team, created_by: user }
-  let!(:user_team) { create :user_team, team: team, user: user }
-  let!(:owner_role) { create :owner_role }
-  before do
-    @projects_overview = ProjectsOverviewService.new(team, user, nil, params)
-  end
-
-  let!(:project_1) do
-    create :project, name: 'test project D', visibility: 1, team: team,
-                     archived: false, created_at: time.advance(hours: 2),
-                     created_by: user
-  end
-  let!(:project_2) do
-    create :project, name: 'test project B', visibility: 1, team: team,
-                     archived: true, created_at: time, created_by: user
-  end
-  let!(:project_3) do
-    create :project, name: 'test project C', visibility: 1, team: team,
-                     archived: false, created_at: time.advance(hours: 3),
-                     created_by: user
-  end
-  let!(:project_4) do
-    create :project, name: 'test project A', visibility: 1, team: team,
-                     archived: true, created_at: time.advance(hours: 1),
-                     created_by: user
-  end
-  let!(:project_5) do
-    create :project, name: 'test project E', visibility: 1, team: team,
-                     archived: true, created_at: time.advance(hours: 5),
-                     created_by: user
-  end
-  let!(:project_6) do
-    create :project, name: 'test project F', visibility: 0, team: team,
-                     archived: false, created_at: time.advance(hours: 4),
-                     created_by: user
-  end
-  (7..PROJECTS_CNT).each do |i|
-    let!("project_#{i}") do
-      create :project, name: "test project #{(64 + i).chr}",
-                       visibility: 1,
-                       team: team, archived: i % 2,
-                       created_at: time.advance(hours: 6, minutes: i),
-                       created_by: user
-    end
-  end
-
-
-
-
-  before do
-    PROJECTS_CNT.times do |i|
-      create_user_assignment(public_send("project_#{i+1}"), owner_role, user)
-    end
-  end
+  include_context 'reference_project_structure', {
+    projects: 3,
+    skip_my_module: true
+  }
 
   describe '#index' do
     let(:params) { { team: team.id, sort: 'atoz' } }
@@ -107,7 +53,7 @@ describe ProjectsController, type: :controller do
 
   describe '#edit' do
     context 'in JSON format' do
-      let(:params) { { id: project_1.id } }
+      let(:params) { { id: projects.first.id } }
 
       it 'returns success response' do
         get :edit, params: params, format: :json
@@ -121,9 +67,9 @@ describe ProjectsController, type: :controller do
     context 'in HTML format' do
       let(:action) { put :update, params: params }
       let(:params) do
-        { id: project_1.id,
-          project: { name: project_1.name, team_id: project_1.team.id,
-                     visibility: project_1.visibility } }
+        { id: projects.first.id,
+          project: { name: projects.first.name, team_id: projects.first.team.id,
+                     visibility: projects.first.visibility } }
       end
 
       it 'returns redirect response' do
@@ -133,7 +79,7 @@ describe ProjectsController, type: :controller do
       end
 
       it 'calls create activity service (change_project_visibility)' do
-        params[:project][:visibility] = 'hidden'
+        params[:project][:visibility] = 'visible'
         expect(Activities::CreateActivityService).to receive(:call)
           .with(hash_including(activity_type: :change_project_visibility))
         action
@@ -147,7 +93,7 @@ describe ProjectsController, type: :controller do
       end
 
       it 'calls create activity service (restore_project)' do
-        project_1.update(archived: true)
+        projects.first.update(archived: true)
         params[:project][:archived] = false
         expect(Activities::CreateActivityService).to receive(:call)
           .with(hash_including(activity_type: :restore_project))
@@ -172,7 +118,7 @@ describe ProjectsController, type: :controller do
   describe '#show' do
     context 'in HTML format' do
       let(:params) do
-        { id: project_1.id, sort: 'old',
+        { id: projects.first.id, sort: 'old',
           project: { name: 'test project A1', team_id: team.id,
                      visibility: 'visible' } }
       end
@@ -188,7 +134,7 @@ describe ProjectsController, type: :controller do
   describe '#notifications' do
     context 'in JSON format' do
       let(:params) do
-        { id: project_1.id,
+        { id: projects.first.id,
           project: { name: 'test project A1', team_id: team.id,
                      visibility: 'visible' } }
       end
@@ -204,7 +150,7 @@ describe ProjectsController, type: :controller do
   describe '#experiment_archive' do
     context 'in HTML format' do
       let(:params) do
-        { id: project_1.id,
+        { id: projects.first.id,
           view_mode: :archived,
           project: { name: 'test project A1', team_id: team.id,
                      visibility: 'visible' } }

spec/permissions/controllers/project_comments_controller_spec.rb

@@ -0,0 +1,48 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe ProjectCommentsController, type: :controller do
+  include PermissionExtends
+
+  it_behaves_like "a controller with authentication", {
+    index: { project_id: 1, id: 1 },
+    create: { project_id: 1, id: 1 },
+    update: { project_id: 1, id: 1 },
+    destroy: { project_id: 1, id: 1 }
+  }, []
+
+  login_user
+
+  describe 'permissions checking' do
+    include_context 'reference_project_structure', {
+      team_role: :normal_user,
+      project_comment: true,
+      skip_my_module: true
+    }
+
+    it_behaves_like "a controller action with permissions checking", :get, :index do
+      let(:testable) { project }
+      let(:permissions) { [ProjectPermissions::READ] }
+      let(:action_params) { { project_id: project.id } }
+    end
+
+    it_behaves_like "a controller action with permissions checking", :post, :create do
+      let(:testable) { project }
+      let(:permissions) { [ProjectPermissions::COMMENTS_CREATE] }
+      let(:action_params) { { project_id: project.id, comment: { message: 'Test' } } }
+    end
+
+    it_behaves_like "a controller action with permissions checking", :put, :update do
+      let(:testable) { project }
+      let(:permissions) { [ProjectPermissions::COMMENTS_MANAGE] }
+      let(:action_params) { { project_id: project.id, id: project_comment.id, comment: { message: 'Test1' } } }
+    end
+
+    it_behaves_like "a controller action with permissions checking", :post, :destroy do
+      let(:testable) { project }
+      let(:permissions) { [ProjectPermissions::COMMENTS_MANAGE] }
+      let(:action_params) { { project_id: project.id, id: project_comment.id } }
+    end
+  end
+end

spec/permissions/controllers/projects_controller_spec.rb

@@ -0,0 +1,77 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe ProjectsController, type: :controller do
+  include PermissionExtends
+
+  it_behaves_like "a controller with authentication", {
+    sidebar: { id: 1 },
+    edit: { id: 1 },
+    update: { id: 1 },
+    show: { id: 1 },
+    experiments_cards: { id: 1 },
+    notifications: { id: 1 },
+    view_type: { id: 1 }
+  }, [:current_folder]
+
+  login_user
+
+  describe 'permissions checking' do
+    include_context 'reference_project_structure', {
+      team_role: :normal_user,
+      projects: 5,
+      skip_my_module: true
+    }
+
+    it_behaves_like "a controller action with permissions checking", :get, :sidebar do
+      let(:testable) { project }
+      let(:permissions) { [ProjectPermissions::READ] }
+      let(:action_params) { { id: project.id } }
+    end
+
+    it_behaves_like "a controller action with permissions checking", :put, :update do
+      let(:testable) { project }
+      let(:permissions) { [ProjectPermissions::MANAGE] }
+      let(:action_params) { { id: project.id, project: { name: 'Test' } } }
+    end
+
+    it_behaves_like "a controller action with permissions checking", :post, :archive_group do
+      let(:testable) { project }
+      let(:permissions) { [ProjectPermissions::MANAGE] }
+      let(:action_params) { { projects_ids: [project.id] } }
+      let(:custom_response_status) { :unprocessable_entity }
+    end
+
+    it_behaves_like "a controller action with permissions checking", :post, :restore_group do
+      let(:testable) { project }
+      let(:permissions) { [ProjectPermissions::MANAGE] }
+      let(:action_params) { { projects_ids: [project.id] } }
+      let(:custom_response_status) { :unprocessable_entity }
+    end
+
+    it_behaves_like "a controller action with permissions checking", :get, :show do
+      let(:testable) { project }
+      let(:permissions) { [ProjectPermissions::READ] }
+      let(:action_params) { { id: project.id } }
+    end
+
+    it_behaves_like "a controller action with permissions checking", :get, :experiments_cards do
+      let(:testable) { project }
+      let(:permissions) { [ProjectPermissions::READ] }
+      let(:action_params) { { id: project.id } }
+    end
+
+    it_behaves_like "a controller action with permissions checking", :get, :notifications do
+      let(:testable) { project }
+      let(:permissions) { [ProjectPermissions::READ] }
+      let(:action_params) { { id: project.id } }
+    end
+
+    it_behaves_like "a controller action with permissions checking", :put, :view_type do
+      let(:testable) { project }
+      let(:permissions) { [ProjectPermissions::READ] }
+      let(:action_params) { { id: project.id } }
+    end
+  end
+end

spec/rails_helper.rb

@@ -1,7 +1,6 @@
 # This file is copied to spec/ when you run 'rails generate rspec:install'
 require 'spec_helper'
 require 'shoulda-matchers'
-require 'database_cleaner'
 require 'devise'
 require_relative 'support/controller_macros'
 ENV['RAILS_ENV'] = 'test'
@@ -40,50 +39,10 @@ RSpec.configure do |config|
   # Remove this line if you're not using ActiveRecord or ActiveRecord fixtures
   config.fixture_path = "#{::Rails.root}/spec/fixtures"
 
-  # If you're not using ActiveRecord, or you'd prefer not to run each of your
-  # examples within a transaction, remove the following line or assign false
-  # instead of true.
-  # http://www.virtuouscode.com/2012/08/31/configuring-database_cleaner-with-rails-rspec-capybara-and-selenium/
-  config.use_transactional_fixtures = false
-  config.before(:suite) do
-    DatabaseCleaner.clean_with(:truncation)
-  end
+  config.use_transactional_fixtures = true
 
-  config.before(:each) do
-    DatabaseCleaner.strategy = :transaction
-  end
+  Delayed::Worker.delay_jobs = false
 
-  config.before(:each, js: true) do
-    DatabaseCleaner.strategy = :truncation
-  end
-
-  config.before(:each) do
-    DatabaseCleaner.start
-  end
-
-  config.after(:each) do
-    DatabaseCleaner.clean
-  end
-
-  config.before(:all) do
-    DatabaseCleaner.strategy = :transaction
-  end
-
-  config.before(:all) do
-    DatabaseCleaner.start
-  end
-
-  config.after(:all) do
-    DatabaseCleaner.clean
-  end
-
-  config.around(:each, type: :background_job) do |example|
-    run_background_jobs_immediately do
-      example.run
-    end
-  end
-
-  config.include BackgroundJobs
   # RSpec Rails can automatically mix in different behaviours to your tests
   # based on their file location, for example enabling you to call `get` and
   # `post` in specs under `spec/controllers`.
@@ -111,6 +70,7 @@ RSpec.configure do |config|
   end
   # Devise
   config.include Devise::Test::ControllerHelpers, type: :controller
+  config.include Devise::Test::IntegrationHelpers, type: :system
   config.include ApiHelper, type: :controller
   config.include ApiHelper, type: :request
   config.extend ControllerMacros, type: :controller

spec/services/notifications/handle_system_notification_in_communication_channel_service_spec.rb

@@ -12,14 +12,6 @@ describe Notifications::HandleSystemNotificationInCommunicationChannelService do
     Notifications::HandleSystemNotificationInCommunicationChannelService.call(system_notification)
   end
 
-  before do
-    Delayed::Worker.delay_jobs = false
-  end
-
-  after do
-    Delayed::Worker.delay_jobs = true
-  end
-
   context 'when user has enabled notifications' do
     it 'calls AppMailer' do
       allow_any_instance_of(User).to receive(:system_message_email_notification).and_return(true)

spec/services/notifications/sync_system_notifications_service_spec.rb

@@ -84,11 +84,9 @@ describe Notifications::SyncSystemNotificationsService do
     end
 
     it 'calls service to notify users about notification' do
-      Delayed::Worker.delay_jobs = false
       expect(Notifications::PushToCommunicationChannelService).to receive(:call).exactly(10)
 
       service_call
-      Delayed::Worker.delay_jobs = true
     end
   end
 
@@ -113,11 +111,9 @@ describe Notifications::SyncSystemNotificationsService do
     end
 
     it 'does not call service to notify users about notification' do
-      Delayed::Worker.delay_jobs = false
       expect(Notifications::PushToCommunicationChannelService).to_not receive(:call)
 
       service_call
-      Delayed::Worker.delay_jobs = true
     end
   end
 end

spec/support/background_job.rb

@@ -1,8 +0,0 @@
-module BackgroundJobs
-  def run_background_jobs_immediately
-    delay_jobs = Delayed::Worker.delay_jobs
-    Delayed::Worker.delay_jobs = false
-    yield
-    Delayed::Worker.delay_jobs = delay_jobs
-  end
-end

spec/support/reference_project_structure_context.rb

@@ -13,15 +13,20 @@
 #  }
 
 RSpec.shared_context 'reference_project_structure' do |config|
-
   config ||= {}
-  let(:user) { subject.current_user }
-  let(:role) { create (config[:role] || :owner_role) } unless config[:skip_role]
+  let!(:user) { subject.current_user }
+  let!(:role) { create (config[:role] || :owner_role) }
   let!(:team) { config[:team] || (create :team, created_by: user) }
-  let!(:user_team) { create :user_team, :admin, user: user, team: team }
-  let(:project) { create :project, team: team }
-  let(:experiment) { create :experiment, project: project }
-  let(:my_module) { create :my_module, experiment: experiment } unless config[:skip_my_module]
+  let!(:user_team) { create :user_team, config[:team_role] || :admin, user: user, team: team }
+
+  let!(:project) { create :project, team: team, created_by: user }
+  let!(:projects) { create_list :project, config[:projects], team: team, created_by: user } if config[:projects]
+
+  let!(:experiment) { create :experiment, project: project } unless config[:skip_experiment]
+  let!(:experiments) { create_list :experiment, config[:experiments], project: project } if config[:experiments]
+
+  let!(:my_module) { create :my_module, experiment: experiment } unless config[:skip_my_module]
+  let!(:my_modules) { create_list :my_module, config[:my_modules], experiment: experiment } if config[:my_modules]
 
   let(:tag) { create :tag, project: project} if config[:tag]
   let(:tags) { create_list :tag, config[:tags], project: project} if config[:tags]

spec/support/shared_examples/controller_action_with_permission_checking.rb

@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.shared_examples 'a controller action with permissions checking' do |verb, action|
+
+  describe 'controller action' do
+    context 'user without permissions' do
+      it "returns forbidden response for action :#{action}" do
+        testable_role = testable.user_assignments.find_by(user: user ).user_role
+        testable_role.update_column(:permissions, testable_role.permissions - permissions)
+        send(verb, action, params: defined?(action_params) ? action_params : {})
+        expect(response).to have_http_status(defined?(custom_response_status) ? custom_response_status : :forbidden)
+      end
+    end
+  end
+end

spec/support/shared_examples/controller_with_authentication.rb

@@ -0,0 +1,21 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.shared_examples 'a controller with authentication' do |actions_with_params, actions_to_skip|
+  let (:protected_actions) { described_class.instance_methods(false) - (actions_to_skip || []) }
+
+  describe 'controller actions' do
+    context 'unauthenticated user' do
+      it 'returns forbidden response for all actions' do
+        sign_out :user
+        actions_with_params ||= {}
+        protected_actions.each do |action|
+          params = actions_with_params[action]
+          get action, params: params
+          expect(response).to have_http_status(:forbidden).or redirect_to('/users/sign_in')
+        end
+      end
+    end
+  end
+end