From 94f77517a8948409ad9a60aec9a33aae80719f0e Mon Sep 17 00:00:00 2001
From: Riccardo Graziosi <31478034+riggraz@users.noreply.github.com>
Date: Fri, 10 Jun 2022 12:03:33 +0200
Subject: [PATCH] Improve rails controllers (#118)

---
 Gemfile                                       |  7 ++-
 Gemfile.lock                                  |  9 ++-
 app/controllers/application_controller.rb     | 12 ++++
 app/controllers/boards_controller.rb          |  9 ++-
 app/controllers/comments_controller.rb        | 56 ++++-------------
 app/controllers/post_statuses_controller.rb   |  7 ++-
 app/controllers/posts_controller.rb           |  8 +--
 app/helpers/boards_helper.rb                  |  2 -
 app/helpers/comments_helper.rb                |  2 -
 app/helpers/likes_helper.rb                   |  2 -
 app/helpers/posts_helper.rb                   |  2 -
 app/policies/application_policy.rb            | 53 ++++++++++++++++
 app/policies/board_policy.rb                  | 17 ++++++
 app/policies/comment_policy.rb                |  5 ++
 app/policies/post_policy.rb                   |  5 ++
 app/policies/post_status_policy.rb            | 17 ++++++
 .../SendNotificationForCommentWorkflow.rb     | 34 +++++++++++
 config/initializers/devise.rb                 |  2 +-
 spec/requests/posts_controller_auth_spec.rb   | 61 +++++++++++++++++++
 19 files changed, 244 insertions(+), 66 deletions(-)
 delete mode 100644 app/helpers/boards_helper.rb
 delete mode 100644 app/helpers/comments_helper.rb
 delete mode 100644 app/helpers/likes_helper.rb
 delete mode 100644 app/helpers/posts_helper.rb
 create mode 100644 app/policies/application_policy.rb
 create mode 100644 app/policies/board_policy.rb
 create mode 100644 app/policies/comment_policy.rb
 create mode 100644 app/policies/post_policy.rb
 create mode 100644 app/policies/post_status_policy.rb
 create mode 100644 app/workflows/SendNotificationForCommentWorkflow.rb
 create mode 100644 spec/requests/posts_controller_auth_spec.rb

diff --git a/Gemfile b/Gemfile
index 6d764bfb..df59d96f 100644
--- a/Gemfile
+++ b/Gemfile
@@ -4,7 +4,6 @@ git_source(:github) { |repo| "https://github.com/#{repo}.git" }
 ruby '2.6.6'
 
 gem 'rails', '6.0.4.7'
-gem 'i18n-js'
 
 gem 'pg', '>= 0.18', '< 2.0'
 
@@ -23,6 +22,12 @@ gem 'bootsnap', '>= 1.4.2', require: false
 # Authentication
 gem 'devise', '4.7.3'
 
+# Authorization
+gem 'pundit', '2.2.0'
+
+# I18n (forward locales to JS)
+gem 'i18n-js'
+
 # Administration panel
 gem "administrate", '0.16.0'
 
diff --git a/Gemfile.lock b/Gemfile.lock
index c68ff19a..16f4d648 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -146,18 +146,22 @@ GEM
     matrix (0.4.2)
     method_source (1.0.0)
     mini_mime (1.1.2)
+    mini_portile2 (2.8.0)
     minitest (5.15.0)
     momentjs-rails (2.29.1.1)
       railties (>= 3.1)
     msgpack (1.5.1)
     nio4r (2.5.8)
-    nokogiri (1.13.3-x86_64-linux)
+    nokogiri (1.13.3)
+      mini_portile2 (~> 2.8.0)
       racc (~> 1.4)
     orm_adapter (0.5.0)
     pg (1.3.5)
     public_suffix (4.0.6)
     puma (4.3.12)
       nio4r (~> 2.0)
+    pundit (2.2.0)
+      activesupport (>= 3.0.0)
     racc (1.6.0)
     rack (2.2.3)
     rack-proxy (0.7.2)
@@ -235,7 +239,7 @@ GEM
       sprockets (>= 2.8, < 4.0)
       sprockets-rails (>= 2.0, < 4.0)
       tilt (>= 1.1, < 3)
-    sassc (2.1.0-x86_64-linux)
+    sassc (2.1.0)
       ffi (~> 1.9)
     sassc-rails (2.1.2)
       railties (>= 4.0.0)
@@ -305,6 +309,7 @@ DEPENDENCIES
   listen (>= 3.0.5, < 3.2)
   pg (>= 0.18, < 2.0)
   puma (~> 4.3)
+  pundit (= 2.2.0)
   rails (= 6.0.4.7)
   react-rails (~> 2.6.0)
   rspec-rails (~> 3.8.2)
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 182b54ef..d71a8ec0 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -1,4 +1,8 @@
 class ApplicationController < ActionController::Base
+  include Pundit::Authorization
+
+  rescue_from Pundit::NotAuthorizedError, with: :user_not_authorized
+
   before_action :configure_permitted_parameters, if: :devise_controller?
   before_action :load_boards
 
@@ -12,4 +16,12 @@ class ApplicationController < ActionController::Base
     def load_boards
       @boards = Board.select(:id, :name).order(order: :asc)
     end
+
+  private
+
+    def user_not_authorized
+      render json: {
+        error: t('backend.errors.unauthorized')
+      }, status: :unauthorized
+    end
 end
diff --git a/app/controllers/boards_controller.rb b/app/controllers/boards_controller.rb
index aa558458..8796e69f 100644
--- a/app/controllers/boards_controller.rb
+++ b/app/controllers/boards_controller.rb
@@ -1,7 +1,7 @@
 class BoardsController < ApplicationController
   include ApplicationHelper
   
-  before_action :authenticate_admin, only: [:create, :update, :update_order, :destroy]
+  before_action :authenticate_user!, only: [:create, :update, :update_order, :destroy]
 
   def index
     boards = Board.order(order: :asc)
@@ -15,6 +15,7 @@ class BoardsController < ApplicationController
 
   def create
     board = Board.new(board_params)
+    authorize board
 
     if board.save
       render json: board, status: :created
@@ -27,13 +28,12 @@ class BoardsController < ApplicationController
 
   def update
     board = Board.find(params[:id])
+    authorize board
     board.assign_attributes(board_params)
 
     if board.save
       render json: board, status: :ok
     else
-      print board.errors.full_messages
-      
       render json: {
         error: board.errors.full_messages
       }, status: :unprocessable_entity
@@ -42,6 +42,7 @@ class BoardsController < ApplicationController
 
   def destroy
     board = Board.find(params[:id])
+    authorize board
 
     if board.destroy
       render json: {
@@ -55,6 +56,8 @@ class BoardsController < ApplicationController
   end
 
   def update_order
+    authorize Board
+    
     workflow_output = ReorderWorkflow.new(
       entity_classname: Board,
       column_name: 'order',
diff --git a/app/controllers/comments_controller.rb b/app/controllers/comments_controller.rb
index 08288634..8b1631e0 100644
--- a/app/controllers/comments_controller.rb
+++ b/app/controllers/comments_controller.rb
@@ -23,10 +23,10 @@ class CommentsController < ApplicationController
     comment = Comment.new(comment_params)
 
     if comment.save
-      send_notifications(comment)
+      SendNotificationForCommentWorkflow.new(comment: comment).run
 
       render json: comment.attributes.merge(
-        { user_full_name: current_user.full_name, user_email: current_user.email}
+        { user_full_name: current_user.full_name, user_email: current_user.email }
       ), status: :created
     else
       render json: {
@@ -37,17 +37,12 @@ class CommentsController < ApplicationController
 
   def update
     comment = Comment.find(params[:id])
-
+    authorize comment
     comment.assign_attributes(comment_params)
 
-    if !current_user.power_user? && current_user.id != post.user_id
-      render json: t('backend.errors.unauthorized'), status: :unauthorized
-      return
-    end
-
     if comment.save
       render json: comment.attributes.merge(
-        { user_full_name: current_user.full_name, user_email: current_user.email}
+        { user_full_name: current_user.full_name, user_email: current_user.email }
       )
     else
       render json: {
@@ -57,41 +52,14 @@ class CommentsController < ApplicationController
   end
 
   private
-  def comment_params
-    params
-      .require(:comment)
-      .permit(:body, :parent_id, :is_post_update)
-      .merge(
-        user_id: current_user.id,
-        post_id: params[:post_id]
-      )
-  end
 
-  def send_notifications(comment)
-    if comment.is_post_update # Post update
-      UserMailer.notify_followers_of_post_update(comment: comment).deliver_later
-      return
+    def comment_params
+      params
+        .require(:comment)
+        .permit(:body, :parent_id, :is_post_update)
+        .merge(
+          user_id: current_user.id,
+          post_id: params[:post_id]
+        )
     end
-    
-    if comment.parent_id == nil # Reply to a post
-      user = comment.post.user
-      
-      if comment.user.id != user.id and
-         user.notifications_enabled? and
-         comment.post.follows.exists?(user_id: user.id)
-        
-        UserMailer.notify_post_owner(comment: comment).deliver_later
-      end
-    else # Reply to a comment
-      parent_comment = comment.parent
-      user = parent_comment.user
-
-      if user.notifications_enabled? and
-         parent_comment.user.id != comment.user.id
-        
-        UserMailer.notify_comment_owner(comment: comment).deliver_later
-      end
-    end
-    
-  end
 end
diff --git a/app/controllers/post_statuses_controller.rb b/app/controllers/post_statuses_controller.rb
index 046d4952..1f245171 100644
--- a/app/controllers/post_statuses_controller.rb
+++ b/app/controllers/post_statuses_controller.rb
@@ -1,7 +1,7 @@
 class PostStatusesController < ApplicationController
   include ApplicationHelper
   
-  before_action :authenticate_admin, only: [:create, :update, :update_order, :destroy]
+  before_action :authenticate_user!, only: [:create, :update, :update_order, :destroy]
 
   def index
     post_statuses = PostStatus.order(order: :asc)
@@ -11,6 +11,7 @@ class PostStatusesController < ApplicationController
 
   def create
     post_status = PostStatus.new(post_status_params)
+    authorize post_status
 
     if post_status.save
       render json: post_status, status: :created
@@ -23,6 +24,7 @@ class PostStatusesController < ApplicationController
 
   def update
     post_status = PostStatus.find(params[:id])
+    authorize post_status
     post_status.assign_attributes(post_status_params)
 
     if post_status.save
@@ -36,6 +38,7 @@ class PostStatusesController < ApplicationController
 
   def destroy
     post_status = PostStatus.find(params[:id])
+    authorize post_status
 
     if post_status.destroy
       render json: {
@@ -49,6 +52,8 @@ class PostStatusesController < ApplicationController
   end
 
   def update_order
+    authorize PostStatus
+    
     workflow_output = ReorderWorkflow.new(
       entity_classname: PostStatus,
       column_name: 'order',
diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb
index a525541a..a2ce7eed 100644
--- a/app/controllers/posts_controller.rb
+++ b/app/controllers/posts_controller.rb
@@ -52,11 +52,7 @@ class PostsController < ApplicationController
 
   def update
     post = Post.find(params[:id])
-    
-    if !current_user.power_user? && current_user.id != post.user_id
-      render json: t('backend.errors.unauthorized'), status: :unauthorized
-      return
-    end
+    authorize post
 
     post.board_id = params[:post][:board_id] if params[:post].has_key?(:board_id)
     
@@ -91,7 +87,7 @@ class PostsController < ApplicationController
   private
   
     def filter_params
-      defaults = { board_id: Board.first.id }
+      defaults = Board.first ? { board_id: Board.first.id } : {}
 
       params
         .permit(:board_id, :post_status_id, :page, :search)
diff --git a/app/helpers/boards_helper.rb b/app/helpers/boards_helper.rb
deleted file mode 100644
index 8b8af150..00000000
--- a/app/helpers/boards_helper.rb
+++ /dev/null
@@ -1,2 +0,0 @@
-module BoardsHelper
-end
diff --git a/app/helpers/comments_helper.rb b/app/helpers/comments_helper.rb
deleted file mode 100644
index 0ec9ca5f..00000000
--- a/app/helpers/comments_helper.rb
+++ /dev/null
@@ -1,2 +0,0 @@
-module CommentsHelper
-end
diff --git a/app/helpers/likes_helper.rb b/app/helpers/likes_helper.rb
deleted file mode 100644
index a78a7596..00000000
--- a/app/helpers/likes_helper.rb
+++ /dev/null
@@ -1,2 +0,0 @@
-module LikesHelper
-end
diff --git a/app/helpers/posts_helper.rb b/app/helpers/posts_helper.rb
deleted file mode 100644
index a7b8cec8..00000000
--- a/app/helpers/posts_helper.rb
+++ /dev/null
@@ -1,2 +0,0 @@
-module PostsHelper
-end
diff --git a/app/policies/application_policy.rb b/app/policies/application_policy.rb
new file mode 100644
index 00000000..e000cba5
--- /dev/null
+++ b/app/policies/application_policy.rb
@@ -0,0 +1,53 @@
+# frozen_string_literal: true
+
+class ApplicationPolicy
+  attr_reader :user, :record
+
+  def initialize(user, record)
+    @user = user
+    @record = record
+  end
+
+  def index?
+    false
+  end
+
+  def show?
+    false
+  end
+
+  def create?
+    false
+  end
+
+  def new?
+    create?
+  end
+
+  def update?
+    false
+  end
+
+  def edit?
+    update?
+  end
+
+  def destroy?
+    false
+  end
+
+  class Scope
+    def initialize(user, scope)
+      @user = user
+      @scope = scope
+    end
+
+    def resolve
+      raise NotImplementedError, "You must define #resolve in #{self.class}"
+    end
+
+    private
+
+    attr_reader :user, :scope
+  end
+end
diff --git a/app/policies/board_policy.rb b/app/policies/board_policy.rb
new file mode 100644
index 00000000..1e1daa95
--- /dev/null
+++ b/app/policies/board_policy.rb
@@ -0,0 +1,17 @@
+class BoardPolicy < ApplicationPolicy
+  def create?
+    user.admin?
+  end
+  
+  def update?
+    user.admin?
+  end
+
+  def destroy?
+    user.admin?
+  end
+
+  def update_order?
+    user.admin?
+  end
+end
\ No newline at end of file
diff --git a/app/policies/comment_policy.rb b/app/policies/comment_policy.rb
new file mode 100644
index 00000000..12bae845
--- /dev/null
+++ b/app/policies/comment_policy.rb
@@ -0,0 +1,5 @@
+class CommentPolicy < ApplicationPolicy
+  def update?
+    user == record.user or user.power_user?
+  end
+end
\ No newline at end of file
diff --git a/app/policies/post_policy.rb b/app/policies/post_policy.rb
new file mode 100644
index 00000000..ca01c3ea
--- /dev/null
+++ b/app/policies/post_policy.rb
@@ -0,0 +1,5 @@
+class PostPolicy < ApplicationPolicy
+  def update?
+    user == record.user or user.power_user?
+  end
+end
\ No newline at end of file
diff --git a/app/policies/post_status_policy.rb b/app/policies/post_status_policy.rb
new file mode 100644
index 00000000..d681305d
--- /dev/null
+++ b/app/policies/post_status_policy.rb
@@ -0,0 +1,17 @@
+class PostStatusPolicy < ApplicationPolicy
+  def create?
+    user.admin?
+  end
+  
+  def update?
+    user.admin?
+  end
+
+  def destroy?
+    user.admin?
+  end
+
+  def update_order?
+    user.admin?
+  end
+end
\ No newline at end of file
diff --git a/app/workflows/SendNotificationForCommentWorkflow.rb b/app/workflows/SendNotificationForCommentWorkflow.rb
new file mode 100644
index 00000000..b74b114d
--- /dev/null
+++ b/app/workflows/SendNotificationForCommentWorkflow.rb
@@ -0,0 +1,34 @@
+class SendNotificationForCommentWorkflow
+  attr_accessor :comment
+
+  def initialize(comment: "")
+    @comment = comment
+  end
+
+  def run
+    if comment.is_post_update # Post update
+      UserMailer.notify_followers_of_post_update(comment: comment).deliver_later
+      return
+    end
+    
+    if comment.parent_id == nil # Reply to a post
+      user = comment.post.user
+      
+      if comment.user.id != user.id and
+        user.notifications_enabled? and
+        comment.post.follows.exists?(user_id: user.id)
+        
+        UserMailer.notify_post_owner(comment: comment).deliver_later
+      end
+    else # Reply to a comment
+      parent_comment = comment.parent
+      user = parent_comment.user
+
+      if user.notifications_enabled? and
+        parent_comment.user.id != comment.user.id
+        
+        UserMailer.notify_comment_owner(comment: comment).deliver_later
+      end
+    end
+  end
+end
\ No newline at end of file
diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb
index 9c97f484..8c6e5b2f 100644
--- a/config/initializers/devise.rb
+++ b/config/initializers/devise.rb
@@ -251,7 +251,7 @@ Devise.setup do |config|
   # should add them to the navigational formats lists.
   #
   # The "*/*" below is required to match Internet Explorer requests.
-  # config.navigational_formats = ['*/*', :html]
+  config.navigational_formats = ['*/*', :html]
 
   # The default HTTP method used to sign out a resource. Default is :delete.
   config.sign_out_via = :delete
diff --git a/spec/requests/posts_controller_auth_spec.rb b/spec/requests/posts_controller_auth_spec.rb
new file mode 100644
index 00000000..0ff86f40
--- /dev/null
+++ b/spec/requests/posts_controller_auth_spec.rb
@@ -0,0 +1,61 @@
+require 'rails_helper'
+
+RSpec.describe 'requests to posts controller', type: :request do
+  let(:user) { FactoryBot.create(:user) }
+  let(:moderator) { FactoryBot.create(:moderator) }
+  let(:admin) { FactoryBot.create(:admin) }
+
+  let(:p) { FactoryBot.create(:post) }
+  let(:board) { FactoryBot.build_stubbed(:board) }
+
+  let(:headers) { headers = { "ACCEPT" => "application/json" } }
+
+  context 'when user is not logged in' do
+    it 'fulfills index action' do
+      get posts_path
+      expect(response).to have_http_status(:success)
+    end
+    it 'fulfills show action' do
+      get posts_path(p)
+      expect(response).to have_http_status(:success)
+    end
+    it 'blocks create action' do
+      post posts_path, params: { post: { title: p.title } }, headers: headers
+      expect(response).to have_http_status(:unauthorized)
+    end
+    it 'blocks update action' do
+      patch post_path(p), params: { post: { title: p.title } }, headers: headers
+      expect(response).to have_http_status(:unauthorized)
+    end
+  end
+
+  context 'user role' do
+    before(:each) do
+      user.confirm
+      sign_in user
+    end
+
+    # it 'fulfills create action' do
+    #   post posts_path, params: { post: { title: p.title, board_id: board.id, user_id: user.id } }, headers: headers
+    #   expect(response).to have_http_status(:success)
+    # end
+    it 'blocks update action if from different user' do
+      expect(user.id).not_to eq(p.user_id)
+      patch post_path(p), params: { post: { title: p.title } }, headers: headers
+      expect(response).to have_http_status(:unauthorized)
+    end
+  end
+
+  context 'moderator role' do
+    before(:each) do
+      moderator.confirm
+      sign_in moderator
+    end
+
+    it 'fulfills update action' do
+      expect(user.id).not_to eq(p.user_id)
+      patch post_path(p), params: { post: { title: p.title } }, headers: headers
+      expect(response).to have_http_status(:success)
+    end
+  end
+end
\ No newline at end of file