[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
This commit is contained in:
Alex 2020-06-30 14:53:18 -04:00 committed by GitHub
parent e364a6ad19
commit 347a4bbc75
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
11 changed files with 205 additions and 22 deletions

View file

@ -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])

View file

@ -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 = "<p>😔 There was an error in your markdown</p><hr><p>#{e}</p>"

11
app/errors/liquid_tags.rb Normal file
View file

@ -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

View file

@ -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

View file

@ -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

View file

@ -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

View file

@ -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

View file

@ -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!

View file

@ -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

View file

@ -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.**

View file

@ -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