From 347a4bbc75c5b182dc0eaeac88efddd88026cb6d Mon Sep 17 00:00:00 2001 From: Alex Date: Tue, 30 Jun 2020 14:53:18 -0400 Subject: [PATCH] [deploy] Add contexts and permissions to liquid tags (#8917) * Make liquid tags adhere to context and permission * Undo updates to all liquid tags and update error * ACTUALLY update errors this time * Typo * Refactor error handling and add support for Comment * Fix specs * Code cleanup * Prettify error message * Add comment clarifying error * parsed --> parse * Use a policy for validating roles * Fix pretty error message * Return early if valid source * Get rid of duplicate current_user * Add comment to Pundit.authorize * LiquidTagPolicy specs * Update liquid tag docs * Update docs * Use parse_context argument for clarity * Remove duplicate guard clause for VALID_ROLES * Update docs --- app/controllers/articles_controller.rb | 2 +- app/controllers/comments_controller.rb | 2 +- app/errors/liquid_tags.rb | 11 ++++ app/labor/markdown_parser.rb | 11 ++-- app/liquid_tags/liquid_tag_base.rb | 28 +++++++++++ app/liquid_tags/user_subscription_tag.rb | 21 ++++---- app/models/article.rb | 4 +- app/models/comment.rb | 2 +- app/policies/liquid_tag_policy.rb | 30 +++++++++++ docs/frontend/liquid-tags.md | 52 +++++++++++++++++-- spec/policies/liquid_tag_policy_spec.rb | 64 ++++++++++++++++++++++++ 11 files changed, 205 insertions(+), 22 deletions(-) create mode 100644 app/errors/liquid_tags.rb create mode 100644 app/policies/liquid_tag_policy.rb create mode 100644 spec/policies/liquid_tag_policy_spec.rb diff --git a/app/controllers/articles_controller.rb b/app/controllers/articles_controller.rb index 8aff7ea51..59cb02760 100644 --- a/app/controllers/articles_controller.rb +++ b/app/controllers/articles_controller.rb @@ -87,7 +87,7 @@ class ArticlesController < ApplicationController begin fixed_body_markdown = MarkdownFixer.fix_for_preview(params[:article_body]) parsed = FrontMatterParser::Parser.new(:md).call(fixed_body_markdown) - parsed_markdown = MarkdownParser.new(parsed.content, source: Article.new(user: current_user)) + parsed_markdown = MarkdownParser.new(parsed.content, source: Article.new, user: current_user) processed_html = parsed_markdown.finalize rescue StandardError => e @article = Article.new(body_markdown: params[:article_body]) diff --git a/app/controllers/comments_controller.rb b/app/controllers/comments_controller.rb index 0e1be38b4..5a874c10e 100644 --- a/app/controllers/comments_controller.rb +++ b/app/controllers/comments_controller.rb @@ -194,7 +194,7 @@ class CommentsController < ApplicationController begin permitted_body_markdown = permitted_attributes(Comment)[:body_markdown] fixed_body_markdown = MarkdownFixer.fix_for_preview(permitted_body_markdown) - parsed_markdown = MarkdownParser.new(fixed_body_markdown) + parsed_markdown = MarkdownParser.new(fixed_body_markdown, source: Comment.new, user: current_user) processed_html = parsed_markdown.finalize rescue StandardError => e processed_html = "

😔 There was an error in your markdown

#{e}

" diff --git a/app/errors/liquid_tags.rb b/app/errors/liquid_tags.rb new file mode 100644 index 000000000..b392803be --- /dev/null +++ b/app/errors/liquid_tags.rb @@ -0,0 +1,11 @@ +module LiquidTags + module Errors + class Error < StandardError + end + + # ParseContexts are options passed to initialize on a LiquidTag. + # An error is raised if any of those options are invalid. + class InvalidParseContext < Error + end + end +end diff --git a/app/labor/markdown_parser.rb b/app/labor/markdown_parser.rb index 85bdc026e..7f7caee80 100644 --- a/app/labor/markdown_parser.rb +++ b/app/labor/markdown_parser.rb @@ -4,9 +4,10 @@ class MarkdownParser WORDS_READ_PER_MINUTE = 275.0 - def initialize(content, source: {}) + def initialize(content, source: nil, user: nil) @content = content @source = source + @user = user end def finalize(link_attributes: {}) @@ -18,8 +19,9 @@ class MarkdownParser html = markdown.render(escaped_content) sanitized_content = sanitize_rendered_markdown(html) begin - parsed_liquid = Liquid::Template.parse(sanitized_content) - html = markdown.render(parsed_liquid.render(nil, registers: { source: @source })) + liquid_tag_options = { source: @source, user: @user } + parsed_liquid = Liquid::Template.parse(sanitized_content, liquid_tag_options) + html = markdown.render(parsed_liquid.render) rescue Liquid::SyntaxError => e html = e.message end @@ -94,7 +96,8 @@ class MarkdownParser cleaned_parsed = escape_liquid_tags_in_codeblock(@content) tags = [] - Liquid::Template.parse(cleaned_parsed).root.nodelist.each do |node| + liquid_tag_options = { source: @source, user: @user } + Liquid::Template.parse(cleaned_parsed, liquid_tag_options).root.nodelist.each do |node| tags << node.class if node.class.superclass.to_s == LiquidTagBase.to_s end tags.uniq diff --git a/app/liquid_tags/liquid_tag_base.rb b/app/liquid_tags/liquid_tag_base.rb index 04fe5f4cb..2ac1a56ee 100644 --- a/app/liquid_tags/liquid_tag_base.rb +++ b/app/liquid_tags/liquid_tag_base.rb @@ -3,6 +3,18 @@ class LiquidTagBase < Liquid::Tag "" end + def initialize(_tag_name, _content, parse_context) + super + validate_contexts + # This check issues DB queries so we keep it as the last one + Pundit.authorize( + parse_context.partial_options[:user], + self, + :initialize?, + policy_class: LiquidTagPolicy, + ) + end + def finalize_html(input) input.gsub(/ {2,}/, ""). gsub(/\n/m, " "). @@ -10,4 +22,20 @@ class LiquidTagBase < Liquid::Tag strip. html_safe end + + private + + def validate_contexts + return unless self.class.const_defined? "VALID_CONTEXTS" + + source = parse_context.partial_options[:source] + raise LiquidTags::Errors::InvalidParseContext, "No source found" unless source + + is_valid_source = self.class::VALID_CONTEXTS.include? source.class.name + return if is_valid_source + + valid_contexts = self.class::VALID_CONTEXTS.map(&:pluralize).join(", ") + invalid_source_error_msg = "Invalid context. This liquid tag can only be used in #{valid_contexts}." + raise LiquidTags::Errors::InvalidParseContext, invalid_source_error_msg + end end diff --git a/app/liquid_tags/user_subscription_tag.rb b/app/liquid_tags/user_subscription_tag.rb index 1b8692b07..91bd11661 100644 --- a/app/liquid_tags/user_subscription_tag.rb +++ b/app/liquid_tags/user_subscription_tag.rb @@ -1,5 +1,10 @@ class UserSubscriptionTag < LiquidTagBase PARTIAL = "liquids/user_subscription".freeze + VALID_CONTEXTS = %w[Article].freeze + VALID_ROLES = %i[ + admin + super_admin + ].freeze SCRIPT = <<~JAVASCRIPT.freeze function isUserSignedIn() { @@ -214,22 +219,20 @@ class UserSubscriptionTag < LiquidTagBase } JAVASCRIPT - def initialize(_tag_name, cta_text, _tokens) + def initialize(_tag_name, cta_text, parse_context) + super @cta_text = cta_text.strip + @source = parse_context.partial_options[:source] + @user = parse_context.partial_options[:user] end - def render(context) - source = context.registers[:source] - author = source&.user - author_profile_image = author&.profile_image_90 - author_username = author&.username - + def render(_context) ActionController::Base.new.render_to_string( partial: PARTIAL, locals: { cta_text: @cta_text, - author_profile_image: author_profile_image, - author_username: author_username + author_profile_image: @user&.profile_image_90, + author_username: @user&.username }, ) end diff --git a/app/models/article.rb b/app/models/article.rb index 1b587402c..57f7e36e7 100644 --- a/app/models/article.rb +++ b/app/models/article.rb @@ -363,7 +363,7 @@ class Article < ApplicationRecord "#{body_markdown}#{comments_blob}" end - MarkdownParser.new(content).tags_used + MarkdownParser.new(content, source: self, user: user).tags_used rescue StandardError [] end @@ -398,7 +398,7 @@ class Article < ApplicationRecord def evaluate_markdown fixed_body_markdown = MarkdownFixer.fix_all(body_markdown || "") parsed = FrontMatterParser::Parser.new(:md).call(fixed_body_markdown) - parsed_markdown = MarkdownParser.new(parsed.content, source: self) + parsed_markdown = MarkdownParser.new(parsed.content, source: self, user: user) self.reading_time = parsed_markdown.calculate_reading_time self.processed_html = parsed_markdown.finalize diff --git a/app/models/comment.rb b/app/models/comment.rb index 964199317..1b12860a1 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -152,7 +152,7 @@ class Comment < ApplicationRecord def evaluate_markdown fixed_body_markdown = MarkdownFixer.fix_for_comment(body_markdown) - parsed_markdown = MarkdownParser.new(fixed_body_markdown) + parsed_markdown = MarkdownParser.new(fixed_body_markdown, source: self, user: user) self.processed_html = parsed_markdown.finalize(link_attributes: { rel: "nofollow" }) wrap_timestamps_if_video_present! if commentable shorten_urls! diff --git a/app/policies/liquid_tag_policy.rb b/app/policies/liquid_tag_policy.rb new file mode 100644 index 000000000..812bbfd19 --- /dev/null +++ b/app/policies/liquid_tag_policy.rb @@ -0,0 +1,30 @@ +# 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 + + def initialize(user, record) + @user = user + @record = record + end + + def initialize? + return true unless record.class.const_defined?("VALID_ROLES") + 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_permitted_to_use_liquid_tag? + + true + end + + private + + def user_permitted_to_use_liquid_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)) + end +end diff --git a/docs/frontend/liquid-tags.md b/docs/frontend/liquid-tags.md index d4cc26efc..859219c9b 100644 --- a/docs/frontend/liquid-tags.md +++ b/docs/frontend/liquid-tags.md @@ -24,7 +24,8 @@ Develop them with that mindset in terms of naming things. They should be documented but also intuitive. They should also be fairly flexible in the arguments they take. Currently, this could use improvements. -_Note: Liquid tags are "compiled" when an article is saved. So you will need to re-save articles to see HTML changes._ +_Note: Liquid tags are "compiled" when an article is saved. So you will need to +re-save articles to see HTML changes._ Here is a bunch of liquid tags supported on DEV: @@ -68,11 +69,11 @@ base, like so... class KotlinTag < LiquidTagBase ``` -Each liquid tag contains an `initialize` method which takes arguments and a -`render` method which calls the appropriate view. +Each liquid tag contains an `initialize` method which takes arguments and calls +`super`. It also has a `render` method which calls the appropriate view. ```ruby - def initialize(tag_name, link, tokens) + def initialize(_tag_name, link, _parse_context) super stripped_link = ActionController::Base.helpers.strip_tags(link) the_link = stripped_link.split(" ").first @@ -106,3 +107,46 @@ etc. Here is an example of a good Liquid Tag pull request... https://github.com/thepracticaldev/dev.to/pull/3801 + +### Restricting liquid tags by roles + +To only allow users with specific roles to use a liquid tag, you need to define +a `VALID_ROLES` constant on the liquid tag itself. It needs to be an `Array` of +valid roles. For [single admin resource roles](/internal), it needs to be an +`Array` with the role and the resource. Here's an example: + +```ruby +class NewLiquidTag < LiquidTagBase + VALID_ROLES = [ + :admin, + [:single_resource_admin, NewLiquidTag] + ].freeze +end +``` + +Here we are saying that the `NewLiquidTag` is only usable by users with the +`admin` role or with a role of `:single_resource_admin` and a specified resource +of `NewLiquidTag`. + +**REMINDER: if you do not define a `VALID_ROLES` constant, the liquid tag will +be usable by all users by default.** + +### Restricting liquid tags by context + +Context, in terms of a liquid tag, is _where_ a liquid tag is being used (i.e. +`Article`, `Comment`, etc.). In other words, if you want to make a liquid tag +that can only be used in articles, you need to restrict the liquid tag by +context. + +To do this you need to add a `VALID_CONTEXTS` constant on the liquid tag itself. +It needs to be an `Array` of class names that are valid. For example, to +restrict a liquid tag to only be usable in articles you would do: + +```ruby +class NewLiquidTag < LiquidTagBase + VALID_CONTEXTS = %w[Article].freeze +end +``` + +**REMINDER: if you do not define a `VALID_CONTEXTS` constant the liquid tag will +be usable in all contexts by default.** diff --git a/spec/policies/liquid_tag_policy_spec.rb b/spec/policies/liquid_tag_policy_spec.rb new file mode 100644 index 000000000..ab76b0e08 --- /dev/null +++ b/spec/policies/liquid_tag_policy_spec.rb @@ -0,0 +1,64 @@ +require "rails_helper" + +RSpec.describe LiquidTagPolicy, type: :policy do + let(:liquid_tag) { instance_double(Liquid::Tag) } + + 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") + 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?) + end + + it "authorizes if user has the correct role" do + user = create(:user, :admin) + 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.not_to raise_error + end + + it "handles single resource roles" do + # TODO: (Alex Smith) - update roles to new liquid tag role for more relevant example/use + user = create(:user, :single_resource_admin, resource: Article) + parse_context = { source: article, user: user } + + allow(liquid_tag).to receive(:parse_context).and_return(parse_context) + stub_const("#{liquid_tag.class}::VALID_ROLES", [[:single_resource_admin, Article]]) + expect do + Pundit.authorize(user, liquid_tag, action, policy_class: described_class) + end.not_to raise_error + 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") + end + end +end