From a102105f27727934b78160b66e1935c8e9302cb1 Mon Sep 17 00:00:00 2001
From: Andrew Ruthven <andrew@etc.gen.nz>
Date: Sun, 25 Feb 2024 00:32:16 +1300
Subject: [PATCH] Hide many sections unless the user can modify the principal

There is potential to leak information when viewing the principal
page for another principal. I think it makes more sense to just
not include all of this information unles the user can change the
principal.
---
 inc/ui/principal-edit.php                     | 44 ++++++++++---------
 .../tests/webui/0003-principal-edit.result    | 12 ++++-
 testing/tests/webui/0003-principal-edit.test  | 16 ++++++-
 3 files changed, 49 insertions(+), 23 deletions(-)

diff --git a/inc/ui/principal-edit.php b/inc/ui/principal-edit.php
index d7d4edf0..9ba65d73 100644
--- a/inc/ui/principal-edit.php
+++ b/inc/ui/principal-edit.php
@@ -1139,26 +1139,28 @@ if ( isset($id) && $id > 0 ) {
     $page_elements[] = confirm_delete_principal($delete_principal_confirmation_required, $editor->Value('displayname'));
 
 
-  $page_elements[] = group_memberships_browser();
-  if ( $editor->Value('type_id') == 3 ) {
-    $grouprow = group_row_editor();
-    $page_elements[] = group_members_browser();
+  if ($can_write_principal) {
+    $page_elements[] = group_memberships_browser();
+    if ( $editor->Value('type_id') == 3 ) {
+      $grouprow = group_row_editor();
+      $page_elements[] = group_members_browser();
+    }
+    $grantrow = grant_row_editor();
+    $page_elements[] = principal_grants_browser();
+    if ( isset($delete_grant_confirmation_required) ) $page_elements[] = confirm_delete_grant($delete_grant_confirmation_required);
+
+    $ticketrow = ticket_row_editor();
+    $page_elements[] = access_ticket_browser();
+    if ( isset($delete_ticket_confirmation_required) ) $page_elements[] = confirm_delete_ticket($delete_ticket_confirmation_required);
+
+    $page_elements[] = principal_collection_browser();
+    if ( isset($delete_collection_confirmation_required) ) $page_elements[] = confirm_delete_collection($delete_collection_confirmation_required);
+
+    $bindingrow = binding_row_editor();
+    $page_elements[] = bindings_to_other_browser();
+    if ( isset($delete_bind_in_confirmation_required) ) $page_elements[] = confirm_delete_bind_in($delete_bind_in_confirmation_required);
+
+    $page_elements[] = bindings_to_us_browser();
+    if ( isset($delete_binding_confirmation_required) ) $page_elements[] = confirm_delete_binding($delete_binding_confirmation_required);
   }
-  $grantrow = grant_row_editor();
-  $page_elements[] = principal_grants_browser();
-  if ( isset($delete_grant_confirmation_required) ) $page_elements[] = confirm_delete_grant($delete_grant_confirmation_required);
-
-  $ticketrow = ticket_row_editor();
-  $page_elements[] = access_ticket_browser();
-  if ( isset($delete_ticket_confirmation_required) ) $page_elements[] = confirm_delete_ticket($delete_ticket_confirmation_required);
-
-  $page_elements[] = principal_collection_browser();
-  if ( isset($delete_collection_confirmation_required) ) $page_elements[] = confirm_delete_collection($delete_collection_confirmation_required);
-
-  $bindingrow = binding_row_editor();
-  $page_elements[] = bindings_to_other_browser();
-  if ( isset($delete_bind_in_confirmation_required) ) $page_elements[] = confirm_delete_bind_in($delete_bind_in_confirmation_required);
-
-  $page_elements[] = bindings_to_us_browser();
-  if ( isset($delete_binding_confirmation_required) ) $page_elements[] = confirm_delete_binding($delete_binding_confirmation_required);
 }
diff --git a/testing/tests/webui/0003-principal-edit.result b/testing/tests/webui/0003-principal-edit.result
index 32b8b250..0ba16c2e 100644
--- a/testing/tests/webui/0003-principal-edit.result
+++ b/testing/tests/webui/0003-principal-edit.result
@@ -1,4 +1,4 @@
-1..30
+1..40
 # Subtest: Login
     1..4
     ok 1 - Fetch first page
@@ -59,6 +59,16 @@ ok 27 - Date format type field correct
 ok 28 - Type field correct
 ok 29 - Submit updated fields on a user we have no access to
 ok 30 - Error message denying access displayed
+ok 31 - Fetch details page for principal ID 1003
+ok 32 - Looking at principal ID 1003
+ok 33 - Change Password missing
+ok 34 - Confirm Password missing
+ok 35 - Group Memberships missing
+ok 36 - Principal Grants missing
+ok 37 - Access Tickets missing
+ok 38 - Principal Collections missing
+ok 39 - Bindings to other collections missing
+ok 40 - Bindings to this Principal's Collections missing
 
  date_format_type: >I<
          dav_name: >/user99/<
diff --git a/testing/tests/webui/0003-principal-edit.test b/testing/tests/webui/0003-principal-edit.test
index 31e6840b..60e81367 100644
--- a/testing/tests/webui/0003-principal-edit.test
+++ b/testing/tests/webui/0003-principal-edit.test
@@ -1,4 +1,4 @@
-MODE=TAP,30
+MODE=TAP,40
 
 BEGINPERL
 
@@ -171,6 +171,20 @@ $mech->content_contains(
     'You do not have permission to modify this record.',
     'Error message denying access displayed');
 
+# Fetch details for another user, make sure any sensitive sections are not
+# visible. Also, no need to show the change password fields.
+$mech->get_ok($action, "Fetch details page for principal ID 1003");
+
+$mech->content_contains('Principal: User 2', 'Looking at principal ID 1003');
+
+for my $missing_text ('Change Password', 'Confirm Password', 'Group Memberships',
+    'Principal Grants', 'Access Tickets', 'Principal Collections',
+    'Bindings to other collections',
+    'Bindings to this Principal\'s Collections') {
+
+    $mech->content_lacks($missing_text, "$missing_text missing");
+}
+
 ENDPERL