Refactoring questions asked of user (#15762)

* Refactoring questions asked of user

In this pull request, I'm extracting and normalizing role-based
questions asked of the user.

Prior to this commit, our codebase has asked two very similar questions
of our user model:

- `user.has_role?(:admin)`
- `user.admin?`

In asking `has_role?(:admin)` we are relying on implementation details
of the rolify gem.  In addition, the `has_role?` question asked
throughout controllers or views means that it's harder to create
hieararchies of permissions.

In favoring `user.admin?` as our question, we can use that indirection
as an opportunity to discuss and decide "Should someone with the
`:super_admin` role be `user.admin? == true`?"

The details of this commit is to do three primary things:

1. Ask the `has_role?` questions in "one place" in the code (e.g. the
   `Authorizer` module)
2. Extract the role based questions that are on the `User` model and
   provde backwards compatable delegation.
3. Structure the code so that it's harder to accidentally call
   `user.has_role?` (e.g., make `User#has_role?` and `User#has_any_role?`
   private).

This is related to #15624 and the updates are informed by discussion in
PR #15691.  This commit supplants #15691.

* Refactoring the liquid tag policy tests

* Fixing typo

* Bump for travis
This commit is contained in:
Jeremy Friesen 2021-12-21 12:45:12 -05:00 committed by GitHub
parent 5fd1f94e85
commit a40efc6bbd
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
45 changed files with 568 additions and 237 deletions

View file

@ -40,7 +40,7 @@ module Admin
private
def extra_authorization
not_authorized unless current_user.has_role?(:creator)
not_authorized unless current_user.creator?
end
def settings_params

View file

@ -32,7 +32,7 @@ module Admin
end
def authorize_super_admin
raise Pundit::NotAuthorizedError unless current_user.has_role?(:super_admin)
raise Pundit::NotAuthorizedError unless current_user.super_admin?
end
end
end

View file

@ -39,7 +39,7 @@ module Api
end
def authorize_super_admin
error_unauthorized unless @user.has_role?(:super_admin)
error_unauthorized unless @user.super_admin?
end
# Checks if the user is authenticated, sets @user to nil otherwise

View file

@ -69,7 +69,7 @@ module Api
end
def update
articles_relation = @user.has_role?(:super_admin) ? Article.includes(:user) : @user.articles
articles_relation = @user.super_admin? ? Article.includes(:user) : @user.articles
article = articles_relation.find(params[:id])
result = Articles::Updater.call(@user, article, article_params)

View file

@ -61,7 +61,7 @@ class ModerationsController < ApplicationController
@adjustments = TagAdjustment.where(article_id: @moderatable.id)
@already_adjusted_tags = @adjustments.map(&:tag_name).join(", ")
@allowed_to_adjust = @moderatable.instance_of?(Article) && (
current_user.has_role?(:super_admin) || @tag_moderator_tags.any?)
current_user.super_admin? || @tag_moderator_tags.any?)
@hidden_comments = @moderatable.comments.where(hidden_by_commentable_user: true)
end
end

View file

@ -1,4 +1,17 @@
class LiquidTagBase < Liquid::Tag
# The method name to send the user to ask whether or not they
# have access to the given liquid tag.
#
# @see LiquidTagPolicy
#
# @note My preference would be to use `class_attribute` as it keeps
# things tidier, but that's not a hard preference.
#
# @note Should we verify that the user responds to this given method?
def self.user_authorization_method_name
nil
end
def self.script
""
end
@ -25,6 +38,12 @@ class LiquidTagBase < Liquid::Tag
.first
end
# A method to help collaborators not need to reach into the class
# implementation details.
def user_authorization_method_name
self.class.user_authorization_method_name
end
private
def validate_contexts

View file

@ -1,6 +1,12 @@
class PollTag < LiquidTagBase
PARTIAL = "liquids/poll".freeze
VALID_CONTEXTS = %w[Article].freeze
# @see LiquidTagBase.user_authorization_method_name for discussion
def self.user_authorization_method_name
:any_admin?
end
VALID_ROLES = %i[
admin
super_admin

View file

@ -1,6 +1,10 @@
class UserSubscriptionTag < LiquidTagBase
PARTIAL = "liquids/user_subscription".freeze
VALID_CONTEXTS = %w[Article].freeze
# @see LiquidTagBase.user_authorization_method_name for discussion
def self.user_authorization_method_name
:user_subscription_tag_available?
end
VALID_ROLES = [
:admin,
[:restricted_liquid_tag, LiquidTags::UserSubscriptionTag],

View file

@ -23,9 +23,7 @@ class TagAdjustment < ApplicationRecord
def has_privilege_to_adjust?
return false unless user
user.has_role?(:tag_moderator, tag) ||
user.has_role?(:admin) ||
user.has_role?(:super_admin)
user.tag_moderator?(tag: tag) || user.any_admin?
end
def article_tag_list

View file

@ -22,7 +22,7 @@ class User < ApplicationRecord
end
include StringAttributeCleaner.for(:email)
ANY_ADMIN_ROLES = %i[admin super_admin].freeze
USERNAME_MAX_LENGTH = 30
USERNAME_REGEXP = /\A[a-zA-Z0-9_]+\z/
MESSAGES = {
@ -341,51 +341,83 @@ class User < ApplicationRecord
end
end
def suspended?
has_role?(:suspended)
##############################################################################
#
# Heads Up: Start Authorization Refactor
#
##############################################################################
#
# What's going on here? First, I'm wanting to encourage folks to
# not call these methods directly. Instead I want to get all of
# these method calls in a single location so we can begin to analyze
# the behavior.
# @api private
#
# The method originally comes from the Rollify gem. Please don't
# call it from controllers or views. Favor `user.tech_admin?` over
# `user.has_role?(:tech_admin)`.
#
# @see Authorizer for further discussion.
private :has_role?
##
# @api private
#
# The method originally comes from the Rollify gem. Please don't
# call it from controllers or views. Favor `user.admin?` over
# `user.has_any_role?(:admin)`.
#
# @see Authorizer for further discussion.
private :has_any_role?
##
# @api private
#
# This is a refactoring step to help move the role questions out of the user object.
#
# @see https://github.com/forem/forem/issues/15624 for more discussion.
def authorizer
@authorizer ||= Authorizer.for(user: self)
end
def warned?
has_role?(:warned)
end
def warned
ActiveSupport::Deprecation.warn("User#warned is deprecated, favor User#warned?")
warned?
end
def super_admin?
has_role?(:super_admin)
end
def creator?
has_role?(:creator)
end
def any_admin?
@any_admin ||= roles.where(name: ANY_ADMIN_ROLES).any?
end
def tech_admin?
has_role?(:tech_admin) || has_role?(:super_admin)
end
def vomited_on?
Reaction.exists?(reactable_id: id, reactable_type: "User", category: "vomit", status: "confirmed")
end
def trusted?
return @trusted if defined? @trusted
@trusted = Rails.cache.fetch("user-#{id}/has_trusted_role", expires_in: 200.hours) do
has_role?(:trusted)
end
end
def trusted
ActiveSupport::Deprecation.warn("User#trusted is deprecated, favor User#trusted?")
trusted?
end
# My preference is to go with:
#
# `Authorize.for(user: user, to: <action>, on: <subject>)`
#
# However, this is a refactor, and its goal is to reduce the direct
# calls to user.<role question>.
delegate(
:admin?,
:administrative_access_to?,
:any_admin?,
:auditable?,
:banished?,
:comment_suspended?,
:creator?,
:has_trusted_role?,
:podcast_admin_for?,
:restricted_liquid_tag_for?,
:single_resource_admin_for?,
:super_admin?,
:support_admin?,
:suspended?,
:tag_moderator?,
:tech_admin?,
:trusted, # TODO: Remove this method from the code-base
:trusted?,
:user_subscription_tag_available?,
:vomited_on?,
:warned, # TODO: Remove this method from the code-base
:warned?,
:workshop_eligible?,
to: :authorizer,
)
##############################################################################
#
# End Authorization Refactor
#
##############################################################################
# The name of the tags moderated by the user.
#
@ -413,14 +445,6 @@ class User < ApplicationRecord
Tag.where(id: tag_ids).pluck(:name)
end
def comment_suspended?
has_role?(:comment_suspended)
end
def workshop_eligible?
has_any_role?(:workshop_pass)
end
def admin_organizations
org_ids = organization_memberships.admin.pluck(:organization_id)
organizations.where(id: org_ids)
@ -457,10 +481,6 @@ class User < ApplicationRecord
errors.add(:username, "has been banished.") if BanishedUser.exists?(username: username)
end
def banished?
username.starts_with?("spam_")
end
def subscribe_to_mailchimp_newsletter
return unless registered && email.present?
return if Settings::General.mailchimp_api_key.blank?
@ -496,14 +516,6 @@ class User < ApplicationRecord
Mailchimp::Bot.new(self).unsubscribe_all_newsletters
end
def auditable?
trusted? || tag_moderator? || any_admin?
end
def tag_moderator?
roles.where(name: "tag_moderator").any?
end
def enough_credits?(num_credits_needed)
credits.unspent.size >= num_credits_needed
end

View file

@ -60,20 +60,18 @@ class ApplicationPolicy
end
def minimal_admin?
user.has_role?(:super_admin) || user.has_role?(:admin)
user.any_admin?
end
def user_admin?
user.has_role?(:super_admin)
user.super_admin?
end
def support_admin?
user.has_role?(:support_admin)
end
delegate :support_admin?, to: :user
delegate :suspended?, to: :user, prefix: true
def user_trusted?
user.has_role?(:trusted)
user.has_trusted_role?
end
end

182
app/policies/authorizer.rb Normal file
View file

@ -0,0 +1,182 @@
##
# This module is providing a "crease in the code" for refactoring.
# The initial purpose is to help move away sending `has_role?`
# messages to User records. Prior to this refactor, there were calls
# to `user.has_role?(:tech_admin)` and `user.tech_admin?`; this
# created leaks in abstraction (see
# Authorizer::RoleBasedQueries#admin? for an example).
#
# By moving towards a single entry point and communicating that
# deprecation, the hope is to make the next conversation about roles
# easier.
module Authorizer
# @api private
#
# @note This method introduces some indirection, the idea being that
# the `Authorizer::RoleBasedQueries` is a refactor to convey
# deprecations and provide guidance on sending things through
# a common method pattern (e.g. favor `user.tech_admin?` over
# `user.has_role?(:tech_admin)`).
#
# @param user [User] the user of whom we're curious about their
# attributes and how the imply permissions.
#
def self.for(user:)
RoleBasedQueries.new(user: user)
end
# @api private
#
# This class is responsible for assisting in moving us away from
# `user.some_property_for_permissions?`.
#
# @see https://github.com/forem/forem/issues/15624
class RoleBasedQueries
ANY_ADMIN_ROLES = %i[admin super_admin].freeze
def initialize(user:)
@user = user
end
attr_reader :user
def admin?
has_role?(:admin)
end
def administrative_access_to?(resource:, role_name: :single_resource_admin)
# The implementation details of rolify are such that we can't
# quite combine these functions.
return true if has_any_role?(*ANY_ADMIN_ROLES)
if resource
has_role?(role_name, resource)
else
has_role?(role_name)
end
end
def any_admin?
has_any_role?(*ANY_ADMIN_ROLES)
end
def auditable?
trusted? || tag_moderator? || any_admin?
end
def banished?
user.username.starts_with?("spam_")
end
def comment_suspended?
has_role?(:comment_suspended)
end
def creator?
has_role?(:creator)
end
# When you need to know if we trust the user, but don't want to
# have stale information that the `trusted?` method might give
# you.
#
# @note You may ask why not use the trusted? method on this class?
# Well, in looking at the code there were explicit calls to
# `user.has_role?(:trusted)` which circumvented the caching
# logic. I'm uncertain which of those is appropriate, so
# I'm adding this method here.
#
# @see #trusted?
#
# @todo Review whether we can use trusted? or if we even need to cache things.
def has_trusted_role?
has_role?(:trusted)
end
def podcast_admin_for?(podcast)
has_role?(:podcast_admin, podcast)
end
def single_resource_admin_for?(resource)
has_role?(:single_resource_admin, resource)
end
# @note This is of "narrower" permissions than
# `#user_subscription_tag_available?`, as it doesn't include
# administrators.
#
# @todo Remove this?
def restricted_liquid_tag_for?(liquid_tag)
has_role?(:restricted_liquid_tag, liquid_tag)
end
def super_admin?
has_role?(:super_admin)
end
def support_admin?
has_role?(:support_admin)
end
def suspended?
has_role?(:suspended)
end
def tag_moderator?(tag: nil)
# Note a fan of "peeking" into the roles table, which in a way
# circumvents the rolify gem. But this was the past implementation.
return user.roles.exists?(name: "tag_moderator") unless tag
has_role?(:tag_moderator, tag)
end
def tech_admin?
has_any_role?(:tech_admin, :super_admin)
end
def trusted
ActiveSupport::Deprecation.warn("User#trusted is deprecated, favor User#trusted?")
trusted?
end
def trusted?
return @trusted if defined? @trusted
@trusted = Rails.cache.fetch("user-#{user.id}/has_trusted_role", expires_in: 200.hours) do
has_role?(:trusted)
end
end
def user_subscription_tag_available?
administrative_access_to?(role_name: :restricted_liquid_tag, resource: LiquidTags::UserSubscriptionTag)
end
def vomited_on?
Reaction.exists?(reactable: user, category: "vomit", status: "confirmed")
end
def warned?
has_role?(:warned)
end
def warned
ActiveSupport::Deprecation.warn("User#warned is deprecated, favor User#warned?")
warned?
end
def workshop_eligible?
has_any_role?(:workshop_pass)
end
private
def has_role?(*args)
user.__send__(:has_role?, *args)
end
def has_any_role?(*args)
user.__send__(:has_any_role?, *args)
end
end
private_constant :RoleBasedQueries
end

View file

@ -1,9 +1,5 @@
class InternalPolicy < ApplicationPolicy
def access?
user.has_any_role?(
{ name: :single_resource_admin, resource: record },
:super_admin,
:admin,
)
user.administrative_access_to?(resource: record)
end
end

View file

@ -1,15 +1,31 @@
# Intentionally not inheriting from ApplicationPolicy because liquid tags behave
# differently than the typical Model/Controller dynamic that Pundit assumes.
# This Policy is responsible for enforcing weither or not the user can utilize
# the given liquid tag.
#
# @note Intentionally not inheriting from ApplicationPolicy because liquid tags
# behave differently than the typical Model/Controller dynamic that Pundit
# assumes.
class LiquidTagPolicy
attr_reader :user, :record
attr_reader :user, :liquid_tag
def initialize(user, record)
# @param user [User]
# @param liquid_tag [LiquidTagBase]
def initialize(user, liquid_tag)
@user = user
@record = record
@liquid_tag = liquid_tag
end
# Check if the given #user can utilize the given #liquid_tag
#
# @return [TrueClass] if the given liquid_tag is available to the user.
# @raise [Pundit::NotAuthorizedError] if the liquid tag is not available to
# the given user.
def initialize?
return true unless record.class.const_defined?("VALID_ROLES")
# NOTE: This check the liquid tag then send that liquid tag's method to the
# user is "fragile". Would it make more sense to ask the liquid tag? Or
# the user given the liquid tag? My inclination is ask the user (and by
# extension the Authorizer). But that is a future refactor.
return true unless liquid_tag.user_authorization_method_name
raise Pundit::NotAuthorizedError, "No user found" unless user
# Manually raise error to use a custom error message
raise Pundit::NotAuthorizedError, "User is not permitted to use this liquid tag" unless user_allowed_to_use_tag?
@ -20,11 +36,6 @@ class LiquidTagPolicy
private
def user_allowed_to_use_tag?
record.class::VALID_ROLES.any? { |valid_role| user_has_valid_role?(valid_role) }
end
def user_has_valid_role?(valid_role)
# Splat array for single resource roles
user.has_role?(*Array(valid_role))
user.public_send(liquid_tag.user_authorization_method_name)
end
end

View file

@ -19,6 +19,6 @@ class TagPolicy < ApplicationPolicy
def has_mod_permission?
user_admin? ||
user.has_role?(:tag_moderator, record)
user.tag_moderator?(tag: record)
end
end

View file

@ -99,7 +99,7 @@ class UserPolicy < ApplicationPolicy
end
def moderation_routes?
(user.has_role?(:trusted) || minimal_admin?) && !user.suspended?
(user.has_trusted_role? || minimal_admin?) && !user.suspended?
end
def update_password?

View file

@ -67,7 +67,7 @@ module Mailchimp
end
def manage_community_moderator_list
return false unless Settings::General.mailchimp_community_moderators_id.present? && user.has_role?(:trusted)
return false unless Settings::General.mailchimp_community_moderators_id.present? && user.has_trusted_role?
success = false
status = user.notification_setting.email_community_mod_newsletter ? "subscribed" : "unsubscribed"

View file

@ -95,7 +95,7 @@ module Moderator
end
def check_super_admin
raise "You need super admin status to take this action" unless @admin.has_role?(:super_admin)
raise "You need super admin status to take this action" unless @admin.super_admin?
end
def comment_suspended

View file

@ -1,7 +1,7 @@
module TagModerators
class AddTrustedRole
def self.call(user)
return if user.has_role?(:trusted) || user.suspended?
return if user.has_trusted_role? || user.suspended?
user.add_role(:trusted)
user.notification_setting.update(email_community_mod_newsletter: true)

View file

@ -1,12 +1,15 @@
module Users
module ApprovedLiquidTags
# TODO: Should this include PollTag
RESTRICTED_LIQUID_TAGS = [UserSubscriptionTag].freeze
def self.call(user)
return [] unless user
RESTRICTED_LIQUID_TAGS.filter_map do |liquid_tag|
liquid_tag if liquid_tag::VALID_ROLES.any? { |role| user.has_role?(*Array(role)) }
# TODO: Should we instead consider asking the liquid tag?
liquid_tag if liquid_tag.user_authorization_method_name &&
user.public_send(liquid_tag.user_authorization_method_name)
end
end
end

View file

@ -74,7 +74,7 @@
<%= render partial: "landing_page_modal", locals: { page: @landing_page } %>
<% end %>
<% if current_user.has_role?(:tech_admin) %>
<% if current_user.tech_admin? %>
<div class="form-group">
<p>
<b><%= link_to "Feature Flag", "/admin/feature_flags" %></b>

View file

@ -1,3 +1,3 @@
<% if current_user.has_role?(:super_admin) %>
<% if current_user.super_admin? %>
<%= f.submit "Update Settings", class: "crayons-btn mt-4", aria: { label: local_assigns[:aria_label] }, data: { disable_with: false } %>
<% end %>

View file

@ -1,5 +1,5 @@
<div class="grid gap-6" data-controller="config">
<% unless current_user.has_role?(:super_admin) %>
<% unless current_user.super_admin? %>
<div class="crayons-notice crayons-notice--info" role="alert">
<p class="mb-1">
Only users with <strong>Super Admin</strong> privileges may edit this page.

View file

@ -37,7 +37,7 @@
<div class="form-group">
<%= f.label "Select new user status", class: "mr-3" %>
<% options = { "Base Roles" => Constants::Role::BASE_ROLES } %>
<% options["Special Roles"] = Constants::Role::SPECIAL_ROLES if current_user.has_role?(:super_admin) %>
<% options["Special Roles"] = Constants::Role::SPECIAL_ROLES if current_user.super_admin? %>
<%= f.select(:user_status, grouped_options_for_select(options), include_blank: true) %>
</div>
<div class="form-group">
@ -132,7 +132,7 @@
<%= form_for(@user, url: banish_admin_user_path(@user), html: { method: :post, onsubmit: "return confirm('Are you sure? This is extremely destructive and irreversible. Banishing will delete all articles and turn their username into @spam_###')" }) do %>
<button class="btn btn-danger">🚫 Banish User for Spam 🚫</button>
<% end %>
<% elsif current_user.has_role?(:super_admin) || current_user.has_role?(:support_admin) %>
<% elsif current_user.super_admin? || current_user.support_admin? %>
<p><strong>This is not a new user.</strong> You are only allowed to take this action because you are a
<strong>super admin or a support admin.</strong></p>
<p>
@ -147,7 +147,7 @@
<% end %>
</div>
<% if current_user.has_role?(:super_admin) %>
<% if current_user.super_admin? %>
<div>
<h3>Fully Delete User</h3>
<p>This will

View file

@ -66,11 +66,11 @@
</button>
</div>
<% if current_user.has_role?(:super_admin) && @moderatable.class.name == "Article" %>
<% if current_user.super_admin? && @moderatable.class.name == "Article" %>
<h3> <a href="<%= @moderatable.path %>/edit"><%= t("views.moderations.actions.edit.post") %></a> |
<a href="/resource_admin/articles/<%= @moderatable.id %>" data-no-instant><%= t("views.moderations.actions.edit.resource_admin") %></a> |
<a href="<%= admin_article_path(@moderatable.id) %>" data-no-instant><%= t("views.moderations.actions.edit.article_admin") %></a></h3>
<% elsif current_user.has_role?(:super_admin) && @moderatable.class.name == "Comment" %>
<% elsif current_user.super_admin? && @moderatable.class.name == "Comment" %>
<h3> <a href="/admin/comments/<%= @moderatable.id %>" data-no-instant><%= t("views.moderations.actions.edit.comment_admin") %></a> |
<a href="<%= admin_user_path(@moderatable.user_id) %>" data-no-instant><%= t("views.moderations.actions.edit.user_admin") %></a></h3>
<% end %>
@ -148,7 +148,7 @@
<%= f.label :adjustment_type, t("views.moderations.actions.tag.add"), value: "addition" %>
</div>
<% end %>
<% if current_user.has_role?(:super_admin) %>
<% if current_user.super_admin? %>
<%= f.text_field :tag_name, placeholder: t("views.moderations.actions.tag.tag_name"), required: true %>
<% else %>
<%= f.select :tag_name, @tag_moderator_tags, { prompt: t("views.moderations.actions.tag.select") }, required: true %>

View file

@ -12,7 +12,7 @@
<%= json_data["comment"]["processed_html"].html_safe %>
</div>
<% if context == "moderation" && current_user.has_role?(:trusted) %>
<% if context == "moderation" && current_user.has_trusted_role? %>
<div class="crayons-card crayons-card--secondary crayons-card--elevated notification__mod-controls">
<button
class="crayons-btn crayons-btn--icon crayons-btn--icon-rounded crayons-btn--ghost inline-flex crayons-btn--s reaction-button <%= Reaction.cached_any_reactions_for?(notification.mocked_object("comment"), current_user, "thumbsdown") ? "reacted" : "" %>"

View file

@ -10,7 +10,7 @@
<% end %>
</h1>
<div class="flex">
<% if current_user.has_role?(:super_admin) || current_user.has_role?(:admin) %>
<% if current_user.any_admin? %>
<a href="<%= edit_admin_tag_path(@tag.id) %>" class="crayons-btn crayons-btn--ghost" data-no-instant><%= t("views.tags.edit.admin") %></a>
<% end %>
<a target="_blank" rel="noopener" href="https://admin.forem.com/docs/forem-basics/tags#editing-a-tag" class="crayons-btn crayons-btn--ghost"><%= t("views.tags.edit.help") %></a>

View file

@ -25,7 +25,7 @@
<%= f.label :email_tag_mod_newsletter, "Send me tag moderator newsletter emails", class: "crayons-field__label" %>
</div>
<% end %>
<% if current_user.has_role?(:trusted) %>
<% if current_user.has_trusted_role? %>
<div class="crayons-field crayons-field--checkbox">
<%= f.check_box :email_community_mod_newsletter, class: "crayons-checkbox" %>
<%= f.label :email_community_mod_newsletter, "Send me community moderator newsletter emails", class: "crayons-field__label" %>

View file

@ -18,7 +18,7 @@
key: "video-upload__#{SecureRandom.hex}",
key_starts_with: "video-upload__",
acl: "public-read",
max_file_size: (current_user.has_role?(:super_admin) ? 20_000 : 6000).megabytes,
max_file_size: (current_user.super_admin? ? 20_000 : 6000).megabytes,
id: "s3-uploader",
class: "upload-form",
data: { key: :val } do %>

View file

@ -9,7 +9,7 @@ describe DataUpdateScripts::BackfillCreatorRoleForFirstSuperAdmin do
it "Only the first super admin should have the creator role" do
described_class.new.run
expect(creator).to have_role(:creator)
expect(admin).not_to have_role(:creator)
expect(creator).to be_creator
expect(admin).not_to be_creator
end
end

View file

@ -35,11 +35,11 @@ RSpec.describe LiquidTagBase, type: :liquid_tag do
end
end
context "when VALID_ROLES are defined" do
context "when .user_authorization_method_name is not nil" do
it "raises an error for invalid roles" do
source = create(:article)
liquid_tag_options = { source: source, user: source.user }
stub_const("#{described_class}::VALID_ROLES", %i[admin])
allow(described_class).to receive(:user_authorization_method_name).and_return(:admin?)
expect do
Liquid::Template.parse("{% liquid_tag_base %}", liquid_tag_options)
end.to raise_error(Pundit::NotAuthorizedError)
@ -49,28 +49,18 @@ RSpec.describe LiquidTagBase, type: :liquid_tag do
author = create(:user, :admin)
source = create(:article, user: author)
liquid_tag_options = { source: source, user: source.user }
stub_const("#{described_class}::VALID_ROLES", %i[admin])
expect do
Liquid::Template.parse("{% liquid_tag_base %}", liquid_tag_options)
end.not_to raise_error
end
it "validates single resource roles" do
# TODO: (Alex Smith) - update roles to new liquid tag role for more relevant example/use
author = create(:user, :single_resource_admin, resource: Article)
source = create(:article, user: author)
liquid_tag_options = { source: source, user: source.user }
stub_const("#{described_class}::VALID_ROLES", [[:single_resource_admin, Article]])
allow(described_class).to receive(:user_authorization_method_name).and_return(:admin?)
expect do
Liquid::Template.parse("{% liquid_tag_base %}", liquid_tag_options)
end.not_to raise_error
end
end
context "when VALID_ROLES are not defined" do
context "when .user_authorization_method_name is nil" do
it "doesn't validate roles" do
source = create(:article)
liquid_tag_options = { source: source, user: source.user }
allow(described_class).to receive(:user_authorization_method_name).and_return(nil)
expect do
Liquid::Template.parse("{% liquid_tag_base %}", liquid_tag_options)
end.not_to raise_error

View file

@ -0,0 +1,9 @@
require "rails_helper"
RSpec.describe PollTag, type: :liquid_tag do
describe ".user_authorization_method_name" do
subject(:result) { described_class.user_authorization_method_name }
it { is_expected.to eq(:any_admin?) }
end
end

View file

@ -15,6 +15,12 @@ RSpec.describe UserSubscriptionTag, type: :liquid_tag do
allow(author).to receive(:has_role?).with(:restricted_liquid_tag, LiquidTags::UserSubscriptionTag).and_return(true)
end
describe ".user_authorization_method_name" do
subject(:result) { described_class.user_authorization_method_name }
it { is_expected.to eq(:user_subscription_tag_available?) }
end
context "when rendering" do
it "renders default data correctly" do
source = create(:article, user: author)

View file

@ -36,6 +36,31 @@ RSpec.describe User, type: :model do
allow(Settings::Authentication).to receive(:providers).and_return(Authentication::Providers.available)
end
describe "delegations" do
it { is_expected.to delegate_method(:admin?).to(:authorizer) }
it { is_expected.to delegate_method(:any_admin?).to(:authorizer) }
it { is_expected.to delegate_method(:auditable?).to(:authorizer) }
it { is_expected.to delegate_method(:banished?).to(:authorizer) }
it { is_expected.to delegate_method(:comment_suspended?).to(:authorizer) }
it { is_expected.to delegate_method(:creator?).to(:authorizer) }
it { is_expected.to delegate_method(:has_trusted_role?).to(:authorizer) }
it { is_expected.to delegate_method(:podcast_admin_for?).to(:authorizer) }
it { is_expected.to delegate_method(:restricted_liquid_tag_for?).to(:authorizer) }
it { is_expected.to delegate_method(:single_resource_admin_for?).to(:authorizer) }
it { is_expected.to delegate_method(:super_admin?).to(:authorizer) }
it { is_expected.to delegate_method(:support_admin?).to(:authorizer) }
it { is_expected.to delegate_method(:suspended?).to(:authorizer) }
it { is_expected.to delegate_method(:tag_moderator?).to(:authorizer) }
it { is_expected.to delegate_method(:tech_admin?).to(:authorizer) }
it { is_expected.to delegate_method(:trusted).to(:authorizer) }
it { is_expected.to delegate_method(:trusted?).to(:authorizer) }
it { is_expected.to delegate_method(:user_subscription_tag_available?).to(:authorizer) }
it { is_expected.to delegate_method(:vomited_on?).to(:authorizer) }
it { is_expected.to delegate_method(:warned).to(:authorizer) }
it { is_expected.to delegate_method(:warned?).to(:authorizer) }
it { is_expected.to delegate_method(:workshop_eligible?).to(:authorizer) }
end
describe "validations" do
describe "builtin validations" do
subject { user }
@ -769,18 +794,6 @@ RSpec.describe User, type: :model do
end
end
describe "#trusted?" do
it "memoizes the result from rolify" do
allow(Rails.cache)
.to receive(:fetch)
.with("user-#{user.id}/has_trusted_role", expires_in: 200.hours)
.and_return(false)
.once
2.times { user.trusted? }
end
end
describe "profiles" do
it "automatically creates a profile for new users", :aggregate_failures do
user = create(:user)

View file

@ -0,0 +1,86 @@
require "rails_helper"
# rubocop:disable RSpec/PredicateMatcher
RSpec.describe Authorizer, type: :policy do
subject(:authorizer) { described_class.for(user: user) }
let(:user) { create(:user) }
describe "#any_admin?" do
# This test che
it "queries the user's roles" do
# I want to test `expect(authorizer.admin?)` but our rubocop
# version squaks.
expect(authorizer.admin?).to be_falsey
end
end
describe "#administrative_access_to?" do
subject(:method_call) { authorizer.administrative_access_to?(resource: resource) }
let(:resource) { nil }
context "when not an admin or super admin and not given a resource" do
let(:user) { build(:user) }
it { is_expected.to be_falsey }
end
context "when an admin and not given a resource" do
let(:user) { build(:user, :admin) }
it { is_expected.to be_truthy }
end
context "when a super_admin and not given a resource" do
let(:user) { build(:user, :super_admin) }
it { is_expected.to be_truthy }
end
context "when given a resource and the user is assigned singular administration" do
let(:resource) { Article }
before do
user.add_role(:single_resource_admin, resource)
end
it { is_expected.to be_truthy }
end
context "when given a resource and the user is assigned singular administration to another" do
let(:resource) { Article }
before do
user.add_role(:single_resource_admin, Comment)
end
it { is_expected.to be_falsey }
end
end
describe "#trusted?" do
# We don't need a saved user. Let's not require that.
let(:user) { instance_double(User, id: 123) }
it "memoizes the result from rolify" do
allow(Rails.cache)
.to receive(:fetch)
.with("user-#{user.id}/has_trusted_role", expires_in: 200.hours)
.and_return(false)
.once
2.times { authorizer.trusted? }
end
end
describe "#vomited_on?" do
subject(:method_call) { authorizer.vomited_on? }
# I included this test because I had mis-copied something and the
# system broke. This test is my "penance" for pushing up broken
# code.
it { is_expected.not_to be_nil }
end
end
# rubocop:enable RSpec/PredicateMatcher

View file

@ -4,29 +4,21 @@ RSpec.describe InternalPolicy, type: :policy do
let(:internal_policy) { described_class }
permissions :access? do
it "does not allow someone without admin privileges to do continue" do
expect(internal_policy).not_to permit(build(:user))
end
let(:user) { instance_double(User) }
it "allow someone with admin privileges to continue" do
expect(internal_policy).to permit(build(:user, :admin))
end
context "when user does not have administrative access (to the record)" do
before { allow(user).to receive(:administrative_access_to?).and_return(false) }
it "allow someone with super_admin privileges to continue" do
expect(internal_policy).to permit(build(:user, :super_admin))
end
context "when tied to a resource" do
let(:user) { create(:user) }
it "grant access based on permitted resource" do
user.add_role(:single_resource_admin, Article)
expect(internal_policy).to permit(user, Article)
it "does not permit the user" do
expect(internal_policy).not_to permit(user)
end
end
it "does not grant cross resource access" do
user.add_role(:single_resource_admin, Article)
expect(internal_policy).not_to permit(user, Comment)
context "when user has administrative access (to the record)" do
before { allow(user).to receive(:administrative_access_to?).and_return(true) }
it "does not permit the user" do
expect(internal_policy).to permit(user)
end
end
end

View file

@ -1,67 +1,72 @@
require "rails_helper"
RSpec.describe LiquidTagPolicy, type: :policy do
let(:liquid_tag) { instance_double(Liquid::Tag) }
let(:liquid_tag) { instance_double(LiquidTagBase, user_authorization_method_name: user_authorization_method_name) }
let(:article) { instance_double(Article) }
let(:parse_context) { { source: article, user: user } }
let(:user) { nil }
let(:user_authorization_method_name) { nil }
before do
allow(liquid_tag).to receive(:user_authorization_method_name).and_return(user_authorization_method_name)
allow(liquid_tag).to receive(:parse_context).and_return(parse_context)
end
describe "initialize?" do
let(:action) { :initialize? }
let(:article) { create(:article) }
it "raises an error if user is missing" do
user = nil
parse_context = { source: article, user: user }
allow(liquid_tag).to receive(:parse_context).and_return(parse_context)
stub_const("#{liquid_tag.class}::VALID_ROLES", [:admin])
expect do
Pundit.authorize(user, liquid_tag, action, policy_class: described_class)
end.to raise_error(Pundit::NotAuthorizedError, "No user found")
context "when parsing a non-restricted tag without a user" do
let(:user_authorization_method_name) { nil }
let(:user) { nil }
it "authorizes" do
expect do
Pundit.authorize(user, liquid_tag, action, policy_class: described_class)
end.not_to raise_error
end
end
it "authorizes and skips logic if liquid tag is not role restricted" do
user = create(:user)
parse_context = { source: article, user: user }
allow(liquid_tag).to receive(:parse_context).and_return(parse_context)
allow(user).to receive(:has_role?)
expect do
Pundit.authorize(user, liquid_tag, action, policy_class: described_class)
end.not_to raise_error
expect(user).not_to have_received(:has_role?)
context "when parsing a non-restricted tag with a user" do
let(:user) { instance_double(User) }
it "authorizes" do
expect do
Pundit.authorize(user, liquid_tag, action, policy_class: described_class)
end.not_to raise_error
end
end
it "authorizes if user has the correct role" do
user = create(:user, :admin)
parse_context = { source: article, user: user }
context "when parsing a restricted tag without a user" do
let(:user_authorization_method_name) { :admin? }
let(:user) { nil }
allow(liquid_tag).to receive(:parse_context).and_return(parse_context)
stub_const("#{liquid_tag.class}::VALID_ROLES", [:admin])
expect do
Pundit.authorize(user, liquid_tag, action, policy_class: described_class)
end.not_to raise_error
it "does not authorize" do
expect do
Pundit.authorize(user, liquid_tag, action, policy_class: described_class)
end.to raise_error(Pundit::NotAuthorizedError, "No user found")
end
end
it "handles single resource roles" do
user = create(:user)
# Stub roles because adding them normally can cause flaky specs
single_resource_role = [:restricted_liquid_tag, LiquidTags::UserSubscriptionTag]
allow(user).to receive(:has_role?).and_call_original
allow(user).to receive(:has_role?).with(*single_resource_role).and_return(true)
parse_context = { source: article, user: user }
context "when parsing a restricted tag with a user who **does not** meet the criteria" do
let(:user_authorization_method_name) { :admin? }
let(:user) { instance_double(User, user_authorization_method_name => false) }
allow(liquid_tag).to receive(:parse_context).and_return(parse_context)
stub_const("#{liquid_tag.class}::VALID_ROLES", [single_resource_role])
expect do
Pundit.authorize(user, liquid_tag, action, policy_class: described_class)
end.not_to raise_error
it "does not authorize" do
expect do
Pundit.authorize(user, liquid_tag, action, policy_class: described_class)
end.to raise_error(Pundit::NotAuthorizedError, "User is not permitted to use this liquid tag")
end
end
it "raises error if user does not have the correct role" do
user = create(:user)
parse_context = { source: article, user: user }
allow(liquid_tag).to receive(:parse_context).and_return(parse_context)
stub_const("#{liquid_tag.class}::VALID_ROLES", [:not_permitted])
expect do
Pundit.authorize(user, liquid_tag, action, policy_class: described_class)
end.to raise_error(Pundit::NotAuthorizedError, "User is not permitted to use this liquid tag")
context "when parsing a restricted tag with a user who meets the criteria" do
let(:user_authorization_method_name) { :admin? }
let(:user) { instance_double(User, user_authorization_method_name => true) }
it "does not authorize" do
expect do
Pundit.authorize(user, liquid_tag, action, policy_class: described_class)
end.not_to raise_error
end
end
end
end

View file

@ -70,7 +70,7 @@ RSpec.describe "/admin/moderation/mods", type: :request do
it "displays mod user" do
put admin_mod_path(regular_user.id)
expect(regular_user.reload.has_role?(:trusted)).to eq true
expect(regular_user.reload.trusted?).to eq true
end
end
end

View file

@ -10,6 +10,7 @@ RSpec.describe "/admin/content_manager/tags/:id/moderator", type: :request do
it "adds the given user as trusted and as a tag moderator" do
post admin_tag_moderator_path(tag.id), params: { tag_id: tag.id, tag: { user_id: user.id } }
expect(user.tag_moderator?).to be true
expect(user.trusted?).to be true
end

View file

@ -146,7 +146,7 @@ RSpec.describe "Admin::Users", type: :request do
params = { user: { user_status: "Super Admin", note_for_current_role: "they deserve it for some reason" } }
patch user_status_admin_user_path(user.id), params: params
expect(user.has_role?(:super_admin)).to be(true)
expect(user.super_admin?).to be(true)
end
it "does not allow non-super-admin to doll out admin" do
@ -157,7 +157,7 @@ RSpec.describe "Admin::Users", type: :request do
params = { user: { user_status: "Super Admin", note_for_current_role: "they deserve it for some reason" } }
patch user_status_admin_user_path(user.id), params: params
expect(user.has_role?(:super_admin)).not_to be false
expect(user.super_admin?).not_to be false
end
it "creates a general note on the user" do
@ -178,7 +178,7 @@ RSpec.describe "Admin::Users", type: :request do
delete admin_user_path(user.id), params: { user_id: user.id, role: :trusted }
end.to change(user.roles, :count).by(-1)
expect(user.has_role?(:trusted)).to be false
expect(user.has_trusted_role?).to be false
expect(request.flash["success"]).to include("successfully removed from the user!")
end
@ -191,8 +191,8 @@ RSpec.describe "Admin::Users", type: :request do
params: { user_id: user.id, role: :single_resource_admin, resource_type: Comment }
end.to change(user.roles, :count).by(-1)
expect(user.has_role?(:single_resource_admin, Comment)).to be false
expect(user.has_role?(:single_resource_admin, Broadcast)).to be true
expect(user.single_resource_admin_for?(Comment)).to be false
expect(user.single_resource_admin_for?(Broadcast)).to be true
expect(request.flash["success"]).to include("successfully removed from the user!")
end
@ -203,7 +203,7 @@ RSpec.describe "Admin::Users", type: :request do
delete admin_user_path(user.id), params: { user_id: user.id, role: :super_admin }
end.not_to change(user.roles, :count)
expect(user.has_role?(:super_admin)).to be true
expect(user.super_admin?).to be true
expect(request.flash["danger"]).to include("cannot be removed.")
end
@ -214,7 +214,7 @@ RSpec.describe "Admin::Users", type: :request do
delete admin_user_path(super_admin.id), params: { user_id: super_admin.id, role: :trusted }
end.not_to change(super_admin.roles, :count)
expect(super_admin.has_role?(:trusted)).to be true
expect(super_admin.trusted?).to be true
expect(request.flash["danger"]).to include("cannot remove roles")
end
end

View file

@ -54,13 +54,13 @@ RSpec.describe "Podcast Create", type: :request do
it "creates a podcast_admin role when created by an owner" do
post podcasts_path, params: { podcast: valid_attributes, i_am_owner: "1" }
pod = Podcast.find_by(title: valid_attributes[:title])
expect(user.has_role?(:podcast_admin, pod)).to be true
expect(user.podcast_admin_for?(pod)).to be true
end
it "doesn't create a podcast_admin role when not created by an owner" do
post podcasts_path, params: { podcast: valid_attributes, i_am_owner: "" }
pod = Podcast.find_by(title: valid_attributes[:title])
expect(user.has_role?(:podcast_admin, pod)).to be false
expect(user.podcast_admin_for?(pod)).to be false
end
it "sets the creator" do

View file

@ -379,8 +379,8 @@ RSpec.describe "Registrations", type: :request do
email: "yoooo#{rand(100)}@yo.co",
password: "PaSSw0rd_yo000",
password_confirmation: "PaSSw0rd_yo000" } }
expect(User.first.has_role?(:super_admin)).to be true
expect(User.first.has_role?(:trusted)).to be true
expect(User.first.super_admin?).to be true
expect(User.first.trusted?).to be true
end
it "creates mascot user" do
@ -407,7 +407,7 @@ RSpec.describe "Registrations", type: :request do
password: "PaSSw0rd_yo000",
forem_owner_secret: "test",
password_confirmation: "PaSSw0rd_yo000" } }
expect(User.first.has_role?(:super_admin)).to be true
expect(User.first.super_admin?).to be true
end
it "does not authorize request in FOREM_OWNER_SECRET scenario if not passed correct value" do
@ -465,7 +465,7 @@ RSpec.describe "Registrations", type: :request do
email: "yoooo#{rand(100)}@yo.co",
password: "PaSSw0rd_yo000",
password_confirmation: "PaSSw0rd_yo000" } }
expect(User.first.has_role?(:super_admin)).to be true
expect(User.first.super_admin?).to be true
end
end
end

View file

@ -22,7 +22,7 @@ RSpec.describe Moderator::ManageActivityAndRoles, type: :service do
user: user,
user_params: { note_for_current_role: "Upgrading to super admin", user_status: "Super Admin" },
)
expect(user.has_role?(:super_admin)).to be true
expect(user.super_admin?).to be true
end
it "assigns trusted role to user that's updated to super admin" do
@ -31,8 +31,8 @@ RSpec.describe Moderator::ManageActivityAndRoles, type: :service do
user: user,
user_params: { note_for_current_role: "Upgrading to super admin", user_status: "Super Admin" },
)
expect(user.has_role?(:super_admin)).to be true
expect(user.has_role?(:trusted)).to be true
expect(user.super_admin?).to be true
expect(user.has_trusted_role?).to be true
end
it "updates user to admin" do
@ -41,7 +41,7 @@ RSpec.describe Moderator::ManageActivityAndRoles, type: :service do
user: user,
user_params: { note_for_current_role: "Upgrading to admin", user_status: "Admin" },
)
expect(user.has_role?(:admin)).to be true
expect(user.admin?).to be true
end
it "assigns trusted role to user that's updated to admin" do
@ -50,8 +50,8 @@ RSpec.describe Moderator::ManageActivityAndRoles, type: :service do
user: user,
user_params: { note_for_current_role: "Upgrading to admin", user_status: "Admin" },
)
expect(user.has_role?(:admin)).to be true
expect(user.has_role?(:trusted)).to be true
expect(user.admin?).to be true
expect(user.has_trusted_role?).to be true
end
it "updates user to tech admin" do
@ -60,8 +60,8 @@ RSpec.describe Moderator::ManageActivityAndRoles, type: :service do
user: user,
user_params: { note_for_current_role: "Upgrading to tech admin", user_status: "Tech Admin" },
)
expect(user.has_role?(:tech_admin)).to be true
expect(user.has_role?(:single_resource_admin, DataUpdateScript)).to be true
expect(user.tech_admin?).to be true
expect(user.single_resource_admin_for?(DataUpdateScript)).to be true
end
it "updates user to single resource admin" do
@ -70,7 +70,7 @@ RSpec.describe Moderator::ManageActivityAndRoles, type: :service do
user: user,
user_params: { note_for_current_role: "Upgrading to super admin", user_status: "Resource Admin: Article" },
)
expect(user.has_role?(:single_resource_admin, Article)).to be true
expect(user.single_resource_admin_for?(Article)).to be true
end
it "updates negative role to positive role" do

View file

@ -45,7 +45,7 @@ RSpec.describe "Admin bans user", type: :system do
expect(user.warned?).to eq(true)
expect(Note.last.reason).to eq "Warn"
expect(user.has_role?(:tag_moderator)).to eq(false)
expect(user.tag_moderator?).to eq(false)
end
# to-do: add spec for invalid bans
@ -61,15 +61,15 @@ RSpec.describe "Admin bans user", type: :system do
suspend_user
expect(user.suspended?).to eq(true)
expect(user.trusted).to eq(false)
expect(user.trusted?).to eq(false)
expect(user.warned?).to eq(false)
expect(user.has_role?(:tag_moderator)).to eq(false)
expect(user.tag_moderator?).to eq(false)
end
it "unsuspends user" do
user.add_role(:suspended)
unsuspend_user
expect(user.has_role?(:suspended)).to eq(false)
expect(user.suspended?).to eq(false)
end
end

View file

@ -21,7 +21,7 @@ RSpec.describe Moderator::BanishUserWorker, type: :worker do
it "makes user suspended and username spam" do
expect(user.username).to include("spam")
expect(user.has_role?(:suspended)).to be true
expect(user.suspended?).to be true
end
it "deletes user content" do