diff --git a/app/controllers/internal/configs_controller.rb b/app/controllers/internal/configs_controller.rb index c1c6f9569..d1923370d 100644 --- a/app/controllers/internal/configs_controller.rb +++ b/app/controllers/internal/configs_controller.rb @@ -113,6 +113,7 @@ class Internal::ConfigsController < Internal::ApplicationController rate_limit_image_upload rate_limit_published_article_creation rate_limit_organization_creation + rate_limit_user_subscription_creation ] end diff --git a/app/controllers/user_subscriptions_controller.rb b/app/controllers/user_subscriptions_controller.rb new file mode 100644 index 000000000..25640939a --- /dev/null +++ b/app/controllers/user_subscriptions_controller.rb @@ -0,0 +1,85 @@ +class UserSubscriptionsController < ApplicationController + before_action :authenticate_user! + + USER_SUBSCRIPTION_PARAMS = %i[source_type source_id subscriber_email].freeze + + def subscribed + params.require(%i[source_type source_id]) + source_type = params[:source_type] + source_id = params[:source_id] + + is_subscribed = UserSubscriptions::SubscriptionCacheChecker.call(current_user, source_type, source_id) + + render json: { is_subscribed: is_subscribed, success: true }, status: :ok + end + + def create + rate_limit!(:user_subscription_creation) + + source_type = user_subscription_params[:source_type] + return error_response("invalid source_type") unless UserSubscription::ALLOWED_TYPES.include?(source_type) + + source_id = user_subscription_params[:source_id] + user_subscription_source = source_type.constantize.find_by(id: source_id) + return error_response("source not found") unless active_source?(source_type, user_subscription_source) + + unless user_subscription_tag_enabled?(source_type, user_subscription_source) + return error_response("user subscriptions are not enabled for the requested source") + end + + return error_response("subscriber email mismatch") if subscriber_email_stale? + + @user_subscription = user_subscription_source.build_user_subscription(current_user) + + if @user_subscription.save + rate_limiter.track_limit_by_action(:user_subscription_creation) + render json: { message: "success", success: true }, status: :ok + else + error_response(@user_subscription.errors.full_messages.to_sentence) + end + end + + private + + def user_subscription_tag_enabled?(source_type, user_subscription_source) + liquid_tags = + case source_type + when "Article" + user_subscription_source.liquid_tags_used(:body) + else + user_subscription_source.liquid_tags_used + end + + liquid_tags.include?(UserSubscriptionTag) + end + + def active_source?(source_type, user_subscription_source) + return false unless user_subscription_source + + # Don't create new user subscriptions for inactive sources + # (i.e. unpublished Articles, deleted Comments, etc.) + case source_type + when "Article" + user_subscription_source.published? + else + false + end + end + + def error_response(msg) + render json: { error: msg, success: false }, status: :unprocessable_entity + end + + # This checks if the email address the user saw/consented to share is the + # same as their current email address. A mismatch occurs if a user updates + # their email address in a new/separate tab and then tries to subscribe on + # the old/stale tab without refreshing. In that case, the user would have + # consented to share their old email address instead of the current one. + def subscriber_email_stale? + current_user&.email != user_subscription_params[:subscriber_email] + end + + def user_subscription_params + params.require(:user_subscription).permit(USER_SUBSCRIPTION_PARAMS) + end +end diff --git a/app/helpers/rate_limit_checker_helper.rb b/app/helpers/rate_limit_checker_helper.rb index 5d449754f..8ccc859b7 100644 --- a/app/helpers/rate_limit_checker_helper.rb +++ b/app/helpers/rate_limit_checker_helper.rb @@ -59,6 +59,11 @@ module RateLimitCheckerHelper min: 1, placeholder: 2, description: "The number of times we will send a confirmation email to a user in a 2 minute period" + }, + rate_limit_user_subscription_creation: { + min: 0, + placeholder: 3, + description: "The number of user subscriptions a user can submit within 30 seconds" } }.freeze diff --git a/app/liquid_tags/user_subscription_tag.rb b/app/liquid_tags/user_subscription_tag.rb new file mode 100644 index 000000000..410419439 --- /dev/null +++ b/app/liquid_tags/user_subscription_tag.rb @@ -0,0 +1,9 @@ +class UserSubscriptionTag < LiquidTagBase + def initialize(_tag_name, cta_text, _tokens) + @cta_text = cta_text.strip + end + + def render(context); end +end + +Liquid::Template.register_tag("user_subscription", UserSubscriptionTag) diff --git a/app/models/article.rb b/app/models/article.rb index f653a0d66..fe6503da7 100644 --- a/app/models/article.rb +++ b/app/models/article.rb @@ -9,6 +9,7 @@ class Article < ApplicationRecord SEARCH_SERIALIZER = Search::ArticleSerializer SEARCH_CLASS = Search::FeedContent DATA_SYNC_CLASS = DataSync::Elasticsearch::Article + RESTRICTED_LIQUID_TAGS = [PollTag, UserSubscriptionTag].freeze acts_as_taggable_on :tags resourcify @@ -351,6 +352,22 @@ class Article < ApplicationRecord spaminess_rating: BlackBox.calculate_spaminess(self)) end + def liquid_tags_used(section = nil) + content = + case section + when :body + body_markdown + when :comments + comments_blob + else + "#{body_markdown}#{comments_blob}" + end + + MarkdownParser.new(content).tags_used + rescue StandardError + [] + end + private def search_score @@ -434,12 +451,6 @@ class Article < ApplicationRecord Rails.logger.error(e) end - def liquid_tags_used - MarkdownParser.new("#{body_markdown}#{comments_blob}").tags_used - rescue StandardError - [] - end - def update_notifications Notification.update_notifications(self, "Published") end @@ -535,9 +546,11 @@ class Article < ApplicationRecord errors.add(:canonical_url, "must not have spaces") if canonical_url.to_s.match?(/[[:space:]]/) end + # TODO: (Alex Smith) refactor liquid tag permissions + # # Admin only beta tags etc. def validate_liquid_tag_permissions - errors.add(:body_markdown, "must only use permitted tags") if liquid_tags_used.include?(PollTag) && !(user.has_role?(:super_admin) || user.has_role?(:admin)) + errors.add(:body_markdown, "must only use permitted tags") if (liquid_tags_used & RESTRICTED_LIQUID_TAGS).any? && !(user.has_role?(:super_admin) || user.has_role?(:admin)) end def create_slug diff --git a/app/models/site_config.rb b/app/models/site_config.rb index 7e0a351bc..1004eae65 100644 --- a/app/models/site_config.rb +++ b/app/models/site_config.rb @@ -107,6 +107,7 @@ class SiteConfig < RailsSettings::Base field :rate_limit_send_email_confirmation, type: :integer, default: 2 field :rate_limit_feedback_message_creation, type: :integer, default: 5 field :rate_limit_user_update, type: :integer, default: 5 + field :rate_limit_user_subscription_creation, type: :integer, default: 3 # Social Media field :staff_user_id, type: :integer, default: 1 diff --git a/app/models/user_subscription.rb b/app/models/user_subscription.rb index 8ebeff98f..269f860e2 100644 --- a/app/models/user_subscription.rb +++ b/app/models/user_subscription.rb @@ -4,6 +4,8 @@ class UserSubscription < ApplicationRecord ALLOWED_TYPES = %w[Article].freeze + counter_culture :subscriber, column_name: "subscribed_to_user_subscriptions_count" + belongs_to :author, class_name: "User", inverse_of: :source_authored_user_subscriptions belongs_to :subscriber, class_name: "User", inverse_of: :subscribed_to_user_subscriptions belongs_to :user_subscription_sourceable, polymorphic: true diff --git a/app/services/rate_limit_checker.rb b/app/services/rate_limit_checker.rb index 477d3dcec..d45126b30 100644 --- a/app/services/rate_limit_checker.rb +++ b/app/services/rate_limit_checker.rb @@ -11,6 +11,7 @@ class RateLimitChecker published_article_creation: { retry_after: 30 }, reaction_creation: { retry_after: 30 }, send_email_confirmation: { retry_after: 120 }, + user_subscription_creation: { retry_after: 30 }, user_update: { retry_after: 30 } }.with_indifferent_access.freeze diff --git a/app/services/user_subscriptions/subscription_cache_checker.rb b/app/services/user_subscriptions/subscription_cache_checker.rb new file mode 100644 index 000000000..aaa220331 --- /dev/null +++ b/app/services/user_subscriptions/subscription_cache_checker.rb @@ -0,0 +1,26 @@ +module UserSubscriptions + class SubscriptionCacheChecker + attr_accessor :user, :source_type, :source_id + + def self.call(*args) + new(*args).call + end + + def initialize(user, source_type, source_id) + @user = user + @source_type = source_type + @source_id = source_id + end + + def call + cache_key = "user-#{user.id}-#{user.updated_at.rfc3339}-#{user.subscribed_to_user_subscriptions_count}/is_subscribed_#{source_type}_#{source_id}" + Rails.cache.fetch(cache_key, expires_in: 24.hours) do + UserSubscription.where( + subscriber_id: user.id, + user_subscription_sourceable_type: source_type, + user_subscription_sourceable_id: source_id, + ).any? + end + end + end +end diff --git a/config/routes.rb b/config/routes.rb index c154c0a60..0b9e1369f 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -235,6 +235,11 @@ Rails.application.routes.draw do resources :podcasts, only: %i[new create] resources :article_approvals, only: %i[create] resources :video_chats, only: %i[show] + resources :user_subscriptions, only: %i[create] do + collection do + get "/subscribed", action: "subscribed" + end + end namespace :followings, defaults: { format: :json } do get :users get :tags diff --git a/db/migrate/20200618212422_add_subscribed_to_user_subscriptions_count_to_users.rb b/db/migrate/20200618212422_add_subscribed_to_user_subscriptions_count_to_users.rb new file mode 100644 index 000000000..c00b63848 --- /dev/null +++ b/db/migrate/20200618212422_add_subscribed_to_user_subscriptions_count_to_users.rb @@ -0,0 +1,9 @@ +class AddSubscribedToUserSubscriptionsCountToUsers < ActiveRecord::Migration[6.0] + def self.up + add_column :users, :subscribed_to_user_subscriptions_count, :integer, null: false, default: 0 + end + + def self.down + remove_column :users, :subscribed_to_user_subscriptions_count + end +end diff --git a/db/schema.rb b/db/schema.rb index ee58423a3..775e0fa1d 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2020_06_17_183058) do +ActiveRecord::Schema.define(version: 2020_06_18_212422) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -1276,6 +1276,7 @@ ActiveRecord::Schema.define(version: 2020_06_17_183058) do t.integer "spent_credits_count", default: 0, null: false t.string "stackoverflow_url" t.string "stripe_id_code" + t.integer "subscribed_to_user_subscriptions_count", default: 0, null: false t.text "summary" t.string "text_color_hex" t.string "twitch_url" diff --git a/spec/models/article_spec.rb b/spec/models/article_spec.rb index 92ff25d82..9d540b1c5 100644 --- a/spec/models/article_spec.rb +++ b/spec/models/article_spec.rb @@ -875,4 +875,30 @@ RSpec.describe Article, type: :model do end end end + + describe "#liquid_tags_used" do + let(:user_liquid_tag) { "{% user #{user.username} %}" } + let(:article_body_markdown) { "---\ntitle: Tag Liquid Tag#{rand(1000)}\npublished: true\n---\n\n#{user_liquid_tag}" } + let(:article_with_user_liquid_tag) { create(:article, body_markdown: article_body_markdown) } + let(:tag) { create(:tag) } + let(:tag_liquid_tag) { "{% tag #{tag.name} %}" } + let(:comment_with_tag_liquid_tag) { create(:comment, body_markdown: tag_liquid_tag, commentable: article_with_user_liquid_tag, score: 20) } + + before do + comment_with_tag_liquid_tag + article_with_user_liquid_tag.reload + end + + it "returns liquid tags from both the body and comments by default" do + expect(article_with_user_liquid_tag.liquid_tags_used).to match_array([TagTag, UserTag]) + end + + it "returns liquid tags from only the body of the Article" do + expect(article_with_user_liquid_tag.liquid_tags_used(:body)).to match_array([UserTag]) + end + + it "returns liquid tags from only the comments of the Article" do + expect(article_with_user_liquid_tag.liquid_tags_used(:comments)).to match_array([TagTag]) + end + end end diff --git a/spec/models/user_subscription_spec.rb b/spec/models/user_subscription_spec.rb index 373135d98..5569684e1 100644 --- a/spec/models/user_subscription_spec.rb +++ b/spec/models/user_subscription_spec.rb @@ -50,4 +50,25 @@ RSpec.describe UserSubscription, type: :model do end end end + + describe "counter_culture" do + let(:user) { create(:user) } + + context "when a UserSubscription is created" do + it "increments subscribed_to_user_subscriptions_count on user" do + expect do + create(:user_subscription, subscriber: user) + end.to change { user.reload.subscribed_to_user_subscriptions_count }.by(1) + end + end + + context "when a UserSubscription is destroyed" do + it "decrements subscribed_to_user_subscriptions_count on user" do + user_subscription = create(:user_subscription, subscriber: user) + expect do + user_subscription.destroy + end.to change { user.reload.subscribed_to_user_subscriptions_count }.by(-1) + end + end + end end diff --git a/spec/requests/internal/configs_spec.rb b/spec/requests/internal/configs_spec.rb index 9d3cdc3fc..249f783b4 100644 --- a/spec/requests/internal/configs_spec.rb +++ b/spec/requests/internal/configs_spec.rb @@ -361,6 +361,12 @@ RSpec.describe "/internal/config", type: :request do post "/internal/config", params: { site_config: { rate_limit_email_recipient: 3 }, confirmation: confirmation_message } end.to change(SiteConfig, :rate_limit_email_recipient).from(5).to(3) end + + it "updates rate_limit_user_subscription_creation" do + expect do + post "/internal/config", params: { site_config: { rate_limit_user_subscription_creation: 1 }, confirmation: confirmation_message } + end.to change(SiteConfig, :rate_limit_user_subscription_creation).from(3).to(1) + end end describe "Social Media" do diff --git a/spec/requests/user_subscriptions_spec.rb b/spec/requests/user_subscriptions_spec.rb new file mode 100644 index 000000000..d735e4cd1 --- /dev/null +++ b/spec/requests/user_subscriptions_spec.rb @@ -0,0 +1,174 @@ +require "rails_helper" + +RSpec.describe "UserSubscriptions", type: :request do + # TODO: (Alex Smith) remove super_admin restriction before final release + let(:super_admin_user) { create(:user, :super_admin) } + let(:user) { create(:user) } + + before { sign_in user } + + describe "GET /user_subscriptions/subscribed - UserSubscriptions#subscribed" do + it "raises an error for missing params" do + expect { get subscribed_user_subscriptions_path, params: {} }.to raise_error(ActionController::ParameterMissing) + end + + it "returns true if a user is already subscribed" do + article = create(:article) + + create(:user_subscription, + subscriber_id: user.id, + subscriber_email: user.email, + author_id: article.user_id, + user_subscription_sourceable: article) + + valid_params = { source_type: article.class_name, source_id: article.id } + get subscribed_user_subscriptions_path, params: valid_params + + expect(response).to have_http_status(:ok) + expect(response.parsed_body["is_subscribed"]).to eq true + end + + it "returns false if a user is not already subscribed" do + valid_params = { source_type: "Article", source_id: 999 } + get subscribed_user_subscriptions_path, params: valid_params + + expect(response).to have_http_status(:ok) + expect(response.parsed_body["is_subscribed"]).to eq false + end + end + + describe "POST /user_subscriptions - UserSubscriptions#create" do + it "creates a UserSubscription" do + article = create(:article, user: super_admin_user, body_markdown: "---\ntitle: User Subscription#{rand(1000)}\npublished: true\n---\n\n{% user_subscription 'CTA text' %}") + valid_attributes = { source_type: article.class_name, source_id: article.id, subscriber_email: user.email } + expect do + post user_subscriptions_path, + headers: { "Content-Type" => "application/json" }, + params: { user_subscription: valid_attributes }.to_json + end.to change(UserSubscription, :count).by(1) + + user_subscription = UserSubscription.last + expect(user_subscription.subscriber_id).to eq user.id + expect(user_subscription.author_id).to eq article.user_id + expect(user_subscription.user_subscription_sourceable_type).to eq article.class_name + expect(user_subscription.user_subscription_sourceable_id).to eq article.id + end + + it "returns an error for an invalid source_type" do + invalid_source_type_attributes = { source_type: "NonExistentSourceType", source_id: "1", subscriber_email: user.email } + expect do + post user_subscriptions_path, + headers: { "Content-Type" => "application/json" }, + params: { user_subscription: invalid_source_type_attributes }.to_json + end.to change(UserSubscription, :count).by(0) + + expect(response).to have_http_status(:unprocessable_entity) + expect(response.parsed_body["error"]).to eq("invalid source_type") + end + + it "returns an error for a source that can't be found" do + invalid_source_attributes = { source_type: "Article", source_id: "99999999" } + expect do + post user_subscriptions_path, + headers: { "Content-Type" => "application/json" }, + params: { user_subscription: invalid_source_attributes }.to_json + end.to change(UserSubscription, :count).by(0) + + expect(response).to have_http_status(:unprocessable_entity) + expect(response.parsed_body["error"]).to eq("source not found") + end + + it "returns an error for an inactive source" do + unpublished_article = create(:article, user: super_admin_user, body_markdown: "---\ntitle: User Subscription#{rand(1000)}\npublished: false\n---\n\n{% user_subscription 'CTA text' %}") + invalid_source_attributes = { source_type: unpublished_article.class_name, source_id: unpublished_article.id, subscriber_email: user.email } + expect do + post user_subscriptions_path, + headers: { "Content-Type" => "application/json" }, + params: { user_subscription: invalid_source_attributes }.to_json + end.to change(UserSubscription, :count).by(0) + + expect(response).to have_http_status(:unprocessable_entity) + expect(response.parsed_body["error"]).to eq("source not found") + end + + it "returns an error for a source that doesn't have the UserSubscription liquid tag enabled" do + article = create(:article) + invalid_source_attributes = { source_type: article.class_name, source_id: article.id, subscriber_email: user.email } + expect do + post user_subscriptions_path, + headers: { "Content-Type" => "application/json" }, + params: { user_subscription: invalid_source_attributes }.to_json + end.to change(UserSubscription, :count).by(0) + + expect(response).to have_http_status(:unprocessable_entity) + expect(response.parsed_body["error"]).to eq("user subscriptions are not enabled for the requested source") + end + + it "returns an error for an invalid UserSubscription" do + article = create(:article, user: super_admin_user, body_markdown: "---\ntitle: User Subscription#{rand(1000)}\npublished: true\n---\n\n{% user_subscription 'CTA text' %}") + + # Create the UserSubscription directly so it results in a + # duplicate/invalid record and returns an error. This mimics a user + # trying to subscribe to the same user via the same source, twice. + create(:user_subscription, + subscriber_id: user.id, + subscriber_email: user.email, + author_id: article.user.id, + user_subscription_sourceable: article) + + invalid_source_attributes = { source_type: article.class_name, source_id: article.id, subscriber_email: user.email } + + expect do + post user_subscriptions_path, + headers: { "Content-Type" => "application/json" }, + params: { user_subscription: invalid_source_attributes }.to_json + end.to change(UserSubscription, :count).by(0) + + expect(response).to have_http_status(:unprocessable_entity) + expect(response.parsed_body["error"]).to eq("Subscriber has already been taken") + end + + it "returns an error for an email mismatch" do + article = create(:article, user: super_admin_user, body_markdown: "---\ntitle: User Subscription#{rand(1000)}\npublished: true\n---\n\n{% user_subscription 'CTA text' %}") + + invalid_source_attributes = { source_type: article.class_name, source_id: article.id, subscriber_email: "old_email@test.com" } + + expect do + post user_subscriptions_path, + headers: { "Content-Type" => "application/json" }, + params: { user_subscription: invalid_source_attributes }.to_json + end.to change(UserSubscription, :count).by(0) + + expect(response).to have_http_status(:unprocessable_entity) + expect(response.parsed_body["error"]).to eq("subscriber email mismatch") + end + end + + context "when rate limiting" do + let(:rate_limiter) { RateLimitChecker.new(user) } + let(:article) { create(:article, user: super_admin_user, body_markdown: "---\ntitle: User Subscription#{rand(1000)}\npublished: true\n---\n\n{% user_subscription 'CTA text' %}") } + let(:valid_attributes) { { source_type: article.class_name, source_id: article.id, subscriber_email: user.email } } + + before { allow(RateLimitChecker).to receive(:new).and_return(rate_limiter) } + + it "increments rate limit for user_subscription_creation" do + allow(rate_limiter).to receive(:track_limit_by_action) + post user_subscriptions_path, + headers: { "Content-Type" => "application/json" }, + params: { user_subscription: valid_attributes }.to_json + + expect(rate_limiter).to have_received(:track_limit_by_action).with(:user_subscription_creation) + end + + it "returns a 429 status when rate limit is reached" do + allow(rate_limiter).to receive(:limit_by_action).and_return(true) + post user_subscriptions_path, + headers: { "Content-Type" => "application/json" }, + params: { user_subscription: valid_attributes }.to_json + + expect(response).to have_http_status(:too_many_requests) + expected_retry_after = RateLimitChecker::ACTION_LIMITERS.dig(:user_subscription_creation, :retry_after) + expect(response.headers["Retry-After"]).to eq(expected_retry_after) + end + end +end diff --git a/spec/services/user_subscriptions/subscription_cache_checker_spec.rb b/spec/services/user_subscriptions/subscription_cache_checker_spec.rb new file mode 100644 index 000000000..7458f04c6 --- /dev/null +++ b/spec/services/user_subscriptions/subscription_cache_checker_spec.rb @@ -0,0 +1,20 @@ +require "rails_helper" + +RSpec.describe UserSubscriptions::SubscriptionCacheChecker, type: :service do + let(:user) { create(:user) } + let(:article) { create(:article) } + + it "checks if subscribed to a thing and returns true if they are" do + create(:user_subscription, + subscriber_id: user.id, + subscriber_email: user.email, + author_id: article.user_id, + user_subscription_sourceable: article) + + expect(described_class.call(user, article.class_name, article.id)).to eq(true) + end + + it "checks if subscribed to a thing and returns false if they are not" do + expect(described_class.call(user, article.class_name, article.id)).to eq(false) + end +end