diff --git a/app/controllers/admin/creator_settings_controller.rb b/app/controllers/admin/creator_settings_controller.rb index 9c49d05b6..bd99da2bd 100644 --- a/app/controllers/admin/creator_settings_controller.rb +++ b/app/controllers/admin/creator_settings_controller.rb @@ -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 diff --git a/app/controllers/admin/settings/base_controller.rb b/app/controllers/admin/settings/base_controller.rb index 6604e4192..8a44891eb 100644 --- a/app/controllers/admin/settings/base_controller.rb +++ b/app/controllers/admin/settings/base_controller.rb @@ -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 diff --git a/app/controllers/api/v0/api_controller.rb b/app/controllers/api/v0/api_controller.rb index 49a1f298b..ef606a255 100644 --- a/app/controllers/api/v0/api_controller.rb +++ b/app/controllers/api/v0/api_controller.rb @@ -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 diff --git a/app/controllers/api/v0/articles_controller.rb b/app/controllers/api/v0/articles_controller.rb index 2635498a2..5cf10e047 100644 --- a/app/controllers/api/v0/articles_controller.rb +++ b/app/controllers/api/v0/articles_controller.rb @@ -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) diff --git a/app/controllers/moderations_controller.rb b/app/controllers/moderations_controller.rb index ee12749ef..2ef8fcf73 100644 --- a/app/controllers/moderations_controller.rb +++ b/app/controllers/moderations_controller.rb @@ -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 diff --git a/app/liquid_tags/liquid_tag_base.rb b/app/liquid_tags/liquid_tag_base.rb index 924de3f56..3fe160e12 100644 --- a/app/liquid_tags/liquid_tag_base.rb +++ b/app/liquid_tags/liquid_tag_base.rb @@ -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 diff --git a/app/liquid_tags/poll_tag.rb b/app/liquid_tags/poll_tag.rb index 35abe039a..264ff008f 100644 --- a/app/liquid_tags/poll_tag.rb +++ b/app/liquid_tags/poll_tag.rb @@ -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 diff --git a/app/liquid_tags/user_subscription_tag.rb b/app/liquid_tags/user_subscription_tag.rb index 83ba0e2f8..a5802c9c2 100644 --- a/app/liquid_tags/user_subscription_tag.rb +++ b/app/liquid_tags/user_subscription_tag.rb @@ -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], diff --git a/app/models/tag_adjustment.rb b/app/models/tag_adjustment.rb index 75fa9a084..5e8981e98 100644 --- a/app/models/tag_adjustment.rb +++ b/app/models/tag_adjustment.rb @@ -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 diff --git a/app/models/user.rb b/app/models/user.rb index 10adabb9e..8e0f18040 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -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: , on: )` + # + # However, this is a refactor, and its goal is to reduce the direct + # calls to user.. + 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 diff --git a/app/policies/application_policy.rb b/app/policies/application_policy.rb index f704132ef..3c8509ac2 100644 --- a/app/policies/application_policy.rb +++ b/app/policies/application_policy.rb @@ -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 diff --git a/app/policies/authorizer.rb b/app/policies/authorizer.rb new file mode 100644 index 000000000..070c440c8 --- /dev/null +++ b/app/policies/authorizer.rb @@ -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 diff --git a/app/policies/internal_policy.rb b/app/policies/internal_policy.rb index 04751d370..1d76ca55b 100644 --- a/app/policies/internal_policy.rb +++ b/app/policies/internal_policy.rb @@ -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 diff --git a/app/policies/liquid_tag_policy.rb b/app/policies/liquid_tag_policy.rb index ee831d2ed..2d34ab018 100644 --- a/app/policies/liquid_tag_policy.rb +++ b/app/policies/liquid_tag_policy.rb @@ -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 diff --git a/app/policies/tag_policy.rb b/app/policies/tag_policy.rb index 5d472fb82..b4793f7e5 100644 --- a/app/policies/tag_policy.rb +++ b/app/policies/tag_policy.rb @@ -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 diff --git a/app/policies/user_policy.rb b/app/policies/user_policy.rb index aab398b9d..3bcbca733 100644 --- a/app/policies/user_policy.rb +++ b/app/policies/user_policy.rb @@ -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? diff --git a/app/services/mailchimp/bot.rb b/app/services/mailchimp/bot.rb index 1b496bc4b..501f9c6b0 100644 --- a/app/services/mailchimp/bot.rb +++ b/app/services/mailchimp/bot.rb @@ -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" diff --git a/app/services/moderator/manage_activity_and_roles.rb b/app/services/moderator/manage_activity_and_roles.rb index 49e36e599..a28bac8ef 100644 --- a/app/services/moderator/manage_activity_and_roles.rb +++ b/app/services/moderator/manage_activity_and_roles.rb @@ -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 diff --git a/app/services/tag_moderators/add_trusted_role.rb b/app/services/tag_moderators/add_trusted_role.rb index 573ffb86a..59a9f1fa6 100644 --- a/app/services/tag_moderators/add_trusted_role.rb +++ b/app/services/tag_moderators/add_trusted_role.rb @@ -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) diff --git a/app/services/users/approved_liquid_tags.rb b/app/services/users/approved_liquid_tags.rb index 8163eda8c..b360a33d1 100644 --- a/app/services/users/approved_liquid_tags.rb +++ b/app/services/users/approved_liquid_tags.rb @@ -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 diff --git a/app/views/admin/pages/_form.html.erb b/app/views/admin/pages/_form.html.erb index 906c9ecea..415aaa5c0 100644 --- a/app/views/admin/pages/_form.html.erb +++ b/app/views/admin/pages/_form.html.erb @@ -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? %>

<%= link_to "Feature Flag", "/admin/feature_flags" %> diff --git a/app/views/admin/settings/_update_setting_button.html.erb b/app/views/admin/settings/_update_setting_button.html.erb index 2b8fe6fad..a2f2d28b8 100644 --- a/app/views/admin/settings/_update_setting_button.html.erb +++ b/app/views/admin/settings/_update_setting_button.html.erb @@ -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 %> diff --git a/app/views/admin/settings/show.html.erb b/app/views/admin/settings/show.html.erb index d5beff0a8..538e89594 100644 --- a/app/views/admin/settings/show.html.erb +++ b/app/views/admin/settings/show.html.erb @@ -1,5 +1,5 @@

- <% unless current_user.has_role?(:super_admin) %> + <% unless current_user.super_admin? %> <% 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 %> diff --git a/app/views/notifications/shared/_comment_box.html.erb b/app/views/notifications/shared/_comment_box.html.erb index f84ad6523..48ae4fcf0 100644 --- a/app/views/notifications/shared/_comment_box.html.erb +++ b/app/views/notifications/shared/_comment_box.html.erb @@ -12,7 +12,7 @@ <%= json_data["comment"]["processed_html"].html_safe %>
- <% if context == "moderation" && current_user.has_role?(:trusted) %> + <% if context == "moderation" && current_user.has_trusted_role? %>