Fix #12553: Improve handling of allow_*_own_attachments options

There exists three existing options to allow users to view, download and
delete their own attachments only (if they don't have wider permission
to view, download and delete ANY attachment within a project). These
options are:
$g_allow_view_own_attachments
$g_allow_download_own_attachments
$g_allow_delete_own_attachments

These options were not being factored into access checks correctly.
Instead of checking who uploaded the attachment we were checking whether
the current user is the reporter of the issue.... sometimes.

It is important to note that the bug_get_attachments() function in
bug_api.php no longer performs any access checks. It is up to the caller
to filter the attachments and validate access permissions. Use
file_get_visible_attachments() from file_api.php instead if you want to
get a filtered list of attachments that factors in access levels.

Thank you to Frank Rodgers for an intial patch and ideas on how to
improve the handling of these options.

api/soap/mc_issue_api.php

@@ -233,6 +233,9 @@ function mci_issue_get_attachments( $p_issue_id ) {
 
 	$t_result = array();
 	foreach( $t_attachment_rows as $t_attachment_row ) {
+		if ( !file_can_view_bug_attachments( $p_issue_id, (int)$t_attachment_row['user_id'] ) ) {
+			continue;
+		}
 		$t_attachment = array();
 		$t_attachment['id'] = $t_attachment_row['id'];
 		$t_attachment['filename'] = $t_attachment_row['filename'];

bug_view_inc.php

@@ -207,7 +207,7 @@
 $tpl_projection = $tpl_show_projection ? string_display_line( get_enum_element( 'projection', $tpl_bug->projection ) ) : '';
 $tpl_show_eta = in_array( 'eta', $t_fields );
 $tpl_eta = $tpl_show_eta ? string_display_line( get_enum_element( 'eta', $tpl_bug->eta ) ) : '';
-$tpl_show_attachments = in_array( 'attachments', $t_fields ) && ( ( $tpl_bug->reporter_id == auth_get_current_user_id() ) || access_has_bug_level( config_get( 'view_attachments_threshold' ), $f_bug_id ) );
+$tpl_show_attachments = in_array( 'attachments', $t_fields );
 $tpl_can_attach_tag = $tpl_show_tags && !$tpl_force_readonly && access_has_bug_level( config_get( 'tag_attach_threshold' ), $f_bug_id );
 $tpl_show_category = in_array( 'category_id', $t_fields );
 $tpl_category = $tpl_show_category ? string_display_line( category_full_name( $tpl_bug->category_id ) ) : '';

core/bug_api.php

@@ -1368,23 +1368,19 @@ function bug_get_bugnote_stats( $p_bug_id ) {
 /**
  * Get array of attachments associated with the specified bug id.  The array will be
  * sorted in terms of date added (ASC).  The array will include the following fields:
- * id, title, diskfile, filename, filesize, file_type, date_added.
+ * id, title, diskfile, filename, filesize, file_type, date_added, user_id.
  * @param int p_bug_id integer representing bug id
  * @return array array of results or null
  * @access public
  * @uses database_api.php
  * @uses file_api.php
  */
 function bug_get_attachments( $p_bug_id ) {
-	if( !file_can_view_bug_attachments( $p_bug_id ) ) {
-		return;
-	}
-
 	$c_bug_id = db_prepare_int( $p_bug_id );
 
 	$t_bug_file_table = db_get_table( 'bug_file' );
 
-	$query = "SELECT id, title, diskfile, filename, filesize, file_type, date_added
+	$query = "SELECT id, title, diskfile, filename, filesize, file_type, date_added, user_id
 		                FROM $t_bug_file_table
 		                WHERE bug_id=" . db_param() . "
 		                ORDER BY date_added";

core/columns_api.php

@@ -1064,8 +1064,10 @@ function print_column_attachment_count( $p_bug, $p_columns_target = COLUMNS_TARG
 	global $t_icon_path;
 
 	# Check for attachments
+	# TODO: factor in the allow_view_own_attachments configuration option
+	# instead of just using a global check.
 	$t_attachment_count = 0;
-	if( file_can_view_bug_attachments( $p_bug->id ) ) {
+	if( file_can_view_bug_attachments( $p_bug->id, null ) ) {
 		$t_attachment_count = file_bug_attachment_count( $p_bug->id );
 	}
 

core/file_api.php

@@ -127,40 +127,30 @@ function file_bug_has_attachments( $p_bug_id ) {
 }
 
 # Check if the current user can view attachments for the specified bug.
-function file_can_view_bug_attachments( $p_bug_id ) {
-	$t_reported_by_me = bug_is_user_reporter( $p_bug_id, auth_get_current_user_id() );
+function file_can_view_bug_attachments( $p_bug_id, $p_uploader_user_id = null ) {
+	$t_uploaded_by_me = auth_get_current_user_id() === $p_uploader_user_id;
 	$t_can_view = access_has_bug_level( config_get( 'view_attachments_threshold' ), $p_bug_id );
-
-	/** @todo Fix this to be readable */
-	$t_can_view = $t_can_view || ( $t_reported_by_me && config_get( 'allow_view_own_attachments' ) );
-
+	$t_can_view = $t_can_view || ( $t_uploaded_by_me && config_get( 'allow_view_own_attachments' ) );
 	return $t_can_view;
 }
 
 # Check if the current user can download attachments for the specified bug.
-function file_can_download_bug_attachments( $p_bug_id ) {
-	$t_reported_by_me = bug_is_user_reporter( $p_bug_id, auth_get_current_user_id() );
+function file_can_download_bug_attachments( $p_bug_id, $p_uploader_user_id = null ) {
+	$t_uploaded_by_me = auth_get_current_user_id() === $p_uploader_user_id;
 	$t_can_download = access_has_bug_level( config_get( 'download_attachments_threshold' ), $p_bug_id );
-
-	/** @todo Fix this to be readable */
-	$t_can_download = $t_can_download || ( $t_reported_by_me && config_get( 'allow_download_own_attachments' ) );
-
+	$t_can_download = $t_can_download || ( $t_uploaded_by_me && config_get( 'allow_download_own_attachments' ) );
 	return $t_can_download;
 }
 
 # Check if the current user can delete attachments from the specified bug.
-function file_can_delete_bug_attachments( $p_bug_id ) {
+function file_can_delete_bug_attachments( $p_bug_id, $p_uploader_user_id = null ) {
 	if( bug_is_readonly( $p_bug_id ) ) {
 		return false;
 	}
-
-	$t_reported_by_me = bug_is_user_reporter( $p_bug_id, auth_get_current_user_id() );
-	$t_can_download = access_has_bug_level( config_get( 'delete_attachments_threshold' ), $p_bug_id );
-
-	/** @todo Fix this to be readable */
-	$t_can_download = $t_can_download || ( $t_reported_by_me && config_get( 'allow_delete_own_attachments' ) );
-
-	return $t_can_download;
+	$t_uploaded_by_me = auth_get_current_user_id() === $p_uploader_user_id;
+	$t_can_delete = access_has_bug_level( config_get( 'delete_attachments_threshold' ), $p_bug_id );
+	$t_can_delete = $t_can_delete || ( $t_uploaded_by_me && config_get( 'allow_delete_own_attachments' ) );
+	return $t_can_delete;
 }
 
 # Get icon corresponding to the specified filename
@@ -282,15 +272,17 @@ function file_get_visible_attachments( $p_bug_id ) {
 
 	$t_attachments = array();
 
-	$t_can_download = file_can_download_bug_attachments( $p_bug_id );
-	$t_can_delete = file_can_delete_bug_attachments( $p_bug_id );
 	$t_preview_text_ext = config_get( 'preview_text_extensions' );
 	$t_preview_image_ext = config_get( 'preview_image_extensions' );
 
 	$image_previewed = false;
 	for( $i = 0;$i < $t_attachments_count;$i++ ) {
 		$t_row = $t_attachment_rows[$i];
 
+		if ( !file_can_view_bug_attachments( $p_bug_id, (int)$t_row['user_id'] ) ) {
+			continue;
+		}
+
 		$t_id = $t_row['id'];
 		$t_filename = $t_row['filename'];
 		$t_filesize = $t_row['filesize'];
@@ -302,10 +294,12 @@ function file_get_visible_attachments( $p_bug_id ) {
 		$t_attachment['display_name'] = file_get_display_name( $t_filename );
 		$t_attachment['size'] = $t_filesize;
 		$t_attachment['date_added'] = $t_date_added;
-		$t_attachment['can_download'] = $t_can_download;
 		$t_attachment['diskfile'] = $t_diskfile;
 
-		if( $t_can_download ) {
+		$t_attachment['can_download'] = file_can_download_bug_attachments( $p_bug_id, (int)$t_row['user_id'] );
+		$t_attachment['can_delete'] = file_can_delete_bug_attachments( $p_bug_id, (int)$t_row['user_id'] );
+
+		if( $t_attachment['can_download'] ) {
 			$t_attachment['download_url'] = "file_download.php?file_id=$t_id&type=bug";
 		}
 
@@ -315,15 +309,14 @@ function file_get_visible_attachments( $p_bug_id ) {
 
 		$t_attachment['exists'] = config_get( 'file_upload_method' ) != DISK || file_exists( $t_diskfile );
 		$t_attachment['icon'] = file_get_icon_url( $t_attachment['display_name'] );
-		$t_attachment['can_delete'] = $t_can_delete;
 
 		$t_attachment['preview'] = false;
 		$t_attachment['type'] = '';
 
 		$t_ext = strtolower( file_get_extension( $t_attachment['display_name'] ) );
 		$t_attachment['alt'] = $t_ext;
 
-		if ( $t_attachment['exists'] && $t_can_download && $t_filesize != 0 && $t_filesize <= config_get( 'preview_attachments_inline_max_size' ) ) {
+		if ( $t_attachment['exists'] && $t_attachment['can_download'] && $t_filesize != 0 && $t_filesize <= config_get( 'preview_attachments_inline_max_size' ) ) {
 			if ( in_array( $t_ext, $t_preview_text_ext, true ) ) {
 				$t_attachment['preview'] = true;
 				$t_attachment['type'] = 'text';

file_download.php

@@ -111,7 +111,7 @@
 # Check access rights
 switch ( $f_type ) {
 	case 'bug':
-		if ( !file_can_download_bug_attachments( $v_bug_id ) ) {
+		if ( !file_can_download_bug_attachments( $v_bug_id, (int)$v_user_id ) ) {
 			access_denied();
 		}
 		break;

my_view_inc.php

@@ -450,7 +450,9 @@
 
 	# Check for attachments
 	$t_attachment_count = 0;
-	if(( file_can_view_bug_attachments( $t_bug->id ) ) ) {
+	# TODO: factor in the allow_view_own_attachments configuration option
+	# instead of just using a global check.
+	if(( file_can_view_bug_attachments( $t_bug->id, null ) ) ) {
 		$t_attachment_count = file_bug_attachment_count( $t_bug->id );
 	}