diff --git a/app/controllers/api/v1/groups_controller.rb b/app/controllers/api/v1/groups_controller.rb index 4ca344a1b..c9e8c6ea7 100644 --- a/app/controllers/api/v1/groups_controller.rb +++ b/app/controllers/api/v1/groups_controller.rb @@ -2,22 +2,21 @@ class Api::V1::GroupsController < Api::V1::BaseController before_action :authenticate_user! skip_before_action :authenticate_user!, only: [:show] if APP_CONFIG['anonymous_access'] - load_and_authorize_resource + before_action :load_group, except: %i(index create) def index - # accessible_by(current_ability) + authorize :group @groups = current_user.groups.paginate(paginate_params) - respond_to :json end def show - respond_to :json + authorize @group end def members + authorize @group @members = @group.members.where('actor_id != ?', @group.owner_id) .order('name').paginate(paginate_params) - respond_to :json end def update @@ -48,4 +47,11 @@ class Api::V1::GroupsController < Api::V1::BaseController update_member_in_subject @group, :actors end + private + + # Private: before_action hook which loads Group. + def load_group + @group = Group.find params[:id] + end + end diff --git a/app/controllers/api/v1/issues_controller.rb b/app/controllers/api/v1/issues_controller.rb index 4dc0e3802..fb4fbc79d 100644 --- a/app/controllers/api/v1/issues_controller.rb +++ b/app/controllers/api/v1/issues_controller.rb @@ -7,7 +7,7 @@ class Api::V1::IssuesController < Api::V1::BaseController before_action :load_group, only: :group_index before_action :load_project skip_before_action :load_project, only: %i(all_index user_index group_index) - before_action :load_issue, only: %i(show update create index) + before_action :load_issue, only: %i(show update index) def index @issues = @project.issues @@ -46,7 +46,7 @@ class Api::V1::IssuesController < Api::V1::BaseController def create @issue = @project.issues.new(params[:issue]) @issue.user = current_user - @issue.assignee = nil if cannot?(:write, @project) + @issue.assignee = nil unless policy(@project).write? create_subject @issue end diff --git a/app/controllers/api/v1/jobs_controller.rb b/app/controllers/api/v1/jobs_controller.rb index 915c19c18..d274f2b8b 100644 --- a/app/controllers/api/v1/jobs_controller.rb +++ b/app/controllers/api/v1/jobs_controller.rb @@ -21,8 +21,8 @@ class Api::V1::JobsController < Api::V1::BaseController @build_list ||= build_lists.external_nodes(:everything).first else @build_list = build_lists.external_nodes(:owned).for_user(current_user).first - @build_list ||= build_lists.external_nodes(:everything). - accessible_by(current_ability, :related).readonly(false).first + @build_list ||= BuildListPolicy::Scope.new(current_user, build_lists).owned. + external_nodes(:everything).readonly(false).first end set_builder end diff --git a/app/controllers/api/v1/maintainers_controller.rb b/app/controllers/api/v1/maintainers_controller.rb index a5045dbb8..2d0666fa2 100644 --- a/app/controllers/api/v1/maintainers_controller.rb +++ b/app/controllers/api/v1/maintainers_controller.rb @@ -1,12 +1,11 @@ class Api::V1::MaintainersController < Api::V1::BaseController before_action :authenticate_user! unless APP_CONFIG['anonymous_access'] - load_and_authorize_resource :platform def index + authorize @platform = Platform.find(params[:platform_id]), :show? @maintainers = BuildList::Package.includes(:project) .actual.by_platform(@platform) .like_name(params[:package_name]) .paginate(paginate_params) - respond_to :json end end diff --git a/app/controllers/api/v1/platforms_controller.rb b/app/controllers/api/v1/platforms_controller.rb index 1aeea6556..2d8a8fbcc 100644 --- a/app/controllers/api/v1/platforms_controller.rb +++ b/app/controllers/api/v1/platforms_controller.rb @@ -2,7 +2,7 @@ class Api::V1::PlatformsController < Api::V1::BaseController before_action :authenticate_user! skip_before_action :authenticate_user!, only: :allowed skip_before_action :authenticate_user!, only: [:show, :platforms_for_build, :members] if APP_CONFIG['anonymous_access'] - load_and_authorize_resource except: :allowed + before_action :load_platform, except: :allowed def allowed if request.authorization.present? @@ -16,20 +16,17 @@ class Api::V1::PlatformsController < Api::V1::BaseController end def index - @platforms = @platforms.accessible_by(current_ability, :related) - .by_type(params[:type]).paginate(paginate_params) - respond_to :json + authorize :platform + @platforms = PlatformPolicy::Scope.new(current_user, Platform).related. + by_type(params[:type]).paginate(paginate_params) end def show - respond_to :json end def platforms_for_build - @platforms = Platform.availables_main_platforms(current_user, current_ability).paginate(paginate_params) - respond_to do |format| - format.json { render :index } - end + @platforms = Platform.availables_main_platforms(current_user).paginate(paginate_params) + render :index end def create @@ -48,7 +45,6 @@ class Api::V1::PlatformsController < Api::V1::BaseController def members @members = @platform.members.order('name').paginate(paginate_params) - respond_to :json end def add_member @@ -79,4 +75,11 @@ class Api::V1::PlatformsController < Api::V1::BaseController destroy_subject @platform end + private + + # Private: before_action hook which loads Platform. + def load_platform + authorize @platform = Platform.find(params[:id]) + end + end diff --git a/app/controllers/api/v1/concerns/issueable.rb b/app/controllers/concerns/api/v1/issueable.rb similarity index 93% rename from app/controllers/api/v1/concerns/issueable.rb rename to app/controllers/concerns/api/v1/issueable.rb index 987718952..b64ef994c 100644 --- a/app/controllers/api/v1/concerns/issueable.rb +++ b/app/controllers/concerns/api/v1/issueable.rb @@ -17,7 +17,7 @@ module Api # Private: before_action hook which loads Issue. def load_issue - authorize @issue = @project.issues.find_by(serial_id: params[:id]), :show? + authorize @issue = @project.issues.find_by!(serial_id: params[:id]), :show? end # Private: Get membered projects. diff --git a/app/controllers/home_controller.rb b/app/controllers/home_controller.rb index a644eb8a1..ce20cb9b8 100644 --- a/app/controllers/home_controller.rb +++ b/app/controllers/home_controller.rb @@ -23,7 +23,7 @@ class HomeController < ApplicationController def issues @created_issues = current_user.issues @assigned_issues = Issue.where(assignee_id: current_user.id) - pr_ids = Project.accessible_by(current_ability, :membered).uniq.pluck(:id) + pr_ids = ProjectPolicy::Scope.new(current_user, Project).membered.uniq.pluck(:id) @all_issues = Issue.where(project_id: pr_ids) @created_issues, @assigned_issues, @all_issues = if action_name == 'issues' @@ -66,4 +66,4 @@ class HomeController < ApplicationController def pull_requests issues end -end \ No newline at end of file +end diff --git a/app/controllers/platforms/products_controller.rb b/app/controllers/platforms/products_controller.rb index 42eec8516..012e3e244 100644 --- a/app/controllers/platforms/products_controller.rb +++ b/app/controllers/platforms/products_controller.rb @@ -53,8 +53,8 @@ class Platforms::ProductsController < Platforms::BaseController end def autocomplete_project - @items = Project.accessible_by(current_ability, :membered) - .by_owner_and_name(params[:query]).limit(20) + @items = ProjectPolicy::Scope.new(current_user, Project).membered. + by_owner_and_name(params[:query]).limit(20) #items.select! {|e| e.repo.branches.count > 0} end diff --git a/app/controllers/projects/build_lists_controller.rb b/app/controllers/projects/build_lists_controller.rb index 22b8cd1ee..1d79f05c8 100644 --- a/app/controllers/projects/build_lists_controller.rb +++ b/app/controllers/projects/build_lists_controller.rb @@ -24,7 +24,7 @@ class Projects::BuildListsController < Projects::BaseController respond_to do |format| format.html format.json do - @filter = BuildList::Filter.new(@project, current_user, current_ability, params[:filter] || {}) + @filter = BuildList::Filter.new(@project, current_user, params[:filter] || {}) params[:page] = params[:page].to_i == 0 ? nil : params[:page] params[:per_page] = if BuildList::Filter::PER_PAGE.include? params[:per_page].to_i params[:per_page].to_i diff --git a/app/controllers/projects/pull_requests_controller.rb b/app/controllers/projects/pull_requests_controller.rb index 254391513..b4cdfe66f 100644 --- a/app/controllers/projects/pull_requests_controller.rb +++ b/app/controllers/projects/pull_requests_controller.rb @@ -106,7 +106,7 @@ class Projects::PullRequestsController < Projects::BaseController term = params[:query].to_s.strip.downcase [ Project.where(id: @project.pull_requests.last.try(:to_project_id)), @project.ancestors, - Project.accessible_by(current_ability, :membered) + ProjectPolicy::Scope.new(current_user, Project).membered ].each do |p| items.concat p.by_owner_and_name(term) end diff --git a/app/controllers/users/profile_controller.rb b/app/controllers/users/profile_controller.rb index c3470371b..66588dfd1 100644 --- a/app/controllers/users/profile_controller.rb +++ b/app/controllers/users/profile_controller.rb @@ -14,9 +14,9 @@ class Users::ProfileController < Users::BaseController when 'open' @projects = @projects.opened when 'hidden' - @projects = @projects.by_visibilities('hidden').accessible_by(current_ability, :read) + @projects = ProjectPolicy::Scope.new(current_user, @projects.by_visibilities('hidden')).read else - @projects = @projects.accessible_by(current_ability, :read) + @projects = ProjectPolicy::Scope.new(current_user, @projects).read end @total_items = @projects.count @projects = @projects.paginate(paginate_params) diff --git a/app/helpers/build_lists_helper.rb b/app/helpers/build_lists_helper.rb index 0ce5fa9a0..ea211e81b 100644 --- a/app/helpers/build_lists_helper.rb +++ b/app/helpers/build_lists_helper.rb @@ -20,7 +20,7 @@ module BuildListsHelper end def availables_main_platforms - Platform.availables_main_platforms current_user, current_ability + Platform.availables_main_platforms current_user end def dependent_projects(package) diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index 7175d00a2..639df967c 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -33,7 +33,7 @@ module ProjectsHelper def mass_import_repositories_for_group_select groups = {} - Platform.accessible_by(current_ability, :related).order(:name).each do |platform| + PlatformPolicy::Scope.new(current_user, Platform).related.order(:name).each do |platform| next unless policy(platform).local_admin_manage? groups[platform.name] = Repository.custom_sort(platform.repositories).map{ |r| [r.name, r.id] } end diff --git a/app/models/comment.rb b/app/models/comment.rb index 574e57c12..e513fb53e 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -134,7 +134,6 @@ class Comment < ActiveRecord::Base def self.create_link_on_issues_from_item item, commits = nil linker = item.user - current_ability = Ability.new(linker) case when item.is_a?(GitHook) diff --git a/app/models/platform.rb b/app/models/platform.rb index 7b4b12a61..1ef32ed4d 100644 --- a/app/models/platform.rb +++ b/app/models/platform.rb @@ -292,10 +292,9 @@ class Platform < ActiveRecord::Base Platform.main.where(automatic_metadata_regeneration: value).each(&:regenerate) end - def self.availables_main_platforms(user, ability = nil) + def self.availables_main_platforms(user) p_ids = Rails.cache.fetch([:availables_main_platforms, user], expires_in: 10.minutes) do - ability ||= Ability.new user - Platform.main.accessible_by(ability, :show).joins(:repositories). + PlatformPolicy::Scope.new(user, Platform).show.main.joins(:repositories). where('repositories.id IS NOT NULL').uniq.pluck(:id) end Platform.preload(:repositories).where(id: p_ids).order(:name) diff --git a/app/models/repository.rb b/app/models/repository.rb index 950f1eb94..f627beb25 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -75,14 +75,13 @@ class Repository < ActiveRecord::Base later :clone_relations, loner: true, queue: :low def add_projects(list, user) - current_ability = Ability.new(user) list.lines.each do |line| begin line.chomp!; line.strip! owner, name = line.split('/') next if owner.blank? || name.blank? - project = Project.where(owner_uname: owner, name: name).accessible_by(current_ability, :read).first + project = ProjectPolicy::Scope.new(user, Project).read.where(owner_uname: owner, name: name).first projects << project if project rescue RuntimeError, Exception end diff --git a/app/models/user.rb b/app/models/user.rb index a7d89fe1a..df2345177 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -161,8 +161,7 @@ class User < Avatar if target.is_a? Project assigned_issues.where(project_id: target.id).update_all(assignee_id: nil) else - ability = Ability.new self - project_ids = Project.accessible_by(ability, :membered).uniq.pluck(:id) + project_ids = ProjectPolicy::Scope.new(self, Project).membered.uniq.pluck(:id) issues = assigned_issues issues = issues.where('project_id not in (?)', project_ids) if project_ids.present? diff --git a/app/policies/platform_policy.rb b/app/policies/platform_policy.rb index 9843d7626..0c4382d9f 100644 --- a/app/policies/platform_policy.rb +++ b/app/policies/platform_policy.rb @@ -54,8 +54,7 @@ class PlatformPolicy < ApplicationPolicy class Scope < Scope def related - policy = Pundit.policy!(user, :platform) - scope.where <<-SQL, { user_id: user.id, user_group_ids: policy.user_group_ids, platform_ids: related_platform_ids } + scope.where <<-SQL, { user_id: policy.user.id, user_group_ids: policy.user_group_ids, platform_ids: related_platform_ids } ( platforms.id IN (:platform_ids) ) OR ( @@ -66,11 +65,29 @@ class PlatformPolicy < ApplicationPolicy SQL end + def show + scope.where <<-SQL, { user_id: policy.user.id, user_group_ids: policy.user_group_ids, platform_ids: related_platform_ids, visibility: Platform::VISIBILITY_OPEN } + ( + platforms.visibility = :visibility + ) OR ( + platforms.id IN (:platform_ids) + ) OR ( + platforms.owner_type = 'User' AND platforms.owner_id = :user_id + ) OR ( + platforms.owner_type = 'Group' AND platforms.owner_id IN (:user_group_ids) + ) + SQL + end + protected + def policy + @policy ||= Pundit.policy!(user, :platform) + end + def related_platform_ids - Rails.cache.fetch(['PlatformPolicy::Scope#related_platform_ids', user]) do - user.repositories.pluck(:platform_id) + Rails.cache.fetch(['PlatformPolicy::Scope#related_platform_ids', policy.user]) do + policy.user.repositories.pluck(:platform_id) end end end diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index b324a5cfa..e2e087001 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -63,8 +63,7 @@ class ProjectPolicy < ApplicationPolicy class Scope < Scope def membered - policy = Pundit.policy!(user, :project) - scope.where <<-SQL, { user_id: user.id, user_group_ids: policy.user_group_ids } + scope.where <<-SQL, { user_id: policy.user.id, user_group_ids: policy.user_group_ids } ( projects.owner_type = 'User' AND projects.owner_id = :user_id ) OR ( @@ -88,6 +87,40 @@ class ProjectPolicy < ApplicationPolicy ) SQL end + + def read + scope.where <<-SQL, { user_id: policy.user.id, user_group_ids: policy.user_group_ids } + ( + projects.visibility = 'open' + ) OR ( + projects.owner_type = 'User' AND projects.owner_id = :user_id + ) OR ( + projects.owner_type = 'Group' AND projects.owner_id IN (:user_group_ids) + ) OR ( + projects.id = ANY ( + ARRAY ( + SELECT target_id + FROM relations + INNER JOIN projects ON projects.id = relations.target_id + WHERE relations.target_type = 'Project' AND + ( + projects.owner_type = 'User' AND projects.owner_id != :user_id OR + projects.owner_type = 'Group' AND projects.owner_id NOT IN (:user_group_ids) + ) AND ( + relations.actor_type = 'User' AND relations.actor_id = :user_id OR + relations.actor_type = 'Group' AND relations.actor_id IN (:user_group_ids) + ) + ) + ) + ) + SQL + end + + protected + + def policy + @policy ||= Pundit.policy!(user, :project) + end end private diff --git a/spec/controllers/api/v1/groups_controller_spec.rb b/spec/controllers/api/v1/groups_controller_spec.rb index f83e5ef27..ffe77dd2b 100644 --- a/spec/controllers/api/v1/groups_controller_spec.rb +++ b/spec/controllers/api/v1/groups_controller_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' shared_examples_for 'api group user with reader rights' do it 'should be able to perform members action' do get :members, id: @group.id, format: :json - response.should be_success + expect(response).to be_success end it_should_behave_like 'api group user with show rights' end @@ -11,19 +11,19 @@ end shared_examples_for 'api group user with show rights' do it 'should be able to perform show action' do get :show, id: @group.id, format: :json - response.should be_success + expect(response).to be_success end it 'should be able to perform index action' do get :index, format: :json - response.should be_success + expect(response).to be_success end end shared_examples_for 'api group user without reader rights' do it 'should not be able to perform members action' do get :members, id: @group.id, format: :json - response.should_not be_success + expect(response).to_not be_success end end @@ -35,11 +35,11 @@ shared_examples_for 'api group user with admin rights' do end it 'should be able to perform update action' do - response.should be_success + expect(response).to be_success end it 'ensures that group has been updated' do @group.reload - @group.description.should == 'new description' + expect(@group.description).to eq 'new description' end end @@ -50,10 +50,10 @@ shared_examples_for 'api group user with admin rights' do end it 'should be able to perform add_member action' do - response.should be_success + expect(response).to be_success end it 'ensures that new member has been added to group' do - @group.members.should include(member) + expect(@group.members).to include(member) end end @@ -65,10 +65,10 @@ shared_examples_for 'api group user with admin rights' do end it 'should be able to perform remove_member action' do - response.should be_success + expect(response).to be_success end it 'ensures that member has been removed from group' do - @group.members.should_not include(member) + expect(@group.members).to_not include(member) end end @@ -80,11 +80,11 @@ shared_examples_for 'api group user with admin rights' do end it 'should be able to perform update_member action' do - response.should be_success + expect(response).to be_success end it 'ensures that member role has been updated in group' do - @group.actors.where(actor_id: member, actor_type: 'User').first. - role.should == 'reader' + role = @group.actors.where(actor_id: member, actor_type: 'User').first.role + expect(role).to eq 'reader' end end end @@ -93,10 +93,12 @@ shared_examples_for 'api group user with owner rights' do context 'api group user with destroy rights' do it 'should be able to perform destroy action' do delete :destroy, id: @group.id, format: :json - response.should be_success + expect(response).to be_success end it 'ensures that group has been destroyed' do - lambda { delete :destroy, id: @group.id, format: :json }.should change{ Group.count }.by(-1) + expect do + delete :destroy, id: @group.id, format: :json + end.to change(Group, :count).by(-1) end end end @@ -110,11 +112,11 @@ shared_examples_for 'api group user without admin rights' do end it 'should not be able to perform update_member action' do - response.should_not be_success + expect(response).to_not be_success end it 'ensures that member role has not been updated in group' do - @group.actors.where(actor_id: member, actor_type: 'User').first. - role.should_not == 'reader' + role = @group.actors.where(actor_id: member, actor_type: 'User').first.role + expect(role).to_not eq 'reader' end end @@ -124,11 +126,10 @@ shared_examples_for 'api group user without admin rights' do end it 'should not be able to perform update action' do - response.should_not be_success + expect(response).to_not be_success end it 'ensures that platform has not been updated' do - @group.reload - @group.description.should_not == 'new description' + expect(@group.reload.description).to_not eq 'new description' end end @@ -139,10 +140,10 @@ shared_examples_for 'api group user without admin rights' do end it 'should not be able to perform add_member action' do - response.should_not be_success + expect(response).to_not be_success end it 'ensures that new member has not been added to group' do - @group.members.should_not include(member) + expect(@group.members).to_not include(member) end end @@ -154,10 +155,10 @@ shared_examples_for 'api group user without admin rights' do end it 'should be able to perform update action' do - response.should_not be_success + expect(response).to_not be_success end it 'ensures that member has not been removed from group' do - @group.members.should include(member) + expect(@group.members).to include(member) end end @@ -167,10 +168,12 @@ shared_examples_for 'api group user without owner rights' do context 'api group user without destroy rights' do it 'should not be able to perform destroy action' do delete :destroy, id: @group.id, format: :json - response.should_not be_success + expect(response).to_not be_success end it 'ensures that group has not been destroyed' do - lambda { delete :destroy, id: @group.id, format: :json }.should_not change{ Group.count } + expect do + delete :destroy, id: @group.id, format: :json + end.to_not change(Group, :count) end end end @@ -187,27 +190,29 @@ describe Api::V1::GroupsController, type: :controller do it "should not be able to perform index action" do get :index, format: :json - response.status.should == 401 + expect(response.status).to eq 401 end it "should not be able to perform show action", :anonymous_access => false do get :show, id: @group.id, format: :json - response.status.should == 401 + expect(response.status).to eq 401 end it "should be able to perform show action", :anonymous_access => true do get :show, id: @group.id, format: :json - response.should be_success + expect(response).to be_success end context 'api group user without create rights' do let(:params) { {group: {uname: 'test_uname'}} } it 'should not be able to perform create action' do post :create, params, format: :json - response.should_not be_success + expect(response).to_not be_success end it 'ensures that group has not been created' do - lambda { post :create, params, format: :json }.should_not change{ Group.count } + expect do + post :create, params, format: :json + end.to_not change(Group, :count) end end @@ -216,17 +221,6 @@ describe Api::V1::GroupsController, type: :controller do it_should_behave_like 'api group user without owner rights' end - context 'for global admin' do - before do - @admin = FactoryGirl.create(:admin) - http_login(@admin) - end - - it_should_behave_like 'api group user with reader rights' - it_should_behave_like 'api group user with admin rights' - it_should_behave_like 'api group user with owner rights' - end - context 'for owner user' do before do @group = FactoryGirl.create(:group, owner: @user) diff --git a/spec/controllers/api/v1/maintainers_controller_spec.rb b/spec/controllers/api/v1/maintainers_controller_spec.rb index ba843f348..ef6111183 100644 --- a/spec/controllers/api/v1/maintainers_controller_spec.rb +++ b/spec/controllers/api/v1/maintainers_controller_spec.rb @@ -3,19 +3,19 @@ require 'spec_helper' shared_examples_for 'api maintainers user with reader rights' do it 'should be able to perform index action' do get :index, platform_id: package.platform_id, format: :json - should render_template(:index) + expect(response).to render_template(:index) end it 'loads all of the maintainers into @maintainers' do get :index, platform_id: package.platform_id, format: :json expect(assigns(:maintainers).count).to eq 2 - assigns(:maintainers).should include(package, package2) + expect(assigns :maintainers).to include(package, package2) end it 'loads all of the maintainers into @maintainers when search by name' do get :index, platform_id: package.platform_id, package_name: 'package1', format: :json expect(assigns(:maintainers).count).to eq 1 - assigns(:maintainers).should include(package) + expect(assigns :maintainers).to include(package) end end @@ -35,7 +35,7 @@ describe Api::V1::MaintainersController, type: :controller do else it 'should not be able to perform index action', :anonymous_access => false do get :index, platform_id: package.platform_id, format: :json - response.status.should == 401 + expect(response.status).to eq 401 end end end