From 584a7ac43c1468a1873f15673e1aeffadeb84511 Mon Sep 17 00:00:00 2001 From: weeklies <80141759+weeklies@users.noreply.github.com> Date: Tue, 18 Feb 2025 19:49:49 +0000 Subject: [PATCH] AO3-4820 AO3-3810 Validate skins with "archive" in title (#4771) * Validate skins with "archive" in title * AO3-4820-AO3-3810 Fix * Normalize * AO3-4820-AO3-3810 Remove support link --- app/controllers/skins_controller.rb | 8 ----- app/models/skin.rb | 30 ++++++++++++++----- config/locales/models/en.yml | 7 +++++ ...6174046_add_unique_index_to_skins_title.rb | 8 +++++ features/admins/admin_skins.feature | 12 ++++++++ features/other_b/skin.feature | 17 ++++------- features/step_definitions/skin_steps.rb | 15 +++++++--- 7 files changed, 67 insertions(+), 30 deletions(-) create mode 100644 db/migrate/20240416174046_add_unique_index_to_skins_title.rb diff --git a/app/controllers/skins_controller.rb b/app/controllers/skins_controller.rb index 16a2d1fcc03..848cb21d941 100644 --- a/app/controllers/skins_controller.rb +++ b/app/controllers/skins_controller.rb @@ -1,7 +1,6 @@ class SkinsController < ApplicationController before_action :users_only, only: [:new, :create, :destroy] before_action :load_skin, except: [:index, :new, :create, :unset] - before_action :check_title, only: [:create, :update] before_action :check_ownership_or_admin, only: [:edit, :update] before_action :check_ownership, only: [:confirm_delete, :destroy] before_action :check_visibility, only: [:show] @@ -198,13 +197,6 @@ def check_editability end end - def check_title - if params[:skin][:title].match(/archive/i) - flash[:error] = ts("You can't use the word 'archive' in your skin title, sorry! (We have to reserve it for official skins.)") - render @skin ? :edit : :new - end - end - # if we've been asked to load the archive parents, we do so and add them to params def load_archive_parents if params[:add_site_parents] diff --git a/app/models/skin.rb b/app/models/skin.rb index 793cb347607..b0d7ac19e96 100755 --- a/app/models/skin.rb +++ b/app/models/skin.rb @@ -94,7 +94,11 @@ def clean_media validate :valid_media def valid_media if media && media.is_a?(Array) && media.any? {|m| !MEDIA.include?(m)} - errors.add(:base, ts("We don't currently support the media type %{media}, sorry! If we should, please let Support know.", media: media.join(', '))) + errors.add( + :base, + :invalid_media, + media: media.join(", ") + ) end end @@ -104,12 +108,24 @@ def valid_media validate :valid_public_preview def valid_public_preview return true if self.official? || !self.public? || self.icon.attached? - - errors.add(:base, ts("You need to upload a screencap if you want to share your skin.")) + errors.add(:base, :no_public_preview) end - validates_presence_of :title - validates :title, uniqueness: { message: ts("must be unique"), case_sensitive: true } + validates :title, presence: true, uniqueness: { case_sensitive: true } + validate :allowed_title + def allowed_title + return true unless self.title.match(/archive/i) + + authorized_roles = if self.is_a?(WorkSkin) + %w[superadmin support] + else + %w[superadmin] + end + + return true if (User.current_user.roles & authorized_roles).present? + + errors.add(:base, :archive_in_title) + end validates_numericality_of :margin, :base_em, allow_nil: true validate :valid_font @@ -479,7 +495,7 @@ def self.load_site_css skin.unusable = true skin.official = true skin.icon.attach(io: File.open("#{version_dir}preview.png", "rb"), content_type: "image/png", filename: "preview.png") - skin.save! + skin.save!(validate: false) skins << skin end @@ -494,7 +510,7 @@ def self.load_site_css end top_skin.icon.attach(io: File.open("#{version_dir}preview.png", "rb"), content_type: "image/png", filename: "preview.png") top_skin.official = true - top_skin.save! + top_skin.save!(validate: false) skins.each_with_index do |skin, index| skin_parent = top_skin.skin_parents.build(child_skin: top_skin, parent_skin: skin, position: index+1) skin_parent.save! diff --git a/config/locales/models/en.yml b/config/locales/models/en.yml index eab07644eea..263fe42ee11 100644 --- a/config/locales/models/en.yml +++ b/config/locales/models/en.yml @@ -136,6 +136,13 @@ en: not_work: Only a link to a work can be listed as an inspiration. protected: You can't use the related works function to cite works by the protected user %{login}. format: "%{message}" + skin: + archive_in_title: Sorry, titles including the word 'Archive' are reserved for official skins. + attributes: + title: + taken: must be unique + invalid_media: We don't currently support the media type %{media}, sorry! If we should, please let Support know. + no_public_preview: You need to upload a screencap if you want to share your skin. skin/skin_parents: attributes: base: diff --git a/db/migrate/20240416174046_add_unique_index_to_skins_title.rb b/db/migrate/20240416174046_add_unique_index_to_skins_title.rb new file mode 100644 index 00000000000..bc31fd6ae19 --- /dev/null +++ b/db/migrate/20240416174046_add_unique_index_to_skins_title.rb @@ -0,0 +1,8 @@ +class AddUniqueIndexToSkinsTitle < ActiveRecord::Migration[6.1] + uses_departure! if Rails.env.staging? || Rails.env.production? + + def change + remove_index :skins, :title + add_index :skins, :title, unique: true + end +end diff --git a/features/admins/admin_skins.feature b/features/admins/admin_skins.feature index 03402175403..a4b13596862 100644 --- a/features/admins/admin_skins.feature +++ b/features/admins/admin_skins.feature @@ -137,3 +137,15 @@ Feature: Admin manage skins # A user created before changing the default skin will still have the same skin When I am logged in as "KnownUser" Then I should not see "{ text-decoration: blink; }" in the page style + + Scenario: Admin can edit a skin with the word "archive" in the title + Given the approved public skin "official archive skin" has reserved words in the title + And I am logged in as a "superadmin" admin + When I go to "official archive skin" skin page + And I follow "Edit" + And I fill in "CSS" with "#greeting.logged-in { text-decoration: blink;}" + And I fill in "Description" with "all your skin are belong to us" + And I submit + Then I should see an update confirmation message + And I should see "all your skin are belong to us" + And I should see "#greeting.logged-in" diff --git a/features/other_b/skin.feature b/features/other_b/skin.feature index a13dc513fdb..3093dba9267 100755 --- a/features/other_b/skin.feature +++ b/features/other_b/skin.feature @@ -182,18 +182,14 @@ Feature: Non-public site and work skins Scenario: The cache should be flushed with a parent and not when unrelated Given I have loaded site skins And I am logged in as "skinner" - When I set up the skin "Complex" - And I select "replace archive skin entirely" from "What it does:" - And I check "Load Archive Skin Components" - And I submit - Then I should see a create confirmation message + And I have a skin "Child" with a parent "Parent" When I am on the new skin page - And I fill in "Title" with "my blinking skin" + And I fill in "Title" with "Unrelated" And I fill in "CSS" with "#title { text-decoration: blink;}" And I submit Then I should see "Skin was successfully created" - And the cache of the skin on "my blinking skin" should not expire after I save "Complex" - And the cache of the skin on "Complex" should expire after I save a parent skin + And the cache of the skin on "Unrelated" should not expire after I save "Child" + And the cache of the skin on "Child" should expire after I save a parent skin Scenario: Users should be able to create skins using @media queries Given I am logged in as "skinner" @@ -231,9 +227,8 @@ Feature: Non-public site and work skins Scenario: A user can't make a skin with "Archive" in the title Given I am logged in as "skinner" And I set up the skin "My Archive Skin" with some css - When "AO3-4820" is fixed - # And I press "Submit" - # Then I should see "You can't use the word 'archive' in your skin title, sorry! (We have to reserve it for official skins.)" + And I press "Submit" + Then I should see "Sorry, titles including the word 'Archive' are reserved for official skins." Scenario: A user can't look at another user's skins Given the user "scully" exists and is activated diff --git a/features/step_definitions/skin_steps.rb b/features/step_definitions/skin_steps.rb index 0b0a8d3e3fb..dc4e9eb0ebd 100644 --- a/features/step_definitions/skin_steps.rb +++ b/features/step_definitions/skin_steps.rb @@ -80,10 +80,9 @@ Skin.load_site_css end -Given /^the approved public skin "([^"]*)" with css "([^"]*)"$/ do |skin_name, css| - step "the unapproved public skin \"#{skin_name}\" with css \"#{css}\"" - step "I am logged in as an admin" - step "I approve the skin \"#{skin_name}\"" +Given "the approved public skin {string} with css {string}" do |skin_name, css| + step %{the unapproved public skin "#{skin_name}" with css "#{css}"} + step %{I approve the skin "#{skin_name}"} step "I am logged out" end @@ -91,6 +90,14 @@ step "the approved public skin \"#{skin_name}\" with css #{DEFAULT_CSS}" end +Given "the approved public skin {string} has reserved words in the title" do |skin_name| + # Admins can't create skins, so we have to create it this way. + FactoryBot.build(:skin, title: skin_name, public: true).save!(validate: false) + + step %{I approve the skin "#{skin_name}"} + step "I am logged out" +end + Given /^"([^"]*)" is using the approved public skin "([^"]*)" with css "([^"]*)"$/ do |login, skin_name, css| step "the approved public skin \"public skin\" with css \"#{css}\"" step "I am logged in as \"#{login}\""