Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#1005] Hide individual attachments #7327

Closed
wants to merge 21 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions app/controllers/admin/foi_attachments_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
##
# Controller to manage FoiAttachment instances
#
class Admin::FoiAttachmentsController < AdminController
before_action :set_foi_attachment, :set_incoming_message, :set_info_request
before_action :check_info_request

def edit
end

def update
if @foi_attachment.update(foi_attachment_params)
@info_request.log_event(
'edit_attachment',
attachment_id: @foi_attachment.id,
editor: admin_current_user,
old_prominence: @foi_attachment.prominence_previously_was,
prominence: @foi_attachment.prominence,
old_prominence_reason: @foi_attachment.prominence_reason_previously_was,
prominence_reason: @foi_attachment.prominence_reason
)
@info_request.expire

flash[:notice] = 'Attachment successfully updated.'
redirect_to edit_admin_incoming_message_path(@incoming_message)
garethrees marked this conversation as resolved.
Show resolved Hide resolved

else
render action: 'edit'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this come with some sort of indication that the update was unsuccessful?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't do this in the other admin controllers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we do when we show model validation errors, but doesn't look like we're doing that here so a fail would be completely silent. I think we only need a error: 'Save unsuccessful' flash on the end of the render call.

end
end

private

def foi_attachment_params
params.require(:foi_attachment).permit(:prominence, :prominence_reason)
end

def set_foi_attachment
@foi_attachment = FoiAttachment.find(params[:id])
end

def set_incoming_message
@incoming_message = @foi_attachment&.incoming_message
end

def set_info_request
@info_request = @incoming_message&.info_request
end

def check_info_request
return if can? :admin, @info_request

raise ActiveRecord::RecordNotFound
end
end
53 changes: 31 additions & 22 deletions app/controllers/attachments_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,11 @@ class AttachmentsController < ApplicationController

include ProminenceHeaders

around_action :cache_attachments

before_action :authenticate_attachment
before_action :authenticate_attachment_as_html, only: :show_as_html

around_action :cache_attachments

def show
# Prevent spam to magic request address. Note that the binary
# substitution method used depends on the content type
Expand Down Expand Up @@ -109,17 +109,24 @@ def authenticate_attachment
return render_hidden('request/hidden_correspondence')
end

return if @attachment

# If we can't find the right attachment, redirect to the incoming message:
redirect_to incoming_message_url(@incoming_message), status: 303
if @attachment
if cannot?(:read, @attachment)
request.format = :html
render_hidden('request/hidden_attachment')
end
elsif params[:file_name]
# If we can't find the right attachment, redirect to the incoming message:
redirect_to incoming_message_url(@incoming_message), status: 303
else
render plain: 'Directory listing not allowed', status: 403
end
end

def authenticate_attachment_as_html
# The conversion process can generate files in the cache directory that can
# be served up directly by the webserver according to httpd.conf, so don't
# allow it unless that's OK.
return if message_is_public?
return if attachment_is_public?

raise ActiveRecord::RecordNotFound, 'Attachment HTML not found.'
end
Expand All @@ -132,12 +139,8 @@ def cache_attachments
if foi_fragment_cache_exists?(cache_key_path)
logger.info("Reading cache for #{cache_key_path}")

if File.directory?(cache_key_path)
render plain: 'Directory listing not allowed', status: 403
else
render body: foi_fragment_cache_read(cache_key_path),
content_type: content_type
end
render body: foi_fragment_cache_read(cache_key_path),
content_type: content_type
return
end

Expand All @@ -148,7 +151,7 @@ def cache_attachments
# various fragment cache functions using Ruby Marshall to write the file
# which adds a header, so isn't compatible with images that have been
# extracted elsewhere from PDFs)
if message_is_cacheable?
if attachment_is_cacheable?
logger.info("Writing cache for #{cache_key_path}")
foi_fragment_cache_write(cache_key_path, response.body)
end
Expand All @@ -162,6 +165,8 @@ def part_number

def original_filename
filename = params[:file_name]
return unless filename

if action_name == 'show_as_html'
filename.gsub(/\.html$/, '')
else
Expand All @@ -176,16 +181,20 @@ def content_type
'application/octet-stream'
end

def message_is_public?
# If this a request and message public then it can be served up without
# authentication
prominence.is_public? && @incoming_message.is_public?
def attachment_is_public?
# If this a request, message and attachment are public then it can be served
# up without authentication
prominence.is_public? &&
@incoming_message.is_public? &&
@attachment.is_public?
end

def message_is_cacheable?
# If this a request searchable and message public then we can cache any
# attachments as there are no custom response headers (EG X-Robots-Tag)
prominence.is_searchable? && message_is_public?
def attachment_is_cacheable?
# If this a request, message and attachment are searchable then we can cache
# as there are no custom response headers (EG X-Robots-Tag)
prominence.is_searchable? &&
@incoming_message.indexed_by_search? &&
@attachment.indexed_by_search?
end

def cache_key_path
Expand Down
1 change: 1 addition & 0 deletions app/controllers/request_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -569,6 +569,7 @@ def make_request_zip(info_request, file_path)
next unless can?(:read, message)
message_index += 1
message.get_attachments_for_display.each do |attachment|
next unless can?(:read, attachment)
filename = "#{message_index}_#{attachment.url_part_number}_#{attachment.display_filename}"
zipfile.get_output_stream(filename) do |f|
body = message.apply_masks(attachment.default_body, attachment.content_type)
Expand Down
11 changes: 11 additions & 0 deletions app/helpers/admin/link_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,17 @@ def incoming_message_both_links(incoming_message)
title: admin_title)
end

def foi_attachment_both_links(foi_attachment)
title = 'View attachment on public website'
icon = prominence_icon(foi_attachment)
info_request = foi_attachment.incoming_message.info_request

link_to(icon, foi_attachment_path(foi_attachment), title: title) + ' ' +
gbp marked this conversation as resolved.
Show resolved Hide resolved
link_to("#{info_request.title} #{foi_attachment.filename}",
edit_admin_foi_attachment_path(foi_attachment),
title: admin_title)
end

def info_request_batch_both_links(batch)
title = 'View batch on public website'
icon = prominence_icon(batch)
Expand Down
14 changes: 13 additions & 1 deletion app/helpers/link_to_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,17 @@ def outgoing_message_path(outgoing_message, options = {})
message_path(outgoing_message, options)
end

def foi_attachment_url(foi_attachment, options = {})
incoming_message_url(
foi_attachment.incoming_message,
options.merge(anchor: dom_id(foi_attachment))
)
end

def foi_attachment_path(foi_attachment, options = {})
foi_attachment_url(foi_attachment, options.merge(only_path: true))
end

def comment_url(comment, options = {})
request_url(comment.info_request, options.merge(:anchor => "comment-#{comment.id}"))
end
Expand Down Expand Up @@ -294,7 +305,8 @@ def current_path_as_json

# Private: Generate a request_url linking to the new correspondence
def message_url(message, options = {})
anchor = dom_id(message)
anchor = options[:anchor]
anchor ||= dom_id(message)

return "##{anchor}" if options[:anchor_only]
default_options = { anchor: anchor }
Expand Down
33 changes: 33 additions & 0 deletions app/helpers/prominence_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -165,4 +165,37 @@ def sign_in_notice(*args)
end

::OutgoingMessage::Prominence::Helper = IncomingMessage::Prominence::Helper

class FoiAttachment::Prominence::Helper < Base # :nodoc:
def user
prominenceable.incoming_message.info_request.user
end

def hidden_notice
if current_user&.is_admin?
_('This attachment has prominence "hidden". {{reason}} You can only ' \
'see it because you are logged in as a super user.', reason: reason)
else
_('This attachment has been hidden. {{reason}}', reason: reason)
end
end

def requester_only_notice
if current_user && current_user == user
_('This attachment is hidden, so that only you, the requester, can ' \
'see it. {{reason}}', reason: reason)
elsif current_user&.is_admin?
_('This attachment has prominence "requester_only". {{reason}} You ' \
'can only see it because you are logged in as a super user.',
reason: reason)
else
_('This attachment has been hidden. {{reason}}', reason: reason)
end
end

def sign_in_notice(*args)
_('If you are the requester, then you may {{sign_in_link}} to view the ' \
'attachment.', *args)
end
end
end
9 changes: 9 additions & 0 deletions app/models/ability.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@ class Ability

attr_reader :user, :project, :public_token

def self.guest
new(nil)
end

def initialize(user, project: nil, public_token: false)
# Define abilities for the passed in user here. For example:
#
Expand Down Expand Up @@ -42,6 +46,11 @@ def initialize(user, project: nil, public_token: false)
end

# Viewing messages with prominence
can :read, FoiAttachment do |attachment|
can_view_with_prominence?(attachment.prominence,
attachment.incoming_message.info_request)
end

can :read, [IncomingMessage, OutgoingMessage] do |msg|
can_view_with_prominence?(msg.prominence, msg.info_request)
end
Expand Down
1 change: 1 addition & 0 deletions app/models/concerns/message_prominence.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ module MessageProminence
extend ActiveSupport::Concern

included do
strip_attributes only: [:prominence_reason]
validates_inclusion_of :prominence, in: self.prominence_states
end

Expand Down
6 changes: 5 additions & 1 deletion app/models/foi_attachment.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# == Schema Information
# Schema version: 20210114161442
# Schema version: 20220916134847
#
# Table name: foi_attachments
#
Expand All @@ -14,6 +14,8 @@
# hexdigest :string(32)
# created_at :datetime
# updated_at :datetime
# prominence :string default("normal")
# prominence_reason :text
#

# models/foi_attachment.rb:
Expand All @@ -26,6 +28,8 @@
require 'digest'

class FoiAttachment < ApplicationRecord
include MessageProminence
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assume it's quite a bit of work to add backpage at this point?


belongs_to :incoming_message,
:inverse_of => :foi_attachments

Expand Down
4 changes: 4 additions & 0 deletions app/models/foi_attachment/prominence.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
class FoiAttachment
module Prominence
end
end
10 changes: 6 additions & 4 deletions app/models/incoming_message.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,6 @@ class IncomingMessage < ApplicationRecord
include CacheAttributesFromRawEmail
include Taggable

strip_attributes only: [:prominence_reason]

MAX_ATTACHMENT_TEXT_CLIPPED = 1000000 # 1Mb ish

belongs_to :info_request,
Expand Down Expand Up @@ -391,7 +389,9 @@ def get_main_body_text_internal
# Given a main text part, converts it to text
def _convert_part_body_to_text(part)
if part.nil?
text = "[ Email has no body, please see attachments ]"
return "[ Email has no body, please see attachments ]"
elsif Ability.guest.cannot?(:read, part)
return ''
else
# whatever kind of attachment it is, get the UTF-8 encoded text
text = part.body_as_text.string
Expand Down Expand Up @@ -623,7 +623,9 @@ def get_attachment_text_clipped

def _extract_text
# Extract text from each attachment
self.get_attachments_for_display.reduce('') { |memo, attachment|
get_attachments_for_display.reduce('') { |memo, attachment|
return memo if Ability.guest.cannot?(:read, attachment)

memo += MailHandler.get_attachment_text_one_file(attachment.content_type,
attachment.default_body,
attachment.charset)
Expand Down
1 change: 1 addition & 0 deletions app/models/info_request_event.rb
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ class InfoRequestEvent < ApplicationRecord
'destroy_outgoing', # deleted an outgoing message (in admin interface)
'redeliver_incoming', # redelivered an incoming message elsewhere (in admin interface)
'edit_incoming', # incoming message edited (in admin interface)
'edit_attachment', # attachment edited (in admin interface)
'move_request', # changed user or public body (in admin interface)
'hide', # hid a request (in admin interface)
'manual', # you did something in the db by hand
Expand Down
2 changes: 0 additions & 2 deletions app/models/outgoing_message.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,6 @@ class OutgoingMessage < ApplicationRecord
include LinkToHelper
include Taggable

strip_attributes only: [:prominence_reason]

STATUS_TYPES = %w(ready sent failed).freeze
MESSAGE_TYPES = %w(initial_request followup).freeze
WHAT_DOING_VALUES = %w(normal_sort
Expand Down
8 changes: 8 additions & 0 deletions app/views/admin/foi_attachments/_intro.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<% @title = "Attachment #{foi_attachment.id} of FOI request '#{info_request.title}'" %>
<h1>Attachment <%= foi_attachment.id %></h1>

<ul class="breadcrumb">
<li>FOI request: <%= both_links(info_request) %> <span class="divider">/</span></li>
<li>Message: <%= both_links(incoming_message) %> <span class="divider">/</span></li>
<li class="active">Attachment: <%= both_links(foi_attachment) %></li>
</ul>
Loading