From 87e1622e6b85132b948367f16262932ff912bc9b Mon Sep 17 00:00:00 2001 From: Jeremy Friesen Date: Thu, 10 Feb 2022 11:09:17 -0500 Subject: [PATCH] Favoring method aliasing over explicit delegation (#16526) Prior to this commit many of the policy methods were of the form: ``` def create? edit? end ``` The above works, but introduces a place for code drift. We can shorten this to: ``` alias create? edit? ``` This helps keep the policy methods compact and scannable. All things authorization policy related should create narrow surface areas. By hand-crafting methods, it is easier for "slippage" to occur. Another advantage of `alias` is it's use is reflected in API documentation. With alias you need only write documentinig comments for one method and the doc generators will relate the alias and the original method. If you "hand craft" a method using explicit delegation, the API documentation won't pick up that two methods are aliases. Further, I believe we should be moving towards parity between the GET/POST/PUT methods that are related (e.g. the create/new pair and the update/edit pair). This is not the case, as we "silently" suspend folks. As we move through the [authorization work][1], we will start hiding the links/buttons when a user can't take the action. Related to #16483 and #16523 [1]:https://github.com/orgs/forem/projects/46 --- app/policies/article_policy.rb | 22 +++++--------- app/policies/comment_policy.rb | 22 +++++--------- app/policies/consumer_app_policy.rb | 12 ++------ app/policies/discussion_lock_policy.rb | 4 +-- app/policies/github_repo_policy.rb | 4 +-- app/policies/html_variant_policy.rb | 24 ++++----------- app/policies/listing_policy.rb | 12 ++------ app/policies/message_policy.rb | 4 +-- app/policies/organization_policy.rb | 4 +-- app/policies/pinned_article_policy.rb | 8 ++--- app/policies/response_template_policy.rb | 8 ++--- app/policies/stripe_active_card_policy.rb | 4 +-- app/policies/stripe_subscription_policy.rb | 4 +-- app/policies/tag_policy.rb | 4 +-- app/policies/user_block_policy.rb | 4 +-- app/policies/user_policy.rb | 34 ++++++---------------- app/policies/video_policy.rb | 4 +-- 17 files changed, 48 insertions(+), 130 deletions(-) diff --git a/app/policies/article_policy.rb b/app/policies/article_policy.rb index 064033723..c8c569d81 100644 --- a/app/policies/article_policy.rb +++ b/app/policies/article_policy.rb @@ -28,25 +28,17 @@ class ArticlePolicy < ApplicationPolicy !user_suspended? end - def delete_confirm? - update? - end + alias delete_confirm? update? - def discussion_lock_confirm? - update? - end + alias discussion_lock_confirm? update? - def discussion_unlock_confirm? - update? - end + alias discussion_unlock_confirm? update? - def destroy? - update? - end + alias destroy? update? - def preview? - true - end + alias edit? update? + + alias preview? new? def stats? user_author? || user_admin? || user_org_admin? diff --git a/app/policies/comment_policy.rb b/app/policies/comment_policy.rb index c5ac2623b..368d36f21 100644 --- a/app/policies/comment_policy.rb +++ b/app/policies/comment_policy.rb @@ -7,21 +7,15 @@ class CommentPolicy < ApplicationPolicy !user_suspended? && !user.comment_suspended? end - def update? - edit? - end + alias new? create? - def destroy? - edit? - end + alias update? edit? - def delete_confirm? - edit? - end + alias destroy? edit? - def settings? - edit? - end + alias delete_confirm? edit? + + alias settings? edit? def preview? true @@ -35,9 +29,7 @@ class CommentPolicy < ApplicationPolicy user_commentable_author? end - def unhide? - user_commentable_author? - end + alias unhide? hide? def admin_delete? minimal_admin? diff --git a/app/policies/consumer_app_policy.rb b/app/policies/consumer_app_policy.rb index ce9a9456d..831583035 100644 --- a/app/policies/consumer_app_policy.rb +++ b/app/policies/consumer_app_policy.rb @@ -7,15 +7,9 @@ class ConsumerAppPolicy < ApplicationPolicy @record.creator_app? end - def edit? - @record.creator_app? - end + alias edit? create? - def update? - @record.creator_app? - end + alias update? create? - def destroy? - @record.creator_app? - end + alias destroy? create? end diff --git a/app/policies/discussion_lock_policy.rb b/app/policies/discussion_lock_policy.rb index df03b1fe3..3f05b36cb 100644 --- a/app/policies/discussion_lock_policy.rb +++ b/app/policies/discussion_lock_policy.rb @@ -5,9 +5,7 @@ class DiscussionLockPolicy < ApplicationPolicy (user_author? || minimal_admin?) && !user_suspended? end - def destroy? - create? - end + alias destroy? create? def permitted_attributes PERMITTED_ATTRIBUTES diff --git a/app/policies/github_repo_policy.rb b/app/policies/github_repo_policy.rb index f78a259b1..59f8e3ef0 100644 --- a/app/policies/github_repo_policy.rb +++ b/app/policies/github_repo_policy.rb @@ -3,9 +3,7 @@ class GithubRepoPolicy < ApplicationPolicy !user_suspended? && user.authenticated_through?(:github) end - def update_or_create? - !user_suspended? && user.authenticated_through?(:github) - end + alias update_or_create? index? def permitted_attributes %i[github_id_code featured] diff --git a/app/policies/html_variant_policy.rb b/app/policies/html_variant_policy.rb index dc7935d66..9fcfe7ee1 100644 --- a/app/policies/html_variant_policy.rb +++ b/app/policies/html_variant_policy.rb @@ -3,29 +3,17 @@ class HtmlVariantPolicy < ApplicationPolicy minimal_admin? end - def show? - minimal_admin? - end + alias show? minimal_admin? - def edit? - minimal_admin? - end + alias edit? minimal_admin? - def update? - minimal_admin? - end + alias update? minimal_admin? - def new? - minimal_admin? - end + alias new? minimal_admin? - def create? - minimal_admin? - end + alias create? minimal_admin? - def destroy? - minimal_admin? - end + alias destroy? minimal_admin? def permitted_attributes %i[html name published approved target_tag group] diff --git a/app/policies/listing_policy.rb b/app/policies/listing_policy.rb index d94d0cd70..bd249fb24 100644 --- a/app/policies/listing_policy.rb +++ b/app/policies/listing_policy.rb @@ -3,21 +3,15 @@ class ListingPolicy < ApplicationPolicy user_author? || authorized_organization_admin_editor? end - def update? - user_author? || authorized_organization_admin_editor? - end + alias update? edit? def authorized_organization_poster? user.org_member?(record.organization_id) end - def delete_confirm? - update? - end + alias delete_confirm? edit? - def destroy? - update? - end + alias destroy? edit? private diff --git a/app/policies/message_policy.rb b/app/policies/message_policy.rb index a735c75e1..05d95c2e2 100644 --- a/app/policies/message_policy.rb +++ b/app/policies/message_policy.rb @@ -7,9 +7,7 @@ class MessagePolicy < ApplicationPolicy user_sender? end - def update? - destroy? - end + alias update? destroy? def permitted_attributes_for_update %i[message_markdown] diff --git a/app/policies/organization_policy.rb b/app/policies/organization_policy.rb index 9479add56..ca6557053 100644 --- a/app/policies/organization_policy.rb +++ b/app/policies/organization_policy.rb @@ -27,7 +27,5 @@ class OrganizationPolicy < ApplicationPolicy OrganizationMembership.exists?(user_id: user.id, organization_id: record.id, type_of_user: "admin") end - def generate_new_secret? - update? - end + alias generate_new_secret? update? end diff --git a/app/policies/pinned_article_policy.rb b/app/policies/pinned_article_policy.rb index ccbcbf012..16ea1b681 100644 --- a/app/policies/pinned_article_policy.rb +++ b/app/policies/pinned_article_policy.rb @@ -3,11 +3,7 @@ class PinnedArticlePolicy < ApplicationPolicy user&.any_admin? end - def update? - show? - end + alias update? show? - def destroy? - show? - end + alias destroy? show? end diff --git a/app/policies/response_template_policy.rb b/app/policies/response_template_policy.rb index ef88a297c..3c48254ae 100644 --- a/app/policies/response_template_policy.rb +++ b/app/policies/response_template_policy.rb @@ -13,9 +13,7 @@ class ResponseTemplatePolicy < ApplicationPolicy user_moderator? end - def create? - true - end + alias create? index? # comes from comments_controller def moderator_create? @@ -26,9 +24,7 @@ class ResponseTemplatePolicy < ApplicationPolicy user_owner? end - def update? - user_owner? - end + alias update? destroy? def permitted_attributes_for_create PERMITTED_ATTRIBUTES diff --git a/app/policies/stripe_active_card_policy.rb b/app/policies/stripe_active_card_policy.rb index a3e906d3a..ace29be70 100644 --- a/app/policies/stripe_active_card_policy.rb +++ b/app/policies/stripe_active_card_policy.rb @@ -3,9 +3,7 @@ class StripeActiveCardPolicy < ApplicationPolicy !user_suspended? end - def update? - !user_suspended? - end + alias update? create? def destroy? true diff --git a/app/policies/stripe_subscription_policy.rb b/app/policies/stripe_subscription_policy.rb index 0aed47557..a1be09c5c 100644 --- a/app/policies/stripe_subscription_policy.rb +++ b/app/policies/stripe_subscription_policy.rb @@ -3,9 +3,7 @@ class StripeSubscriptionPolicy < ApplicationPolicy !user_suspended? end - def update? - !user_suspended? - end + alias update? create? def destroy? true diff --git a/app/policies/tag_policy.rb b/app/policies/tag_policy.rb index b4793f7e5..ae9c735b8 100644 --- a/app/policies/tag_policy.rb +++ b/app/policies/tag_policy.rb @@ -7,9 +7,7 @@ class TagPolicy < ApplicationPolicy has_mod_permission? end - def update? - has_mod_permission? - end + alias update? edit? def admin? user_admin? diff --git a/app/policies/user_block_policy.rb b/app/policies/user_block_policy.rb index ac6cbb09a..4e9e4c77d 100644 --- a/app/policies/user_block_policy.rb +++ b/app/policies/user_block_policy.rb @@ -3,9 +3,7 @@ class UserBlockPolicy < ApplicationPolicy !user_suspended? end - def destroy? - !user_suspended? - end + alias destroy? create? def permitted_attributes %i[id blocked_id] diff --git a/app/policies/user_policy.rb b/app/policies/user_policy.rb index 4e7f9277a..49467fc9b 100644 --- a/app/policies/user_policy.rb +++ b/app/policies/user_policy.rb @@ -54,33 +54,21 @@ class UserPolicy < ApplicationPolicy true end - def onboarding_checkbox_update? - true - end + alias onboarding_checkbox_update? onboarding_update? - def onboarding_notifications_checkbox_update? - true - end + alias onboarding_notifications_checkbox_update? onboarding_update? def update? - current_user? && !user_suspended? + edit? && !user_suspended? end - def destroy? - current_user? - end + alias destroy? edit? - def confirm_destroy? - current_user? - end + alias confirm_destroy? edit? - def full_delete? - current_user? - end + alias full_delete? edit? - def request_destroy? - current_user? - end + alias request_destroy? edit? def join_org? !user_suspended? @@ -90,9 +78,7 @@ class UserPolicy < ApplicationPolicy OrganizationMembership.exists?(user_id: user.id, organization_id: record.id) end - def remove_identity? - current_user? - end + alias remove_identity? edit? def dashboard_show? current_user? || user_admin? || minimal_admin? @@ -102,9 +88,7 @@ class UserPolicy < ApplicationPolicy (user.has_trusted_role? || minimal_admin?) && !user.suspended? end - def update_password? - current_user? - end + alias update_password? edit? def permitted_attributes PERMITTED_ATTRIBUTES diff --git a/app/policies/video_policy.rb b/app/policies/video_policy.rb index 6982371ea..4014d00dc 100644 --- a/app/policies/video_policy.rb +++ b/app/policies/video_policy.rb @@ -3,9 +3,7 @@ class VideoPolicy < ApplicationPolicy user.created_at < 2.weeks.ago if user.created_at end - def create? - user.created_at < 2.weeks.ago if user.created_at - end + alias create? new? def enabled? Settings::General.enable_video_upload