#465: Update API controllers, specs

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

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
 

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

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

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

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.

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
+end

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
 

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

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

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)

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)

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

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)

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)

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

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?

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

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

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)

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