From 7212a2ad512b9822fa45c9e0f59d08c8568a23bf Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Fri, 2 Dec 2022 11:15:43 +1000 Subject: [PATCH] FIX: Ensure chat channel slug uniqueness at DB level (#19277) There must have been a small loophole that allowed setting the channel slug in the DB which has led to conflicts in some cases. This commit fixes the conflicting chat channel slugs and then changes the channel slug index to a unique one in the DB. --- plugins/chat/app/models/chat_channel.rb | 2 +- ...58_make_channel_slugs_unique_with_index.rb | 58 +++++++++++++++++++ ...add_slug_unique_index_for_chat_channels.rb | 8 +++ ...0221201032830_drop_tmp_chat_slug_tables.rb | 12 ++++ .../chat/spec/models/category_channel_spec.rb | 20 +++++-- 5 files changed, 93 insertions(+), 7 deletions(-) create mode 100644 plugins/chat/db/migrate/20221201024458_make_channel_slugs_unique_with_index.rb create mode 100644 plugins/chat/db/migrate/20221201035918_add_slug_unique_index_for_chat_channels.rb create mode 100644 plugins/chat/db/post_migrate/20221201032830_drop_tmp_chat_slug_tables.rb diff --git a/plugins/chat/app/models/chat_channel.rb b/plugins/chat/app/models/chat_channel.rb index 1a0e47d7f3e..a8e4fdfec7f 100644 --- a/plugins/chat/app/models/chat_channel.rb +++ b/plugins/chat/app/models/chat_channel.rb @@ -144,6 +144,6 @@ end # # index_chat_channels_on_chatable_id (chatable_id) # index_chat_channels_on_chatable_id_and_chatable_type (chatable_id,chatable_type) -# index_chat_channels_on_slug (slug) +# index_chat_channels_on_slug (slug) UNIQUE # index_chat_channels_on_status (status) # diff --git a/plugins/chat/db/migrate/20221201024458_make_channel_slugs_unique_with_index.rb b/plugins/chat/db/migrate/20221201024458_make_channel_slugs_unique_with_index.rb new file mode 100644 index 00000000000..7f1c751d8cf --- /dev/null +++ b/plugins/chat/db/migrate/20221201024458_make_channel_slugs_unique_with_index.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true + +class MakeChannelSlugsUniqueWithIndex < ActiveRecord::Migration[7.0] + def up + DB.exec("CREATE TEMPORARY TABLE tmp_chat_channel_slugs_conflicts(id int, slug text)") + + channels_with_conflicting_slugs = DB.query(<<~SQL) + SELECT chat_channels.id, subquery.slug + FROM ( + SELECT slug, count(*) + FROM chat_channels + GROUP BY slug + HAVING count(*) > 1 + ) subquery + INNER JOIN chat_channels ON chat_channels.slug = subquery.slug + ORDER BY slug, created_at ASC + SQL + + current_slug = nil + slugs_to_update = [] + channels_with_conflicting_slugs.each do |channel| + if current_slug != channel.slug + current_slug = channel.slug + + # Continue since we want to keep the slug for the first + # matching channel and just update subsequent channels. + next + end + + # Deduplicate slugs with the channel IDs, we can always improve + # slugs later on. + slugs_to_update << [channel.id, "#{channel.slug}-#{channel.id}"] + end + + values_to_insert = + slugs_to_update.map do |channel_pair| + "(#{channel_pair[0]}, '#{PG::Connection.escape_string(channel_pair[1])}')" + end + + if values_to_insert.any? + DB.exec( + "INSERT INTO tmp_chat_channel_slugs_conflicts + VALUES #{values_to_insert.join(",\n")}", + ) + + DB.exec(<<~SQL) + UPDATE chat_channels cc + SET slug = tmp.slug + FROM tmp_chat_channel_slugs_conflicts tmp + WHERE cc.id = tmp.id + SQL + end + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/plugins/chat/db/migrate/20221201035918_add_slug_unique_index_for_chat_channels.rb b/plugins/chat/db/migrate/20221201035918_add_slug_unique_index_for_chat_channels.rb new file mode 100644 index 00000000000..1f738ef8de2 --- /dev/null +++ b/plugins/chat/db/migrate/20221201035918_add_slug_unique_index_for_chat_channels.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +class AddSlugUniqueIndexForChatChannels < ActiveRecord::Migration[7.0] + def change + remove_index :chat_channels, :slug + add_index :chat_channels, :slug, unique: true + end +end diff --git a/plugins/chat/db/post_migrate/20221201032830_drop_tmp_chat_slug_tables.rb b/plugins/chat/db/post_migrate/20221201032830_drop_tmp_chat_slug_tables.rb new file mode 100644 index 00000000000..2ac238d59d9 --- /dev/null +++ b/plugins/chat/db/post_migrate/20221201032830_drop_tmp_chat_slug_tables.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +class DropTmpChatSlugTables < ActiveRecord::Migration[7.0] + def up + DB.exec("DROP TABLE IF EXISTS tmp_chat_channel_slugs") + DB.exec("DROP TABLE IF EXISTS tmp_chat_channel_slugs_conflicts") + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/plugins/chat/spec/models/category_channel_spec.rb b/plugins/chat/spec/models/category_channel_spec.rb index 89c950f5f7a..17d9139f6e7 100644 --- a/plugins/chat/spec/models/category_channel_spec.rb +++ b/plugins/chat/spec/models/category_channel_spec.rb @@ -88,9 +88,7 @@ RSpec.describe CategoryChannel do subject(:channel) { Fabricate(:category_channel) } context "when slug is not provided" do - before do - channel.slug = nil - end + before { channel.slug = nil } it "uses channel name when present" do channel.name = "Some Cool Stuff" @@ -129,12 +127,20 @@ RSpec.describe CategoryChannel do end context "when there is a duplicate slug" do - before { Fabricate(:category_channel, slug: "awesome-channel") } + fab!(:awesome_channel) { Fabricate(:category_channel, slug: "awesome-channel") } it "adds a validation error" do channel.slug = "awesome-channel" channel.validate - expect(channel.errors.full_messages.first).to include(I18n.t("chat.category_channel.errors.is_already_in_use")) + expect(channel.errors.full_messages.first).to include( + I18n.t("chat.category_channel.errors.is_already_in_use"), + ) + end + + it "does not allow setting the slug conflict directly in SQL" do + expect { + DB.exec("UPDATE chat_channels SET slug = 'awesome-channel' WHERE id = #{channel.id}") + }.to raise_error(PG::UniqueViolation) end end @@ -144,7 +150,9 @@ RSpec.describe CategoryChannel do it "fails if slug contains non-ascii characters" do channel.slug = "sem-acentuação" channel.validate - expect(channel.errors.full_messages.first).to match(/#{I18n.t("chat.category_channel.errors.slug_contains_non_ascii_chars")}/) + expect(channel.errors.full_messages.first).to match( + /#{I18n.t("chat.category_channel.errors.slug_contains_non_ascii_chars")}/, + ) end end end