Allow users to customize ActiveStorage controllers without having to override them

[santib]

Summary

Related to #41503 , it should be normal for production apps to add authentication and authorization to their ActiveStorage controllers. Unfortunately, there are 2 possible ways to achieve it currently:

• Not drawing ActiveStorage routes and do everything by yourself
• Override/monkey patch ActiveStorage controllers

None of them is ideal because in the end you can't benefit from Rails upgrades (bug fixes, etc) so the intention of this PR is to let people define a parent controller (inspired by Devise, maybe @carlosantoniodasilva can tell us his experience on this feature) so that people can add authentication and authorization in a single place and still benefit from the default controllers.

Usage example:

# app/controllers/active_storage/application_controller.rb
class ActiveStorage::ApplicationController < ActiveStorage::BaseController
  before_action :authenticate_user!, if: :needs_authentication?
  before_action :authorize_access, if: :needs_authorization?

  # ...
end

# config/application.rb
  # ...
  config.active_storage.parent_controller = "ActiveStorage::ApplicationController"
  # ...

If there is interest in a feature like this I can add tests, add it to the guides and CHANGELOG. Please let me know

[santib]

Another option would be to have rails active_storage:install generate a app/controllers/active_storage/base_controller.rb with a content like

# frozen_string_literal: true

# The base class for all Active Storage controllers.
# You should implement authentication and authorization mechanisms appropriate to your needs.
# For example, for the `create` and `update` actions you can authenticate the user to prevent distributed attacks
# and for `show` action you can authenticate the user + authorize access to the specific `ActiveStorage::Blob` according to your business logic.
class ActiveStorage::BaseController < ActionController::Base
  include ActiveStorage::SetCurrent, ActiveStorage::Streaming

  protect_from_forgery with: :exception

  self.etag_with_template_digest = false
end

idk.. there are many possible solutions but I'd love to hear your thoughts @georgeclaghorn because I think this is important, and I'd love to help making ActiveStorage a more secure place.

[georgeclaghorn]

I’m not sure about this specific approach yet—need to think about it more. This is a good start, though, and I’d like to do something along these lines. Another thing to consider are these authorization monkey-patches we use (cleaned up a bit for sharing), which I’d love to upstream in some form:

ActiveSupport.on_load :active_storage_blob do
  def accessible_to?(accessor)
    attachments.includes(:record).any? { |attachment| attachment.accessible_to?(accessor) } || attachments.none?
  end
end

ActiveSupport.on_load :active_storage_attachment do
  def accessible_to?(accessor)
    record.try(:accessible_to?, accessor)
  end
end

ActiveSupport.on_load :action_text_rich_text do
  def accessible_to?(accessor)
    record.try(:accessible_to?, accessor)
  end
end

module ActiveStorage::Authorize
  extend ActiveSupport::Concern

  included do
    before_action :require_authorization
  end

  private
    def require_authorization
      head :forbidden unless authorized?
    end

    def authorized?
      @blob.accessible_to?(Current.identity)
    end
end

Rails.application.config.to_prepare do
  ActiveStorage::Blobs::RedirectController.include ActiveStorage::Authorize
  ActiveStorage::Blobs::ProxyController.include ActiveStorage::Authorize
  ActiveStorage::Representations::RedirectController.include ActiveStorage::Authorize
  ActiveStorage::Representations::ProxyController.include ActiveStorage::Authorize
end

Application models implement accessible_to?(accessor) to indicate authorization:

class Entry < ApplicationRecord
  def accessible_to?(accessor)
    in? accessor.accessible_entries
  end
end

[santib]

Glad to hear you are onboard with this 😄 I'll continue thinking on alternatives and analyze their pros and cons.

Regarding authorization, I think it's fair to provide out of the box a mechanism just for ActiveStorage. Specifically talking about the code you shared, I like it a lot but the only doubt I have is if the accessible_to? could need to behave differently not just depending on the record but also depending on the association. For example I could have

class User < ApplicationRecord
  has_one_attached :passport
  has_one_attached :avatar

  def accessible_to?(accessor, attachment_name)
    return true if attachment_name == "avatar" # avatar is publicly accessible

    in? accessor.accessible_entries
  end
end

thoughts?

[rails-bot]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

[zzak]

🦄

[zzak]

@santib What's left here? I'm happy to help move this along if I can.

[santib]

@santib What's left here? I'm happy to help move this along if I can.

❤️ That'd be amazing!

Quoting George:

I’m not sure about this specific approach yet—need to think about it more. This is a good start, though, and I’d like to do something along these lines.

So I think we need to discuss what's the best approach for this. Some options:

• Configurable base controller (current changes)
• When installing ActiveStorage the base controller gets created in your app (like I mentioned here Allow users to customize ActiveStorage controllers without having to override them #41505 (comment))
• Other?

All of them have their pros and cons but for now I'm inclined to the current PR approach since it doesn't affect anyone but allows further customization.

Once we've decided which approach to follow we can polish the implementation and write some docs to explain users how to add authorization on ActiveStorage

[zzak]

My initial thought was partial to Configurable base controller (current changes), but this seems like kind of an anti-pattern. How are other libraries solving this? It seems that the Rails Way would be to generate the controller (either on install, and/or using rails g) -- so the more I think about this that seems like the better option.

Customizing that base controller means the user can opt-out completely and diverge from AS::Base which may create bigger gaps making upgrades even more difficult. While providing a generator means, there is still a common base (unless ofcourse the user changes it) allowing greater control between upgrades without diverging too far. For example, the user could generate a new controller and start migrating their old controller there -- but if they're monkey-patching the crap out of AS::Base it will make our job more difficult, possibly. Just some random thoughts here, sorry if it's kind of a mess.

Anyways, I'm not sure who's best to make this decision -- I'd like to consider what other libraries are doing first but perhaps @pixeltrix or @carlosantoniodasilva knows?

[pixeltrix]

Still having a think about this one but #32238 is relevant here - wondering whether having a on_load hook would be better like George's authorisation patches.

The pattern of inheriting from a configuration is certainly one that's seen in the wild - as the original comment says, it's what Devise uses but it's not a pattern we've used in Rails before.

[p8]

What about making @georgeclaghorn's monkey patches a commented-out initializer?
Just like the Content Security Policy: https://github.com/rails/rails/blob/5647a9c1ced68d20338552d47a3b755e10a271c4/railties/lib/rails/generators/rails/app/templates/config/initializers/content_security_policy.rb.tt

[rails-bot]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

[tomrossi7]

I'm partial to the solution originally proposed. It follows a pattern already established in Rails. For example, using an application-specific ApplicationStorageController which inherits from ActiveStorage::BaseController is very similar to the ApplicationRecord which inherits from ActiveRecord::Base or ApplicationJob which inherits from ActiveJob::Base.

I'm not saying its a better solution that what @georgeclaghorn provided, but it seems to fit the Rails approach in other areas. 🤷‍♂️

[TylerRick]

This is a good idea! Can we remove the stale label and revive this PR?

Having a ApplicationStorageController which inherits from ActiveStorage::BaseController approach sounds the most similar to the usual Rails way to me. Would someone like to create a PR for that approach?

Which approach does the Rails core team prefer?

[sedubois]

It seems nowadays the recommended way to ask whether a PR could be revived is from Discord (there should be a channel for that).

[TylerRick]

I don't understand why it should be so hard to keep issues open / reopen them. That's just going to cause people to open a duplicate issue/PR — or (if they notice in time) cause people to add extra "not stale" noise when the bot warns it's about to be closed. Wouldn't it be preferable to keep the discussion together in one place instead of spreading across duplicate issues? (Similarly, moving the meta conversation about an issue out to a completely separate system (Discord) seems like the wrong direction, because it wouldn't be visible to/discoverable by those arriving at the closed issue.)

I get how it's useful to have stale issues not cluttering the list. But if interes/activity later picks up again, then "stale" is no longer accurate and its status should be automatically updated to reflect its newfound freshness... like it did back here:

[TylerRick]

Thinking more about the ApplicationStorageController idea... Based on the name, I bet most people would assume this is implementing the same pattern — an application-specific concrete class inheriting from a framework-provided base class — as ApplicationRecord/ApplicationJob. When in reality, it's doing the exact opposite — defining a base class that the 4 framework-provided concrete controllers inherit from — which I think could be confusing.

What would make it less confusing is if you had to explicitly configure it to use your class as a base class, like config.active_storage.base_controller = "ActiveStorage::ApplicationController", which is what this #41505 proposes. That is sounding more and more like the best solution to me. Does anyone have an approach they would like better?

In the meantime, given that the docs-recommended approach has the problems listed by the OP (it's a do-it-all-yourself copy-paste solution that doesn't benefit from the default routes provided by ActiveStorage or from improvements made upstream), the only reasonable option seems to be to monkey patch... and @georgeclaghorn's approach seems like a really clean way to do that — thanks for that!

[santib]

Hey @TylerRick I feel you, but don't worry, this has already been addressed by the Rails Core team https://rubyonrails.org/2022/6/13/rails-discord-server-is-now-open-to-the-public

We can ask them in the discord to continue the conversation here 😄

[skipkayhil]

this has already been addressed by the Rails Core team https://rubyonrails.org/2022/6/13/rails-discord-server-is-now-open-to-the-public

Exactly, PRs no longer go stale for this reason. I'll re-open so it hopefully gets more attention