From c1548035ec084d7626a558d784ad5d65de809d22 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A2u=20Cao?= Date: Mon, 2 Nov 2026 13:04:42 +0400 Subject: [PATCH 1/5] Add storage dashboard card and per-app details modal - Rework RsAuthComponent into a card that shows the app, its device, an access summary and the permission status - Group connected apps by app and device, so repeated authorizations for the same app and device (e.g. after a browser update) are shown together, each with its own revoke action - Add RsAuthDetailsModalComponent with the full scope list, device, connected tokens and permission expiry, plus Launch and Revoke actions (built on the existing ModalComponent) - Add RemoteStorageAuthorization#device_name (parsed from the user agent) and #expired? Part of #254 --- app/components/rs_auth_component.html.erb | 62 ++++++----- app/components/rs_auth_component.rb | 39 ++++++- .../rs_auth_details_modal_component.html.erb | 47 ++++++++ .../rs_auth_details_modal_component.rb | 35 ++++++ .../services/rs_auths_controller.rb | 12 ++- app/models/remote_storage_authorization.rb | 28 +++++ app/views/services/rs_auths/index.html.erb | 8 +- spec/features/services/rs_auths_spec.rb | 101 ++++++++++++++++++ .../remote_storage_authorization_spec.rb | 51 +++++++++ 9 files changed, 350 insertions(+), 33 deletions(-) create mode 100644 app/components/rs_auth_details_modal_component.html.erb create mode 100644 app/components/rs_auth_details_modal_component.rb create mode 100644 spec/features/services/rs_auths_spec.rb diff --git a/app/components/rs_auth_component.html.erb b/app/components/rs_auth_component.html.erb index a26d35d..863af3a 100644 --- a/app/components/rs_auth_component.html.erb +++ b/app/components/rs_auth_component.html.erb @@ -1,27 +1,41 @@ -
-
- <%= render AppCatalog::WebAppIconComponent.new(web_app: @web_app) %> +
+
+
+ <%= render AppCatalog::WebAppIconComponent.new(web_app: @web_app) %> +
+
+

<%= name %>

+

<%= @auth.client_id %>

+
-
-

- <%= @web_app&.name || @auth.app_name %> -

-

- <%= @auth.client_id %> -

+ +
+
+
Access
+
<%= scope_summary %>
+
+
+
Device
+
<%= device_name %>
+
+
+
Status
+
"><%= expiry_label %>
+
+ <% if multiple_authorizations? %> +
+
Tokens
+
<%= @authorizations.size %>
+
+ <% end %> +
+ +
+
- <%= render DropdownComponent.new do %> - <%= render DropdownLinkComponent.new( - href: launch_app_services_storage_rs_auth_url(@auth), - open_in_new_tab: true - ) do %> - Launch app - <% end %> - <%= render DropdownLinkComponent.new( - href: revoke_services_storage_rs_auth_url(@auth), - separator: true, add_class: "text-red-700" - ) do %> - Revoke access - <% end %> - <% end %> + + <%= render RsAuthDetailsModalComponent.new(authorizations: @authorizations) %>
diff --git a/app/components/rs_auth_component.rb b/app/components/rs_auth_component.rb index ed588bf..56e003a 100644 --- a/app/components/rs_auth_component.rb +++ b/app/components/rs_auth_component.rb @@ -1,8 +1,41 @@ # frozen_string_literal: true class RsAuthComponent < ViewComponent::Base - def initialize(auth:) - @auth = auth - @web_app = auth.web_app + include OauthHelper + + def initialize(authorizations:) + @authorizations = authorizations + @auth = authorizations.first + @web_app = @auth.web_app + end + + def name + @web_app&.name.presence || @auth.app_name.presence || @auth.client_id + end + + def root_access? + (@auth.permissions & [":r", ":rw"]).any? + end + + def scope_summary + return "All folders and files" if root_access? + + count = @auth.permissions.size + "#{count} #{'folder'.pluralize(count)}" + end + + def device_name + @auth.device_name.presence || "Unknown device" + 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 end end diff --git a/app/components/rs_auth_details_modal_component.html.erb b/app/components/rs_auth_details_modal_component.html.erb new file mode 100644 index 0000000..ce0233a --- /dev/null +++ b/app/components/rs_auth_details_modal_component.html.erb @@ -0,0 +1,47 @@ +<%= render ModalComponent.new do %> +
+
+ <%= render AppCatalog::WebAppIconComponent.new(web_app: @web_app) %> +
+
+

<%= 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" %> +
+
    + <% @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" %> +
    • + <% end %> +
+ +
+ <%= link_to "Launch app", launch_app_services_storage_rs_auth_path(@auth), + class: "btn-md btn-blue", target: "_blank", rel: "noopener" %> +
+<% end %> diff --git a/app/components/rs_auth_details_modal_component.rb b/app/components/rs_auth_details_modal_component.rb new file mode 100644 index 0000000..257329c --- /dev/null +++ b/app/components/rs_auth_details_modal_component.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +class RsAuthDetailsModalComponent < ViewComponent::Base + include OauthHelper + + def initialize(authorizations:) + @authorizations = authorizations + @auth = authorizations.first + @web_app = @auth.web_app + end + + def name + @web_app&.name.presence || @auth.app_name.presence || @auth.client_id + end + + def root_access? + (@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" + end + + def status_label(auth) + return "Never expires" if auth.expire_at.blank? + return "Expired on #{auth.expire_at.strftime('%-d %b %Y')}" if auth.expired? + + "Expires #{auth.expire_at.strftime('%-d %b %Y')}" + end +end diff --git a/app/controllers/services/rs_auths_controller.rb b/app/controllers/services/rs_auths_controller.rb index c081538..3518b2a 100644 --- a/app/controllers/services/rs_auths_controller.rb +++ b/app/controllers/services/rs_auths_controller.rb @@ -5,8 +5,12 @@ class Services::RsAuthsController < Services::BaseController before_action :find_rs_auth, only: [:destroy, :launch_app] def index - @rs_auths = current_user.remote_storage_authorizations - # TODO sort by app name? + @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 } end def destroy @@ -38,6 +42,10 @@ class Services::RsAuthsController < Services::BaseController private + def app_name_for(auth) + auth.web_app&.name.presence || auth.app_name.presence || auth.client_id + end + def require_service_enabled unless current_user.service_enabled?(:remotestorage) http_status :forbidden diff --git a/app/models/remote_storage_authorization.rb b/app/models/remote_storage_authorization.rb index a8dfd1c..b27e5c2 100644 --- a/app/models/remote_storage_authorization.rb +++ b/app/models/remote_storage_authorization.rb @@ -40,6 +40,34 @@ class RemoteStorageAuthorization < ApplicationRecord end end + def expired? + expire_at.present? && expire_at.past? + end + + def device_name + return nil if user_agent.blank? + + browser = + case user_agent + when /Firefox/ then "Firefox" + when /Edg\// then "Edge" + when /Chrome/ then "Chrome" + when /Safari/ then "Safari" + else "Unknown browser" + end + + os = + case user_agent + when /Android/ then "Android" + when /iPhone|iPad|iPod/ then "iOS" + when /Mac OS X/ then "macOS" + when /Windows/ then "Windows" + when /Linux/ then "Linux" + end + + os ? "#{browser} on #{os}" : browser + end + def delete_token_from_redis key = "authorizations:#{user.cn}:#{token}" redis.srem? key, redis.smembers(key) diff --git a/app/views/services/rs_auths/index.html.erb b/app/views/services/rs_auths/index.html.erb index 939df14..9141066 100644 --- a/app/views/services/rs_auths/index.html.erb +++ b/app/views/services/rs_auths/index.html.erb @@ -10,10 +10,10 @@ <%= render partial: "shared/tabnav_remotestorage" %>
- <% if @rs_auths.any? %> -
- <% @rs_auths.each do |auth| %> - <%= render RsAuthComponent.new(auth: auth) %> + <% if @rs_auth_groups.any? %> +
+ <% @rs_auth_groups.each do |authorizations| %> + <%= render RsAuthComponent.new(authorizations: authorizations) %> <% end %>
<% else %> diff --git a/spec/features/services/rs_auths_spec.rb b/spec/features/services/rs_auths_spec.rb new file mode 100644 index 0000000..9fd69b0 --- /dev/null +++ b/spec/features/services/rs_auths_spec.rb @@ -0,0 +1,101 @@ +require 'rails_helper' + +RSpec.describe "Storage connected apps", type: :feature do + let(:user) { create :user } + + let(:firefox_ua) do + "Mozilla/5.0 (X11; Linux x86_64; rv:120.0) Gecko/20100101 Firefox/120.0" + end + + let(:chrome_ua) do + "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 " \ + "(KHTML, like Gecko) Chrome/119.0.0.0 Safari/537.36" + end + + before do + allow_any_instance_of(AppCatalog::WebApp).to receive(:update_metadata).and_return(true) + allow_any_instance_of(RemoteStorageAuthorization).to receive(:remove_token_expiry_job).and_return(nil) + allow_any_instance_of(User).to receive(:ldap_entry).and_return( + uid: user.cn, ou: user.ou, mail: user.email, admin: nil, + services_enabled: ["remotestorage"] + ) + login_as user, scope: :user + end + + def create_auth(client:, device:, permissions: %w(documents:rw)) + user.remote_storage_authorizations.create!( + permissions: permissions, + client_id: client, + redirect_uri: "https://#{client}", + expire_at: 1.month.from_now, + user_agent: device + ) + end + + context "with a connected app" do + before do + AppCatalog::WebApp.create!(url: "https://notes.example.com", name: "Notes", metadata: {}) + end + + let!(:auth) do + create_auth(client: "notes.example.com", device: firefox_ua, + permissions: %w(documents:rw photos:r)) + end + + after { auth.destroy } + + it "shows the app, its device and permission status" 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") + end + + it "renders a details modal with scopes and actions" 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) + 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) } + + after do + first.destroy + second.destroy + other.destroy + end + + it "groups authorizations for the same app and device into one card" 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 + + it "lists every authorization in the group with its own revoke action" do + visit apps_services_storage_path + + expect(page).to have_selector("a", text: "Revoke access", visible: false, count: 3) + end + end + + context "without connected apps" do + it "shows the empty state" do + visit apps_services_storage_path + + expect(page).to have_content("No apps connected") + end + end +end diff --git a/spec/models/remote_storage_authorization_spec.rb b/spec/models/remote_storage_authorization_spec.rb index 378ebb6..70e0377 100644 --- a/spec/models/remote_storage_authorization_spec.rb +++ b/spec/models/remote_storage_authorization_spec.rb @@ -303,4 +303,55 @@ RSpec.describe RemoteStorageAuthorization, type: :model do end end end + describe "#expired?" do + def auth_with_expiry(expire_at) + RemoteStorageAuthorization.new( + user: user, permissions: %w(documents:rw), client_id: "example.com", + redirect_uri: "https://example.com", expire_at: expire_at + ) + end + + it "is false without an expiry" do + expect(auth_with_expiry(nil)).not_to be_expired + end + + it "is false before the expiry" do + expect(auth_with_expiry(1.day.from_now)).not_to be_expired + end + + it "is true after the expiry" do + expect(auth_with_expiry(1.day.ago)).to be_expired + end + end + + describe "#device_name" do + def auth_with_user_agent(user_agent) + RemoteStorageAuthorization.new(user: user, user_agent: user_agent) + end + + it "returns nil without a user agent" do + expect(auth_with_user_agent(nil).device_name).to be_nil + end + + it "recognizes Firefox on Linux" do + ua = "Mozilla/5.0 (X11; Linux x86_64; rv:120.0) Gecko/20100101 Firefox/120.0" + expect(auth_with_user_agent(ua).device_name).to eq("Firefox on Linux") + end + + it "recognizes Chrome on macOS" do + ua = "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 " \ + "(KHTML, like Gecko) Chrome/119.0.0.0 Safari/537.36" + expect(auth_with_user_agent(ua).device_name).to eq("Chrome on macOS") + end + + it "recognizes Safari on iOS" do + ua = "Mozilla/5.0 (iPhone; CPU iPhone OS 17_1 like Mac OS X) " \ + "AppleWebKit/605.1.15 (KHTML, like Gecko) Version/17.1 Mobile/15E148 Safari/604.1" + expect(auth_with_user_agent(ua).device_name).to eq("Safari on iOS") + end + + it "falls back to the browser when the OS is unknown" do + expect(auth_with_user_agent("Firefox/120.0").device_name).to eq("Firefox") + end + end end -- 2.50.1 From c4df1772e0e79565b631fbc837f967f98e7dadc5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A2u=20Cao?= Date: Mon, 2 Nov 2026 22:58:49 +0400 Subject: [PATCH 2/5] Determine device names with device_detector Replace the hand-rolled user-agent regexes with the device_detector gem. Its UA database recognises many more clients (and, e.g., GNOME Web when its UA carries the Epiphany token). WebKit "Safari" user agents on Linux (WebKitGTK, which GNOME Web sends) are labelled "WebKit on Linux", since Safari never runs on Linux. Part of #254 --- Gemfile | 1 + Gemfile.lock | 2 ++ app/models/remote_storage_authorization.rb | 26 +++++++------------ .../remote_storage_authorization_spec.rb | 22 ++++++++++++++-- 4 files changed, 32 insertions(+), 19 deletions(-) diff --git a/Gemfile b/Gemfile index 5ec961f..e41a186 100644 --- a/Gemfile +++ b/Gemfile @@ -43,6 +43,7 @@ gem 'flipper-ui' gem 'gpgme', '~> 2.0.24' gem 'zbase32', '~> 0.1.1' gem 'kramdown' +gem 'device_detector' # HTTP requests gem 'faraday' diff --git a/Gemfile.lock b/Gemfile.lock index 07a76dc..9838525 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -136,6 +136,7 @@ GEM database_cleaner-core (~> 2.0.0) database_cleaner-core (2.0.1) date (3.5.1) + device_detector (1.1.3) devise (5.0.4) bcrypt (~> 3.0) orm_adapter (~> 0.1) @@ -543,6 +544,7 @@ DEPENDENCIES capybara cssbundling-rails database_cleaner + device_detector devise (~> 5.0) devise_ldap_authenticatable discourse_api diff --git a/app/models/remote_storage_authorization.rb b/app/models/remote_storage_authorization.rb index b27e5c2..d7011e2 100644 --- a/app/models/remote_storage_authorization.rb +++ b/app/models/remote_storage_authorization.rb @@ -47,25 +47,17 @@ class RemoteStorageAuthorization < ApplicationRecord def device_name return nil if user_agent.blank? - browser = - case user_agent - when /Firefox/ then "Firefox" - when /Edg\// then "Edge" - when /Chrome/ then "Chrome" - when /Safari/ then "Safari" - else "Unknown browser" - end + client = DeviceDetector.new(user_agent) + name = client.name.presence + os = client.os_name.presence + os = "Linux" if os == "GNU/Linux" + os = "macOS" if os == "Mac" - os = - case user_agent - when /Android/ then "Android" - when /iPhone|iPad|iPod/ then "iOS" - when /Mac OS X/ then "macOS" - when /Windows/ then "Windows" - when /Linux/ then "Linux" - end + # Safari never runs on Linux. A WebKit "Safari" user agent on Linux is a + # WebKitGTK browser (e.g. GNOME Web), which sends a Safari-compatible UA. + name = "WebKit" if name == "Safari" && os == "Linux" - os ? "#{browser} on #{os}" : browser + [name, os].compact.join(" on ").presence end def delete_token_from_redis diff --git a/spec/models/remote_storage_authorization_spec.rb b/spec/models/remote_storage_authorization_spec.rb index 70e0377..51cd7a0 100644 --- a/spec/models/remote_storage_authorization_spec.rb +++ b/spec/models/remote_storage_authorization_spec.rb @@ -344,10 +344,28 @@ RSpec.describe RemoteStorageAuthorization, type: :model do expect(auth_with_user_agent(ua).device_name).to eq("Chrome on macOS") end - it "recognizes Safari on iOS" do + it "recognizes Mobile Safari on iOS" do ua = "Mozilla/5.0 (iPhone; CPU iPhone OS 17_1 like Mac OS X) " \ "AppleWebKit/605.1.15 (KHTML, like Gecko) Version/17.1 Mobile/15E148 Safari/604.1" - expect(auth_with_user_agent(ua).device_name).to eq("Safari on iOS") + expect(auth_with_user_agent(ua).device_name).to eq("Mobile Safari on iOS") + end + + it "recognizes GNOME Web on Linux (Epiphany's Safari-like user agent)" do + ua = "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/605.1.15 (KHTML, like Gecko) " \ + "Version/16.0 Safari/605.1.15 Epiphany/45.0" + expect(auth_with_user_agent(ua).device_name).to eq("GNOME Web on Linux") + end + + it "treats a WebKit Safari user agent on Linux as WebKit (WebKitGTK)" do + ua = "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/605.1.15 (KHTML, like Gecko) " \ + "Version/60.5 Safari/605.1.15" + expect(auth_with_user_agent(ua).device_name).to eq("WebKit on Linux") + end + + it "keeps Safari on macOS as Safari" do + ua = "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 " \ + "(KHTML, like Gecko) Version/17.1 Safari/605.1.15" + expect(auth_with_user_agent(ua).device_name).to eq("Safari on macOS") end it "falls back to the browser when the OS is unknown" do -- 2.50.1 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 3/5] 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 -- 2.50.1 From 27168dde0639418f436a1f206943fa56a09c6a11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A2u=20Cao?= Date: Tue, 3 Nov 2026 11:44:27 +0400 Subject: [PATCH 4/5] Address review nits in the storage dashboard - Move the app display name and a root-access helper onto RemoteStorageAuthorization, used by both components and the controller, removing the duplicated fallback chain and the unused OauthHelper includes - Summarise the card's categories from the union of the app's authorizations' scopes, so an older token with broader access is not understated Part of #254 --- app/components/rs_auth_component.html.erb | 2 +- app/components/rs_auth_component.rb | 12 ++++++------ .../rs_auth_details_modal_component.html.erb | 4 ++-- app/components/rs_auth_details_modal_component.rb | 10 ---------- app/controllers/services/rs_auths_controller.rb | 2 +- app/models/remote_storage_authorization.rb | 8 ++++++++ 6 files changed, 18 insertions(+), 20 deletions(-) diff --git a/app/components/rs_auth_component.html.erb b/app/components/rs_auth_component.html.erb index 205a760..64ea405 100644 --- a/app/components/rs_auth_component.html.erb +++ b/app/components/rs_auth_component.html.erb @@ -10,7 +10,7 @@
-

<%= name %>

+

<%= @auth.display_name %>

<%= @auth.client_id %>

<%= device_summary %> diff --git a/app/components/rs_auth_component.rb b/app/components/rs_auth_component.rb index c5f90a1..b80d225 100644 --- a/app/components/rs_auth_component.rb +++ b/app/components/rs_auth_component.rb @@ -1,26 +1,26 @@ # frozen_string_literal: true class RsAuthComponent < ViewComponent::Base - include OauthHelper - def initialize(authorizations:) @authorizations = authorizations @auth = authorizations.first @web_app = @auth.web_app end - def name - @web_app&.name.presence || @auth.app_name.presence || @auth.client_id + # The card summarises the app, so reflect the union of its authorizations' + # scopes (an older token may have broader access than the newest one). + def permissions + @authorizations.flat_map(&:permissions).uniq end def root_access? - (@auth.permissions & [":r", ":rw"]).any? + (permissions & [":r", ":rw"]).any? end def category_summary return "All categories and files" if root_access? - count = @auth.permissions.size + count = permissions.size "#{count} #{'category'.pluralize(count)}" end diff --git a/app/components/rs_auth_details_modal_component.html.erb b/app/components/rs_auth_details_modal_component.html.erb index 7955bfe..dcb64b1 100644 --- a/app/components/rs_auth_details_modal_component.html.erb +++ b/app/components/rs_auth_details_modal_component.html.erb @@ -4,7 +4,7 @@ <%= render AppCatalog::WebAppIconComponent.new(web_app: @web_app) %>

-

<%= name %>

+

<%= @auth.display_name %>

<%= @auth.client_id %>

@@ -12,7 +12,7 @@
Access
- <%= render RsScopeListComponent.new(scopes: @auth.permissions, root: root_access?) %> + <%= render RsScopeListComponent.new(scopes: @auth.permissions, root: @auth.root_access?) %>
Connected devices diff --git a/app/components/rs_auth_details_modal_component.rb b/app/components/rs_auth_details_modal_component.rb index 3bd9481..0f281cc 100644 --- a/app/components/rs_auth_details_modal_component.rb +++ b/app/components/rs_auth_details_modal_component.rb @@ -1,22 +1,12 @@ # frozen_string_literal: true class RsAuthDetailsModalComponent < ViewComponent::Base - include OauthHelper - def initialize(authorizations:) @authorizations = authorizations @auth = authorizations.first @web_app = @auth.web_app end - def name - @web_app&.name.presence || @auth.app_name.presence || @auth.client_id - end - - def root_access? - (@auth.permissions & [":r", ":rw"]).any? - end - def device_name(auth) auth.device_name.presence || "Unknown device" end diff --git a/app/controllers/services/rs_auths_controller.rb b/app/controllers/services/rs_auths_controller.rb index b60a684..f4df2c1 100644 --- a/app/controllers/services/rs_auths_controller.rb +++ b/app/controllers/services/rs_auths_controller.rb @@ -49,7 +49,7 @@ class Services::RsAuthsController < Services::BaseController private def app_name_for(auth) - auth.web_app&.name.presence || auth.app_name.presence || auth.client_id + auth.display_name end def require_service_enabled diff --git a/app/models/remote_storage_authorization.rb b/app/models/remote_storage_authorization.rb index d7011e2..cf5167f 100644 --- a/app/models/remote_storage_authorization.rb +++ b/app/models/remote_storage_authorization.rb @@ -60,6 +60,14 @@ class RemoteStorageAuthorization < ApplicationRecord [name, os].compact.join(" on ").presence end + def display_name + web_app&.name.presence || app_name.presence || client_id + end + + def root_access? + (permissions & [":r", ":rw"]).any? + end + def delete_token_from_redis key = "authorizations:#{user.cn}:#{token}" redis.srem? key, redis.smembers(key) -- 2.50.1 From 30e45d3d34b9a6f9bd37bd807af74d95bb5e9769 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A2u=20Cao?= Date: Tue, 3 Nov 2026 12:01:56 +0400 Subject: [PATCH 5/5] Address second-round review nits in the storage dashboard Show the union of the app's authorizations' scopes in the modal's Access section too, so it matches the card's summary, and cover the union summary with a feature spec. Part of #254 --- .../rs_auth_details_modal_component.html.erb | 2 +- .../rs_auth_details_modal_component.rb | 9 ++++++ spec/features/services/rs_auths_spec.rb | 28 +++++++++++++++++++ 3 files changed, 38 insertions(+), 1 deletion(-) diff --git a/app/components/rs_auth_details_modal_component.html.erb b/app/components/rs_auth_details_modal_component.html.erb index dcb64b1..bf86eef 100644 --- a/app/components/rs_auth_details_modal_component.html.erb +++ b/app/components/rs_auth_details_modal_component.html.erb @@ -12,7 +12,7 @@
Access
- <%= render RsScopeListComponent.new(scopes: @auth.permissions, root: @auth.root_access?) %> + <%= render RsScopeListComponent.new(scopes: permissions, root: root_access?) %>
Connected devices diff --git a/app/components/rs_auth_details_modal_component.rb b/app/components/rs_auth_details_modal_component.rb index 0f281cc..46daa37 100644 --- a/app/components/rs_auth_details_modal_component.rb +++ b/app/components/rs_auth_details_modal_component.rb @@ -7,6 +7,15 @@ class RsAuthDetailsModalComponent < ViewComponent::Base @web_app = @auth.web_app end + # Reflect the union of the app's scopes, matching the card's summary. + def permissions + @authorizations.flat_map(&:permissions).uniq + end + + def root_access? + (permissions & [":r", ":rw"]).any? + end + def device_name(auth) auth.device_name.presence || "Unknown device" end diff --git a/spec/features/services/rs_auths_spec.rb b/spec/features/services/rs_auths_spec.rb index 6cec9a7..7496ba0 100644 --- a/spec/features/services/rs_auths_spec.rb +++ b/spec/features/services/rs_auths_spec.rb @@ -112,6 +112,34 @@ RSpec.describe "Storage connected apps", type: :feature do end end + context "with one app on multiple devices and differing scopes" do + let!(:laptop) do + create_auth(client: "notes.example.com", device: firefox_ua, permissions: %w(documents:rw)) + end + + let!(:phone) do + create_auth(client: "notes.example.com", device: chrome_ua, + permissions: %w(documents:rw photos:r)) + end + + after do + laptop.destroy + phone.destroy + end + + it "summarises the union of the app's scopes in the card and the modal" do + visit apps_services_storage_path + + within ".rs-auth-card" do + expect(page).to have_content("2 devices") + expect(page).to have_content("2 categories") + end + + expect(page).to have_selector("p.scope", text: "documents", visible: false) + expect(page).to have_selector("p.scope", text: "photos", visible: false) + end + end + context "without connected apps" do it "shows the empty state" do visit apps_services_storage_path -- 2.50.1