From f69480ce7784a0e60c826b24ad65ba4646cf8327 Mon Sep 17 00:00:00 2001 From: Andrew Ruthven Date: Sun, 10 Mar 2024 00:08:01 +1300 Subject: [PATCH] Test that deletion of a principal's items are secure Test that other users can't delete: - collections - tickets - bindings No significant change, just return an error message rather than assume that things worked. --- inc/ui/principal-edit.php | 27 ++- .../tests/webui/0005-delete-collection.result | 38 ++++ .../tests/webui/0005-delete-collection.test | 151 ++++++++++++++ testing/tests/webui/0006-delete-ticket.result | 45 +++++ testing/tests/webui/0006-delete-ticket.test | 186 ++++++++++++++++++ testing/tests/webui/0007-delete-bind.result | 40 ++++ testing/tests/webui/0007-delete-bind.test | 165 ++++++++++++++++ 7 files changed, 646 insertions(+), 6 deletions(-) create mode 100644 testing/tests/webui/0005-delete-collection.result create mode 100644 testing/tests/webui/0005-delete-collection.test create mode 100644 testing/tests/webui/0006-delete-ticket.result create mode 100644 testing/tests/webui/0006-delete-ticket.test create mode 100644 testing/tests/webui/0007-delete-bind.result create mode 100644 testing/tests/webui/0007-delete-bind.test diff --git a/inc/ui/principal-edit.php b/inc/ui/principal-edit.php index 9a1e6684..fc40763e 100644 --- a/inc/ui/principal-edit.php +++ b/inc/ui/principal-edit.php @@ -69,8 +69,13 @@ function handle_subaction( $subaction ) { dbg_error_log('admin-principal-edit',':handle_action: Allowed to delete collection %s for principal %d', $_GET['collection_id'], $id ); $qry = new AwlQuery('DELETE FROM collection WHERE collection_id=:collection_id AND user_no = (select user_no from principal where principal_id = :principal_id )', array( ':collection_id' => intval($_GET['collection_id']), ':principal_id' => $id)); if ( $qry->Exec() ) { - $c->messages[] = i18n('Collection deleted.'); - return true; + if ( $qry->rows() == 1) { + $c->messages[] = i18n('Collection deleted.'); + return true; + } else { + $c->messages[] = i18n('Collection deletion failed.'); + return false; + } } else { $c->messages[] = i18n('There was an error writing to the database.'); @@ -121,8 +126,13 @@ function handle_subaction( $subaction ) { dbg_error_log('admin-principal-edit',':handle_action: Allowed to delete ticket "%s" for principal %d', $_GET['ticket_id'], $id ); $qry = new AwlQuery('DELETE FROM access_ticket WHERE ticket_id=:ticket_id AND dav_owner_id = :dav_owner_id', array( ':ticket_id' => $_GET['ticket_id'], ':dav_owner_id' => $id)); if ( $qry->Exec() ) { - $c->messages[] = i18n('Access ticket deleted.'); - return true; + if ($qry->rows() == 1) { + $c->messages[] = i18n('Access ticket deleted.'); + return true; + } else { + $c->messages[] = i18n('Access ticket deletion failed.'); + return false; + } } else { $c->messages[] = i18n('There was an error writing to the database.'); @@ -148,8 +158,13 @@ function handle_subaction( $subaction ) { dbg_error_log('admin-principal-edit',':handle_action: Allowed to delete binding "%s" for principal %d', $_GET['bind_id'], $id ); $qry = new AwlQuery('DELETE FROM dav_binding WHERE bind_id=:bind_id AND dav_owner_id = :dav_owner_id', array( ':bind_id' => $_GET['bind_id'], ':dav_owner_id' => $id)); if ( $qry->Exec() ) { - $c->messages[] = i18n('Binding deleted.'); - return true; + if ( $qry->rows() == 1 ) { + $c->messages[] = i18n('Binding deleted.'); + return true; + } else { + $c->messages[] = i18n('Binding deletion failed.'); + return true; + } } else { $c->messages[] = i18n('There was an error writing to the database.'); diff --git a/testing/tests/webui/0005-delete-collection.result b/testing/tests/webui/0005-delete-collection.result new file mode 100644 index 00000000..72939a7f --- /dev/null +++ b/testing/tests/webui/0005-delete-collection.result @@ -0,0 +1,38 @@ +1..16 +# Subtest: Login + 1..4 + ok 1 - Fetch first page + ok 2 - Not logged in + ok 3 - Login to site + ok 4 - Logged in +ok 1 - Login +ok 2 - Create collection - 0 +ok 3 - Collection created message displayed - 0 +ok 4 - Create collection - 1 +ok 5 - Collection created message displayed - 1 +ok 6 - Create collection - 2 +ok 7 - Collection created message displayed - 2 +ok 8 - New collection exists - 0 +ok 9 - New collection exists - 1 +ok 10 - New collection exists - 2 +ok 11 # skip Database error thrown see #319 +ok 12 # skip Database error thrown see #319 +ok 13 - Collection deleted message displayed +# Subtest: Login as user1 + 1..4 + ok 1 - Fetch first page + ok 2 - Not logged in + ok 3 - Login to site + ok 4 - Logged in +ok 14 - Login as user1 +ok 15 - Collection deletion error displayed when specifying other principal and their collection +ok 16 - Collection deletion error display when specifying our principal and their collection + + dav_displayname: >Test Collection 1< + dav_name: >/user2/test_collection_1/< + user_no: >11< + + dav_displayname: >Test Collection 2< + dav_name: >/user2/test_collection_2/< + user_no: >11< + diff --git a/testing/tests/webui/0005-delete-collection.test b/testing/tests/webui/0005-delete-collection.test new file mode 100644 index 00000000..cd5ef528 --- /dev/null +++ b/testing/tests/webui/0005-delete-collection.test @@ -0,0 +1,151 @@ +MODE=TAP,16 + +BEGINPERL + +my $mech; + +subtest 'Login' => sub { + plan tests => 4; + + $mech = webui_login( + username => 'user2', + password => 'user2', + url => "http://$webhost", + ); +}; + +$mech->follow_link( text_regex => qr/View My Details/ ); + +(my $principal_id = $mech->uri()) =~ s/^.*&id=(\d+)$/$1/; + +$mech->follow_link( text_regex => qr/Create Collection/ ); +my $create_url = $mech->uri(); + +# Create 3 collections for testing deletion. +# 0 = Delete by the principal who created it. +# 1 = Try to delete by another principal +# 2 = Try to delete by another principal +my @col_id; + +for (my $i = 0; $i < 3; $i++) { + $col_id[$i] = create_collection($mech, $create_url, $i); +} + +$mech->follow_link( text_regex => qr/View My Details/ ); + +for (my $i = 0; $i < 3; $i++) { + $mech->content_contains( + "/user2/test_collection_$i", + "New collection exists - $i" + ); +} + +$mech->save_content("$save_location/$case-A", binmode => ':utf8'); + +SKIP: { + skip "Database error thrown see #319", 2; + +# Expect this one to fail. +create_collection($mech, $create_url, 0, 1); +} + +# Delete our first collection. +$mech->get("http://$webhost/admin.php?action=edit&t=principal&id=$principal_id&collection_id=" . $col_id[0] . "&subaction=delete_collection"); +$mech->follow_link( text_regex => qr/Confirm Deletion of the Collection/ ); + +$mech->content_contains( + 'Collection deleted', + 'Collection deleted message displayed' +); + +#diag("Saved content of B to $save_location/$case-B"); +#$mech->save_content("$save_location/$case-B", binmode => ':utf8'); + +my $mech_other; +subtest 'Login as user1' => sub { + plan tests => 4; + + $mech_other = webui_login( + username => 'user1', + password => 'user1', + url => "http://$webhost", + ); +}; + +$mech_other->follow_link( text_regex => qr/View My Details/ ); +(my $other_principal_id = $mech_other->uri()) =~ s/^.*&id=(\d+)$/$1/; + +# Try delete collection as another user, should be rejected. +$mech_other->get("http://$webhost/admin.php?action=edit&t=principal&id=$principal_id&collection_id=" . $col_id[1] . "&subaction=delete_collection"); +$mech->follow_link( text_regex => qr/Confirm Deletion of the Collection/ ); + +$mech_other->content_contains( + 'You are not allowed to delete collections for this principal.', + 'Collection deletion error displayed when specifying other principal and their collection' +); + +#diag("Saved content of C to $save_location/$case-C"); +#$mech_other->save_content("$save_location/$case-C", binmode => ':utf8'); + +# Try delete other users collection as us, should be rejected. +$mech_other->get("http://$webhost/admin.php?action=edit&t=principal&id=$other_principal_id&collection_id=" . $col_id[2] . "&subaction=delete_collection"); +$mech_other->follow_link( text_regex => qr/Confirm Deletion of the Collection/ ); + +$mech_other->content_contains( + 'Collection deletion failed.', + 'Collection deletion error display when specifying our principal and their collection' +); + +#diag("Saved content of D to $save_location/$case-D"); +#$mech_other->save_content("$save_location/$case-D", binmode => ':utf8'); + +sub create_collection { + my ($mech, $create_url, $i, $fail) = @_; + + $mech->get($create_url); + + # Create a collection + $mech->submit_form_ok( + { + form_number => 1, + button => 'submit', + fields => { + collection_name => "test_collection_$i", + dav_displayname => "Test Collection $i", + description => "Description for Collection $i", + }, + }, "Create collection - $i" + ); + + if (! defined $fail) { + $mech->content_contains( + 'Creating new Collection.', + "Collection created message displayed - $i" + ); + + if ($mech->content() =~ /Collection ID:.*?(\d+)/m) { + return $1; + } + } else { + $mech->content_contains( + 'Failed to create new collection.', + "Collection failed message displayed - $i" + ); + + return; + } +} + +ENDPERL + + +# Check that the state of the following collections: +# 0 = Deleted +# 1 = Exists +# 2 = Exists +BEGINQUERY +SELECT user_no, dav_name, dav_displayname +FROM collection +WHERE dav_name like '/user2/test_collection%' +ORDER BY dav_name; +ENDQUERY diff --git a/testing/tests/webui/0006-delete-ticket.result b/testing/tests/webui/0006-delete-ticket.result new file mode 100644 index 00000000..4e52d4b4 --- /dev/null +++ b/testing/tests/webui/0006-delete-ticket.result @@ -0,0 +1,45 @@ +1..23 +# Subtest: Login + 1..4 + ok 1 - Fetch first page + ok 2 - Not logged in + ok 3 - Login to site + ok 4 - Logged in +ok 1 - Login +ok 2 - Create collection - 0 +ok 3 - Collection created message displayed - 0 +ok 4 - Found new ticket ID - 0 +ok 5 - Create ticket - 0 +ok 6 - Ticket created message displayed - 0 +ok 7 - Ticket with collection path displayed - 0 +ok 8 - Create collection - 1 +ok 9 - Collection created message displayed - 1 +ok 10 - Found new ticket ID - 1 +ok 11 - Create ticket - 1 +ok 12 - Ticket created message displayed - 1 +ok 13 - Ticket with collection path displayed - 1 +ok 14 - Create collection - 2 +ok 15 - Collection created message displayed - 2 +ok 16 - Found new ticket ID - 2 +ok 17 - Create ticket - 2 +ok 18 - Ticket created message displayed - 2 +ok 19 - Ticket with collection path displayed - 2 +ok 20 - Access ticket deleted message displayed +# Subtest: Login as user1 + 1..4 + ok 1 - Fetch first page + ok 2 - Not logged in + ok 3 - Login to site + ok 4 - Logged in +ok 21 - Login as user1 +ok 22 - Collection deletion error displayed when specifying other principal and their ticket +ok 23 - Ticket deletion error display when specifying our principal and their collection + + dav_displayname: >Test Ticket_Collection 1< + dav_name: >/user2/test_ticket_collection_1/< + privileges: >000000000000000000000101< + + dav_displayname: >Test Ticket_Collection 2< + dav_name: >/user2/test_ticket_collection_2/< + privileges: >000000000000000000000101< + diff --git a/testing/tests/webui/0006-delete-ticket.test b/testing/tests/webui/0006-delete-ticket.test new file mode 100644 index 00000000..23e9ef8f --- /dev/null +++ b/testing/tests/webui/0006-delete-ticket.test @@ -0,0 +1,186 @@ +MODE=TAP,23 + +# Test creating deleting tickets. + +BEGINPERL + +my $mech; + +subtest 'Login' => sub { + plan tests => 4; + + $mech = webui_login( + username => 'user2', + password => 'user2', + url => "http://$webhost", + ); +}; + +$mech->follow_link( text_regex => qr/View My Details/ ); + +(my $principal_id = $mech->uri()) =~ s/^.*&id=(\d+)$/$1/; + +my $edit_url = $mech->uri(); + +$mech->follow_link( text_regex => qr/Create Collection/ ); +my $create_collection_url = $mech->uri(); + +# Create 3 tickets for testing deletion. +# 0 = Delete by the principal who created it. +# 1 = Try to delete by another principal +# 2 = Try to delete by another principal +my @col_id; +my @ticket_id; + +for (my $i = 0; $i < 3; $i++) { + $col_id[$i] = create_collection($mech, $create_collection_url, $i); + $ticket_id[$i] = create_ticket($mech, $edit_url, $i, "/user2/test_ticket_collection_$i/"); +} + +#$mech->save_content("$save_location/$case-A", binmode => ':utf8'); + +# Delete our first ticket. +$mech->get("http://$webhost/admin.php?action=edit&t=principal&id=$principal_id&ticket_id=" . $ticket_id[0] . "&subaction=delete_ticket"); +$mech->follow_link( text_regex => qr/Confirm Deletion of the Ticket/ ); + +$mech->content_contains( + 'Access ticket deleted', + 'Access ticket deleted message displayed' +); + +#diag("Saved content of B to $save_location/$case-B"); +#$mech->save_content("$save_location/$case-B", binmode => ':utf8'); + +my $mech_other; +subtest 'Login as user1' => sub { + plan tests => 4; + + $mech_other = webui_login( + username => 'user1', + password => 'user1', + url => "http://$webhost", + ); +}; + +$mech_other->follow_link( text_regex => qr/View My Details/ ); +(my $other_principal_id = $mech_other->uri()) =~ s/^.*&id=(\d+)$/$1/; + +# Try delete ticket as another user, should be rejected. +$mech_other->get("http://$webhost/admin.php?action=edit&t=principal&id=$principal_id&ticket_id=" . $ticket_id[1] . "&subaction=delete_ticket"); +$mech->follow_link( text_regex => qr/Confirm Deletion of the Ticket/ ); + +$mech_other->content_contains( + 'You are not allowed to delete tickets for this principal.', + 'Collection deletion error displayed when specifying other principal and their ticket' +); + +#diag("Saved content of C to $save_location/$case-C"); +#$mech_other->save_content("$save_location/$case-C", binmode => ':utf8'); + + +# Try delete other users ticket as us, should be rejected. +$mech_other->get("http://$webhost/admin.php?action=edit&t=principal&id=$other_principal_id&collection_id=" . $ticket_id[2] . "&subaction=delete_ticket"); +$mech_other->follow_link( text_regex => qr/Confirm Deletion of the Ticket/ ); + +$mech_other->content_contains( + 'Access ticket deletion failed.', + 'Ticket deletion error display when specifying our principal and their collection' +); + +#diag("Saved content of D to $save_location/$case-D"); +#$mech_other->save_content("$save_location/$case-D", binmode => ':utf8'); + +sub create_collection { + my ($mech, $create_url, $i) = @_; + + $mech->get($create_url); + + # Create a collection + $mech->submit_form_ok( + { + form_number => 1, + button => 'submit', + fields => { + collection_name => "test_ticket_collection_$i", + dav_displayname => "Test Ticket_Collection $i", + description => "Description for Ticket Collection $i", + }, + }, "Create collection - $i" + ); + + $mech->content_contains( + 'Creating new Collection.', + "Collection created message displayed - $i" + ); + + if ($mech->content() =~ /Collection ID:.*?(\d+)/m) { + return $1; + } +} + + + +sub create_ticket { + my ($mech, $create_url, $i, $collection, $fail) = @_; + + $mech->get($create_url); + + # Find the new ticket ID + my $ticket_id; + if ($mech->content() =~ qr//) { + $ticket_id = $1; + + like($ticket_id, qr/^[0-9a-z]{8}$/i, "Found new ticket ID - $i"); + } else { + fail("No ticket ID found - $i"); + } + + # Create a ticket + $mech->submit_form_ok( + { + form_number => 3, + button => 'ticketrow', + fields => { + target => $collection, + 'ticket_privileges[read]' => 1, + 'ticket_privileges[write-content]' => 1, + }, + }, "Create ticket - $i" + ); + + if (! defined $fail) { + $mech->content_contains( + 'Creating new ticket granting privileges to this Principal', + "Ticket created message displayed - $i" + ); + + $mech->content_like( + qr,^(?:|)$ticket_id\s*$collection,m, + "Ticket with collection path displayed - $i" + ); + + return $ticket_id; + } else { + $mech->content_contains( + 'Can only add tickets for existing collection paths which you own', + "Ticket creation failed message displayed - $i" + ); + + return; + } +} + +ENDPERL + + +# Check that the state of the following collections: +# 0 = Deleted +# 1 = Exists +# 2 = Exists +# Hard as the ticket ID will change... Exclude that from the query. +BEGINQUERY +SELECT t.privileges, c.dav_name, c.dav_displayname +FROM access_ticket AS t LEFT JOIN collection AS c ON (t.target_collection_id = c.collection_id) +WHERE c.dav_name like '/user2/test_ticket_collection%' +ORDER BY c.dav_name; +ENDQUERY diff --git a/testing/tests/webui/0007-delete-bind.result b/testing/tests/webui/0007-delete-bind.result new file mode 100644 index 00000000..f7407edd --- /dev/null +++ b/testing/tests/webui/0007-delete-bind.result @@ -0,0 +1,40 @@ +1..20 +# Subtest: Login + 1..4 + ok 1 - Fetch first page + ok 2 - Not logged in + ok 3 - Login to site + ok 4 - Logged in +ok 1 - Login +ok 2 - Create collection - 0 +ok 3 - Collection created message displayed - 0 +ok 4 - Create binding - 0 +ok 5 - Binding created message displayed - 0 +ok 6 - Binding with collection path displayed - 0 +ok 7 - Create collection - 1 +ok 8 - Collection created message displayed - 1 +ok 9 - Create binding - 1 +ok 10 - Binding created message displayed - 1 +ok 11 - Binding with collection path displayed - 1 +ok 12 - Create collection - 2 +ok 13 - Collection created message displayed - 2 +ok 14 - Create binding - 2 +ok 15 - Binding created message displayed - 2 +ok 16 - Binding with collection path displayed - 2 +ok 17 - Binding deleted message displayed +# Subtest: Login as user1 + 1..4 + ok 1 - Fetch first page + ok 2 - Not logged in + ok 3 - Login to site + ok 4 - Logged in +ok 18 - Login as user1 +ok 19 - Binding deletion error displayed when specifying other principal and their bind +ok 20 - Binding deletion error display when specifying our principal and their collection + + dav_displayname: >Binding 1< + dav_name: >/user2/test_binding_collection_1/< + + dav_displayname: >Binding 2< + dav_name: >/user2/test_binding_collection_2/< + diff --git a/testing/tests/webui/0007-delete-bind.test b/testing/tests/webui/0007-delete-bind.test new file mode 100644 index 00000000..e437f1e2 --- /dev/null +++ b/testing/tests/webui/0007-delete-bind.test @@ -0,0 +1,165 @@ +MODE=TAP,20 + +# Test creating deleting bindings. + +BEGINPERL + +my $mech; + +subtest 'Login' => sub { + plan tests => 4; + + $mech = webui_login( + username => 'user2', + password => 'user2', + url => "http://$webhost", + ); +}; + +$mech->follow_link( text_regex => qr/View My Details/ ); + +(my $principal_id = $mech->uri()) =~ s/^.*&id=(\d+)$/$1/; + +my $edit_url = $mech->uri(); + +$mech->follow_link( text_regex => qr/Create Collection/ ); +my $create_collection_url = $mech->uri(); + +# Create 3 binds for testing deletion. +# 0 = Delete by the principal who created it. +# 1 = Try to delete by another principal +# 2 = Try to delete by another principal +my @col_id; +my @binding_id; + +for (my $i = 0; $i < 3; $i++) { + $col_id[$i] = create_collection($mech, $create_collection_url, $i); + $binding_id[$i] = create_binding($mech, $edit_url, $i, "/user2/test_binding_collection_$i/"); +} + +#$mech->save_content("$save_location/$case-A", binmode => ':utf8'); + +# Delete our first binding. +$mech->get("http://$webhost/admin.php?action=edit&t=principal&id=$principal_id&bind_id=" . $binding_id[0] . "&subaction=delete_bind_in"); +$mech->follow_link( text_regex => qr/Confirm Deletion of the Binding/ ); + +$mech->content_contains( + 'Binding deleted', + 'Binding deleted message displayed' +); + +#diag("Saved content of B to $save_location/$case-B"); +#$mech->save_content("$save_location/$case-B", binmode => ':utf8'); + +my $mech_other; +subtest 'Login as user1' => sub { + plan tests => 4; + + $mech_other = webui_login( + username => 'user1', + password => 'user1', + url => "http://$webhost", + ); +}; + +$mech_other->follow_link( text_regex => qr/View My Details/ ); +(my $other_principal_id = $mech_other->uri()) =~ s/^.*&id=(\d+)$/$1/; + +# Try delete binding as another user, should be rejected. +$mech_other->get("http://$webhost/admin.php?action=edit&t=principal&id=$principal_id&bind_id=" . $binding_id[1] . "&subaction=delete_bind_in"); +$mech_other->follow_link( text_regex => qr/Confirm Deletion of the Binding/ ); + +$mech_other->content_contains( + 'You are not allowed to delete bindings for this principal.', + 'Binding deletion error displayed when specifying other principal and their bind' +); + +#diag("Saved content of C to $save_location/$case-C"); +#$mech_other->save_content("$save_location/$case-C", binmode => ':utf8'); + + +# Try delete other users binding as us, should be rejected. +$mech_other->get("http://$webhost/admin.php?action=edit&t=principal&id=$other_principal_id&bind_id=" . $binding_id[2] . "&subaction=delete_bind_in"); +$mech_other->follow_link( text_regex => qr/Confirm Deletion of the Binding/ ); + +$mech_other->content_contains( + 'Binding deletion failed.', + 'Binding deletion error display when specifying our principal and their collection' +); + +#diag("Saved content of D to $save_location/$case-D"); +#$mech_other->save_content("$save_location/$case-D", binmode => ':utf8'); + +sub create_collection { + my ($mech, $create_url, $i) = @_; + + $mech->get($create_url); + + # Create a collection + $mech->submit_form_ok( + { + form_number => 1, + button => 'submit', + fields => { + collection_name => "test_binding_collection_$i", + dav_displayname => "Test binding_Collection $i", + description => "Description for Binding Collection $i", + }, + }, "Create collection - $i" + ); + + $mech->content_contains( + 'Creating new Collection.', + "Collection created message displayed - $i" + ); + + if ($mech->content() =~ /Collection ID:.*?(\d+)/m) { + return $1; + } +} + + + +sub create_binding { + my ($mech, $create_url, $i, $collection) = @_; + + $mech->get($create_url); + + # Create a binding + $mech->submit_form_ok( + { + form_number => 4, + button => 'bindingrow', + fields => { + dav_name => "/user2/binding$i", + dav_displayname => "Binding $i", + source => $collection, + }, + }, "Create binding - $i" + ); + + $mech->content_contains( + 'Creating new binding for this principal', + "Binding created message displayed - $i" + ); + + if ($mech->content() =~ m,^(?:|)(\d+)\s*$collection,m) { + pass("Binding with collection path displayed - $i"); + return $1; + } +} + +ENDPERL + + +# Check that the state of the following collections: +# 0 = Deleted +# 1 = Exists +# 2 = Exists +# Hard as the ticket ID will change... Exclude that from the query. +BEGINQUERY +SELECT d.dav_name, d.dav_displayname, c.dav_name +FROM dav_binding AS d LEFT JOIN collection AS c ON (d.bound_source_id = c.collection_id) +WHERE d.dav_name like '/user2/binding%' +ORDER BY d.dav_name; +ENDQUERY