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/components/rs_auth_component.html.erb b/app/components/rs_auth_component.html.erb index a26d35d..64ea405 100644 --- a/app/components/rs_auth_component.html.erb +++ b/app/components/rs_auth_component.html.erb @@ -1,27 +1,28 @@ -
-
- <%= render AppCatalog::WebAppIconComponent.new(web_app: @web_app) %> -
-
-

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

-

- <%= @auth.client_id %> -

-
- <%= 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..b80d225 100644 --- a/app/components/rs_auth_component.rb +++ b/app/components/rs_auth_component.rb @@ -1,8 +1,34 @@ # frozen_string_literal: true class RsAuthComponent < ViewComponent::Base - def initialize(auth:) - @auth = auth - @web_app = auth.web_app + def initialize(authorizations:) + @authorizations = authorizations + @auth = authorizations.first + @web_app = @auth.web_app + end + + # 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? + (permissions & [":r", ":rw"]).any? + end + + def category_summary + return "All categories and files" if root_access? + + count = permissions.size + "#{count} #{'category'.pluralize(count)}" + end + + def device_count + @authorizations.map { |auth| auth.device_name.presence || "Unknown device" }.uniq.size + end + + 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 new file mode 100644 index 0000000..bf86eef --- /dev/null +++ b/app/components/rs_auth_details_modal_component.html.erb @@ -0,0 +1,42 @@ +<%= render ModalComponent.new do %> +
+
+ <%= render AppCatalog::WebAppIconComponent.new(web_app: @web_app) %> +
+
+

<%= @auth.display_name %>

+

<%= @auth.client_id %>

+
+
+ +
+ Access +
+ <%= render RsScopeListComponent.new(scopes: permissions, root: root_access?) %> + +
+ Connected devices +
+ + +
+ <%= 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 new file mode 100644 index 0000000..46daa37 --- /dev/null +++ b/app/components/rs_auth_details_modal_component.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +class RsAuthDetailsModalComponent < ViewComponent::Base + def initialize(authorizations:) + @authorizations = authorizations + @auth = authorizations.first + @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 + + 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/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 c081538..f4df2c1 100644 --- a/app/controllers/services/rs_auths_controller.rb +++ b/app/controllers/services/rs_auths_controller.rb @@ -2,11 +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_auths = current_user.remote_storage_authorizations - # TODO sort by app name? + @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 @@ -22,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}" @@ -31,13 +41,17 @@ 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 private + def app_name_for(auth) + auth.display_name + end + def require_service_enabled unless current_user.service_enabled?(:remotestorage) http_status :forbidden @@ -49,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/models/remote_storage_authorization.rb b/app/models/remote_storage_authorization.rb index a8dfd1c..cf5167f 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? + + 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" + + # 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" + + [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) 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 939df14..edd6ade 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_apps.any? %> +
+ <% @rs_apps.each do |authorizations| %> + <%= render RsAuthComponent.new(authorizations: authorizations) %> <% end %>
<% else %> 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 new file mode 100644 index 0000000..7496ba0 --- /dev/null +++ b/spec/features/services/rs_auths_spec.rb @@ -0,0 +1,150 @@ +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 with device and category counts" do + visit apps_services_storage_path + + 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 "does not show the permission expiry on the card" do + visit apps_services_storage_path + + 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) + end + end + + 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 + notes.destroy + photos.destroy + end + + it "shows one card per app" do + visit apps_services_storage_path + + expect(page).to have_selector(".rs-auth-card", count: 2) + 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 "shows a single card with the device count" do + visit apps_services_storage_path + + 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 + + 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 + + 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..51cd7a0 100644 --- a/spec/models/remote_storage_authorization_spec.rb +++ b/spec/models/remote_storage_authorization_spec.rb @@ -303,4 +303,73 @@ 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 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("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 + expect(auth_with_user_agent("Firefox/120.0").device_name).to eq("Firefox") + end + end end