Edem/improvements/pundit coverage (#498)

* added organization policy + spec

* user specs for is_org_admin?

* added authroize to organization controller

* admin policy + specs

* deleted enforce admin due to pundit policy redundancy

* applied admin policy to entire admin namespace

* refactoring analytics controller WIP - wanna test codeship

* Add protection against reactions to unpublished articles (#473)

* Add chat channel policy and spec (#474)

* Add comment policy and specs (#475)

* Fix edge case with apostrophes

* Add comment policy and specs

* Add login for deleting comment spec

* Change test to raise pundit error instead of 404

* Clean up comment destroy request specs

* Remove redundant raise

* Whitelist columns on to_json call (#477)

* Add pundit policy for several controllers (#476)

* Add pundit policy for several controllers

* Adjust video spec

* Fix tag request specs

* Add proper twilio tokens request specs

* Remove puts statements

* Add a couple basic request specs (#478)

* Add a few tests and fix user tag color bug (#482)

* Refactor handle_tag_index in stories_controller (#481)

* Modify valid_request_origin? (#483)

* Add misc specs and remove banned attribute from user model (#484)

*  Fix missing Cloudinary tags and misc specs (#486)

* removing current_user_is_admin? to use .is_admin? method

* added missing org policy routes

* Add comment for all public controllers

* Fix edge case for test

* Authorize mod controller and add specs

* Refactor methods via inheritance and use only super_admin role

* Create policy method for analytics via article_policy and refactor

* Capitalize all buttons in dashboard page

* Fix org tests and remove old admin test

* Use only happy path for analytics

* Fix tests to use Pundit error

* Update org_policy spec
This commit is contained in:
Edem Attikese 2018-06-28 09:38:20 -04:00 committed by Ben Halpern
parent 441a1ba8b8
commit 0ad8cd9eab
44 changed files with 216 additions and 176 deletions

View file

@ -1,4 +1,5 @@
class AdditionalContentBoxesController < ApplicationController
# No authorization required for entirely public controller
def index
article_ids = params[:article_id].split(",")
@article = Article.find(article_ids[0])

View file

@ -6,26 +6,17 @@
# you're free to overwrite the RESTful controller actions.
module Admin
class ApplicationController < Administrate::ApplicationController
skip_before_action :verify_authenticity_token
include EnforceAdmin
before_action :authenticate_admin
include Pundit
before_action :authorize_admin
def authenticate_admin
unless current_user_is_admin?
authenticate_or_request_with_http_basic do |username, password|
username == ENV["APP_NAME"] && password == ENV["APP_PASSWORD"]
end
end
end
# Override this value to specify the number of elements to display at a time
# on index pages. Defaults to 20.
# def records_per_page
# params[:per_page] || 20
# end
def order
@_order ||= Administrate::Order.new(params[:order] || "id",params[:direction] || "desc")
end
private
def authorize_admin
authorize :admin, :show?
end
end
end

View file

@ -2,39 +2,21 @@ class AnalyticsController < ApplicationController
caches_action :index,
cache_path: Proc.new { "#{request.params}___#{current_user.id}" },
expires_in: 15.minutes
after_action :verify_authorized
def index
article_ids = analytics_params.split(",")
if has_analytics_privilege?(article_ids.first, current_user)
cache_name = "pageviews-#{article_ids}/dashboard-index"
pageviews = Rails.cache.fetch(cache_name, expires_in: 15.minutes) do
GoogleAnalytics.new(article_ids).get_pageviews
end
render json: pageviews.to_json
else
render json: {}
article_to_check = Article.find_by(id: article_ids.first)
authorize article_to_check, :analytics_index?
cache_name = "pageviews-#{article_ids}/dashboard-index"
pageviews = Rails.cache.fetch(cache_name, expires_in: 15.minutes) do
GoogleAnalytics.new(article_ids).get_pageviews
end
render json: pageviews.to_json
end
private
def has_analytics_privilege?(article_id, current_user)
return false unless current_user
current_user_is_admin? ||
current_user_is_author_with_beta_acess?(article_id, current_user) ||
current_user_is_org_admin?(article_id, current_user)
end
def current_user_is_author_with_beta_acess?(article_id, user)
author = Article.find_by_id(article_id)&.user
author == user && user.has_role?(:analytics_beta_tester)
end
def current_user_is_org_admin?(article_id, user)
org_id = Article.find_by_id(article_id)&.organization_id
user.org_admin && user.organization_id == org_id
end
def analytics_params
params.require(:article_ids)
end

View file

@ -1,14 +1,8 @@
class ApplicationController < ActionController::Base
protect_from_forgery with: :exception, prepend: true
include EnforceAdmin
include Pundit
# before_action :require_http_auth if ENV["APP_NAME"] == "dev_stage"
#before_action :customize_params
def require_http_auth
authenticate_or_request_with_http_basic do |username, password|
username == ENV["APP_NAME"] && password == ENV["APP_PASSWORD"]

View file

@ -1,7 +1,9 @@
class BlocksController < ApplicationController
before_action :set_block, only: [:show, :edit, :update, :destroy]
after_action :verify_authorized
# GET /blocks
# GET /blocks.json
def index
@ -78,4 +80,5 @@ class BlocksController < ApplicationController
def set_block
@block = Block.find(params[:id])
end
end

View file

@ -1,4 +1,5 @@
class BufferedArticlesController < ApplicationController
# No authorization required for entirely public controller
def index
@article_urls = buffered_article_urls
render json: {

View file

@ -1,17 +0,0 @@
module EnforceAdmin
extend ActiveSupport::Concern
def require_super_admin
return verify_admin_status if current_user
redirect_to "/enter"
end
def verify_admin_status
return if current_user_is_admin?
redirect_to "/", status: 422
end
def current_user_is_admin?
current_user&.has_any_role?(:super_admin, :admin)
end
end

View file

@ -4,7 +4,7 @@ class DashboardsController < ApplicationController
after_action :verify_authorized
def show
@user = if params[:username] && current_user_is_admin?
@user = if params[:username] && current_user.is_admin?
User.find_by_username(params[:username])
else
current_user

View file

@ -1,4 +1,5 @@
class FollowedArticlesController < ApplicationController
# No authorization required for entirely public controller
caches_action :index,
:cache_path => Proc.new { "followed_articles_#{current_user.id}__#{current_user.updated_at}__#{user_signed_in?.to_s}" },

View file

@ -1,4 +1,5 @@
class GaEventsController < ApplicationController
# No authorization required for entirely public controller
# This controller is for tracking activity when GA script fails
# IP is scrambled as to not be persisted to limit fingerprinting abilities on our end.

View file

@ -1,4 +1,9 @@
class Internal::ApplicationController < ApplicationController
include EnforceAdmin
before_action :require_super_admin
before_action :authorize_admin
private
def authorize_admin
authorize :admin, :show?
end
end

View file

@ -3,26 +3,12 @@ class Internal::BroadcastsController < Internal::ApplicationController
def create
@broadcast = Broadcast.new(broadcast_params)
if @broadcast.save
# custom notifications not in use yet
# if @broadcast.sent && @broadcast.type_of == "Announcement"
# # only send new notifications for announcements
# # onboarding notifications are automated
# Notification.send_all(@broadcast, @broadcast.type_of)
# end
end
redirect_to "/internal/broadcasts"
end
def update
@broadcast = Broadcast.find(params[:id])
@broadcast.update(broadcast_params)
# if @broadcast.save
# if @broadcast.sent && @broadcast.type_of == "Announcement"
# # see create action comments
# Notification.send_all(@broadcast, @broadcast.type_of)
# end
# end
redirect_to "/internal/broadcasts"
end

View file

@ -1,4 +1,5 @@
class LiveArticlesController < ApplicationController
# No authorization required for entirely public controller
before_action :set_cache_control_headers, only: [:index]
def index

View file

@ -1,20 +1,15 @@
class ModerationsController < ApplicationController
before_action :check_trusted
after_action :verify_authorized
def article
authorize(User, :moderation_routes?)
@moderatable = Article.find_by_slug(params[:slug])
render template: "moderations/mod"
end
def comment
authorize(User, :moderation_routes?)
@moderatable = Comment.find(params[:id_code].to_i(26))
render template: "moderations/mod"
end
private
def check_trusted
not_found unless current_user&.has_role?(:trusted)
end
end

View file

@ -4,7 +4,7 @@ class NotificationsController < ApplicationController
def index
if user_signed_in?
@notifications_index = true
@user = if params[:username] && current_user_is_admin?
@user = if params[:username] && current_user.is_admin?
User.find_by_username(params[:username])
else
current_user

View file

@ -1,11 +1,13 @@
class OrganizationsController < ApplicationController
before_action :authenticate_user!, except: [:show]
after_action :verify_authorized
def create
@tab = "organization"
@user = current_user
@tab_list = @user.settings_tab_list
@organization = Organization.new(organization_params)
authorize @organization
if @organization.save
current_user.update(organization_id: @organization.id, org_admin: true)
redirect_to "/settings/organization", notice:
@ -21,8 +23,9 @@ class OrganizationsController < ApplicationController
@user = current_user
@tab = "organization"
@tab_list = @user.settings_tab_list
raise unless @user.org_admin
@organization = @user.organization
authorize @organization
if @organization.update(organization_params)
redirect_to "/settings/organization", notice: "Your organization was successfully updated."
else
@ -33,6 +36,7 @@ class OrganizationsController < ApplicationController
def generate_new_secret
raise unless current_user.org_admin
@organization = current_user.organization
authorize @organization
@organization.secret = @organization.generated_random_secret
@organization.save
redirect_to "/settings/organization", notice: "Your org secret was updated"

View file

@ -1,7 +1,6 @@
class PodcastEpisodesController < ApplicationController
before_action :require_http_auth, only: [:edit,:update,:admin]
before_action :set_cache_control_headers, only: [:index, :show]
# No authorization required for entirely public controller
before_action :set_cache_control_headers, only: [:index]
def index
@podcast_index = true

View file

@ -1,4 +1,5 @@
class SocialPreviewsController < ApplicationController
# No authorization required for entirely public controller
def article
@article = Article.find(params[:id])
@ -20,5 +21,4 @@ class SocialPreviewsController < ApplicationController
@tag = Tag.find(params[:id]) || not_found
render layout: false
end
end
end

View file

@ -89,8 +89,7 @@ class StoriesController < ApplicationController
@stories = @stories.where(approved: true)
end
@stories = stories_by_timeframe
@stories = stories_by_timeframe
@stories = @stories.decorate
@featured_story = Article.new

View file

@ -217,6 +217,10 @@ class User < ApplicationRecord
has_role? :warned
end
def is_admin?
has_role?(:super_admin)
end
def trusted
Rails.cache.fetch("user-#{id}/has_trusted_role", expires_in: 200.hours) do
has_role? :trusted
@ -246,6 +250,10 @@ class User < ApplicationRecord
has_any_role?(:workshop_pass, :level_3_member, :level_4_member, :triple_unicorn_member)
end
def is_org_admin?(organization)
user.org_admin && user.organization_id == organization.id
end
def unique_including_orgs
errors.add(:username, "is taken.") if Organization.find_by_slug(username)
end
@ -264,7 +272,7 @@ class User < ApplicationRecord
handle_asynchronously :subscribe_to_mailchimp_newsletter
def can_view_analytics?
has_any_role?(:super_admin, :admin, :analytics_beta_tester)
has_any_role?(:super_admin, :analytics_beta_tester)
end
def a_sustaining_member?
@ -293,6 +301,7 @@ class User < ApplicationRecord
end
private
def unescape_summary

View file

@ -0,0 +1,5 @@
class AdminPolicy < ApplicationPolicy
def show?
user_is_admin?
end
end

View file

@ -54,10 +54,10 @@ class ApplicationPolicy
end
def user_is_admin?
user.has_any_role?(:super_admin, :admin)
user.has_role?(:super_admin)
end
def user_is_banned?
user.has_role?(:banned)
user.banned
end
end

View file

@ -23,6 +23,10 @@ class ArticlePolicy < ApplicationPolicy
true
end
def analytics_index?
(user_is_author? && user_can_view_analytics?) || user_is_org_admin?
end
def permitted_attributes
%i[title body_html body_markdown user_id main_image published
description allow_small_edits allow_big_edits tag_list publish_under_org
@ -39,7 +43,7 @@ class ArticlePolicy < ApplicationPolicy
user.org_admin && user.organization_id == record.organization_id
end
def user_is_banned?
user.has_role?(:banned)
def user_can_view_analytics?
user.can_view_analytics?
end
end

View file

@ -30,10 +30,4 @@ class BlockPolicy < ApplicationPolicy
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

View file

@ -40,8 +40,4 @@ class CommentPolicy < ApplicationPolicy
def user_is_author?
record.user_id == user.id
end
def user_is_banned?
user.has_role?(:banned)
end
end

View file

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

View file

@ -1,10 +1,10 @@
class GithubRepoPolicy < ApplicationPolicy
def create?
!user.banned
!user_is_banned?
end
def update?
!user.banned && user_is_owner?
!user_is_banned? && user_is_owner?
end
def permitted_attributes

View file

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

View file

@ -2,10 +2,4 @@ class MessagePolicy < ApplicationPolicy
def create?
!user_is_banned?
end
private
def user_is_banned?
user&.has_role?(:banned)
end
end

View file

@ -0,0 +1,13 @@
class OrganizationPolicy < ApplicationPolicy
def create?
!user.banned
end
def update?
user.is_org_admin?(record)
end
def generate_new_secret?
update?
end
end

View file

@ -14,7 +14,7 @@ class TagPolicy < ApplicationPolicy
private
def has_mod_permission?
user.has_role?(:super_admin) ||
user_is_admin? ||
user.has_role?(:tag_moderator, record)
end
end

View file

@ -35,6 +35,10 @@ class UserPolicy < ApplicationPolicy
current_user? || user_is_admin?
end
def moderation_routes?
user.has_role?(:trusted) && !user.banned
end
private
def within_the_same_org?
@ -48,8 +52,4 @@ class UserPolicy < ApplicationPolicy
def current_user?
user == record
end
def user_is_admin?
user.has_role? :super_admin
end
end

View file

@ -3,8 +3,8 @@
<div class="dashboard-container">
<div class="actions">
<a class="action <%= 'active' if params[:which] == "organization" || params[:which].blank? %>" href="/dashboard">POSTS (<%= @user.articles_count %>)</a>
<a class="action <%= 'active' if params[:which] == "user_followers" %>" href="/dashboard/user_followers">Followers (<%= @user.followers_count %>)</a>
<a class="action <%= 'active' if params[:which] == "following_users" %>" href="/dashboard/following_users">Following (<%= @user.following_users_count %>)</a>
<a class="action <%= 'active' if params[:which] == "user_followers" %>" href="/dashboard/user_followers">FOLLOWERS (<%= @user.followers_count %>)</a>
<a class="action <%= 'active' if params[:which] == "following_users" %>" href="/dashboard/following_users">FOLLOWING (<%= @user.following_users_count %>)</a>
</div>
<% if @user.org_admin && @user.organization %>
<h1>
@ -26,7 +26,7 @@
<span class="pill cta yellow">DRAFT</span>
<% end %>
<a href="<%= article.path %>/edit" class="pill cta green">EDIT</a>
<% if article.published && current_user.has_role?(:analytics_beta_tester) %>
<% if article.published && current_user.can_view_analytics? %>
<span id="pageviews-<%= article.id %>" class="cta pill dashboard-pageviews-indicator" data-analytics-pageviews data-article-id="<%= article.id %>">fetching stats...</span>
<% end %>
</div>
@ -77,7 +77,7 @@
<% end %>
<a href="<%= article.path %>/edit" class="cta pill green">EDIT</a>
<a href="<%= article.path %>/delete_confirm" data-no-instant class="cta pill black">DELETE</a>
<% if article.published && current_user.has_role?(:analytics_beta_tester) %>
<% if article.published && current_user.can_view_analytics? %>
<span id="pageviews-<%= article.id %>" class="cta pill dashboard-pageviews-indicator" data-analytics-pageviews data-article-id="<%= article.id %>">fetching stats...</span>
<% end %>
</div>

View file

@ -26,6 +26,10 @@ FactoryBot.define do
after(:build) { |user| user.add_role(:admin) }
end
trait :trusted do
after(:build) { |user| user.add_role(:trusted) }
end
trait :banned do
after(:build) { |user| user.add_role(:banned) }
end
@ -41,6 +45,10 @@ FactoryBot.define do
end
end
trait :analytics do
after(:build) { |user| user.add_role(:analytics_beta_tester) }
end
after(:create) do |user|
create(:identity, user_id: user.id)
end

View file

@ -36,4 +36,5 @@ RSpec.describe BadgeRewarder do
BadgeRewarder.new.reward_top_seven_badges([user.username, user_other.username])
expect(BadgeAchievement.where(badge_id: badge.id).size).to eq(2)
end
end

View file

@ -7,6 +7,7 @@ RSpec.describe User, type: :model do
let(:article) { create(:article,user_id:user.id) }
let(:tag) { create(:tag) }
let(:org) { create(:organization) }
let (:second_org) { create(:organization) }
it { is_expected.to have_many(:articles) }
it { is_expected.to have_many(:badge_achievements) }
@ -308,17 +309,30 @@ RSpec.describe User, type: :model do
end
end
describe "organization admin privileges" do
it "recognizes an org admin" do
user.update(organization: org, org_admin: true)
expect(user.is_org_admin?(org)).to be true
end
it "forbids an incorrect org admin" do
user.update(organization: org, org_admin: true)
expect(user.is_org_admin?(second_org)).to be false
expect(second_user.is_org_admin?(org)).to be false
end
it "responds to nil" do
expect(user.is_org_admin?(nil)).to be false
expect(second_user.is_org_admin?(nil)).to be false
end
end
describe "#can_view_analytics?" do
it "returns true for users with :super_admin role" do
user.add_role(:super_admin)
expect(user.can_view_analytics?).to be true
end
it "returns true for users with :admin role" do
user.add_role(:admin)
expect(user.can_view_analytics?).to be true
end
it "returns true for users with :analytics_beta_tester role" do
user.add_role(:analytics_beta_tester)
expect(user.can_view_analytics?).to be true

View file

@ -0,0 +1,21 @@
require "rails_helper"
RSpec.describe AdminPolicy do
subject { described_class }
permissions :show? do
context "non admin" do
let(:user) {build(:user)}
it "should not allow someone without admin privileges to do continue" do
expect(subject).not_to permit(user)
end
end
context "admin" do
let(:user) {build(:user, :super_admin)}
it "allow someone with admin privileges to continue" do
expect(subject).to permit(user)
end
end
end
end

View file

@ -43,12 +43,6 @@ RSpec.describe ArticlePolicy do
end
end
context "when user is an admin" do
let(:user) { build(:user, :admin) }
it { is_expected.to permit_actions(%i[update new create delete_confirm destroy preview]) }
end
context "when user is a super_admin" do
let(:user) { build(:user, :super_admin) }

View file

@ -0,0 +1,47 @@
require "rails_helper"
RSpec.describe OrganizationPolicy do
subject { described_class.new(user, organization) }
let(:organization) { build(:organization) }
context "when user is not signed-in" do
let(:user) { nil }
it { within_block_is_expected.to raise_error(Pundit::NotAuthorizedError) }
end
context "when a non-org user" do
let(:user) { build(:user) }
it { is_expected.to forbid_action(:update) }
it { is_expected.to permit_action(:create) }
end
context "when user is banned" do
let(:user) { build(:user, :banned) }
it { is_expected.to forbid_actions(%i[create update]) }
end
context "when user is an org admin of an org" do
let(:user) { build(:user) }
before { user.update(organization: organization, org_admin: true) }
it "allows the user to update their own org" do
is_expected.to permit_action(:update)
end
end
context "when user is an org admin of another org" do
let(:user) { build(:user) }
let(:new_org) { build(:organization) }
before { user.update(organization: new_org, org_admin: true) }
it "does not allow the user to update another org" do
is_expected.to forbid_action(:update)
end
end
end

View file

@ -19,7 +19,7 @@ RSpec.describe UserPolicy do
context "with banned status" do
before { user.add_role(:banned) }
it { is_expected.to forbid_actions(%i[join_org]) }
it { is_expected.to forbid_actions(%i[join_org moderation_routes]) }
end
end
@ -52,4 +52,16 @@ RSpec.describe UserPolicy do
it { is_expected.to forbid_actions(%i[remove_org_admin]) }
end
end
context "when user is trusted" do
let(:user) { build(:user, :trusted) }
it { is_expected.to permit_actions(%i[moderation_routes]) }
end
context "when user is not trusted" do
let(:user) { build(:user) }
it { is_expected.to forbid_actions(%i[moderation_routes]) }
end
end

View file

@ -1,6 +1,7 @@
require "rails_helper"
RSpec.describe "AdditionalContentBoxes", type: :request do
let(:regular_article) { create(:article) }
describe "GET /additional_content_boxes" do

View file

@ -6,47 +6,28 @@ vcr_option = {
RSpec.describe "Analytics", type: :request, vcr: vcr_option do
describe "GET /analytics" do
it "returns json" do
get "/analytics?article_ids=0,1,2,3"
expect(response.content_type).to eq("application/json")
end
it "returns 200" do
get "/analytics?article_ids=0,1,2,3"
expect(response).to have_http_status(200)
end
it "raise ParameterMissing if no proper params is given" do
expect { get "/analytics" }.to raise_error ActionController::ParameterMissing
end
it "returns empty json when user is not signed in" do
get "/analytics?article_ids=0,1,2,3"
expect(response.body).to eq("{}")
end
context "when signed in" do
let(:user) { create(:user) }
context "when signed in as an authorized user" do
let(:user) { create(:user, :analytics) }
let(:article1) { create(:article, user_id: user.id) }
let(:article2) { create(:article, user_id: user.id) }
let(:article3) { create(:article, user_id: user.id) }
before do
login_as user
end
it "returns empty json if current user has no priviledge" do
get "/analytics?article_ids=0,1,2,3"
expect(response.body).to eq("{}")
it "raise ParameterMissing if no proper params is given" do
expect { get "/analytics" }.to raise_error ActionController::ParameterMissing
end
it "returns pageviews if user is an admin" do
user.add_role(:admin)
it "returns pageviews" do
get "/analytics?article_ids=#{article1.id},#{article2.id}"
expect(JSON.parse(response.body)).to eq(article1.id.to_s => "0", article2.id.to_s => "0")
end
it "returns pageviews if user is has beta access" do
user.add_role(:analytics_beta_tester)
it "returns pageviews for super_admins" do
user.remove_role :analytics_beta_tester
user.add_role :super_admin
get "/analytics?article_ids=#{article1.id},#{article2.id}"
expect(JSON.parse(response.body)).to eq(article1.id.to_s => "0", article2.id.to_s => "0")
end

View file

@ -69,7 +69,7 @@ RSpec.describe "Dashboards", type: :request do
user.follow second_user
login_as user
get "/dashboard/following_users"
expect(response.body).to include second_user.name
expect(response.body).to include CGI.escapeHTML(second_user.name)
end
end
end
@ -87,7 +87,7 @@ RSpec.describe "Dashboards", type: :request do
second_user.follow user
login_as user
get "/dashboard/user_followers"
expect(response.body).to include second_user.name
expect(response.body).to include CGI.escapeHTML(second_user.name)
end
end
end

View file

@ -23,7 +23,7 @@ RSpec.describe "Moderations", type: :request do
it "returns 404 if user trusted not trusted" do
expect do
get article.path + "/mod"
end.to raise_error(ActionController::RoutingError)
end.to raise_error(Pundit::NotAuthorizedError)
end
end
@ -36,7 +36,7 @@ RSpec.describe "Moderations", type: :request do
it "returns 404 if user trusted not trusted" do
expect do
get comment.path + "/mod"
end.to raise_error(ActionController::RoutingError)
end.to raise_error(Pundit::NotAuthorizedError)
end
end
end