From 755360eca895ab10e94fa7e9f09fd4e2db0140d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Tamargo?= Date: Fri, 9 Dec 2022 12:46:52 +0200 Subject: [PATCH] MBS-12781: Avoid admin accidentally breaking options trees When editing attributes, relationship types and relationship attributes, you can change their parent type. There's currently nothing stopping an admin from accidentally selecting the entity itself from the dropdown, which makes MB so confused it doesn't know where to show the entity - so it doesn't. Fun times. I also tried blocking linking a type as a child of its own child, which also causes similar breaking loop messes. I don't think it's possible to do that completely with the tools we have now, though. This code blocks a root from being set under any of its children and a direct parent from being set under its direct children, which should cover most possible cases; in a very deep tree a->b->c->d it's probably still possible to set b as a child of d but I don't think it's worth adding a new recursive query to get all child IDs just to avoid that scenario. --- .../Controller/Relationship/LinkType.pm | 2 +- .../Server/Form/Admin/Attributes.pm | 26 ++++++++++++++++ .../Server/Form/Admin/LinkAttributeType.pm | 28 +++++++++++++++-- lib/MusicBrainz/Server/Form/Admin/LinkType.pm | 31 +++++++++++++++++++ 4 files changed, 83 insertions(+), 4 deletions(-) diff --git a/lib/MusicBrainz/Server/Controller/Relationship/LinkType.pm b/lib/MusicBrainz/Server/Controller/Relationship/LinkType.pm index 5ddbbe6bbca..203765ed5e4 100644 --- a/lib/MusicBrainz/Server/Controller/Relationship/LinkType.pm +++ b/lib/MusicBrainz/Server/Controller/Relationship/LinkType.pm @@ -170,7 +170,7 @@ sub edit : Chained('load') RequireAuth(relationship_editor) init_object => { attributes => $attribs, map { $_ => $link_type->$_ } - qw( parent_id child_order name link_phrase reverse_link_phrase + qw( id parent_id child_order name link_phrase reverse_link_phrase long_link_phrase description priority documentation examples is_deprecated has_dates entity0_cardinality entity1_cardinality orderable_direction ) diff --git a/lib/MusicBrainz/Server/Form/Admin/Attributes.pm b/lib/MusicBrainz/Server/Form/Admin/Attributes.pm index 6341806c42d..5d5d2042e2b 100644 --- a/lib/MusicBrainz/Server/Form/Admin/Attributes.pm +++ b/lib/MusicBrainz/Server/Form/Admin/Attributes.pm @@ -5,6 +5,7 @@ use warnings; use HTML::FormHandler::Moose; use MusicBrainz::Server::Form::Utils qw( select_options_tree ); use MusicBrainz::Server::Constants qw( entities_with ); +use MusicBrainz::Server::Translation qw( l ); extends 'MusicBrainz::Server::Form'; with 'MusicBrainz::Server::Form::Role::Edit'; @@ -60,6 +61,31 @@ sub options_item_entity_type { return map { $_ => $_ } sort { $a cmp $b } entities_with($entity_type); } +after validate => sub { + my ($self) = @_; + + my $parent = $self->field('parent_id')->value ? + $self->ctx->model('LinkAttributeType')->get_by_id($self->field('parent_id')->value) : + undef; + my $own_id = defined $self->init_object ? $self->init_object->{id} : undef; + + if (defined $parent && defined $own_id) { + my $is_self_parent = $parent->id == $own_id; + if ($is_self_parent) { + $self->field('parent_id')->add_error( + l('An attribute cannot be its own parent.'), + ); + } else { + my $is_parent_parent = $parent->parent_id == $own_id; + if ($is_parent_parent) { + $self->field('parent_id')->add_error( + l('An attribute cannot be a child of its own child.'), + ); + } + } + } +}; + 1; =head1 COPYRIGHT AND LICENSE diff --git a/lib/MusicBrainz/Server/Form/Admin/LinkAttributeType.pm b/lib/MusicBrainz/Server/Form/Admin/LinkAttributeType.pm index 4d2be31a9fc..a04a2bca37d 100644 --- a/lib/MusicBrainz/Server/Form/Admin/LinkAttributeType.pm +++ b/lib/MusicBrainz/Server/Form/Admin/LinkAttributeType.pm @@ -53,10 +53,32 @@ after validate => sub { my $parent = $self->field('parent_id')->value ? $self->ctx->model('LinkAttributeType')->get_by_id($self->field('parent_id')->value) : undef; + my $own_id = defined $self->init_object ? $self->init_object->id : undef; + + if (defined $parent && defined $own_id) { + my $is_self_parent = $parent->id == $own_id; + if ($is_self_parent) { + $self->field('parent_id')->add_error( + l('An attribute cannot be its own parent.'), + ); + } else { + my $is_parent_root = $parent->root_id == $own_id; + my $is_parent_parent = $parent->parent_id == $own_id; + if ($is_parent_root || $is_parent_parent) { + $self->field('parent_id')->add_error( + l('An attribute cannot be a child of its own child.'), + ); + } + } + } - my $root = defined $parent ? ($parent->root_id // $parent->id) : 0; - if ($root == $INSTRUMENT_ROOT_ID) { - $self->field('parent_id')->add_error(l('Cannot add or edit instruments here; use the instrument editing forms instead.')); + my $is_root_instrument = defined $parent && ( + $parent->root_id == $INSTRUMENT_ROOT_ID || + $parent->id == $INSTRUMENT_ROOT_ID); + if ($is_root_instrument) { + $self->field('parent_id')->add_error( + l('Cannot add or edit instruments here; use the instrument editing forms instead.'), + ); } }; diff --git a/lib/MusicBrainz/Server/Form/Admin/LinkType.pm b/lib/MusicBrainz/Server/Form/Admin/LinkType.pm index 47087f5bdeb..73074177572 100644 --- a/lib/MusicBrainz/Server/Form/Admin/LinkType.pm +++ b/lib/MusicBrainz/Server/Form/Admin/LinkType.pm @@ -4,6 +4,7 @@ use warnings; use HTML::FormHandler::Moose; use MusicBrainz::Server::Form::Utils qw( select_options_tree ); +use MusicBrainz::Server::Translation qw( l ); extends 'MusicBrainz::Server::Form'; with 'MusicBrainz::Server::Form::Role::Edit'; @@ -126,6 +127,36 @@ sub options_parent_id return select_options_tree($self->ctx, $self->root, accessor => 'name'); } +after validate => sub { + my ($self) = @_; + + my $parent = $self->field('parent_id')->value ? + $self->ctx->model('LinkType')->get_by_id($self->field('parent_id')->value) : + undef; + my $own_id = defined $self->init_object + ? $self->init_object->{id} + : undef; + + if (defined $parent && defined $own_id) { + my $is_self_parent = $parent->id == $own_id; + if ($is_self_parent) { + $self->field('parent_id')->add_error( + l('A relationship type cannot be its own parent.'), + ); + } else { + $self->ctx->model('LinkType')->load_root_ids($parent); + + my $is_parent_root = $parent->root_id == $own_id; + my $is_parent_parent = $parent->parent_id == $own_id; + if ($is_parent_root || $is_parent_parent) { + $self->field('parent_id')->add_error( + l('A relationship type cannot be a child of its own child.'), + ); + } + } + } +}; + 1; =head1 COPYRIGHT AND LICENSE