[Done] Add BDEFGI Policies and Specs (#487)

* Add comment for public controllers

* Add block policy and specs

* Add policy for authorizing dashboard

* Add follow policy and specs

* Refactor a tiny bit

* Prevent banned users from following anything

* Add a note for email sub controller about auth

* Add image upload policies

* Add policy for github repos

* Fix typo and use correct github repo variable

* Fix image uploader and use regular params

* Add authenticate_user before action back in

* Rename test

* Update slack bot message formatting and fix reported URL

app/controllers/badges_controller.rb

@@ -1,5 +1,7 @@
 class BadgesController < ApplicationController
   before_action :set_cache_control_headers, only: [:show]
+  # No authorization required for entirely public controller
+
   def show
     @badge = Badge.find_by_slug(params[:slug])
     set_surrogate_key_header "badges-show-action"

app/controllers/blocks_controller.rb

@@ -1,36 +1,40 @@
 class BlocksController < ApplicationController
   before_action :set_block, only: [:show, :edit, :update, :destroy]
-
-  before_action :require_super_admin
+  after_action :verify_authorized
 
   # GET /blocks
   # GET /blocks.json
   def index
+    authorize Block
     @blocks = Block.order("index_position ASC")
   end
 
   # GET /blocks/1
   # GET /blocks/1.json
   def show
+    authorize @block
   end
 
   # GET /blocks/new
   def new
+    authorize Block
     @block = Block.new
   end
 
   # GET /blocks/1/edit
   def edit
+    authorize @block
   end
 
   # POST /blocks
   # POST /blocks.json
   def create
-    @block = Block.new(block_params)
+    authorize Block
+    @block = Block.new(permitted_attributes(Block))
     @block.user_id = current_user.id
     respond_to do |format|
       if @block.save
-        format.html { redirect_to @block, notice: 'Block was successfully created.' }
+        format.html { redirect_to @block, notice: "Block was successfully created." }
         format.json { render :show, status: :created, location: @block }
       else
         format.html { render :new }
@@ -42,12 +46,13 @@ class BlocksController < ApplicationController
   # PATCH/PUT /blocks/1
   # PATCH/PUT /blocks/1.json
   def update
+    authorize @block
     respond_to do |format|
-      if @block.update(block_params)
-        if block_params[:publish_now]
+      if @block.update(permitted_attributes(@block))
+        if permitted_attributes(@block)[:publish_now]
           @block.publish!
         end
-        format.html { redirect_to @block, notice: 'Block was successfully updated.' }
+        format.html { redirect_to @block, notice: "Block was successfully updated." }
         format.json { render :show, status: :ok, location: @block }
       else
         format.html { render :edit }
@@ -59,26 +64,18 @@ class BlocksController < ApplicationController
   # DELETE /blocks/1
   # DELETE /blocks/1.json
   def destroy
+    authorize @block
     @block.destroy
     respond_to do |format|
-      format.html { redirect_to blocks_url, notice: 'Block was successfully destroyed.' }
+      format.html { redirect_to blocks_url, notice: "Block was successfully destroyed." }
       format.json { head :no_content }
     end
   end
 
   private
-    # Use callbacks to share common setup or constraints between actions.
-    def set_block
-      @block = Block.find(params[:id])
-    end
 
-    # Never trust parameters from the scary internet, only allow the white list through.
-    def block_params
-      params.require(:block).permit(:input_html,
-                                      :input_css,
-                                      :input_javascript,
-                                      :featured,
-                                      :index_position,
-                                      :publish_now)
-    end
+  # Use callbacks to share common setup or constraints between actions.
+  def set_block
+    @block = Block.find(params[:id])
+  end
 end

app/controllers/dashboards_controller.rb

@@ -1,12 +1,15 @@
 class DashboardsController < ApplicationController
   before_action :set_no_cache_header
   before_action :authenticate_user!
+  after_action :verify_authorized
+
   def show
     @user = if params[:username] && current_user_is_admin?
               User.find_by_username(params[:username])
             else
               current_user
             end
+    authorize (@user || User), :dashboard_show?
     if params[:which] == "following_users"
       @follows = @user.follows_by_type("User").
         order("created_at DESC").includes(:followable).limit(80)

app/controllers/email_subscriptions_controller.rb

@@ -1,4 +1,5 @@
 class EmailSubscriptionsController < ApplicationController
+  # No need to authorize this because its implicit when unsubbing
   def unsubscribe
     verified_params = Rails.application.message_verifier(:unsubscribe).verify(params[:ut])
 

app/controllers/events_controller.rb

@@ -1,5 +1,7 @@
 class EventsController < ApplicationController
   before_action :set_cache_control_headers, only: [:index]
+  # No authorization required for entirely public controller
+
   def index
     @events = Event.in_the_future_and_published.sort_by(&:starts_at)
     @past_events = Event.in_the_past_and_published.sort_by(&:starts_at)

app/controllers/feedback_messages_controller.rb

@@ -1,4 +1,5 @@
 class FeedbackMessagesController < ApplicationController
+  # No authorization required for entirely public controller
   skip_before_action :verify_authenticity_token
 
   def create
@@ -42,19 +43,18 @@ class FeedbackMessagesController < ApplicationController
     <<~HEREDOC
       #{generate_user_detail}
       Category: #{feedback_message_params[:category]}
-      Message: #{feedback_message_params[:message]}
-      URL: #{params[:reported_url]}
+      *_Reported URL: #{feedback_message_params[:reported_url]}_*
+      -----
+      *Message:* #{feedback_message_params[:message]}
     HEREDOC
   end
 
   def generate_user_detail
-    return "" unless current_user
+    return "*Anonymous report:*" unless current_user
     <<~HEREDOC
       *Logged in user:*
-      username: #{current_user.username} - https://dev.to/#{current_user.username}
+      reporter: #{current_user.username} - https://dev.to/#{current_user.username}
       email: <mailto:#{current_user.email}|#{current_user.email}>
-      twitter: #{current_user.twitter_username}
-      github: #{current_user.github_username}
     HEREDOC
   end
 

app/controllers/follows_controller.rb

@@ -1,6 +1,8 @@
 class FollowsController < ApplicationController
+  after_action :verify_authorized
 
   def show
+    skip_authorization
     unless current_user
       render plain: "not-logged-in"
       return
@@ -13,20 +15,21 @@ class FollowsController < ApplicationController
   end
 
   def create
-    if params[:followable_type] == "Organization"
-      followable = Organization.find(params[:followable_id])
-    elsif params[:followable_type] == "Tag"
-      followable = Tag.find(params[:followable_id])
-    else
-      followable = User.find(params[:followable_id])
-    end
-    if params[:verb] == "unfollow"
-      current_user.stop_following(followable)
-      @result = "unfollowed"
-    else
-      current_user.follow(followable)
-      @result = "followed"
-    end
+    authorize Follow
+    followable = if params[:followable_type] == "Organization"
+                   Organization.find(params[:followable_id])
+                 elsif params[:followable_type] == "Tag"
+                   Tag.find(params[:followable_id])
+                 else
+                   User.find(params[:followable_id])
+                 end
+    @result = if params[:verb] == "unfollow"
+                current_user.stop_following(followable)
+                "unfollowed"
+              else
+                current_user.follow(followable)
+                "followed"
+              end
     current_user.save
     current_user.touch
     render json: { outcome: @result }

app/controllers/github_repos_controller.rb

@@ -1,5 +1,8 @@
 class GithubReposController < ApplicationController
+  after_action :verify_authorized
+
   def create
+    authorize GithubRepo
     @client = create_octokit_client
     @repo = GithubRepo.find_or_create(fetched_repo_params)
     if @repo.valid?
@@ -12,6 +15,7 @@ class GithubReposController < ApplicationController
 
   def update
     @repo = GithubRepo.find(params[:id])
+    authorize @repo
     if @repo.update(featured: false)
       redirect_to "/settings/integrations", notice: "GitHub repo added"
     else
@@ -31,7 +35,7 @@ class GithubReposController < ApplicationController
 
   def fetched_repo_params
     fetched_repo = @client.repositories.select do |repo|
-      repo.id == github_repo_params[:github_id_code].to_i
+      repo.id == permitted_attributes(GithubRepo)[:github_id_code].to_i
     end.first
     {
       github_id_code: fetched_repo.id,
@@ -48,8 +52,4 @@ class GithubReposController < ApplicationController
       info_hash: fetched_repo.to_hash,
     }
   end
-
-  def github_repo_params
-    params.require(:github_repo).permit(:github_id_code)
-  end
 end

app/controllers/image_uploads_controller.rb

@@ -1,7 +1,9 @@
 class ImageUploadsController < ApplicationController
   before_action :authenticate_user!
+  after_action :verify_authorized
 
   def create
+    authorize :image_upload
     uploader = ArticleImageUploader.new
     uploader.store!(params[:image])
     link = uploader.url

app/controllers/pages_controller.rb

@@ -1,4 +1,5 @@
 class PagesController < ApplicationController
+  # No authorization required for entirely public controller
   before_action :set_cache_control_headers, only: [:rlyweb, :now, :events, :membership]
 
   def now

app/labor/follow_checker.rb

@@ -10,13 +10,13 @@ class FollowChecker
 
   def cached_follow_check
     Rails.cache.fetch("user-#{follower.id}-#{follower.updated_at}/is_following_#{followable_type}_#{followable_id}", expires_in: 100.hours) do
-      if followable_type == "Tag"
-        followable = Tag.find(followable_id)
-      elsif followable_type == "Organization"
-        followable = Organization.find(followable_id)
-      else
-        followable = User.find(followable_id)
-      end
+      followable = if followable_type == "Tag"
+                     Tag.find(followable_id)
+                   elsif followable_type == "Organization"
+                     Organization.find(followable_id)
+                   else
+                     User.find(followable_id)
+                   end
       follower.following?(followable)
     end
   end

app/policies/block_policy.rb

@@ -0,0 +1,39 @@
+class BlockPolicy < ApplicationPolicy
+  def index?
+    user_is_admin?
+  end
+
+  def show?
+    user_is_admin?
+  end
+
+  def new?
+    user_is_admin?
+  end
+
+  def edit?
+    user_is_admin?
+  end
+
+  def create?
+    user_is_admin?
+  end
+
+  def update?
+    user_is_admin?
+  end
+
+  def destroy?
+    user_is_admin?
+  end
+
+  def permitted_attributes
+    %i[input_html input_css input_javascript featured index_position publish_now]
+  end
+
+  private
+
+  def user_is_admin?
+    user.has_role? :super_admin
+  end
+end

app/policies/follow_policy.rb

@@ -0,0 +1,5 @@
+class FollowPolicy < ApplicationPolicy
+  def create?
+    !user.banned
+  end
+end

app/policies/github_repo_policy.rb

@@ -0,0 +1,19 @@
+class GithubRepoPolicy < ApplicationPolicy
+  def create?
+    !user.banned
+  end
+
+  def update?
+    !user.banned && user_is_owner?
+  end
+
+  def permitted_attributes
+    %i[github_id_code]
+  end
+
+  private
+
+  def user_is_owner?
+    record.user_id == user.id
+  end
+end

app/policies/image_upload_policy.rb

@@ -0,0 +1,5 @@
+class ImageUploadPolicy < ApplicationPolicy
+  def create?
+    !user.banned
+  end
+end

app/policies/user_policy.rb

@@ -31,6 +31,10 @@ class UserPolicy < ApplicationPolicy
     user.org_admin && not_self? && within_the_same_org?
   end
 
+  def dashboard_show?
+    current_user? || user_is_admin?
+  end
+
   private
 
   def within_the_same_org?
@@ -40,4 +44,12 @@ class UserPolicy < ApplicationPolicy
   def not_self?
     user != record
   end
+
+  def current_user?
+    user == record
+  end
+
+  def user_is_admin?
+    user.has_role? :super_admin
+  end
 end

config/routes.rb

@@ -81,7 +81,7 @@ Rails.application.routes.draw do
   get "/reports/:slug", to: "feedback_messages#show"
   resources :organizations, only: [:update, :create]
   resources :followed_articles, only: [:index]
-  resources :follows, only: [:index,:show,:create]
+  resources :follows, only: [:show,:create] 
   resources :giveaways, only: [:create,:update]
   resources :image_uploads, only: [:create]
   resources :blocks

spec/factories/github_repos.rb

@@ -1,5 +1,6 @@
 FactoryBot.define do
   factory :github_repo do
+    user
     name               { Faker::Book.title }
     url                { Faker::Internet.url }
     description        { Faker::Book.title }

spec/policies/block_policy_spec.rb

@@ -0,0 +1,32 @@
+require "rails_helper"
+
+RSpec.describe BlockPolicy do
+  subject { described_class.new(user, block) }
+
+  let(:block) { build(:block) }
+
+  let(:valid_attributes) do
+    %i[input_html input_css input_javascript featured index_position publish_now]
+  end
+  
+  context "when not signed in" do
+    let(:user) { nil }
+
+    it { within_block_is_expected.to raise_error(Pundit::NotAuthorizedError) }
+  end
+
+  context "when signed in as a regular user" do
+    let(:user) { build(:user) }
+
+    it { is_expected.to forbid_actions(%i[index show new edit create update destroy]) }
+  end
+
+  context "when user is signed in as a super admin" do
+    let(:user) { build(:user, :super_admin) }
+
+    before { login_as user }
+
+    it { is_expected.to permit_actions(%i[index show new edit create update destroy]) }
+    it { is_expected.to permit_mass_assignment_of(valid_attributes) }
+  end
+end

spec/policies/follow_policy_spec.rb

@@ -0,0 +1,23 @@
+require "rails_helper"
+
+RSpec.describe FollowPolicy do
+  subject { described_class.new(user, Follow) }
+
+  context "when user is not signed in" do
+    let(:user) { nil }
+
+    it { within_block_is_expected.to raise_error(Pundit::NotAuthorizedError) }
+  end
+
+  context "when user is signed in" do
+    let(:user) { build(:user) }
+
+    it { is_expected.to permit_actions(%i[create]) }
+
+    context "when user is banned" do
+      let(:user) { build(:user, :banned) }
+
+      it { is_expected.to forbid_actions(%i[create]) }
+    end
+  end
+end

spec/policies/github_repo_policy_spec.rb

@@ -0,0 +1,39 @@
+require "rails_helper"
+
+RSpec.describe GithubRepoPolicy do
+  subject { described_class.new(user, github_repo) }
+
+  let(:github_repo) { build(:github_repo) }
+  let(:valid_attributes) { %i[github_id_code] }
+
+  context "when user is not signed in" do
+    let(:user) { nil }
+
+    it { within_block_is_expected.to raise_error(Pundit::NotAuthorizedError) }
+  end
+
+  context "when user is not the owner" do
+    let(:user) { build(:user) }
+
+    it { is_expected.to permit_actions(%i[create]) }
+    it { is_expected.to forbid_actions(%i[update]) }
+
+    context "when user is banned" do
+      let(:user) { build(:user, :banned) }
+
+      it { is_expected.to forbid_actions(%i[create update]) }
+    end
+  end
+
+  context "when user is the owner" do
+    let(:user) { github_repo.user }
+
+    it { is_expected.to permit_actions(%i[create update]) }
+
+    context "when user is banned" do
+      let(:user) { build(:user, :banned) }
+
+      it { is_expected.to forbid_actions(%i[create update]) }
+    end
+  end
+end

spec/policies/image_upload_policy_spec.rb

@@ -0,0 +1,25 @@
+require "rails_helper"
+
+RSpec.describe ImageUploadPolicy do
+  subject { described_class.new(user, image) }
+  
+  let(:image) { "📸.jpg" }
+
+  context "when user is not signed in" do
+    let(:user) { nil }
+
+    it { within_block_is_expected.to raise_error(Pundit::NotAuthorizedError) }
+  end
+
+  context "when user is signed in" do
+    let(:user) { build(:user) }
+
+    it { is_expected.to permit_actions(%i[create]) }
+
+    context "when user is banned" do
+      let(:user) { build(:user, :banned) }
+
+      it { is_expected.to forbid_actions(%i[create]) }
+    end
+  end
+end

spec/policies/user_policy_spec.rb

@@ -14,7 +14,7 @@ RSpec.describe UserPolicy do
   context "when user is signed-in" do
     let(:user) { other_user }
 
-    it { is_expected.to permit_actions(%i[edit update onboarding_update join_org leave_org]) }
+    it { is_expected.to permit_actions(%i[edit update onboarding_update join_org leave_org dashboard_show]) }
 
     context "with banned status" do
       before { user.add_role(:banned) }
@@ -28,7 +28,6 @@ RSpec.describe UserPolicy do
     let(:other_org) { build(:organization) }
     let(:user) { build(:user, org_admin: true, organization: org) }
 
-
     context "with other_user as org_member of same org" do
       let(:other_user) { build(:user, organization: org) }
 

spec/requests/blocks_spec.rb

@@ -1,12 +1,9 @@
 require "rails_helper"
 
 RSpec.describe "Blocks", type: :request do
-  let(:user) { create(:user) }
+  let(:user) { create(:user, :super_admin) }
 
-  before do
-    user.add_role(:super_admin)
-    sign_in user
-  end
+  before { sign_in user }
 
   describe "GET blocks index" do
     it "renders proper blocks index" do
@@ -43,5 +40,4 @@ RSpec.describe "Blocks", type: :request do
       expect(Block.all.size).to eq(0)
     end
   end
-
 end

spec/requests/image_uploads_spec.rb

@@ -6,7 +6,7 @@ RSpec.describe "ImageUploads", type: :request do
     let(:headers) { { "Content-Type": "application/json", Accept: "application/json" } }
 
     context "when not logged-in" do
-      it "redirects to /enter" do
+      it "responds with 401" do
         post "/image_uploads", headers: headers
         expect(response).to have_http_status(401)
       end