From f2248ea744eee6c46741c0d9df92ba9d19d99a17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A2u=20Cao?= Date: Mon, 2 Nov 2026 22:59:06 +0400 Subject: [PATCH] Rework storage dashboard and fix authorization revocation - One card per app (grouped by client_id) showing distinct device and category counts; the whole card opens the details modal (with hover and focus highlight) instead of a separate Details button - Details modal: access list styled like the OAuth consent dialog via a shared RsScopeListComponent, plus per-token device, connected date and expiry, each with its own Revoke - "folder" -> "category" in the card, modal, consent dialog and the auth-created notification - Make revoke a real DELETE: it was a GET link that Turbo 8's prefetch-on-hover fired on its own, silently deleting authorizations. Missing or foreign authorizations now return 404 instead of 500. - Move "Launch app" to the app level (GET /services/storage/apps/:client_id/launch); no token id in the URL Part of #254 --- app/components/rs_auth_component.html.erb | 53 ++++++--------- app/components/rs_auth_component.rb | 21 ++---- .../rs_auth_details_modal_component.html.erb | 53 +++++++-------- .../rs_auth_details_modal_component.rb | 9 +-- .../rs_scope_list_component.html.erb | 25 +++++++ app/components/rs_scope_list_component.rb | 14 ++++ .../services/rs_auths_controller.rb | 26 ++++--- app/mailers/notification_mailer.rb | 2 +- app/views/rs/oauth/new.html.erb | 28 +------- app/views/services/rs_auths/index.html.erb | 4 +- config/routes.rb | 9 +-- .../services/rs_auths_controller_spec.rb | 56 ++++++++++++++-- spec/features/rs/oauth_spec.rb | 2 +- spec/features/services/rs_auths_spec.rb | 67 ++++++++++++------- 14 files changed, 213 insertions(+), 156 deletions(-) create mode 100644 app/components/rs_scope_list_component.html.erb create mode 100644 app/components/rs_scope_list_component.rb diff --git a/app/components/rs_auth_component.html.erb b/app/components/rs_auth_component.html.erb index 863af3a..205a760 100644 --- a/app/components/rs_auth_component.html.erb +++ b/app/components/rs_auth_component.html.erb @@ -1,41 +1,28 @@ -
-
-
+
+ -
+ + <%= render RsAuthDetailsModalComponent.new(authorizations: @authorizations) %>
diff --git a/app/components/rs_auth_component.rb b/app/components/rs_auth_component.rb index 56e003a..c5f90a1 100644 --- a/app/components/rs_auth_component.rb +++ b/app/components/rs_auth_component.rb @@ -17,25 +17,18 @@ class RsAuthComponent < ViewComponent::Base (@auth.permissions & [":r", ":rw"]).any? end - def scope_summary - return "All folders and files" if root_access? + def category_summary + return "All categories and files" if root_access? count = @auth.permissions.size - "#{count} #{'folder'.pluralize(count)}" + "#{count} #{'category'.pluralize(count)}" end - def device_name - @auth.device_name.presence || "Unknown device" + def device_count + @authorizations.map { |auth| auth.device_name.presence || "Unknown device" }.uniq.size end - def expiry_label - return "Never expires" if @auth.expire_at.blank? - return "Expired" if @auth.expired? - - "Expires #{@auth.expire_at.strftime('%-d %b %Y')}" - end - - def multiple_authorizations? - @authorizations.size > 1 + def device_summary + "#{device_count} #{'device'.pluralize(device_count)}" end end diff --git a/app/components/rs_auth_details_modal_component.html.erb b/app/components/rs_auth_details_modal_component.html.erb index ce0233a..7955bfe 100644 --- a/app/components/rs_auth_details_modal_component.html.erb +++ b/app/components/rs_auth_details_modal_component.html.erb @@ -1,47 +1,42 @@ <%= render ModalComponent.new do %>
-
+
<%= render AppCatalog::WebAppIconComponent.new(web_app: @web_app) %>
-

<%= name %>

+

<%= name %>

<%= @auth.client_id %>

-
Access
- <% if root_access? %> -

All folders and files

- <% else %> -
    - <% @auth.permissions.each do |scope| %> -
  • <%= scope_label(scope) %>
    • - <% end %> -
- <% end %> - -
Device
-

<%= device_name %>

- -
- <%= @authorizations.size > 1 ? "Connected tokens" : "Connected" %> +
+ Access
-
    + <%= render RsScopeListComponent.new(scopes: @auth.permissions, root: root_access?) %> + +
    + Connected devices +
    +
      <% @authorizations.each do |auth| %> -
    • - - <%= auth.created_at.strftime('%-d %b %Y') %> - · - <%= status_label(auth) %> - - <%= link_to "Revoke access", revoke_services_storage_rs_auth_path(auth), - class: "text-red-700 whitespace-nowrap" %> +
    • +
      +

      <%= device_name(auth) %>

      +

      + Connected <%= auth.created_at.strftime('%-d %b %Y') %> + · + "><%= status_label(auth) %> +

      +
      + <%= button_to "Revoke access", services_storage_rs_auth_path(auth), method: :delete, + form: { class: "flex-none" }, + class: "whitespace-nowrap text-sm text-red-700 hover:underline bg-transparent border-0 p-0 cursor-pointer" %>
      • <% end %>
    - <%= link_to "Launch app", launch_app_services_storage_rs_auth_path(@auth), - class: "btn-md btn-blue", target: "_blank", rel: "noopener" %> + <%= link_to "Launch app", launch_app_services_storage_path(@auth.client_id), + class: "btn-md btn-blue", target: "_blank", rel: "noopener", data: { turbo: false } %>
    <% end %> diff --git a/app/components/rs_auth_details_modal_component.rb b/app/components/rs_auth_details_modal_component.rb index 257329c..3bd9481 100644 --- a/app/components/rs_auth_details_modal_component.rb +++ b/app/components/rs_auth_details_modal_component.rb @@ -17,13 +17,8 @@ class RsAuthDetailsModalComponent < ViewComponent::Base (@auth.permissions & [":r", ":rw"]).any? end - def scope_label(scope) - permission = scope_permissions(scope) == "r" ? "read only" : "read/write" - "#{scope_name(scope)} (#{permission})" - end - - def device_name - @auth.device_name.presence || "Unknown device" + def device_name(auth) + auth.device_name.presence || "Unknown device" end def status_label(auth) diff --git a/app/components/rs_scope_list_component.html.erb b/app/components/rs_scope_list_component.html.erb new file mode 100644 index 0000000..42ae256 --- /dev/null +++ b/app/components/rs_scope_list_component.html.erb @@ -0,0 +1,25 @@ +<% if @root %> +

    + + <%= render partial: "icons/alert-triangle", + locals: { custom_class: "inline-block align-bottom mr-1.5" } %> + All categories and files + + <% if read_only? %> + (read only) + <% end %> +

    +<% else %> + <% @scopes.each do |scope| %> +

    + + <%= render partial: "icons/folder", + locals: { custom_class: "inline-block align-bottom mr-1.5" } %> + <%= scope_name(scope) %> + + <% if scope_permissions(scope) == "r" %> + (read only) + <% end %> +

    + <% end %> +<% end %> diff --git a/app/components/rs_scope_list_component.rb b/app/components/rs_scope_list_component.rb new file mode 100644 index 0000000..198ef08 --- /dev/null +++ b/app/components/rs_scope_list_component.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +class RsScopeListComponent < ViewComponent::Base + include OauthHelper + + def initialize(scopes:, root: false) + @scopes = scopes + @root = root + end + + def read_only? + @scopes.include?(":r") + end +end diff --git a/app/controllers/services/rs_auths_controller.rb b/app/controllers/services/rs_auths_controller.rb index 3518b2a..b60a684 100644 --- a/app/controllers/services/rs_auths_controller.rb +++ b/app/controllers/services/rs_auths_controller.rb @@ -2,15 +2,15 @@ class Services::RsAuthsController < Services::BaseController before_action :authenticate_user! before_action :require_service_enabled before_action :require_service_available - before_action :find_rs_auth, only: [:destroy, :launch_app] + before_action :find_rs_auth, only: [:destroy] def index - @rs_auth_groups = current_user.remote_storage_authorizations - .includes(:web_app) - .group_by { |auth| [auth.client_id, auth.device_name] } - .values - .map { |auths| auths.sort_by(&:created_at).reverse } - .sort_by { |auths| app_name_for(auths.first).downcase } + @rs_apps = current_user.remote_storage_authorizations + .includes(:web_app) + .group_by(&:client_id) + .values + .map { |auths| auths.sort_by(&:created_at).reverse } + .sort_by { |auths| app_name_for(auths.first).downcase } end def destroy @@ -26,6 +26,12 @@ class Services::RsAuthsController < Services::BaseController end def launch_app + auth = current_user.remote_storage_authorizations + .where(client_id: params[:client_id]) + .order(created_at: :desc) + .first + http_status(:not_found) and return unless auth + user_address = if Rails.env.development? "#{current_user.cn}@#{request.host_with_port}" @@ -35,7 +41,7 @@ class Services::RsAuthsController < Services::BaseController # Storage-first bearer token issuance (remoteStorage spec §11): the app # receives the address and runs the application-first flow itself. - launch_url = "#{@auth.launch_url}#remotestorage=#{user_address}" + launch_url = "#{auth.launch_url}#remotestorage=#{user_address}" redirect_to launch_url, allow_other_host: true end @@ -57,7 +63,7 @@ class Services::RsAuthsController < Services::BaseController end def find_rs_auth - @auth = current_user.remote_storage_authorizations.find(params[:id]) - http_status :not_found unless @auth.present? + @auth = current_user.remote_storage_authorizations.find_by(id: params[:id]) + http_status :not_found unless @auth end end diff --git a/app/mailers/notification_mailer.rb b/app/mailers/notification_mailer.rb index d281380..0f2414b 100644 --- a/app/mailers/notification_mailer.rb +++ b/app/mailers/notification_mailer.rb @@ -11,7 +11,7 @@ class NotificationMailer < ApplicationMailer @auth = params[:auth] @permissions = @auth.permissions.map do |p| access = p.split(":")[1] == 'r' ? 'read' : 'read/write' - directory = p.split(':')[0] == '' ? 'all folders and files' : p.split(':')[0] + directory = p.split(':')[0] == '' ? 'all categories and files' : p.split(':')[0] "#{access} #{directory}" end @subject = "New app connected to your storage" diff --git a/app/views/rs/oauth/new.html.erb b/app/views/rs/oauth/new.html.erb index 4b7bc25..2668c14 100644 --- a/app/views/rs/oauth/new.html.erb +++ b/app/views/rs/oauth/new.html.erb @@ -5,34 +5,10 @@

    The app on <%= link_to @client_id, "https://#{@client_id}" %> - is asking for access to these folders: + is asking for access to these categories:

    - <% if @root_access_requested %> -

    - - <%= render partial: "icons/alert-triangle", - locals: { custom_class: "inline-block align-bottom mr-1.5" } %> - All files and directories - - <% if (@scopes & [":r"]).any? %> - (read only) - <% end %> -

    - <% else %> - <% @scopes.each do |scope| %> -

    - - <%= render partial: "icons/folder", - locals: { custom_class: "inline-block align-bottom mr-1.5" } %> - <%= scope_name(scope) %> - - <% if scope_permissions(scope) == "r" %> - (read only) - <% end %> -

    - <% end %> - <% end %> + <%= render RsScopeListComponent.new(scopes: @scopes, root: @root_access_requested) %> <%= form_with(url: rs_oauth_path, method: :post, data: { turbo: false }) do |f| %> <%= f.hidden_field :redirect_uri, value: @redirect_uri %> diff --git a/app/views/services/rs_auths/index.html.erb b/app/views/services/rs_auths/index.html.erb index 9141066..edd6ade 100644 --- a/app/views/services/rs_auths/index.html.erb +++ b/app/views/services/rs_auths/index.html.erb @@ -10,9 +10,9 @@ <%= render partial: "shared/tabnav_remotestorage" %>
    - <% if @rs_auth_groups.any? %> + <% if @rs_apps.any? %>
    - <% @rs_auth_groups.each do |authorizations| %> + <% @rs_apps.each do |authorizations| %> <%= render RsAuthComponent.new(authorizations: authorizations) %> <% end %>
    diff --git a/config/routes.rb b/config/routes.rb index d8af5b2..5b78bad 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -52,12 +52,9 @@ Rails.application.routes.draw do resource :storage, controller: 'remotestorage', only: [:show] do get :apps, to: "rs_auths#index" - resources :rs_auths, only: [:index, :destroy] do - member do - get :revoke, to: 'rs_auths#destroy' - get :launch_app - end - end + get "apps/:client_id/launch", to: "rs_auths#launch_app", as: :launch_app, + constraints: { client_id: /[^\/]+/ } + resources :rs_auths, only: [:index, :destroy] end end diff --git a/spec/controllers/services/rs_auths_controller_spec.rb b/spec/controllers/services/rs_auths_controller_spec.rb index 812a842..0970c3c 100644 --- a/spec/controllers/services/rs_auths_controller_spec.rb +++ b/spec/controllers/services/rs_auths_controller_spec.rb @@ -11,13 +11,13 @@ RSpec.describe Services::RsAuthsController, type: :controller do ]) end - describe "GET /services/storage/rs_auths/:id/launch_app (storage-first)" do + describe "GET /services/storage/apps/:client_id/launch (storage-first)" do context "when user is signed in" do before do sign_in user end - context "token exists" do + context "authorization exists" do before do @auth = user.remote_storage_authorizations.create!( permissions: %w(documents), client_id: "app.example.com", @@ -25,7 +25,7 @@ RSpec.describe Services::RsAuthsController, type: :controller do expire_at: 2.days.from_now ) - get :launch_app, params: { id: @auth.id } + get :launch_app, params: { client_id: @auth.client_id } end after do @@ -48,7 +48,7 @@ RSpec.describe Services::RsAuthsController, type: :controller do { services_enabled: [] } ]) - get :launch_app, params: { id: 1 } + get :launch_app, params: { client_id: "app.example.com" } end it "responds with forbidden" do @@ -57,4 +57,52 @@ RSpec.describe Services::RsAuthsController, type: :controller do end end end + + describe "GET /services/storage/apps/:client_id/launch for an unknown app" do + before { sign_in user } + + it "returns 404 instead of raising" do + get :launch_app, params: { client_id: "nonexistent.example.com" } + + expect(response).to have_http_status(:not_found) + end + end + + describe "DELETE /services/storage/rs_auths/:id" do + context "when user is signed in" do + before { sign_in user } + + it "revokes the authorization" do + auth = user.remote_storage_authorizations.create!( + permissions: %w(documents), client_id: "app.example.com", + redirect_uri: "https://app.example.com", expire_at: 2.days.from_now + ) + + expect { + delete :destroy, params: { id: auth.id } + }.to change(RemoteStorageAuthorization, :count).by(-1) + + expect(response).to redirect_to(apps_services_storage_url) + end + + it "returns 404 for an unknown authorization" do + delete :destroy, params: { id: 0 } + + expect(response).to have_http_status(:not_found) + end + + it "returns 404 for another user's authorization" do + other = create :user, id: 99, cn: "otheruser", email: "other@example.com" + auth = other.remote_storage_authorizations.create!( + permissions: %w(documents), client_id: "app.example.com", + redirect_uri: "https://app.example.com" + ) + + delete :destroy, params: { id: auth.id } + + expect(response).to have_http_status(:not_found) + expect(RemoteStorageAuthorization.exists?(auth.id)).to be(true) + end + end + end end diff --git a/spec/features/rs/oauth_spec.rb b/spec/features/rs/oauth_spec.rb index de0527d..085198e 100644 --- a/spec/features/rs/oauth_spec.rb +++ b/spec/features/rs/oauth_spec.rb @@ -44,7 +44,7 @@ RSpec.describe 'remoteStorage OAuth Dialog', type: :feature do it "shows a special permission for all files and dirs" do within ".scope" do - expect(page).to have_content("All files and directories") + expect(page).to have_content("All categories and files") end end end diff --git a/spec/features/services/rs_auths_spec.rb b/spec/features/services/rs_auths_spec.rb index 9fd69b0..6cec9a7 100644 --- a/spec/features/services/rs_auths_spec.rb +++ b/spec/features/services/rs_auths_spec.rb @@ -44,50 +44,71 @@ RSpec.describe "Storage connected apps", type: :feature do after { auth.destroy } - it "shows the app, its device and permission status" do + it "shows the app with device and category counts" do visit apps_services_storage_path - expect(page).to have_content("Notes") - expect(page).to have_content("notes.example.com") - expect(page).to have_content("Firefox on Linux") - expect(page).to have_content("2 folders") - expect(page).to have_content("Expires") + within ".rs-auth-card" do + expect(page).to have_content("Notes") + expect(page).to have_content("notes.example.com") + expect(page).to have_content("1 device") + expect(page).to have_content("2 categories") + end end - it "renders a details modal with scopes and actions" do + it "does not show the permission expiry on the card" do visit apps_services_storage_path - expect(page).to have_selector("li", text: "documents (read/write)", visible: false) - expect(page).to have_selector("li", text: "photos (read only)", visible: false) + within ".rs-auth-card" do + expect(page).not_to have_content("Expires") + end + end + + it "renders access and per-token details in the modal" do + visit apps_services_storage_path + + expect(page).to have_selector("p.scope", text: "documents", visible: false) + expect(page).to have_selector("p.scope", text: "photos", visible: false) + expect(page).to have_selector("h5", text: "Connected devices", visible: false) + expect(page).to have_selector("p", text: "Firefox on Linux", visible: false) + expect(page).to have_selector("p", text: /Expires/, visible: false) + expect(page).to have_button("Revoke access", visible: false) expect(page).to have_link("Launch app", visible: false) - expect(page).to have_selector("a", text: "Revoke access", visible: false, count: 1) end end - context "with duplicates" do - let!(:first) { create_auth(client: "notes.example.com", device: firefox_ua) } - let!(:second) { create_auth(client: "notes.example.com", device: firefox_ua) } - let!(:other) { create_auth(client: "notes.example.com", device: chrome_ua) } + context "with multiple apps" do + let!(:notes) { create_auth(client: "notes.example.com", device: firefox_ua) } + let!(:photos) { create_auth(client: "photos.example.com", device: firefox_ua) } after do - first.destroy - second.destroy - other.destroy + notes.destroy + photos.destroy end - it "groups authorizations for the same app and device into one card" do + it "shows one card per app" do visit apps_services_storage_path expect(page).to have_selector(".rs-auth-card", count: 2) - expect(page).to have_content("Firefox on Linux") - expect(page).to have_content("Chrome on macOS") - expect(page).to have_content("Tokens") + end + end + + context "with one app on multiple devices" do + let!(:laptop) { create_auth(client: "notes.example.com", device: firefox_ua) } + let!(:phone) { create_auth(client: "notes.example.com", device: chrome_ua) } + + after do + laptop.destroy + phone.destroy end - it "lists every authorization in the group with its own revoke action" do + it "shows a single card with the device count" do visit apps_services_storage_path - expect(page).to have_selector("a", text: "Revoke access", visible: false, count: 3) + expect(page).to have_selector(".rs-auth-card", count: 1) + + within ".rs-auth-card" do + expect(page).to have_content("2 devices") + end end end