Upload the register photo direct to R2

.github/workflows/ci.yml

@@ -221,6 +221,11 @@ jobs:
       TRANSLATION_BRANCH: main
       TRANSLATIONS_SYNC_APP_ID: ${{ secrets.TRANSLATIONS_SYNC_APP_ID }}
       RETRY_FLAKY: true
+      # spec/integration/register_spec.rb uploads to the bikeindex-test bucket for real - the
+      # only way to exercise a presigned cross-origin PUT. Absent, that one example skips.
+      R2_TEST_ACCESS_KEY: ${{ secrets.R2_TEST_ACCESS_KEY }}
+      R2_TEST_ACCESS_KEY_SECRET: ${{ secrets.R2_TEST_ACCESS_KEY_SECRET }}
+      R2_TEST_ENDPOINT: ${{ secrets.R2_TEST_ENDPOINT }}
       KNAPSACK_PRO_TEST_SUITE_TOKEN_RSPEC: ${{ secrets.KNAPSACK_PRO_TEST_SUITE_TOKEN_RSPEC }}
       KNAPSACK_PRO_CI_NODE_TOTAL: ${{ matrix.ci_node_total }}
       KNAPSACK_PRO_CI_NODE_INDEX: ${{ matrix.ci_node_index }}

app/components/register/step2/component.html.erb

@@ -111,26 +111,22 @@
       </div>
     </div>
 
-    <% if show_photo? %>
-      <div class="tw:mb-4">
-        <span class="twlabel">
-          <%= translation(".attach_photo", cycle_type:) %>
-          <%= optional_badge %>
-        </span>
-
-        <%= render UI::Forms::FileUpload::Component.new(form_builder: f, attribute: :image,
-              accept: ImageUploader.permitted_extensions) %>
+    <%= render UI::Forms::Group::Component.new(form_builder: f, attribute: :image,
+        label_text: translation(".attach_photo", cycle_type:)) do %>
+      <%# The token rides the query string - DirectUpload posts to this url verbatim %>
+      <%= render UI::Forms::FileUpload::Component.new(form_builder: f, attribute: :image,
+            accept: ImageUploader.permitted_extensions,
+            direct_upload_url: register_direct_uploads_path(b_param_token: @b_param.id_token)) %>
 
-        <p
-          class="
-            tw:mt-1.5 tw:flex tw:items-start tw:gap-1.5 tw:text-xs
-            tw:text-gray-500 tw:dark:text-gray-400
-          "
-        >
-          <%= helpers.inline_svg_tag("icons/info.svg", class: "tw:mt-px tw:h-3.5 tw:w-3.5 tw:shrink-0", aria_hidden: true) %>
-          <%= translation(".database_public_note") %>
-        </p>
-      </div>
+      <p
+        class="
+          tw:mt-1.5 tw:flex tw:items-start tw:gap-1.5 tw:text-xs
+          tw:text-gray-500 tw:dark:text-gray-400
+        "
+      >
+        <%= helpers.inline_svg_tag("icons/info.svg", class: "tw:mt-px tw:h-3.5 tw:w-3.5 tw:shrink-0", aria_hidden: true) %>
+        <%= translation(".database_public_note") %>
+      </p>
     <% end %>
 
     <div class="tw:mb-4" data-controller="ui--collapse register--serial">

app/components/register/step2/component.rb

@@ -15,12 +15,6 @@ def cycle_type
         @b_param.type
       end
 
-      # An upload needs an account behind it, so an anonymous registrant gets the
-      # option once the confirmation link proves the address is theirs
-      def show_photo?
-        @current_user.present? || @b_param.email_confirmed?
-      end
-
       # A signed-in registration creates the bike straight from this step, so only
       # an anonymous one is ever waiting on the address being confirmed
       def awaiting_confirmation?

app/components/ui/forms/file_upload/component.en.yml

@@ -7,3 +7,5 @@ en:
           no_file_chosen: No file chosen
           take_picture: Take picture
           upload: Upload
+          upload_failed: upload failed
+          uploading: uploading

app/components/ui/forms/file_upload/component.html.erb

@@ -2,6 +2,10 @@
 <div
   data-controller="ui--forms--file-upload"
   data-ui--forms--file-upload-placeholder-value="<%= @placeholder %>"
+  <%# Blank unless the caller named an endpoint - the controller only uploads when it has one %>
+  data-ui--forms--file-upload-url-value="<%= @direct_upload_url %>"
+  data-ui--forms--file-upload-uploading-value="<%= translation(".uploading") %>"
+  data-ui--forms--file-upload-failed-value="<%= translation(".upload_failed") %>"
   data-action="drop->ui--forms--file-upload#drop dragover@document->ui--forms--file-upload#dragOver dragleave@document->ui--forms--file-upload#endDrag drop@document->ui--forms--file-upload#endDrag"
 >
   <%#
@@ -42,6 +46,10 @@
       <%# sr-only keeps the native input focusable and in the accessibility tree; the label below is the visible, clickable button. %>
       <%= @form_builder.file_field(@attribute, @html_options) %>
 
+      <% if @signed_id_field %>
+        <%= hidden_field_tag @signed_id_field, nil, data: {"ui--forms--file-upload-target": "signedId"} %>
+      <% end %>
+
       <%# Stimulus has no default event for a label, so name click explicitly. %>
       <%= @form_builder.label @attribute, label_content, class: @label_classes, data: {action: "click->ui--forms--file-upload#chooseFile"} %>
 

app/components/ui/forms/file_upload/component.rb

@@ -3,12 +3,15 @@
 module UI
   module Forms
     module FileUpload
+      # direct_upload_url: with JS, uploads on pick and posts the blob's signed id instead of
+      # the bytes. The field still renders as an ordinary one, so it posts the file when JS
+      # doesn't run - the controller drops its name only once it's driving the upload.
       class Component < ApplicationComponent
         # mb-0 cancels legacy bootstrap's `label` margin, which items-center would
         # otherwise center along with the button next to it.
         LABEL_CLASSES = "tw:mb-0 tw:whitespace-nowrap tw:peer-focus-visible:ring-3 tw:peer-focus-visible:ring-blue-500/40"
 
-        def initialize(form_builder:, attribute:, accept: nil, camera: nil, html_options: {})
+        def initialize(form_builder:, attribute:, accept: nil, camera: nil, direct_upload_url: nil, html_options: {})
           @form_builder = form_builder
           @attribute = attribute
           @placeholder = translation(".no_file_chosen")
@@ -21,6 +24,10 @@ def initialize(form_builder:, attribute:, accept: nil, camera: nil, html_options
           @attachment_url = attached_url
           @thumbnail_url = thumbnail_version_url || @attachment_url
 
+          @direct_upload_url = direct_upload_url
+          # Carries the blob the browser uploaded. Scoped to the form builder like every other
+          # field here, so two of these on one page don't collide on the same param
+          @signed_id_field = "#{form_builder.object_name}[#{attribute}_signed_id]" if direct_upload_url.present?
           @html_options = {
             class: "tw:peer tw:sr-only",
             accept: accept_list.join(",").presence,

app/controllers/direct_uploads/base_controller.rb

@@ -0,0 +1,45 @@
+# frozen_string_literal: true
+
+# ActiveStorage's own direct uploads controller accepts any content type, at any size, from
+# anyone. Both endpoints below it hold uploads to what PublicImage can serve; each subclass
+# adds who it will accept them from.
+module DirectUploads
+  class BaseController < ActiveStorage::DirectUploadsController
+    # Subclasses prepend their authorization, so it runs before this - binx_data can rely
+    # on whoever is asking having been resolved
+    before_action :require_permitted_file
+
+    # create_before_direct_upload! takes explicit keywords, so binx_data can't ride along in
+    # blob_args. Otherwise the same as ActiveStorage's.
+    def create
+      blob = ActiveStorage::Blob.create_before_direct_upload!(**blob_args)
+      blob.update!(binx_data:) if binx_data.present?
+      render json: direct_upload_json(blob)
+    end
+
+    private
+
+    # Refuses on what the browser declares, before any bytes are written - the presigned URL is
+    # signed against these values, so S3 holds the upload to the size it caps. PublicImage's
+    # validation is what checks the stored bytes, once there are some to check.
+    def require_permitted_file
+      return if PublicImage.file_permitted?(**blob_args.slice(:content_type, :byte_size))
+
+      head :unprocessable_entity
+    end
+
+    # ActiveStorage permits the client's metadata. Nothing reads it - analyze writes the real
+    # values and our own keys live in binx_data - so drop it rather than persist client input.
+    def blob_args
+      super.merge(metadata: {})
+    end
+
+    def binx_data
+      {}
+    end
+
+    def current_user
+      @current_user ||= AuthRestriction.user_from(request)
+    end
+  end
+end

app/controllers/direct_uploads_controller.rb

@@ -0,0 +1,14 @@
+# frozen_string_literal: true
+
+# Shadows ActiveStorage's own direct uploads route (see config/routes.rb) so the stock
+# controller isn't reachable. Signed-in surfaces upload here; the registration flow has its
+# own endpoint because it runs before there's an account to check.
+class DirectUploadsController < DirectUploads::BaseController
+  prepend_before_action :require_current_user
+
+  private
+
+  def require_current_user
+    head :forbidden if current_user.blank?
+  end
+end

app/controllers/register/direct_uploads_controller.rb

@@ -0,0 +1,28 @@
+# frozen_string_literal: true
+
+module Register
+  # A registration's photo is picked before there's an account to attach it to, so the b_param
+  # token stands in for a session. Anyone can start a registration, so this is scoping and
+  # attribution rather than a gate - the hourly per-IP throttle in rack_attack.rb is the teeth.
+  class DirectUploadsController < DirectUploads::BaseController
+    prepend_before_action :require_registration
+
+    private
+
+    def require_registration
+      head :forbidden if b_param.blank?
+    end
+
+    def b_param
+      return @b_param if defined?(@b_param)
+
+      @b_param = BikeServices::Register.find_token(user: current_user, params_token: params[:b_param_token])
+    end
+
+    # A signed id is a bearer token, so without this any registration could claim any blob.
+    # BParam#image_blob only hands back one stamped with its own id.
+    def binx_data
+      {"b_param_id" => b_param.id}
+    end
+  end
+end

app/controllers/register_controller.rb

@@ -57,8 +57,10 @@ def create
   end
 
   def update
+    # Both read straight from params - update_params is stored as json, which an upload can't be
     BikeServices::Register.save_step_2(@b_param, user: current_user,
-      image: params.dig(:bike, :image), bike_params: update_params)
+      image: params.dig(:bike, :image), image_signed_id: params.dig(:bike, :image_signed_id),
+      bike_params: update_params)
     if BikeServices::Register.creator_available?(@b_param)
       redirect_after_bike_creation(BikeServices::Register.create_bike(@b_param, ip_address: forwarded_ip_address))
     else

app/javascript/controllers/ui/forms/file_upload_controller.js

@@ -1,17 +1,40 @@
 import { Controller } from '@hotwired/stimulus'
+import { DirectUpload } from '@rails/activestorage'
 import { collapse } from 'utils/collapse_utils'
 
 // Connects to data-controller='ui--forms--file-upload'
 // Shows the selected filename (or a count for multiple files) in the field, previews an
 // image pick, and frames the controls as a drop target while a file is dragged over the page.
+// With a url value, uploads the pick straight to storage and posts its signed blob id.
 export default class extends Controller {
-  static targets = ['input', 'filename', 'dropZone', 'preview', 'previewImage']
-  static values = { placeholder: String }
+  static targets = ['input', 'filename', 'dropZone', 'preview', 'previewImage', 'signedId']
+  // stall: how long without progress before the upload is treated as dead
+  static values = {
+    placeholder: String,
+    url: String,
+    uploading: String,
+    failed: String,
+    stall: { type: Number, default: 30000 }
+  }
+
+  connect () {
+    this.boundHold = this.hold.bind(this)
+    this.form?.addEventListener('submit', this.boundHold)
+    // The field posts its own bytes until this runs, which is what makes the form work
+    // without JS - once we're uploading, the signed id is what the form carries instead.
+    if (this.urlValue) this.inputTarget.removeAttribute('name')
+  }
 
   disconnect () {
+    this.form?.removeEventListener('submit', this.boundHold)
+    clearTimeout(this.stallTimer)
     this.releaseObjectUrl()
   }
 
+  get form () {
+    return this.element.closest('form')
+  }
+
   // Both buttons open the one input; `capture` is what sends it to the camera.
   takePicture () {
     this.inputTarget.setAttribute('capture', 'environment')
@@ -75,6 +98,7 @@ export default class extends Controller {
         ? this.placeholderValue
         : files.length === 1 ? files[0].name : `${files.length} files`
     this.showPreview(files[0])
+    if (this.urlValue && files[0]) this.upload(files[0])
   }
 
   // Reads the file the browser already holds, so the preview lands on the pick rather than
@@ -105,6 +129,86 @@ export default class extends Controller {
     URL.revokeObjectURL(this.objectUrl)
     this.objectUrl = null
   }
+
+  // Only reached with a direct_upload_url, where the form carries the blob's signed id
+  // rather than the bytes.
+  upload (file) {
+    this.abortUpload() // Picking again shouldn't leave the discarded file uploading
+    this.signedIdTarget.value = ''
+    this.uploadingFile = file
+    this.status(file, this.uploadingValue)
+
+    const upload = new DirectUpload(file, this.urlValue, this)
+    this.currentUpload = upload
+    this.pending = new Promise((resolve) => { this.settle = resolve })
+    upload.create((error, blob) => {
+      if (this.currentUpload !== upload) return // A newer pick owns the field now
+
+      if (!error) this.signedIdTarget.value = blob.signed_id
+      this.status(file, error && this.failedValue)
+      this.finish()
+    })
+    this.watchForStall()
+  }
+
+  // Every ending runs through here, because a submit waiting on `pending` only moves when
+  // it settles - including the endings DirectUpload never reports.
+  finish () {
+    clearTimeout(this.stallTimer)
+    this.currentUpload = null
+    this.pending = null
+    this.settle?.()
+    this.settle = null
+  }
+
+  // DirectUpload listens for load and error, not abort, so an aborted upload never reaches
+  // its callback - without settling it here a held submit would wait on it forever.
+  abortUpload () {
+    if (!this.currentUpload) return
+
+    this.xhr?.abort()
+    this.finish()
+  }
+
+  // A connection that stops moving never errors, so nothing else would end the upload -
+  // the form would sit disabled behind a spinner until the page was reloaded.
+  watchForStall () {
+    clearTimeout(this.stallTimer)
+    this.stallTimer = setTimeout(() => {
+      this.signedIdTarget.value = ''
+      this.status(this.uploadingFile, this.failedValue)
+      this.abortUpload()
+    }, this.stallValue)
+  }
+
+  status (file, suffix) {
+    this.filenameTarget.textContent = suffix ? `${file.name} — ${suffix}` : file.name
+  }
+
+  // DirectUpload delegate hook - the handle that makes a discarded upload cancellable
+  directUploadWillStoreFileWithXHR (xhr) {
+    this.xhr = xhr
+    xhr.upload.addEventListener('progress', (event) => this.showProgress(event))
+  }
+
+  // Bytes moving is also what proves the connection is alive, so this restarts the stall watch
+  showProgress (event) {
+    this.watchForStall()
+    if (!event.lengthComputable) return
+
+    const percent = Math.round((event.loaded / event.total) * 100)
+    this.status(this.uploadingFile, `${this.uploadingValue} ${percent}%`)
+  }
+
+  // Submitting mid-upload would drop the file, so hold the form until the blob lands.
+  // A failed upload submits anyway - the rest of the form matters more.
+  async hold (event) {
+    if (!this.pending) return
+
+    event.preventDefault()
+    await this.pending
+    this.form.requestSubmit()
+  }
 }
 
 // Dragged text and page elements fire these events too; only files matter here.

app/models/b_param.rb

@@ -441,6 +441,21 @@ def confirm_email!
     email_confirmed? || update(params: params.merge("email_confirmed_at" => Time.current))
   end
 
+  # An ActiveStorage blob the browser uploaded straight to the bucket. Held as a signed id
+  # rather than an attachment so the blob's only owner is the PublicImage created from it -
+  # a b_param attachment would purge the blob out from under the bike when it's cleaned up.
+  def image_signed_id
+    params["image_signed_id"]
+  end
+
+  # nil once CleanUnattachedBlobsJob has reaped it, which a late registration has to survive.
+  # Only a blob this registration minted - a signed id is a bearer token, so without the stamp
+  # any registration could claim any other's photo.
+  def image_blob
+    blob = ActiveStorage::Blob.find_signed(image_signed_id)
+    blob if blob&.binx_data&.dig("b_param_id") == id
+  end
+
   def primary_frame_color
     primary_frame_color_id.present? && Color.find_by_id(primary_frame_color_id)&.name
   end