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

Restrict access to uploads #2088

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
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
2 changes: 2 additions & 0 deletions .env.example
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
DOMAIN=bops.localhost:3000
PAAPI_HOST=paapi.services
OS_VECTOR_TILES_API_KEY=xxxxx
OTP_SECRET_ENCRYPTION_KEY=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
Expand All @@ -6,3 +7,4 @@ GROUPED_PROPOSAL_DETAILS_FEATURE=enabled
GOOGLE_TAG_MANAGER_ID=GTM-XXXXXXXX
UPLOADS_HOSTNAME=uploads.bops.localhost
UPLOADS_BASE_URL=http://uploads.bops.localhost:3000
USE_SIGNED_URLS=false
Copy link
Member Author

Choose a reason for hiding this comment

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

Need to deploy this to staging so we can test it but don't want to break the app so this controls whether it uses the current urls or the new urls.

2 changes: 2 additions & 0 deletions .env.test
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
DOMAIN="example.com"
STAGING_API_URL="bops-staging.services"
STAGING_API_BEARER="fjisdfjsdiofjdsoi"
OS_VECTOR_TILES_API_KEY="testtest"
NOTIFY_LETTER_API_KEY='testtest'
UPLOADS_HOSTNAME=uploads.example.com
UPLOADS_BASE_URL=http://uploads.example.com
USE_SIGNED_URLS=false
1 change: 1 addition & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ gem "aasm"
gem "activerecord-postgis-adapter", "~> 10.0"
gem "acts_as_list"
gem "appsignal"
gem "aws-sdk-cloudfront", require: false
gem "aws-sdk-s3", require: false
gem "bootsnap", ">= 1.4.2", require: false
gem "business_time"
Expand Down
4 changes: 4 additions & 0 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,9 @@ GEM
ast (2.4.2)
aws-eventstream (1.1.1)
aws-partitions (1.489.0)
aws-sdk-cloudfront (1.55.0)
aws-sdk-core (~> 3, >= 3.119.0)
aws-sigv4 (~> 1.1)
aws-sdk-core (3.119.1)
aws-eventstream (~> 1, >= 1.0.2)
aws-partitions (~> 1, >= 1.239.0)
Expand Down Expand Up @@ -655,6 +658,7 @@ DEPENDENCIES
activerecord-postgis-adapter (~> 10.0)
acts_as_list
appsignal
aws-sdk-cloudfront
aws-sdk-s3
bootsnap (>= 1.4.2)
bops_admin!
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/documents_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ class DocumentsController < AuthenticationController
before_action :replacement_document_validation_request, only: %i[edit update]

def index
@documents = @planning_application.documents.with_file_attachment
@documents = @planning_application.documents.default.with_file_attachment
@additional_document_validation_requests = @planning_application
.additional_document_validation_requests
.post_validation
Expand Down
1 change: 0 additions & 1 deletion app/helpers/document_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ def link_to_document(link_text, document, **args)
link_text,
url_for_document(document),
new_tab:,
download: reference_or_file_name(document),
**args
)
end
Expand Down
54 changes: 48 additions & 6 deletions app/models/document.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,22 @@ class Document < ApplicationRecord
class Routing
include Rails.application.routes.url_helpers
include Rails.application.routes.mounted_helpers

def initialize(subdomain)
@subdomain = subdomain
end

def default_url_options
{host: "#{subdomain}.#{domain}"}
end

private

attr_reader :subdomain

def domain
Rails.configuration.domain
end
end

class NotArchiveableError < StandardError; end
Expand All @@ -29,6 +45,7 @@ class NotArchiveableError < StandardError; end
inverse_of: false

delegate :audits, to: :planning_application
delegate :local_authority, to: :planning_application
delegate :blob, :representable?, to: :file

include Auditable
Expand Down Expand Up @@ -209,12 +226,11 @@ class NotArchiveableError < StandardError; end
validate :numbered
validate :created_date_is_in_the_past

default_scope -> { no_owner.or(not_excluded_owners) }

Copy link
Member Author

Choose a reason for hiding this comment

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

This needed to be removed so the uploads engine could find all documents for checking the blob key

scope :no_owner, -> { where(owner_type: nil) }
scope :not_excluded_owners, -> { where.not(owner_type: EXCLUDED_OWNERS) }
scope :default, -> { no_owner.or(not_excluded_owners) }
scope :by_created_at, -> { order(created_at: :asc) }
scope :active, -> { where(archived_at: nil) }
scope :active, -> { default.where(archived_at: nil) }
scope :invalidated, -> { where(validated: false) }
scope :validated, -> { where(validated: true) }
scope :redacted, -> { where(redacted: true) }
Expand All @@ -225,8 +241,8 @@ class NotArchiveableError < StandardError; end
)
scope :publishable, -> { where(publishable: true) }

scope :for_publication, -> { active.publishable }
scope :for_display, -> { active.referenced_in_decision_notice }
scope :for_publication, -> { publishable }
scope :for_display, -> { referenced_in_decision_notice }
benjamineskola marked this conversation as resolved.
Show resolved Hide resolved

scope :with_tag, ->(tag) { where(arel_table[:tags].contains(Array.wrap(tag))) }
scope :with_siteplan_tags, -> { where(arel_table[:tags].overlaps(%w[sitePlan.existing sitePlan.proposed])) }
Expand All @@ -247,7 +263,33 @@ class NotArchiveableError < StandardError; end
self.user ||= Current.user
end

has_many :file_variant_records, through: :file_blob, source: :variant_records
has_many :file_variant_attachments, through: :file_variant_records, source: :image_attachment
has_many :file_variant_blobs, through: :file_variant_attachments, source: :blob

has_one :file_preview_image_attachment, through: :file_blob, source: :preview_image_attachment
has_one :file_preview_blob, through: :file_preview_image_attachment, source: :blob
has_many :file_preview_variant_records, through: :file_preview_blob, source: :variant_records
has_many :file_preview_variant_attachments, through: :file_preview_variant_records, source: :image_attachment
has_many :file_preview_variant_blobs, through: :file_preview_variant_attachments, source: :blob

class << self
def find_by_blob!(key:)
blob_associations = %i[
file_blob
file_preview_blob
file_variant_blobs
file_preview_variant_blobs
]

left_joins(*blob_associations)
.where(file_blob: {key:})
.or(where(file_variant_blobs: {key:}))
.or(where(file_preview_blob: {key:}))
.or(where(file_preview_variant_blobs: {key:}))
.distinct.sole
end

Copy link
Member Author

Choose a reason for hiding this comment

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

This huge block is so we can find a document through a local authority and prevent someone using access in one local authority to access a document in another authority.

Copy link
Member Author

Choose a reason for hiding this comment

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

Doing an EXPLAIN on the query shows it's not great so continuing to look at other options.

def tags(key)
case key.to_s
when "plans"
Expand Down Expand Up @@ -359,7 +401,7 @@ def representation_url(transformations = {resize_to_limit: [1000, 1000]})
private

def routes
@_routes ||= Routing.new
@_routes ||= Routing.new(local_authority.subdomain)
end

def no_open_replacement_request
Expand Down
1 change: 1 addition & 0 deletions app/models/local_authority.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ class LocalAuthority < ApplicationRecord

with_options through: :planning_applications do
has_many :audits
has_many :documents
has_many :validation_requests
end

Expand Down
4 changes: 2 additions & 2 deletions app/models/planning_application.rb
Original file line number Diff line number Diff line change
Expand Up @@ -471,7 +471,7 @@ def agent_or_applicant_name
end

def documents_for_decision_notice
documents.for_display
documents.active.for_display
end

def valid_from
Expand Down Expand Up @@ -972,7 +972,7 @@ def make_public=(value)
end

def documents_for_publication
documents.for_publication.or(site_notice_documents_for_publication)
documents.active.for_publication.or(site_notice_documents_for_publication)
end

def reporting_type_detail
Expand Down
2 changes: 1 addition & 1 deletion app/views/api/v1/planning_applications/_show.json.jbuilder
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ end
json.received_date planning_application.received_at
json.decision planning_application.decision if planning_application.determined?
json.constraints planning_application.planning_application_constraints.map(&:constraint).map(&:type_code) if planning_application.planning_application_constraints.any?
json.documents planning_application.documents.for_publication do |document|
json.documents planning_application.documents.active.for_publication do |document|
json.url api_v1_planning_application_document_url(planning_application, document)
json.blob_url document.representation_url
json.extract! document,
Expand Down
1 change: 0 additions & 1 deletion config/application.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
# frozen_string_literal: true

require_relative "boot"
require_relative "../lib/constraints/subdomain"

require "rails"
# Pick the frameworks you want:
Expand Down
2 changes: 2 additions & 0 deletions config/application.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,5 @@ shared:
staging_api_url: <%= ENV["STAGING_API_URL"] %>
uploads_hostname: <%= ENV.fetch("UPLOADS_HOSTNAME", "uploads.bops.localhost:3000") %>
uploads_base_url: <%= ENV.fetch("UPLOADS_BASE_URL", "http://uploads.bops.localhost:3000") %>
domain: <%= ENV.fetch("DOMAIN", "bops.localhost:3000") %>
use_signed_urls: <%= ENV.fetch("USE_SIGNED_URLS", "false") == "true" %>
12 changes: 6 additions & 6 deletions config/routes.rb
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
# frozen_string_literal: true

Rails.application.routes.draw do
extend BopsCore::Routing

get :healthcheck, to: proc { [200, {}, %w[OK]] }

constraints Constraints::DeviseSubdomain do
devise_subdomain do
devise_for :users, controllers: {
sessions: "users/sessions",
confirmations: "confirmations"
Expand All @@ -16,7 +18,7 @@
end
end

constraints Constraints::LocalAuthoritySubdomain do
local_authority_subdomain do
root to: "planning_applications#index"

concern :positionable do |options|
Expand Down Expand Up @@ -380,11 +382,9 @@
end
end

constraints Constraints::ConfigSubdomain do
config_subdomain do
mount BopsConfig::Engine, at: "/", as: :bops_config
end

constraints Constraints::UploadsSubdomain do
mount BopsUploads::Engine, at: "/", as: :bops_uploads
end
mount BopsUploads::Engine, at: "/", as: :bops_uploads
end
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ json.validAt planning_application.validated_at
json.publishedAt planning_application.published_at
json.decision planning_application.decision if planning_application.determined?
json.constraints planning_application.planning_application_constraints.map(&:constraint).map(&:type_code) if planning_application.planning_application_constraints.any?
json.documents planning_application.documents.for_publication do |document|
json.documents planning_application.documents.active.for_publication do |document|
json.url main_app.api_v1_planning_application_document_url(planning_application, document)
json.extract! document,
:created_at,
Expand Down
1 change: 1 addition & 0 deletions engines/bops_core/lib/bops_core.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# frozen_string_literal: true

require "bops_core/engine"
require "bops_core/routing"

module BopsCore
class << self
Expand Down
55 changes: 55 additions & 0 deletions engines/bops_core/lib/bops_core/routing.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
# frozen_string_literal: true

module BopsCore
module Routing
extend ActiveSupport::Concern

class LocalAuthoritySubdomain
class << self
def matches?(request)
LocalAuthority.pluck(:subdomain).include?(request.subdomain)
end
end
end

class ConfigSubdomain
class << self
def matches?(request)
request.subdomain == "config"
end
end
end

class DeviseSubdomain
class << self
def matches?(request)
ConfigSubdomain.matches?(request) || LocalAuthoritySubdomain.matches?(request)
end
end
end

class UploadsSubdomain
class << self
def matches?(request)
request.subdomain == "uploads"
end
end
end

def local_authority_subdomain(&)
constraints(LocalAuthoritySubdomain, &)
end

def config_subdomain(&)
constraints(ConfigSubdomain, &)
end

def devise_subdomain(&)
constraints(DeviseSubdomain, &)
end

def uploads_subdomain(&)
constraints(UploadsSubdomain, &)
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,23 @@

module BopsUploads
class ApplicationController < ActionController::Base
include BopsCore::ApplicationController

with_options to: :BopsUploads do
delegate :key_pair_id, :private_key, :cookie_signer
end

before_action :set_service
before_action :set_blob

private

def set_service
@service = ActiveStorage::Blob.service
end

def set_blob
@blob = ActiveStorage::Blob.find_by!(key: params[:key])
end
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
# frozen_string_literal: true

module BopsUploads
class BlobsController < ApplicationController
def show
serve_file(blob_path, content_type:, disposition:)
rescue Errno::ENOENT
head :not_found
end

private

def blob_path
@service.path_for(@blob.key)
end

def forcibly_serve_as_binary?
ActiveStorage.content_types_to_serve_as_binary.include?(@blob.content_type)
end

def allowed_inline?
ActiveStorage.content_types_allowed_inline.include?(@blob.content_type)
end

def content_type
forcibly_serve_as_binary? ? ActiveStorage.binary_content_type : @blob.content_type
end

def disposition
if forcibly_serve_as_binary? || !allowed_inline?
:attachment
else
:inline
end
end

def serve_file(path, content_type:, disposition:)
::Rack::Files.new(nil).serving(request, path).tap do |(status, headers, body)|
self.status = status
self.response_body = body

headers.each do |name, value|
response.headers[name] = value
end

response.headers.except!("X-Cascade", "x-cascade") if status == 416
response.headers["Content-Type"] = content_type || DEFAULT_SEND_FILE_TYPE
response.headers["Content-Disposition"] = disposition || DEFAULT_SEND_FILE_DISPOSITION
end
end
end
end
Loading
Loading