From 8dd0bf10cecebc45d79f0ce5dd466943af6710cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A2u=20Cao?= Date: Mon, 2 Nov 2026 13:00:01 +0400 Subject: [PATCH 01/16] Add PKCE support to remoteStorage OAuth Implements the optional OAuth 2.0 Authorization Code grant with PKCE (RFC 7636) for remoteStorage, as described in the remoteStorage spec (section 10.1). - Advertise the authorization and token endpoints plus the supported code_challenge method (S256) in WebFinger, and bump the advertised spec version to draft-dejong-remotestorage-27 - Add RsAuthorizationCode: a short-lived (5 min), single-use authorization code stored only as a SHA256 digest - Add RsOauthManager services for issuing and exchanging codes, including S256 verification - Add a cross-origin POST /rs/oauth/token endpoint that exchanges a code (and code_verifier) for an access token, with CORS and no CSRF token - Consent dialog now carries response_type / code_challenge through the form and redirects with the code in the query string for the code flow - Remove the existing-token reuse shortcut so every authorization results in a distinct authorization (and token) Part of #254 --- app/controllers/rs/oauth_controller.rb | 156 +++++++++---- app/controllers/webfinger_controller.rb | 11 +- app/models/rs_authorization_code.rb | 37 +++ .../rs_oauth_manager/exchange_code.rb | 35 +++ app/services/rs_oauth_manager/issue_code.rb | 27 +++ app/services/rs_oauth_manager/verify_pkce.rb | 21 ++ app/services/rs_oauth_manager_service.rb | 27 +++ app/views/rs/oauth/new.html.erb | 3 + config/routes.rb | 2 + ...102120000_create_rs_authorization_codes.rb | 19 ++ db/schema.rb | 19 +- spec/controllers/rs/oauth_controller_spec.rb | 221 +++++++++++++++--- spec/features/rs/oauth_spec.rb | 18 ++ spec/models/rs_authorization_code_spec.rb | 65 ++++++ spec/requests/rs/oauth_spec.rb | 57 +++++ spec/requests/webfinger_spec.rb | 9 + .../rs_oauth_manager/exchange_code_spec.rb | 103 ++++++++ .../rs_oauth_manager/issue_code_spec.rb | 29 +++ .../rs_oauth_manager/verify_pkce_spec.rb | 41 ++++ 19 files changed, 827 insertions(+), 73 deletions(-) create mode 100644 app/models/rs_authorization_code.rb create mode 100644 app/services/rs_oauth_manager/exchange_code.rb create mode 100644 app/services/rs_oauth_manager/issue_code.rb create mode 100644 app/services/rs_oauth_manager/verify_pkce.rb create mode 100644 app/services/rs_oauth_manager_service.rb create mode 100644 db/migrate/20261102120000_create_rs_authorization_codes.rb create mode 100644 spec/models/rs_authorization_code_spec.rb create mode 100644 spec/requests/rs/oauth_spec.rb create mode 100644 spec/services/rs_oauth_manager/exchange_code_spec.rb create mode 100644 spec/services/rs_oauth_manager/issue_code_spec.rb create mode 100644 spec/services/rs_oauth_manager/verify_pkce_spec.rb diff --git a/app/controllers/rs/oauth_controller.rb b/app/controllers/rs/oauth_controller.rb index 360d50c..c51b2a7 100644 --- a/app/controllers/rs/oauth_controller.rb +++ b/app/controllers/rs/oauth_controller.rb @@ -1,7 +1,10 @@ class Rs::OauthController < ApplicationController + skip_before_action :verify_authenticity_token, only: :token + prepend_before_action :assert_redirect_uri, only: [:new, :create] before_action :require_signed_in_with_username, only: :new before_action :authenticate_user!, only: :create + before_action :set_cors_headers, only: :token def new @user = User.where(cn: params[:username].downcase, ou: Setting.primary_domain).first @@ -9,9 +12,12 @@ class Rs::OauthController < ApplicationController @redirect_uri = params[:redirect_uri] @client_id = params[:client_id] @state = params[:state] + @response_type = params[:response_type].presence || "token" + @code_challenge = params[:code_challenge] + @code_challenge_method = params[:code_challenge_method].presence || "S256" @root_access_requested = (@scopes & [":r",":rw"]).any? - @denial_url = url_with_state("#{@redirect_uri}#error=access_denied", @state) + @denial_url = error_url(@redirect_uri, "error=access_denied", @state) @expire_at_dates = [["Never", nil], ["In 1 month", 1.month.from_now], @@ -20,35 +26,41 @@ class Rs::OauthController < ApplicationController unless current_user == @user sign_out :user - redirect_to new_rs_oauth_url(@user.cn, - scope: params[:scope], - redirect_uri: params[:redirect_uri], - client_id: params[:client_id], - state: params[:state]) + redirect_params = { + scope: params[:scope], + redirect_uri: params[:redirect_uri], + client_id: params[:client_id], + state: params[:state] + } + + if @response_type == "code" + redirect_params[:response_type] = "code" + redirect_params[:code_challenge] = @code_challenge + redirect_params[:code_challenge_method] = @code_challenge_method + end + + redirect_to new_rs_oauth_url(@user.cn, redirect_params) return end unless @client_id.present? - redirect_to(url_with_state("#{@redirect_uri}#error=invalid_request", @state), - allow_other_host: true) and return + error_redirect(@redirect_uri, "error=invalid_request", @state) and return end if @scopes.empty? - redirect_to(url_with_state("#{@redirect_uri}#error=invalid_scope", @state), - allow_other_host: true) and return + error_redirect(@redirect_uri, "error=invalid_scope", @state) and return end unless hostname_of(@client_id) == hostname_of(@redirect_uri) - redirect_to(url_with_state("#{@redirect_uri}#error=invalid_client", @state), - allow_other_host: true) and return + error_redirect(@redirect_uri, "error=invalid_client", @state) and return + end + + if @response_type == "code" && + (@code_challenge.blank? || @code_challenge_method != "S256") + error_redirect(@redirect_uri, "error=invalid_request", @state) and return end @client_id.gsub!(/http(s)?:\/\//, "") - - if auth = current_user.remote_storage_authorizations.valid.where(permissions: @scopes, client_id: @client_id).first - redirect_to(url_with_state("#{@redirect_uri}#access_token=#{auth.token}", @state), - allow_other_host: true) and return - end end def create @@ -57,6 +69,7 @@ class Rs::OauthController < ApplicationController http_status :forbidden and return end + response_type = params[:response_type].presence || "token" permissions = parse_scopes params[:scope] redirect_uri = params[:redirect_uri].presence client_id = params[:client_id].presence @@ -64,32 +77,71 @@ class Rs::OauthController < ApplicationController expire_at = params[:expire_at].presence if permissions.empty? - redirect_to(url_with_state("#{redirect_uri}#error=invalid_scope", state), - allow_other_host: true) and return + error_redirect(redirect_uri, "error=invalid_scope", state, response_type) and return end unless client_id.present? - redirect_to(url_with_state("#{redirect_uri}#error=invalid_request", state), - allow_other_host: true) and return + error_redirect(redirect_uri, "error=invalid_request", state, response_type) and return end unless hostname_of(client_id) == hostname_of(redirect_uri) - redirect_to(url_with_state("#{redirect_uri}#error=invalid_client", state), - allow_other_host: true) and return + error_redirect(redirect_uri, "error=invalid_client", state, response_type) and return end client_id.gsub!(/http(s)?:\/\//, "") - auth = current_user.remote_storage_authorizations.create!( - permissions: permissions, - client_id: client_id, - redirect_uri: redirect_uri, - app_name: client_id, - expire_at: expire_at + if response_type == "code" + challenge = params[:code_challenge].presence + method = params[:code_challenge_method].presence || "S256" + + if challenge.blank? || method != "S256" + error_redirect(redirect_uri, "error=invalid_request", state, response_type) and return + end + + code = RsOauthManager::IssueCode.call( + user: current_user, + client_id: client_id, + redirect_uri: redirect_uri, + permissions: permissions, + expire_at: expire_at, + code_challenge: challenge, + code_challenge_method: method + ) + + redirect_to url_with_state("#{redirect_uri}?code=#{CGI.escape(code.code)}", state), + allow_other_host: true + else + auth = current_user.remote_storage_authorizations.create!( + permissions: permissions, + client_id: client_id, + redirect_uri: redirect_uri, + app_name: client_id, + expire_at: expire_at + ) + + redirect_to url_with_state("#{redirect_uri}#access_token=#{auth.token}", state), + allow_other_host: true + end + end + + def token + return head :ok if request.options? + + unless params[:grant_type] == "authorization_code" + return render json: { error: "unsupported_grant_type" }, status: :bad_request + end + + auth = RsOauthManager::ExchangeCode.call( + code: params[:code], + client_id: params[:client_id], + redirect_uri: params[:redirect_uri], + code_verifier: params[:code_verifier] ) - redirect_to url_with_state("#{redirect_uri}#access_token=#{auth.token}", state), - allow_other_host: true + response.headers["Cache-Control"] = "no-store" + render json: token_response(auth) + rescue RsOauthManagerService::InvalidGrant + render json: { error: "invalid_grant" }, status: :bad_request end private @@ -105,24 +157,42 @@ class Rs::OauthController < ApplicationController end end + def set_cors_headers + headers["Access-Control-Allow-Origin"] = "*" + headers["Access-Control-Allow-Methods"] = "POST, OPTIONS" + headers["Access-Control-Allow-Headers"] = "Content-Type" + end + + def token_response(auth) + body = { + access_token: auth.token, + token_type: "bearer", + scope: auth.permissions.join(" ") + } + + if auth.expire_at.present? + body[:expires_in] = (auth.expire_at - Time.current).to_i + end + + body + end + def hostname_of(uri) - uri.gsub(/http(s)?:\/\//, "").split(":")[0].split("/")[0] + RsOauthManagerService.hostname_of(uri) end def parse_scopes(scope_string) - return [] if scope_string.blank? + RsOauthManagerService.parse_scopes(scope_string) + end - scopes = scope_string. - gsub(/\[|\]/, ""). - gsub(/\,/, " "). - gsub(/\/:/, ":"). - split(/\s/).map(&:strip). - reject(&:empty?) + def error_redirect(redirect_uri, error, state, response_type = @response_type) + redirect_to error_url(redirect_uri, error, state, response_type), + allow_other_host: true + end - scopes = [":r"] if scopes.include?("*:r") - scopes = [":rw"] if scopes.include?("*:rw") - - scopes + def error_url(redirect_uri, error, state, response_type = @response_type) + separator = response_type == "code" ? "?" : "#" + url_with_state("#{redirect_uri}#{separator}#{error}", state) end def url_with_state(url, state) diff --git a/app/controllers/webfinger_controller.rb b/app/controllers/webfinger_controller.rb index 175fe5a..259336b 100644 --- a/app/controllers/webfinger_controller.rb +++ b/app/controllers/webfinger_controller.rb @@ -101,8 +101,11 @@ class WebfingerController < WellKnownController rel: "http://tools.ietf.org/id/draft-dejong-remotestorage", href: storage_url, properties: { - "http://remotestorage.io/spec/version" => "draft-dejong-remotestorage-13", + "http://remotestorage.io/spec/version" => "draft-dejong-remotestorage-27", "http://tools.ietf.org/html/rfc6749#section-4.2" => auth_url, + "http://tools.ietf.org/html/rfc6749#section-3.1" => auth_url, + "http://tools.ietf.org/html/rfc6749#section-3.2" => token_endpoint_url, + "http://tools.ietf.org/html/rfc7636" => "S256", "http://tools.ietf.org/html/rfc6750#section-2.3" => nil, # access token via a HTTP query parameter "http://tools.ietf.org/html/rfc7233": "GET", # content range requests "http://remotestorage.io/spec/web-authoring": nil @@ -110,6 +113,12 @@ class WebfingerController < WellKnownController } end + def token_endpoint_url + return rs_oauth_token_url if Rails.env.development? + + rs_oauth_token_url(host: Setting.rs_accounts_domain) + end + def remotestorage_storage_base_url return Setting.rs_storage_url unless Rails.env.development? diff --git a/app/models/rs_authorization_code.rb b/app/models/rs_authorization_code.rb new file mode 100644 index 0000000..bd51a9f --- /dev/null +++ b/app/models/rs_authorization_code.rb @@ -0,0 +1,37 @@ +class RsAuthorizationCode < ApplicationRecord + belongs_to :user + + CODE_TTL = 5.minutes + + serialize :permissions, coder: YAML + + attr_accessor :code + + before_validation :generate_code + + scope :active, -> { where(expires_at: Time.current..) } + + def self.find_by_code(raw_code) + return nil if raw_code.blank? + + find_by(code_digest: Digest::SHA256.hexdigest(raw_code)) + end + + def expired? + expires_at <= Time.current + end + + def consume! + destroy! + end + + private + + def generate_code + return if code_digest.present? + + self.code = SecureRandom.urlsafe_base64(32) + self.code_digest = Digest::SHA256.hexdigest(code) + self.expires_at ||= CODE_TTL.from_now + end +end diff --git a/app/services/rs_oauth_manager/exchange_code.rb b/app/services/rs_oauth_manager/exchange_code.rb new file mode 100644 index 0000000..e2afff4 --- /dev/null +++ b/app/services/rs_oauth_manager/exchange_code.rb @@ -0,0 +1,35 @@ +module RsOauthManager + class ExchangeCode < RsOauthManagerService + def initialize(code:, client_id:, redirect_uri:, code_verifier:) + @code = code + @client_id = client_id + @redirect_uri = redirect_uri + @code_verifier = code_verifier + end + + def call + auth_code = RsAuthorizationCode.find_by_code(@code) + raise InvalidGrant if auth_code.nil? || auth_code.expired? + + raise InvalidGrant unless hostname_of(@client_id) == hostname_of(@redirect_uri) + raise InvalidGrant unless @redirect_uri == auth_code.redirect_uri + + raise InvalidGrant unless VerifyPkce.call( + code_challenge: auth_code.code_challenge, + code_challenge_method: auth_code.code_challenge_method, + code_verifier: @code_verifier + ) + + auth = auth_code.user.remote_storage_authorizations.create!( + permissions: auth_code.permissions, + client_id: auth_code.client_id, + redirect_uri: auth_code.redirect_uri, + app_name: auth_code.client_id, + expire_at: auth_code.permission_expire_at + ) + + auth_code.consume! + auth + end + end +end diff --git a/app/services/rs_oauth_manager/issue_code.rb b/app/services/rs_oauth_manager/issue_code.rb new file mode 100644 index 0000000..b155117 --- /dev/null +++ b/app/services/rs_oauth_manager/issue_code.rb @@ -0,0 +1,27 @@ +module RsOauthManager + class IssueCode < RsOauthManagerService + def initialize(user:, client_id:, redirect_uri:, permissions:, + code_challenge:, code_challenge_method: "S256", + expire_at: nil) + @user = user + @client_id = client_id + @redirect_uri = redirect_uri + @permissions = permissions + @code_challenge = code_challenge + @code_challenge_method = code_challenge_method + @expire_at = expire_at + end + + def call + RsAuthorizationCode.create!( + user: @user, + client_id: @client_id, + redirect_uri: @redirect_uri, + permissions: @permissions, + permission_expire_at: @expire_at, + code_challenge: @code_challenge, + code_challenge_method: @code_challenge_method + ) + end + end +end diff --git a/app/services/rs_oauth_manager/verify_pkce.rb b/app/services/rs_oauth_manager/verify_pkce.rb new file mode 100644 index 0000000..84fdc16 --- /dev/null +++ b/app/services/rs_oauth_manager/verify_pkce.rb @@ -0,0 +1,21 @@ +module RsOauthManager + class VerifyPkce < RsOauthManagerService + def initialize(code_challenge:, code_challenge_method:, code_verifier:) + @code_challenge = code_challenge + @code_challenge_method = code_challenge_method + @code_verifier = code_verifier.to_s + end + + def call + return false unless @code_challenge_method == "S256" + + expected = Base64.urlsafe_encode64( + Digest::SHA256.digest(@code_verifier), padding: false + ) + + ActiveSupport::SecurityUtils.secure_compare( + expected, @code_challenge.to_s + ) + end + end +end diff --git a/app/services/rs_oauth_manager_service.rb b/app/services/rs_oauth_manager_service.rb new file mode 100644 index 0000000..0b310db --- /dev/null +++ b/app/services/rs_oauth_manager_service.rb @@ -0,0 +1,27 @@ +class RsOauthManagerService < ApplicationService + InvalidGrant = Class.new(StandardError) + + def self.parse_scopes(scope_string) + return [] if scope_string.blank? + + scopes = scope_string. + gsub(/\[|\]/, ""). + gsub(/\,/, " "). + gsub(/\/:/, ":"). + split(/\s/).map(&:strip). + reject(&:empty?) + + scopes = [":r"] if scopes.include?("*:r") + scopes = [":rw"] if scopes.include?("*:rw") + + scopes + end + + def self.hostname_of(uri) + uri.to_s.gsub(/http(s)?:\/\//, "").split(":")[0].split("/")[0] + end + + def hostname_of(uri) + self.class.hostname_of(uri) + end +end diff --git a/app/views/rs/oauth/new.html.erb b/app/views/rs/oauth/new.html.erb index 8cb9498..4b7bc25 100644 --- a/app/views/rs/oauth/new.html.erb +++ b/app/views/rs/oauth/new.html.erb @@ -40,6 +40,9 @@ <%= f.hidden_field :user_id, value: @user.id %> <%= f.hidden_field :client_id, value: @client_id %> <%= f.hidden_field :state, value: @state %> + <%= f.hidden_field :response_type, value: @response_type %> + <%= f.hidden_field :code_challenge, value: @code_challenge %> + <%= f.hidden_field :code_challenge_method, value: @code_challenge_method %>

<%= f.label :expire_at, "Permission expires:", class: "mr-1.5" %> <%= f.select :expire_at, options_for_select(@expire_at_dates) %> diff --git a/config/routes.rb b/config/routes.rb index 548d59f..d8af5b2 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -115,6 +115,8 @@ Rails.application.routes.draw do end namespace :rs do + post 'oauth/token', to: 'oauth#token', as: :oauth_token + match 'oauth/token', to: 'oauth#token', via: :options resource :oauth, only: [:new, :create], path_names: { new: ':username', create: ':username' }, controller: 'oauth' diff --git a/db/migrate/20261102120000_create_rs_authorization_codes.rb b/db/migrate/20261102120000_create_rs_authorization_codes.rb new file mode 100644 index 0000000..803203d --- /dev/null +++ b/db/migrate/20261102120000_create_rs_authorization_codes.rb @@ -0,0 +1,19 @@ +class CreateRsAuthorizationCodes < ActiveRecord::Migration[8.1] + def change + create_table :rs_authorization_codes do |t| + t.references :user, null: false, foreign_key: true + t.string :client_id, null: false + t.string :redirect_uri, null: false + t.text :permissions, default: [].to_yaml + t.datetime :permission_expire_at + t.string :code_digest, null: false + t.string :code_challenge, null: false + t.string :code_challenge_method, null: false, default: "S256" + t.datetime :expires_at, null: false + + t.timestamps + end + + add_index :rs_authorization_codes, :code_digest, unique: true + end +end diff --git a/db/schema.rb b/db/schema.rb index 15e3cda..daab063 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.1].define(version: 2025_05_28_092931) do +ActiveRecord::Schema[8.1].define(version: 2026_11_02_120000) do create_table "active_storage_attachments", force: :cascade do |t| t.bigint "blob_id", null: false t.datetime "created_at", null: false @@ -117,6 +117,22 @@ ActiveRecord::Schema[8.1].define(version: 2025_05_28_092931) do t.index ["web_app_id"], name: "index_remote_storage_authorizations_on_web_app_id" end + create_table "rs_authorization_codes", force: :cascade do |t| + t.string "client_id", null: false + t.string "code_challenge", null: false + t.string "code_challenge_method", default: "S256", null: false + t.string "code_digest", null: false + t.datetime "created_at", null: false + t.datetime "expires_at", null: false + t.datetime "permission_expire_at" + t.text "permissions", default: "--- []\n" + t.string "redirect_uri", null: false + t.datetime "updated_at", null: false + t.integer "user_id", null: false + t.index ["code_digest"], name: "index_rs_authorization_codes_on_code_digest", unique: true + t.index ["user_id"], name: "index_rs_authorization_codes_on_user_id" + end + create_table "settings", force: :cascade do |t| t.datetime "created_at", null: false t.datetime "updated_at", null: false @@ -164,5 +180,6 @@ ActiveRecord::Schema[8.1].define(version: 2025_05_28_092931) do add_foreign_key "active_storage_variant_records", "active_storage_blobs", column: "blob_id" add_foreign_key "remote_storage_authorizations", "app_catalog_web_apps", column: "web_app_id" add_foreign_key "remote_storage_authorizations", "users" + add_foreign_key "rs_authorization_codes", "users" add_foreign_key "zaps", "users" end diff --git a/spec/controllers/rs/oauth_controller_spec.rb b/spec/controllers/rs/oauth_controller_spec.rb index 78ea3d0..8bcef77 100644 --- a/spec/controllers/rs/oauth_controller_spec.rb +++ b/spec/controllers/rs/oauth_controller_spec.rb @@ -128,8 +128,8 @@ RSpec.describe Rs::OauthController, type: :controller do } end - it "redirects to the redirect_uri with the existing token" do - expect(response).to redirect_to("https://example.com#access_token=#{@auth.token}") + it "does not reuse the existing token (renders the consent dialog)" do + expect(response.response_code).to eq(200) end end @@ -147,36 +147,66 @@ RSpec.describe Rs::OauthController, type: :controller do expect(response).to redirect_to("https://app.example.com#error=invalid_client") end end + end - context "with different redirect_uri" do - before do - get :new, params: { - username: user.cn, - scope: "documents,[photos], contacts:rw videos:r tasks/work/:r", - redirect_uri: "https://example.com/a_new_route", - client_id: "https://example.com" - } - end - - it "redirects to the new redirect_uri" do - expect(response).to redirect_to("https://example.com/a_new_route#access_token=#{@auth.token}") - end + context "when response_type is code" do + before do + get :new, params: { + username: user.cn, + redirect_uri: "https://example.com", + client_id: "example.com", + scope: "documents", + response_type: "code", + code_challenge: "challenge123", + code_challenge_method: "S256", + state: "foobar123" + } end - context "with state param given" do - before do - get :new, params: { - username: user.cn, - scope: "documents,[photos], contacts:rw videos:r tasks/work/:r", - redirect_uri: "https://example.com", - client_id: "https://example.com", - state: "foobar123" - } - end + it "renders the consent dialog and exposes the code challenge" do + expect(response.response_code).to eq(200) + expect(assigns["response_type"]).to eq("code") + expect(assigns["code_challenge"]).to eq("challenge123") + expect(assigns["code_challenge_method"]).to eq("S256") + end - it "redirects to the redirect_uri with token and state" do - expect(response).to redirect_to("https://example.com#access_token=#{@auth.token}&state=foobar123") - end + it "uses a query-based denial url" do + expect(assigns["denial_url"]).to eq("https://example.com?error=access_denied&state=foobar123") + end + end + + context "when response_type is code without a code_challenge" do + before do + get :new, params: { + username: user.cn, + redirect_uri: "https://example.com", + client_id: "example.com", + scope: "documents", + response_type: "code", + state: "foobar123" + } + end + + it "redirects with invalid_request using a query param" do + expect(response).to redirect_to("https://example.com?error=invalid_request&state=foobar123") + end + end + + context "when response_type is code with the plain method" do + before do + get :new, params: { + username: user.cn, + redirect_uri: "https://example.com", + client_id: "example.com", + scope: "documents", + response_type: "code", + code_challenge: "challenge123", + code_challenge_method: "plain" + } + end + + it "redirects with invalid_request" do + expect(response).to redirect_to("https://example.com?error=invalid_request") end end @@ -284,6 +314,7 @@ RSpec.describe Rs::OauthController, type: :controller do after do user.remote_storage_authorizations.destroy_all + RsAuthorizationCode.destroy_all end context "when no valid token exists" do @@ -412,6 +443,63 @@ RSpec.describe Rs::OauthController, type: :controller do end end + context "with response_type=code" do + before do + post :create, params: { + user_id: user.id, + scope: "documents,[photos], contacts:rw", + redirect_uri: "https://example.com", + client_id: "example.com", + state: "foobar123", + response_type: "code", + code_challenge: "challenge123", + code_challenge_method: "S256", + expire_at: 1.month.from_now + } + @code = RsAuthorizationCode.last + end + + it "creates an authorization code" do + expect(@code.permissions).to eq(%w(documents photos contacts:rw)) + expect(@code.code_challenge).to eq("challenge123") + expect(@code.code_challenge_method).to eq("S256") + end + + it "does not create a remote storage authorization" do + expect(user.reload.remote_storage_authorizations.count).to eq(0) + end + + it "redirects with the code in the query string" do + location = response.location + expect(location).to match(%r{\Ahttps://example\.com\?code=.+&state=foobar123\z}) + + raw_code = CGI.parse(URI.parse(location).query)["code"].first + expect(RsAuthorizationCode.find_by_code(raw_code)).to eq(@code) + end + end + + context "with response_type=code but no code_challenge" do + before do + post :create, params: { + user_id: user.id, + scope: "documents", + redirect_uri: "https://example.com", + client_id: "example.com", + state: "foobar123", + response_type: "code", + expire_at: 1.month.from_now + } + end + + it "redirects with invalid_request using a query param" do + expect(response).to redirect_to("https://example.com?error=invalid_request&state=foobar123") + end + + it "does not create an authorization code" do + expect(RsAuthorizationCode.count).to eq(0) + end + end + context "when the user_id is different from the signed in user" do before do post :create, params: { @@ -451,4 +539,81 @@ RSpec.describe Rs::OauthController, type: :controller do end end end + + describe "POST /rs/oauth/token" do + # Example values from RFC 7636, Appendix B + let(:code_verifier) { "dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk" } + let(:code_challenge) { "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM" } + + def issue_code + RsOauthManager::IssueCode.call( + user: user, + client_id: "example.com", + redirect_uri: "https://example.com", + permissions: %w(documents:rw), + code_challenge: code_challenge, + code_challenge_method: "S256" + ) + end + + def token_params(code, **overrides) + { + grant_type: "authorization_code", + code: code.code, + client_id: "https://example.com", + redirect_uri: "https://example.com", + code_verifier: code_verifier + }.merge(overrides) + end + + after do + user.remote_storage_authorizations.destroy_all + RsAuthorizationCode.destroy_all + end + + context "with a valid code" do + before do + @code = issue_code + post :token, params: token_params(@code) + end + + it "returns an access token" do + expect(response.response_code).to eq(200) + + json = JSON.parse(response.body) + expect(json["access_token"]).to be_present + expect(json["token_type"]).to eq("bearer") + expect(json["scope"]).to eq("documents:rw") + end + + it "does not cache the response" do + expect(response.headers["Cache-Control"]).to eq("no-store") + end + + it "sets CORS headers" do + expect(response.headers["Access-Control-Allow-Origin"]).to eq("*") + end + end + + context "with a bad code_verifier" do + before do + @code = issue_code + post :token, params: token_params(@code, code_verifier: "wrong-verifier") + end + + it "returns invalid_grant" do + expect(response.response_code).to eq(400) + expect(JSON.parse(response.body)["error"]).to eq("invalid_grant") + end + end + + context "with an unsupported grant_type" do + before { post :token, params: { grant_type: "password" } } + + it "returns unsupported_grant_type" do + expect(response.response_code).to eq(400) + expect(JSON.parse(response.body)["error"]).to eq("unsupported_grant_type") + end + end + end end diff --git a/spec/features/rs/oauth_spec.rb b/spec/features/rs/oauth_spec.rb index 9e499b8..3cfa5dc 100644 --- a/spec/features/rs/oauth_spec.rb +++ b/spec/features/rs/oauth_spec.rb @@ -49,6 +49,24 @@ RSpec.describe 'remoteStorage OAuth Dialog', type: :feature do end end end + + context "with PKCE parameters" do + before do + visit new_rs_oauth_path(username: user.cn, + redirect_uri: "http://example.com", + client_id: "http://example.com", + scope: "documents:rw", + response_type: "code", + code_challenge: "challenge123", + code_challenge_method: "S256") + end + + it "includes the PKCE parameters in the consent form" do + expect(page).to have_selector("input[name='response_type'][value='code']", visible: false) + expect(page).to have_selector("input[name='code_challenge'][value='challenge123']", visible: false) + expect(page).to have_selector("input[name='code_challenge_method'][value='S256']", visible: false) + end + end end context "when signed out" do diff --git a/spec/models/rs_authorization_code_spec.rb b/spec/models/rs_authorization_code_spec.rb new file mode 100644 index 0000000..9368792 --- /dev/null +++ b/spec/models/rs_authorization_code_spec.rb @@ -0,0 +1,65 @@ +require 'rails_helper' + +RSpec.describe RsAuthorizationCode, type: :model do + let(:user) { create :user } + + def build_code(**attrs) + RsAuthorizationCode.create!( + user: user, + client_id: "example.com", + redirect_uri: "https://example.com", + permissions: %w(documents:rw), + code_challenge: "challenge", + **attrs + ) + end + + describe "#create" do + it "generates a raw code and stores only its digest" do + code = build_code + + expect(code.code).to be_present + expect(code.code_digest).to eq(Digest::SHA256.hexdigest(code.code)) + end + + it "sets a short expiry" do + code = build_code + + expect(code.expires_at).to be_within(5.seconds).of(5.minutes.from_now) + end + end + + describe ".find_by_code" do + it "finds a code by its raw value" do + code = build_code + + expect(RsAuthorizationCode.find_by_code(code.code)).to eq(code) + end + + it "returns nil for an unknown code" do + expect(RsAuthorizationCode.find_by_code("does-not-exist")).to be_nil + end + + it "returns nil for a blank code" do + expect(RsAuthorizationCode.find_by_code(nil)).to be_nil + end + end + + describe "#expired?" do + it "is false before the expiry" do + expect(build_code).not_to be_expired + end + + it "is true after the expiry" do + expect(build_code(expires_at: 1.minute.ago)).to be_expired + end + end + + describe "#consume!" do + it "destroys the record" do + code = build_code + + expect { code.consume! }.to change(RsAuthorizationCode, :count).by(-1) + end + end +end diff --git a/spec/requests/rs/oauth_spec.rb b/spec/requests/rs/oauth_spec.rb new file mode 100644 index 0000000..84429d5 --- /dev/null +++ b/spec/requests/rs/oauth_spec.rb @@ -0,0 +1,57 @@ +require 'rails_helper' + +RSpec.describe "remoteStorage OAuth token endpoint", type: :request do + let(:user) { create :user } + + # Example values from RFC 7636, Appendix B + let(:code_verifier) { "dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk" } + let(:code_challenge) { "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM" } + + 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) + end + + after do + user.remote_storage_authorizations.destroy_all + RsAuthorizationCode.destroy_all + end + + def issue_code + RsOauthManager::IssueCode.call( + user: user, + client_id: "example.com", + redirect_uri: "https://example.com", + permissions: %w(documents:rw), + code_challenge: code_challenge, + code_challenge_method: "S256" + ) + end + + it "exchanges a code for an access token without a session" do + code = issue_code + + post "/rs/oauth/token", params: { + grant_type: "authorization_code", + code: code.code, + client_id: "https://example.com", + redirect_uri: "https://example.com", + code_verifier: code_verifier + } + + expect(response).to have_http_status(:ok) + expect(response.media_type).to eq("application/json") + + json = JSON.parse(response.body) + expect(json["access_token"]).to be_present + expect(json["token_type"]).to eq("bearer") + expect(response.headers["Access-Control-Allow-Origin"]).to eq("*") + end + + it "answers CORS preflight requests" do + options "/rs/oauth/token" + + expect(response).to have_http_status(:ok) + expect(response.headers["Access-Control-Allow-Methods"]).to include("POST") + end +end diff --git a/spec/requests/webfinger_spec.rb b/spec/requests/webfinger_spec.rb index a37d3cb..fc063d2 100644 --- a/spec/requests/webfinger_spec.rb +++ b/spec/requests/webfinger_spec.rb @@ -133,6 +133,15 @@ RSpec.describe "WebFinger", type: :request do oauth_url = rs_link["properties"]["http://tools.ietf.org/html/rfc6749#section-4.2"] expect(oauth_url).to eql("http://localhost/rs/oauth/tony") + + authorization_url = rs_link["properties"]["http://tools.ietf.org/html/rfc6749#section-3.1"] + expect(authorization_url).to eql("http://localhost/rs/oauth/tony") + + token_url = rs_link["properties"]["http://tools.ietf.org/html/rfc6749#section-3.2"] + expect(token_url).to eql("http://localhost/rs/oauth/token") + + expect(rs_link["properties"]["http://tools.ietf.org/html/rfc7636"]).to eql("S256") + expect(rs_link["properties"]["http://remotestorage.io/spec/version"]).to eql("draft-dejong-remotestorage-27") end it "returns CORS headers" do diff --git a/spec/services/rs_oauth_manager/exchange_code_spec.rb b/spec/services/rs_oauth_manager/exchange_code_spec.rb new file mode 100644 index 0000000..900b9e8 --- /dev/null +++ b/spec/services/rs_oauth_manager/exchange_code_spec.rb @@ -0,0 +1,103 @@ +require 'rails_helper' + +RSpec.describe RsOauthManager::ExchangeCode do + let(:user) { create :user } + + # Example values from RFC 7636, Appendix B + let(:code_verifier) { "dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk" } + let(:code_challenge) { "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM" } + + 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) + end + + def issue_code(**overrides) + RsOauthManager::IssueCode.call( + user: user, + client_id: "example.com", + redirect_uri: "https://example.com", + permissions: %w(documents:rw), + code_challenge: code_challenge, + code_challenge_method: "S256", + **overrides + ) + end + + def exchange(code, **overrides) + described_class.call( + code: code.code, + client_id: "https://example.com", + redirect_uri: "https://example.com", + code_verifier: code_verifier, + **overrides + ) + end + + it "creates an authorization and consumes the code" do + code = issue_code + auth = nil + + expect { auth = exchange(code) }.to change(RemoteStorageAuthorization, :count).by(1) + + expect(auth.permissions).to eq(%w(documents:rw)) + expect(auth.client_id).to eq("example.com") + expect(auth.token).to be_present + expect(RsAuthorizationCode.exists?(code.id)).to be(false) + end + + it "carries over the permission expiry" do + expire_at = 1.month.from_now + code = issue_code(expire_at: expire_at) + + auth = exchange(code) + + expect(auth.expire_at).to be_within(1.second).of(expire_at) + end + + it "rejects an unknown code" do + expect do + described_class.call( + code: "unknown", + client_id: "https://example.com", + redirect_uri: "https://example.com", + code_verifier: code_verifier + ) + end.to raise_error(RsOauthManagerService::InvalidGrant) + end + + it "rejects a reused code" do + code = issue_code + exchange(code) + + expect { exchange(code) }.to raise_error(RsOauthManagerService::InvalidGrant) + end + + it "rejects an expired code" do + code = issue_code + code.update_columns(expires_at: 1.minute.ago) + + expect { exchange(code) }.to raise_error(RsOauthManagerService::InvalidGrant) + end + + it "rejects a mismatching redirect_uri" do + code = issue_code + + expect { exchange(code, redirect_uri: "https://example.com/other") } + .to raise_error(RsOauthManagerService::InvalidGrant) + end + + it "rejects a mismatching client_id host" do + code = issue_code + + expect { exchange(code, client_id: "https://evil.example.org") } + .to raise_error(RsOauthManagerService::InvalidGrant) + end + + it "rejects a bad code_verifier" do + code = issue_code + + expect { exchange(code, code_verifier: "wrong-verifier") } + .to raise_error(RsOauthManagerService::InvalidGrant) + end +end diff --git a/spec/services/rs_oauth_manager/issue_code_spec.rb b/spec/services/rs_oauth_manager/issue_code_spec.rb new file mode 100644 index 0000000..429c357 --- /dev/null +++ b/spec/services/rs_oauth_manager/issue_code_spec.rb @@ -0,0 +1,29 @@ +require 'rails_helper' + +RSpec.describe RsOauthManager::IssueCode do + let(:user) { create :user } + + subject do + described_class.call( + user: user, + client_id: "example.com", + redirect_uri: "https://example.com", + permissions: %w(documents:rw), + code_challenge: "challenge", + code_challenge_method: "S256", + expire_at: 1.month.from_now + ) + end + + it "creates a short-lived authorization code" do + expect { subject }.to change(RsAuthorizationCode, :count).by(1) + expect(subject.code).to be_present + expect(subject.client_id).to eq("example.com") + expect(subject.permissions).to eq(%w(documents:rw)) + expect(subject.code_challenge).to eq("challenge") + end + + it "does not create a remote storage authorization" do + expect { subject }.not_to change(RemoteStorageAuthorization, :count) + end +end diff --git a/spec/services/rs_oauth_manager/verify_pkce_spec.rb b/spec/services/rs_oauth_manager/verify_pkce_spec.rb new file mode 100644 index 0000000..46deef7 --- /dev/null +++ b/spec/services/rs_oauth_manager/verify_pkce_spec.rb @@ -0,0 +1,41 @@ +require 'rails_helper' + +RSpec.describe RsOauthManager::VerifyPkce do + # Example values from RFC 7636, Appendix B + let(:code_verifier) { "dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk" } + let(:code_challenge) { "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM" } + + subject do + described_class.call( + code_challenge: code_challenge, + code_challenge_method: code_challenge_method, + code_verifier: code_verifier + ) + end + + context "with the S256 method" do + let(:code_challenge_method) { "S256" } + + it "accepts a matching verifier" do + expect(subject).to be(true) + end + + it "rejects a non-matching verifier" do + expect( + described_class.call( + code_challenge: code_challenge, + code_challenge_method: "S256", + code_verifier: "wrong-verifier" + ) + ).to be(false) + end + end + + context "with an unsupported method" do + let(:code_challenge_method) { "plain" } + + it "rejects the verifier" do + expect(subject).to be(false) + end + end +end From 1e699577773d75f51836d0b89950ebb44d5b8b65 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A2u=20Cao?= Date: Tue, 3 Nov 2026 08:07:34 +0400 Subject: [PATCH 02/16] Shorten the authorization code lifetime to 60 seconds The code is redeemed by the client immediately in the OAuth callback, so a 60-second window is plenty (and matches common practice; RFC 6749 caps authorization codes at 10 minutes). Part of #254 --- app/models/rs_authorization_code.rb | 2 +- spec/models/rs_authorization_code_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/rs_authorization_code.rb b/app/models/rs_authorization_code.rb index bd51a9f..d07e0f8 100644 --- a/app/models/rs_authorization_code.rb +++ b/app/models/rs_authorization_code.rb @@ -1,7 +1,7 @@ class RsAuthorizationCode < ApplicationRecord belongs_to :user - CODE_TTL = 5.minutes + CODE_TTL = 60.seconds serialize :permissions, coder: YAML diff --git a/spec/models/rs_authorization_code_spec.rb b/spec/models/rs_authorization_code_spec.rb index 9368792..0df92e5 100644 --- a/spec/models/rs_authorization_code_spec.rb +++ b/spec/models/rs_authorization_code_spec.rb @@ -25,7 +25,7 @@ RSpec.describe RsAuthorizationCode, type: :model do it "sets a short expiry" do code = build_code - expect(code.expires_at).to be_within(5.seconds).of(5.minutes.from_now) + expect(code.expires_at).to be_within(5.seconds).of(60.seconds.from_now) end end From 295261b332129f09420e9d11c612b5ce699d8776 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A2u=20Cao?= Date: Tue, 3 Nov 2026 11:40:18 +0400 Subject: [PATCH 03/16] Address review findings in the PKCE flow - Make authorization-code redemption atomic: claim the code with a guarded delete inside the transaction that creates the authorization, so concurrent exchanges can't both issue a token (RFC 6749 4.1.3) - Compare the token request's client_id host to the code's stored client_id (defense-in-depth alongside the exact redirect_uri match) - Reject unknown response_type values with unsupported_response_type - Validate the PKCE code_challenge (43-128 unreserved chars, S256) so a client can't persist an over-long or malformed challenge - Return invalid_request for missing token-endpoint parameters, and set Cache-Control: no-store on error responses too - Use the right separator when the redirect_uri already has a query string - Look codes up through the active scope; drop the unused consume!; serialize permissions as JSON - Purge abandoned authorization codes via a recurring Solid Queue task Part of #254 --- app/controllers/rs/oauth_controller.rb | 46 ++++++-- app/models/rs_authorization_code.rb | 8 +- .../rs_oauth_manager/exchange_code.rb | 29 +++-- config/recurring.yml | 5 + ...102120000_create_rs_authorization_codes.rb | 2 +- db/schema.rb | 2 +- spec/controllers/rs/oauth_controller_spec.rb | 100 +++++++++++++++++- spec/features/rs/oauth_spec.rb | 4 +- spec/models/rs_authorization_code_spec.rb | 13 +-- 9 files changed, 169 insertions(+), 40 deletions(-) diff --git a/app/controllers/rs/oauth_controller.rb b/app/controllers/rs/oauth_controller.rb index c51b2a7..fe69198 100644 --- a/app/controllers/rs/oauth_controller.rb +++ b/app/controllers/rs/oauth_controller.rb @@ -55,8 +55,12 @@ class Rs::OauthController < ApplicationController error_redirect(@redirect_uri, "error=invalid_client", @state) and return end + unless %w[token code].include?(@response_type) + error_redirect(@redirect_uri, "error=unsupported_response_type", @state) and return + end + if @response_type == "code" && - (@code_challenge.blank? || @code_challenge_method != "S256") + !valid_code_challenge?(@code_challenge, @code_challenge_method) error_redirect(@redirect_uri, "error=invalid_request", @state) and return end @@ -88,13 +92,17 @@ class Rs::OauthController < ApplicationController error_redirect(redirect_uri, "error=invalid_client", state, response_type) and return end + unless %w[token code].include?(response_type) + error_redirect(redirect_uri, "error=unsupported_response_type", state, response_type) and return + end + client_id.gsub!(/http(s)?:\/\//, "") if response_type == "code" challenge = params[:code_challenge].presence method = params[:code_challenge_method].presence || "S256" - if challenge.blank? || method != "S256" + unless valid_code_challenge?(challenge, method) error_redirect(redirect_uri, "error=invalid_request", state, response_type) and return end @@ -108,7 +116,7 @@ class Rs::OauthController < ApplicationController code_challenge_method: method ) - redirect_to url_with_state("#{redirect_uri}?code=#{CGI.escape(code.code)}", state), + redirect_to url_with_state("#{redirect_uri}#{query_separator(redirect_uri)}code=#{CGI.escape(code.code)}", state), allow_other_host: true else auth = current_user.remote_storage_authorizations.create!( @@ -128,7 +136,12 @@ class Rs::OauthController < ApplicationController return head :ok if request.options? unless params[:grant_type] == "authorization_code" - return render json: { error: "unsupported_grant_type" }, status: :bad_request + return render_token_error("unsupported_grant_type") + end + + if params[:code].blank? || params[:code_verifier].blank? || + params[:client_id].blank? || params[:redirect_uri].blank? + return render_token_error("invalid_request") end auth = RsOauthManager::ExchangeCode.call( @@ -138,10 +151,9 @@ class Rs::OauthController < ApplicationController code_verifier: params[:code_verifier] ) - response.headers["Cache-Control"] = "no-store" - render json: token_response(auth) + render_token(token_response(auth)) rescue RsOauthManagerService::InvalidGrant - render json: { error: "invalid_grant" }, status: :bad_request + render_token_error("invalid_grant") end private @@ -191,10 +203,28 @@ class Rs::OauthController < ApplicationController end def error_url(redirect_uri, error, state, response_type = @response_type) - separator = response_type == "code" ? "?" : "#" + separator = response_type == "code" ? query_separator(redirect_uri) : "#" url_with_state("#{redirect_uri}#{separator}#{error}", state) end + def query_separator(uri) + uri.to_s.include?("?") ? "&" : "?" + end + + def valid_code_challenge?(challenge, method) + method == "S256" && challenge.to_s.match?(/\A[A-Za-z0-9\-._~]{43,128}\z/) + end + + def render_token(body) + response.headers["Cache-Control"] = "no-store" + render json: body + end + + def render_token_error(error) + response.headers["Cache-Control"] = "no-store" + render json: { error: error }, status: :bad_request + end + def url_with_state(url, state) state ? "#{url}&state=#{CGI.escape(state)}" : url end diff --git a/app/models/rs_authorization_code.rb b/app/models/rs_authorization_code.rb index d07e0f8..a433ddf 100644 --- a/app/models/rs_authorization_code.rb +++ b/app/models/rs_authorization_code.rb @@ -3,7 +3,7 @@ class RsAuthorizationCode < ApplicationRecord CODE_TTL = 60.seconds - serialize :permissions, coder: YAML + serialize :permissions, coder: JSON attr_accessor :code @@ -14,17 +14,13 @@ class RsAuthorizationCode < ApplicationRecord def self.find_by_code(raw_code) return nil if raw_code.blank? - find_by(code_digest: Digest::SHA256.hexdigest(raw_code)) + active.find_by(code_digest: Digest::SHA256.hexdigest(raw_code)) end def expired? expires_at <= Time.current end - def consume! - destroy! - end - private def generate_code diff --git a/app/services/rs_oauth_manager/exchange_code.rb b/app/services/rs_oauth_manager/exchange_code.rb index e2afff4..131fbe2 100644 --- a/app/services/rs_oauth_manager/exchange_code.rb +++ b/app/services/rs_oauth_manager/exchange_code.rb @@ -9,9 +9,10 @@ module RsOauthManager def call auth_code = RsAuthorizationCode.find_by_code(@code) - raise InvalidGrant if auth_code.nil? || auth_code.expired? + raise InvalidGrant if auth_code.nil? raise InvalidGrant unless hostname_of(@client_id) == hostname_of(@redirect_uri) + raise InvalidGrant unless hostname_of(@client_id) == hostname_of(auth_code.client_id) raise InvalidGrant unless @redirect_uri == auth_code.redirect_uri raise InvalidGrant unless VerifyPkce.call( @@ -20,15 +21,25 @@ module RsOauthManager code_verifier: @code_verifier ) - auth = auth_code.user.remote_storage_authorizations.create!( - permissions: auth_code.permissions, - client_id: auth_code.client_id, - redirect_uri: auth_code.redirect_uri, - app_name: auth_code.client_id, - expire_at: auth_code.permission_expire_at - ) + user = auth_code.user + auth = nil + + # Claim the code atomically so a concurrent redemption can't issue a + # second token. The whole exchange is one transaction: if creating the + # authorization fails, the code is not consumed and can be retried. + RsAuthorizationCode.transaction do + claimed = RsAuthorizationCode.where(id: auth_code.id).delete_all + raise InvalidGrant if claimed != 1 + + auth = user.remote_storage_authorizations.create!( + permissions: auth_code.permissions, + client_id: auth_code.client_id, + redirect_uri: auth_code.redirect_uri, + app_name: auth_code.client_id, + expire_at: auth_code.permission_expire_at + ) + end - auth_code.consume! auth end end diff --git a/config/recurring.yml b/config/recurring.yml index d045b19..67b575c 100644 --- a/config/recurring.yml +++ b/config/recurring.yml @@ -8,3 +8,8 @@ # command: "SoftDeletedRecord.due.delete_all" # priority: 2 # schedule: at 5am every day + +production: + purge_rs_authorization_codes: + command: "RsAuthorizationCode.where(expires_at: ..1.hour.ago).delete_all" + schedule: every hour diff --git a/db/migrate/20261102120000_create_rs_authorization_codes.rb b/db/migrate/20261102120000_create_rs_authorization_codes.rb index 803203d..38e4b3f 100644 --- a/db/migrate/20261102120000_create_rs_authorization_codes.rb +++ b/db/migrate/20261102120000_create_rs_authorization_codes.rb @@ -4,7 +4,7 @@ class CreateRsAuthorizationCodes < ActiveRecord::Migration[8.1] t.references :user, null: false, foreign_key: true t.string :client_id, null: false t.string :redirect_uri, null: false - t.text :permissions, default: [].to_yaml + t.text :permissions, default: [].to_json t.datetime :permission_expire_at t.string :code_digest, null: false t.string :code_challenge, null: false diff --git a/db/schema.rb b/db/schema.rb index daab063..79a53bc 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -125,7 +125,7 @@ ActiveRecord::Schema[8.1].define(version: 2026_11_02_120000) do t.datetime "created_at", null: false t.datetime "expires_at", null: false t.datetime "permission_expire_at" - t.text "permissions", default: "--- []\n" + t.text "permissions", default: "[]" t.string "redirect_uri", null: false t.datetime "updated_at", null: false t.integer "user_id", null: false diff --git a/spec/controllers/rs/oauth_controller_spec.rb b/spec/controllers/rs/oauth_controller_spec.rb index 8bcef77..6e2c233 100644 --- a/spec/controllers/rs/oauth_controller_spec.rb +++ b/spec/controllers/rs/oauth_controller_spec.rb @@ -130,6 +130,8 @@ RSpec.describe Rs::OauthController, type: :controller do it "does not reuse the existing token (renders the consent dialog)" do expect(response.response_code).to eq(200) + expect(response.location).to be_nil + expect(user.remote_storage_authorizations.count).to eq(1) end end @@ -157,7 +159,7 @@ RSpec.describe Rs::OauthController, type: :controller do client_id: "example.com", scope: "documents", response_type: "code", - code_challenge: "challenge123", + code_challenge: "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM", code_challenge_method: "S256", state: "foobar123" } @@ -166,7 +168,7 @@ RSpec.describe Rs::OauthController, type: :controller do it "renders the consent dialog and exposes the code challenge" do expect(response.response_code).to eq(200) expect(assigns["response_type"]).to eq("code") - expect(assigns["code_challenge"]).to eq("challenge123") + expect(assigns["code_challenge"]).to eq("E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM") expect(assigns["code_challenge_method"]).to eq("S256") end @@ -200,7 +202,7 @@ RSpec.describe Rs::OauthController, type: :controller do client_id: "example.com", scope: "documents", response_type: "code", - code_challenge: "challenge123", + code_challenge: "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM", code_challenge_method: "plain" } end @@ -210,6 +212,40 @@ RSpec.describe Rs::OauthController, type: :controller do end end + context "with an unsupported response_type" do + before do + get :new, params: { + username: user.cn, + redirect_uri: "https://example.com", + client_id: "example.com", + scope: "documents", + response_type: "id_token" + } + end + + it "redirects with unsupported_response_type" do + expect(response).to redirect_to("https://example.com#error=unsupported_response_type") + end + end + + context "with a malformed code_challenge" do + before do + get :new, params: { + username: user.cn, + redirect_uri: "https://example.com", + client_id: "example.com", + scope: "documents", + response_type: "code", + code_challenge: "too-short", + code_challenge_method: "S256" + } + end + + it "redirects with invalid_request" do + expect(response).to redirect_to("https://example.com?error=invalid_request") + end + end + context "no scope" do before do get :new, params: { @@ -452,7 +488,7 @@ RSpec.describe Rs::OauthController, type: :controller do client_id: "example.com", state: "foobar123", response_type: "code", - code_challenge: "challenge123", + code_challenge: "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM", code_challenge_method: "S256", expire_at: 1.month.from_now } @@ -461,7 +497,7 @@ RSpec.describe Rs::OauthController, type: :controller do it "creates an authorization code" do expect(@code.permissions).to eq(%w(documents photos contacts:rw)) - expect(@code.code_challenge).to eq("challenge123") + expect(@code.code_challenge).to eq("E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM") expect(@code.code_challenge_method).to eq("S256") end @@ -500,6 +536,43 @@ RSpec.describe Rs::OauthController, type: :controller do end end + context "with an unsupported response_type" do + before do + post :create, params: { + user_id: user.id, + scope: "documents", + redirect_uri: "https://example.com", + client_id: "example.com", + response_type: "id_token", + expire_at: 1.month.from_now + } + end + + it "redirects with unsupported_response_type" do + expect(response).to redirect_to("https://example.com#error=unsupported_response_type") + end + end + + context "with response_type=code and a redirect_uri that already has a query" do + before do + post :create, params: { + user_id: user.id, + scope: "documents", + redirect_uri: "https://example.com/?foo=bar", + client_id: "example.com", + state: "foobar123", + response_type: "code", + code_challenge: "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM", + code_challenge_method: "S256", + expire_at: 1.month.from_now + } + end + + it "appends the code with an ampersand" do + expect(response.location).to match(%r{\Ahttps://example\.com/\?foo=bar&code=.+&state=foobar123\z}) + end + end + context "when the user_id is different from the signed in user" do before do post :create, params: { @@ -615,5 +688,22 @@ RSpec.describe Rs::OauthController, type: :controller do expect(JSON.parse(response.body)["error"]).to eq("unsupported_grant_type") end end + + context "with missing required parameters" do + before { post :token, params: { grant_type: "authorization_code", code: "x" } } + + it "returns invalid_request" do + expect(response.response_code).to eq(400) + expect(JSON.parse(response.body)["error"]).to eq("invalid_request") + end + end + + context "on error" do + before { post :token, params: { grant_type: "password" } } + + it "does not cache the response" do + expect(response.headers["Cache-Control"]).to eq("no-store") + end + end end end diff --git a/spec/features/rs/oauth_spec.rb b/spec/features/rs/oauth_spec.rb index 3cfa5dc..de0527d 100644 --- a/spec/features/rs/oauth_spec.rb +++ b/spec/features/rs/oauth_spec.rb @@ -57,13 +57,13 @@ RSpec.describe 'remoteStorage OAuth Dialog', type: :feature do client_id: "http://example.com", scope: "documents:rw", response_type: "code", - code_challenge: "challenge123", + code_challenge: "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM", code_challenge_method: "S256") end it "includes the PKCE parameters in the consent form" do expect(page).to have_selector("input[name='response_type'][value='code']", visible: false) - expect(page).to have_selector("input[name='code_challenge'][value='challenge123']", visible: false) + expect(page).to have_selector("input[name='code_challenge'][value='E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM']", visible: false) expect(page).to have_selector("input[name='code_challenge_method'][value='S256']", visible: false) end end diff --git a/spec/models/rs_authorization_code_spec.rb b/spec/models/rs_authorization_code_spec.rb index 0df92e5..ba3dc77 100644 --- a/spec/models/rs_authorization_code_spec.rb +++ b/spec/models/rs_authorization_code_spec.rb @@ -43,6 +43,11 @@ RSpec.describe RsAuthorizationCode, type: :model do it "returns nil for a blank code" do expect(RsAuthorizationCode.find_by_code(nil)).to be_nil end + + it "returns nil for an expired code" do + code = build_code(expires_at: 1.minute.ago) + expect(RsAuthorizationCode.find_by_code(code.code)).to be_nil + end end describe "#expired?" do @@ -54,12 +59,4 @@ RSpec.describe RsAuthorizationCode, type: :model do expect(build_code(expires_at: 1.minute.ago)).to be_expired end end - - describe "#consume!" do - it "destroys the record" do - code = build_code - - expect { code.consume! }.to change(RsAuthorizationCode, :count).by(-1) - end - end end From cd2b4c9ba588974cb30c049319e1f77bd3fbc0b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A2u=20Cao?= Date: Tue, 3 Nov 2026 11:59:12 +0400 Subject: [PATCH 04/16] Address second-round review findings in the PKCE core - Enqueue the expiry job and the "app connected" notification on after_commit instead of after_create, so they only fire once the (now transactional) code exchange commits - Enforce the PKCE code_challenge format (43-128 unreserved chars, S256) with model validations, not only at the controller - Drop the now-unused RsAuthorizationCode#expired? - Cover the client_id-vs-stored-client_id check Part of #254 --- app/models/remote_storage_authorization.rb | 4 +- app/models/rs_authorization_code.rb | 8 ++-- spec/models/rs_authorization_code_spec.rb | 43 +++++++++++++------ .../rs_oauth_manager/exchange_code_spec.rb | 13 ++++++ .../rs_oauth_manager/issue_code_spec.rb | 4 +- 5 files changed, 51 insertions(+), 21 deletions(-) diff --git a/app/models/remote_storage_authorization.rb b/app/models/remote_storage_authorization.rb index f6a7fed..a8dfd1c 100644 --- a/app/models/remote_storage_authorization.rb +++ b/app/models/remote_storage_authorization.rb @@ -17,8 +17,8 @@ class RemoteStorageAuthorization < ApplicationRecord before_create :generate_token before_create :store_token_in_redis before_create :find_or_create_web_app - after_create :schedule_token_expiry - after_create :notify_user + after_commit :schedule_token_expiry, on: :create + after_commit :notify_user, on: :create before_destroy :delete_token_from_redis after_destroy :remove_token_expiry_job diff --git a/app/models/rs_authorization_code.rb b/app/models/rs_authorization_code.rb index a433ddf..482746c 100644 --- a/app/models/rs_authorization_code.rb +++ b/app/models/rs_authorization_code.rb @@ -9,6 +9,10 @@ class RsAuthorizationCode < ApplicationRecord before_validation :generate_code + validates :code_challenge, presence: true, + format: { with: /\A[A-Za-z0-9\-._~]{43,128}\z/ } + validates :code_challenge_method, inclusion: { in: %w[S256] } + scope :active, -> { where(expires_at: Time.current..) } def self.find_by_code(raw_code) @@ -17,10 +21,6 @@ class RsAuthorizationCode < ApplicationRecord active.find_by(code_digest: Digest::SHA256.hexdigest(raw_code)) end - def expired? - expires_at <= Time.current - end - private def generate_code diff --git a/spec/models/rs_authorization_code_spec.rb b/spec/models/rs_authorization_code_spec.rb index ba3dc77..4593446 100644 --- a/spec/models/rs_authorization_code_spec.rb +++ b/spec/models/rs_authorization_code_spec.rb @@ -3,17 +3,44 @@ require 'rails_helper' RSpec.describe RsAuthorizationCode, type: :model do let(:user) { create :user } - def build_code(**attrs) - RsAuthorizationCode.create!( + # Example value from RFC 7636, Appendix B + let(:challenge) { "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM" } + + def new_code(**attrs) + RsAuthorizationCode.new( user: user, client_id: "example.com", redirect_uri: "https://example.com", permissions: %w(documents:rw), - code_challenge: "challenge", + code_challenge: challenge, **attrs ) end + def build_code(**attrs) + new_code(**attrs).tap(&:save!) + end + + describe "validations" do + it "is valid with a well-formed S256 challenge" do + expect(new_code).to be_valid + end + + it "rejects a challenge shorter than 43 characters" do + expect(new_code(code_challenge: "short")).not_to be_valid + end + + it "rejects a challenge with invalid characters" do + expect(new_code(code_challenge: "#{'a' * 42}!")).not_to be_valid + end + + it "rejects an unsupported challenge method" do + code = new_code(code_challenge_method: "plain") + expect(code).not_to be_valid + expect(code.errors[:code_challenge_method]).to be_present + end + end + describe "#create" do it "generates a raw code and stores only its digest" do code = build_code @@ -49,14 +76,4 @@ RSpec.describe RsAuthorizationCode, type: :model do expect(RsAuthorizationCode.find_by_code(code.code)).to be_nil end end - - describe "#expired?" do - it "is false before the expiry" do - expect(build_code).not_to be_expired - end - - it "is true after the expiry" do - expect(build_code(expires_at: 1.minute.ago)).to be_expired - end - end end diff --git a/spec/services/rs_oauth_manager/exchange_code_spec.rb b/spec/services/rs_oauth_manager/exchange_code_spec.rb index 900b9e8..8bf1661 100644 --- a/spec/services/rs_oauth_manager/exchange_code_spec.rb +++ b/spec/services/rs_oauth_manager/exchange_code_spec.rb @@ -94,6 +94,19 @@ RSpec.describe RsOauthManager::ExchangeCode do .to raise_error(RsOauthManagerService::InvalidGrant) end + it "rejects a code whose stored client_id host differs from the presented one" do + code = RsAuthorizationCode.create!( + user: user, + client_id: "evil.example.org", + redirect_uri: "https://example.com", + permissions: %w(documents:rw), + code_challenge: code_challenge, + code_challenge_method: "S256" + ) + + expect { exchange(code) }.to raise_error(RsOauthManagerService::InvalidGrant) + end + it "rejects a bad code_verifier" do code = issue_code diff --git a/spec/services/rs_oauth_manager/issue_code_spec.rb b/spec/services/rs_oauth_manager/issue_code_spec.rb index 429c357..c80593f 100644 --- a/spec/services/rs_oauth_manager/issue_code_spec.rb +++ b/spec/services/rs_oauth_manager/issue_code_spec.rb @@ -9,7 +9,7 @@ RSpec.describe RsOauthManager::IssueCode do client_id: "example.com", redirect_uri: "https://example.com", permissions: %w(documents:rw), - code_challenge: "challenge", + code_challenge: "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM", code_challenge_method: "S256", expire_at: 1.month.from_now ) @@ -20,7 +20,7 @@ RSpec.describe RsOauthManager::IssueCode do expect(subject.code).to be_present expect(subject.client_id).to eq("example.com") expect(subject.permissions).to eq(%w(documents:rw)) - expect(subject.code_challenge).to eq("challenge") + expect(subject.code_challenge).to eq("E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM") end it "does not create a remote storage authorization" do From 7c4bad860af70dc5a3bc10f609746774e112c633 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A2u=20Cao?= Date: Mon, 2 Nov 2026 13:01:44 +0400 Subject: [PATCH 05/16] Store per-device user agent for remoteStorage authorizations - Add user_agent to remote_storage_authorizations and rs_authorization_codes - Capture the browser's user agent at consent, carry it on the authorization code, and store it on the authorization when the code is exchanged - Store it directly for the implicit flow too With the reuse shortcut gone, each authorization is already a distinct per-device token; this adds the metadata needed to label and display them. Part of #254 --- app/controllers/rs/oauth_controller.rb | 4 +++- app/services/rs_oauth_manager/exchange_code.rb | 3 ++- app/services/rs_oauth_manager/issue_code.rb | 4 +++- .../20261102130000_add_user_agent_to_rs_authorizations.rb | 6 ++++++ db/schema.rb | 4 +++- spec/controllers/rs/oauth_controller_spec.rb | 6 ++++++ spec/services/rs_oauth_manager/exchange_code_spec.rb | 2 ++ spec/services/rs_oauth_manager/issue_code_spec.rb | 4 +++- 8 files changed, 28 insertions(+), 5 deletions(-) create mode 100644 db/migrate/20261102130000_add_user_agent_to_rs_authorizations.rb diff --git a/app/controllers/rs/oauth_controller.rb b/app/controllers/rs/oauth_controller.rb index fe69198..c5e1952 100644 --- a/app/controllers/rs/oauth_controller.rb +++ b/app/controllers/rs/oauth_controller.rb @@ -112,6 +112,7 @@ class Rs::OauthController < ApplicationController redirect_uri: redirect_uri, permissions: permissions, expire_at: expire_at, + user_agent: request.user_agent, code_challenge: challenge, code_challenge_method: method ) @@ -124,7 +125,8 @@ class Rs::OauthController < ApplicationController client_id: client_id, redirect_uri: redirect_uri, app_name: client_id, - expire_at: expire_at + expire_at: expire_at, + user_agent: request.user_agent ) redirect_to url_with_state("#{redirect_uri}#access_token=#{auth.token}", state), diff --git a/app/services/rs_oauth_manager/exchange_code.rb b/app/services/rs_oauth_manager/exchange_code.rb index 131fbe2..8ed5298 100644 --- a/app/services/rs_oauth_manager/exchange_code.rb +++ b/app/services/rs_oauth_manager/exchange_code.rb @@ -36,7 +36,8 @@ module RsOauthManager client_id: auth_code.client_id, redirect_uri: auth_code.redirect_uri, app_name: auth_code.client_id, - expire_at: auth_code.permission_expire_at + expire_at: auth_code.permission_expire_at, + user_agent: auth_code.user_agent ) end diff --git a/app/services/rs_oauth_manager/issue_code.rb b/app/services/rs_oauth_manager/issue_code.rb index b155117..87ccb79 100644 --- a/app/services/rs_oauth_manager/issue_code.rb +++ b/app/services/rs_oauth_manager/issue_code.rb @@ -2,7 +2,7 @@ module RsOauthManager class IssueCode < RsOauthManagerService def initialize(user:, client_id:, redirect_uri:, permissions:, code_challenge:, code_challenge_method: "S256", - expire_at: nil) + expire_at: nil, user_agent: nil) @user = user @client_id = client_id @redirect_uri = redirect_uri @@ -10,6 +10,7 @@ module RsOauthManager @code_challenge = code_challenge @code_challenge_method = code_challenge_method @expire_at = expire_at + @user_agent = user_agent end def call @@ -19,6 +20,7 @@ module RsOauthManager redirect_uri: @redirect_uri, permissions: @permissions, permission_expire_at: @expire_at, + user_agent: @user_agent, code_challenge: @code_challenge, code_challenge_method: @code_challenge_method ) diff --git a/db/migrate/20261102130000_add_user_agent_to_rs_authorizations.rb b/db/migrate/20261102130000_add_user_agent_to_rs_authorizations.rb new file mode 100644 index 0000000..841369e --- /dev/null +++ b/db/migrate/20261102130000_add_user_agent_to_rs_authorizations.rb @@ -0,0 +1,6 @@ +class AddUserAgentToRsAuthorizations < ActiveRecord::Migration[8.1] + def change + add_column :remote_storage_authorizations, :user_agent, :string + add_column :rs_authorization_codes, :user_agent, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index 79a53bc..524de14 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.1].define(version: 2026_11_02_120000) do +ActiveRecord::Schema[8.1].define(version: 2026_11_02_130000) do create_table "active_storage_attachments", force: :cascade do |t| t.bigint "blob_id", null: false t.datetime "created_at", null: false @@ -110,6 +110,7 @@ ActiveRecord::Schema[8.1].define(version: 2026_11_02_120000) do t.string "redirect_uri" t.string "token" t.datetime "updated_at", null: false + t.string "user_agent" t.integer "user_id", null: false t.integer "web_app_id" t.index ["permissions"], name: "index_remote_storage_authorizations_on_permissions" @@ -128,6 +129,7 @@ ActiveRecord::Schema[8.1].define(version: 2026_11_02_120000) do t.text "permissions", default: "[]" t.string "redirect_uri", null: false t.datetime "updated_at", null: false + t.string "user_agent" t.integer "user_id", null: false t.index ["code_digest"], name: "index_rs_authorization_codes_on_code_digest", unique: true t.index ["user_id"], name: "index_rs_authorization_codes_on_user_id" diff --git a/spec/controllers/rs/oauth_controller_spec.rb b/spec/controllers/rs/oauth_controller_spec.rb index 6e2c233..e06be1a 100644 --- a/spec/controllers/rs/oauth_controller_spec.rb +++ b/spec/controllers/rs/oauth_controller_spec.rb @@ -346,6 +346,7 @@ RSpec.describe Rs::OauthController, type: :controller do context "when user is signed in" do before do sign_in user + request.user_agent = "RSpec Device" end after do @@ -370,6 +371,10 @@ RSpec.describe Rs::OauthController, type: :controller do expect(@auth.permissions).to eq(%w(documents photos contacts:rw videos:r tasks/work:r)) end + it "stores the device's user agent" do + expect(@auth.user_agent).to eq("RSpec Device") + end + it "redirects to the redirect_uri" do expect(response).to redirect_to("https://example.com#access_token=#{@auth.token}&state=foobar123") end @@ -499,6 +504,7 @@ RSpec.describe Rs::OauthController, type: :controller do expect(@code.permissions).to eq(%w(documents photos contacts:rw)) expect(@code.code_challenge).to eq("E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM") expect(@code.code_challenge_method).to eq("S256") + expect(@code.user_agent).to eq("RSpec Device") end it "does not create a remote storage authorization" do diff --git a/spec/services/rs_oauth_manager/exchange_code_spec.rb b/spec/services/rs_oauth_manager/exchange_code_spec.rb index 8bf1661..e0169a2 100644 --- a/spec/services/rs_oauth_manager/exchange_code_spec.rb +++ b/spec/services/rs_oauth_manager/exchange_code_spec.rb @@ -20,6 +20,7 @@ RSpec.describe RsOauthManager::ExchangeCode do permissions: %w(documents:rw), code_challenge: code_challenge, code_challenge_method: "S256", + user_agent: "RSpec Device", **overrides ) end @@ -43,6 +44,7 @@ RSpec.describe RsOauthManager::ExchangeCode do expect(auth.permissions).to eq(%w(documents:rw)) expect(auth.client_id).to eq("example.com") expect(auth.token).to be_present + expect(auth.user_agent).to eq("RSpec Device") expect(RsAuthorizationCode.exists?(code.id)).to be(false) end diff --git a/spec/services/rs_oauth_manager/issue_code_spec.rb b/spec/services/rs_oauth_manager/issue_code_spec.rb index c80593f..3e424d1 100644 --- a/spec/services/rs_oauth_manager/issue_code_spec.rb +++ b/spec/services/rs_oauth_manager/issue_code_spec.rb @@ -11,7 +11,8 @@ RSpec.describe RsOauthManager::IssueCode do permissions: %w(documents:rw), code_challenge: "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM", code_challenge_method: "S256", - expire_at: 1.month.from_now + expire_at: 1.month.from_now, + user_agent: "RSpec Device" ) end @@ -21,6 +22,7 @@ RSpec.describe RsOauthManager::IssueCode do expect(subject.client_id).to eq("example.com") expect(subject.permissions).to eq(%w(documents:rw)) expect(subject.code_challenge).to eq("E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM") + expect(subject.user_agent).to eq("RSpec Device") end it "does not create a remote storage authorization" do From eeb399ff8389632a683a4b2291168d135ca8328f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A2u=20Cao?= Date: Mon, 2 Nov 2026 17:40:00 +0400 Subject: [PATCH 06/16] Document the storage-first launch flow Add an explicit spec for the storage-first handoff (remoteStorage spec section 11): launching an app passes only `#remotestorage=

`, never an access token. The app then runs the application-first (now PKCE-capable) flow itself. Part of #254 --- app/controllers/services/rs_auths_controller.rb | 2 ++ spec/controllers/services/rs_auths_controller_spec.rb | 8 ++++++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/app/controllers/services/rs_auths_controller.rb b/app/controllers/services/rs_auths_controller.rb index 2fff161..c081538 100644 --- a/app/controllers/services/rs_auths_controller.rb +++ b/app/controllers/services/rs_auths_controller.rb @@ -29,6 +29,8 @@ class Services::RsAuthsController < Services::BaseController current_user.address end + # 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}" redirect_to launch_url, allow_other_host: true diff --git a/spec/controllers/services/rs_auths_controller_spec.rb b/spec/controllers/services/rs_auths_controller_spec.rb index 1c619c2..812a842 100644 --- a/spec/controllers/services/rs_auths_controller_spec.rb +++ b/spec/controllers/services/rs_auths_controller_spec.rb @@ -11,7 +11,7 @@ RSpec.describe Services::RsAuthsController, type: :controller do ]) end - describe "GET /services/storage/rs_auths/:id/launch_app" do + describe "GET /services/storage/rs_auths/:id/launch_app (storage-first)" do context "when user is signed in" do before do sign_in user @@ -32,10 +32,14 @@ RSpec.describe Services::RsAuthsController, type: :controller do @auth.destroy end - it "redirects to the given URL with the correct RS URL fragment params" do + it "hands the app the user address in the URL fragment" do launch_url = "https://app.example.com#remotestorage=#{user.address}" expect(response).to redirect_to(launch_url) end + + it "does not hand over an access token" do + expect(response.location).not_to include("access_token") + end end context "when remoteStorage is not enabled for the user" do From 3dcd263d438f8b41457278218c7a06e7221ee91c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A2u=20Cao?= Date: Tue, 3 Nov 2026 11:42:06 +0400 Subject: [PATCH 07/16] Store the remoteStorage user agent as text, not varchar(255) Real-world user agents (in-app browsers, Android WebView variants) commonly exceed 255 characters. On PostgreSQL that would raise and 500 the consent POST and token exchange; SQLite dev/test does not enforce the limit, so it went unnoticed. Use text columns for user_agent on both tables. Part of #254 --- .../20261102130000_add_user_agent_to_rs_authorizations.rb | 4 ++-- db/schema.rb | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/db/migrate/20261102130000_add_user_agent_to_rs_authorizations.rb b/db/migrate/20261102130000_add_user_agent_to_rs_authorizations.rb index 841369e..d310450 100644 --- a/db/migrate/20261102130000_add_user_agent_to_rs_authorizations.rb +++ b/db/migrate/20261102130000_add_user_agent_to_rs_authorizations.rb @@ -1,6 +1,6 @@ class AddUserAgentToRsAuthorizations < ActiveRecord::Migration[8.1] def change - add_column :remote_storage_authorizations, :user_agent, :string - add_column :rs_authorization_codes, :user_agent, :string + add_column :remote_storage_authorizations, :user_agent, :text + add_column :rs_authorization_codes, :user_agent, :text end end diff --git a/db/schema.rb b/db/schema.rb index 524de14..4518582 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -110,7 +110,7 @@ ActiveRecord::Schema[8.1].define(version: 2026_11_02_130000) do t.string "redirect_uri" t.string "token" t.datetime "updated_at", null: false - t.string "user_agent" + t.text "user_agent" t.integer "user_id", null: false t.integer "web_app_id" t.index ["permissions"], name: "index_remote_storage_authorizations_on_permissions" @@ -129,7 +129,7 @@ ActiveRecord::Schema[8.1].define(version: 2026_11_02_130000) do t.text "permissions", default: "[]" t.string "redirect_uri", null: false t.datetime "updated_at", null: false - t.string "user_agent" + t.text "user_agent" t.integer "user_id", null: false t.index ["code_digest"], name: "index_rs_authorization_codes_on_code_digest", unique: true t.index ["user_id"], name: "index_rs_authorization_codes_on_user_id" 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 08/16] 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 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 09/16] 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 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 10/16] 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 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 11/16] 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) 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 12/16] 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 From d94fda6d26960629d5edc453417051d9a4f348e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A2u=20Cao?= Date: Tue, 3 Nov 2026 14:36:39 +0400 Subject: [PATCH 13/16] Polish the storage dashboard and details modal Visual refinement pass on the connected-apps dashboard introduced for #254. Card: - Restructure into a header row (app icon, name, host, chevron) with the device/category summary as a full-width footer, so the icon aligns with the name and the metrics sit on a consistent baseline - Always show the host alongside the app name, except when it is identical - Render the icon fallback as a neutral rounded tile instead of a faint watermark, and stop distorting real icons (object-contain) - Summarise the union of the app's scopes, with monitor/folder icons Details modal: - Replace the duplicated blue buttons with a secondary Close and a primary Launch app, split 50:50 on desktop like the consent dialog - Tighten the header, section spacing and device rows; hide a redundant host - Add exact date/time tooltips to the connected and expiry labels - Align the scope-list folder icons with their labels Misc: - Use fewer near-identical grays across the cards and modal - Drop the redundant top margin on the app grid Part of #254 --- .../web_app_icon_component.html.erb | 6 ++- app/components/rs_auth_component.html.erb | 42 ++++++++++++------- .../rs_auth_details_modal_component.html.erb | 37 ++++++++++------ .../rs_auth_details_modal_component.rb | 8 ++++ .../rs_scope_list_component.html.erb | 39 ++++++++--------- app/components/rs_scope_list_component.rb | 7 +++- app/views/services/rs_auths/index.html.erb | 2 +- 7 files changed, 87 insertions(+), 54 deletions(-) diff --git a/app/components/app_catalog/web_app_icon_component.html.erb b/app/components/app_catalog/web_app_icon_component.html.erb index 3f9c5f3..37af872 100644 --- a/app/components/app_catalog/web_app_icon_component.html.erb +++ b/app/components/app_catalog/web_app_icon_component.html.erb @@ -1,5 +1,7 @@ <% if @image_url %> - <%= image_tag @image_url, class: "h-full w-full" %> + <%= image_tag @image_url, class: "h-full w-full rounded-lg object-contain" %> <% else %> - <%= render partial: "icons/remotestorage", locals: { custom_class: "h-full w-full p-0.5 text-gray-200" } %> + + <%= render partial: "icons/remotestorage", locals: { custom_class: "h-1/2 w-1/2 text-gray-400" } %> + <% end %> diff --git a/app/components/rs_auth_component.html.erb b/app/components/rs_auth_component.html.erb index 64ea405..3bc335e 100644 --- a/app/components/rs_auth_component.html.erb +++ b/app/components/rs_auth_component.html.erb @@ -1,26 +1,36 @@
diff --git a/app/components/rs_auth_details_modal_component.html.erb b/app/components/rs_auth_details_modal_component.html.erb index bf86eef..6fd0dfe 100644 --- a/app/components/rs_auth_details_modal_component.html.erb +++ b/app/components/rs_auth_details_modal_component.html.erb @@ -1,31 +1,37 @@ -<%= render ModalComponent.new do %> +<%= render ModalComponent.new(show_close_button: false) do %>
-
+
<%= render AppCatalog::WebAppIconComponent.new(web_app: @web_app) %>
-

<%= @auth.display_name %>

-

<%= @auth.client_id %>

+

<%= @auth.display_name %>

+ <% if @auth.client_id != @auth.display_name %> +

<%= @auth.client_id %>

+ <% end %>
-
+
Access
- <%= render RsScopeListComponent.new(scopes: permissions, root: root_access?) %> + <%= render RsScopeListComponent.new(scopes: permissions, root: root_access?, + icon_class: "h-5 w-5") %> -
+
Connected devices
-
    +
      <% @authorizations.each do |auth| %>
    • -

      <%= device_name(auth) %>

      +

      <%= device_name(auth) %>

      - Connected <%= auth.created_at.strftime('%-d %b %Y') %> + <%= tag.span "Connected #{auth.created_at.strftime('%-d %b %Y')}", + title: connected_at_title(auth) %> · - "><%= status_label(auth) %> + <%= tag.span status_label(auth), + title: (expires_at_title(auth) if auth.expire_at.present?), + class: ("text-red-700" if auth.expired?) %>

      <%= button_to "Revoke access", services_storage_rs_auth_path(auth), method: :delete, @@ -35,8 +41,13 @@ <% end %>
    -
    +
    <%= link_to "Launch app", launch_app_services_storage_path(@auth.client_id), - class: "btn-md btn-blue", target: "_blank", rel: "noopener", data: { turbo: false } %> + class: "btn-md btn-blue w-full sm:grow sm:order-last text-center", + 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 46daa37..20c55d5 100644 --- a/app/components/rs_auth_details_modal_component.rb +++ b/app/components/rs_auth_details_modal_component.rb @@ -26,4 +26,12 @@ class RsAuthDetailsModalComponent < ViewComponent::Base "Expires #{auth.expire_at.strftime('%-d %b %Y')}" end + + def connected_at_title(auth) + auth.created_at.strftime("%-d %b %Y, %H:%M") + end + + def expires_at_title(auth) + auth.expire_at.strftime("%-d %b %Y, %H:%M") + end end diff --git a/app/components/rs_scope_list_component.html.erb b/app/components/rs_scope_list_component.html.erb index 42ae256..7391b3d 100644 --- a/app/components/rs_scope_list_component.html.erb +++ b/app/components/rs_scope_list_component.html.erb @@ -1,25 +1,22 @@ -<% if @root %> -

    - +

    + <% 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) + locals: { custom_class: "#{icon_classes} text-red-700" } %> + All categories and files + <% if read_only? %> + (read only) <% end %>

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

    + <%= render partial: "icons/folder", locals: { custom_class: icon_classes } %> + <%= scope_name(scope) %> + <% if scope_permissions(scope) == "r" %> + (read only) + <% end %> +

    + <% end %> <% end %> -<% end %> +
    diff --git a/app/components/rs_scope_list_component.rb b/app/components/rs_scope_list_component.rb index 198ef08..43c431a 100644 --- a/app/components/rs_scope_list_component.rb +++ b/app/components/rs_scope_list_component.rb @@ -3,12 +3,17 @@ class RsScopeListComponent < ViewComponent::Base include OauthHelper - def initialize(scopes:, root: false) + def initialize(scopes:, root: false, icon_class: nil) @scopes = scopes @root = root + @icon_class = icon_class end def read_only? @scopes.include?(":r") end + + def icon_classes + ["flex-none", @icon_class].compact.join(" ") + end end diff --git a/app/views/services/rs_auths/index.html.erb b/app/views/services/rs_auths/index.html.erb index edd6ade..cbb6352 100644 --- a/app/views/services/rs_auths/index.html.erb +++ b/app/views/services/rs_auths/index.html.erb @@ -11,7 +11,7 @@
    <% if @rs_apps.any? %> -
    +
    <% @rs_apps.each do |authorizations| %> <%= render RsAuthComponent.new(authorizations: authorizations) %> <% end %> From 614ead90a956573d6fe356402881a09f6c25f8df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A2u=20Cao?= Date: Tue, 3 Nov 2026 16:55:05 +0400 Subject: [PATCH 14/16] Distinguish tokens without a recorded device Tokens issued before user agents were captured cannot be attributed to a single device, and the per-app tokens issued earlier could even be reused from several devices at once. - Add RemoteStorageAuthorization#device_label, falling back to "Unknown device(s)" so the card and the details modal share one source - Show a lower-bound device count with a "+" whenever an app has such a token, so it reads as "could be more" instead of claiming a number we cannot support - Drop the now-redundant device-name wrapper from the details modal Part of #254 --- app/components/rs_auth_component.rb | 16 +++++++-- .../rs_auth_details_modal_component.html.erb | 2 +- .../rs_auth_details_modal_component.rb | 4 --- app/models/remote_storage_authorization.rb | 8 +++++ spec/features/services/rs_auths_spec.rb | 34 +++++++++++++++++++ .../remote_storage_authorization_spec.rb | 15 ++++++++ 6 files changed, 71 insertions(+), 8 deletions(-) diff --git a/app/components/rs_auth_component.rb b/app/components/rs_auth_component.rb index b80d225..5984941 100644 --- a/app/components/rs_auth_component.rb +++ b/app/components/rs_auth_component.rb @@ -24,11 +24,21 @@ class RsAuthComponent < ViewComponent::Base "#{count} #{'category'.pluralize(count)}" end - def device_count - @authorizations.map { |auth| auth.device_name.presence || "Unknown device" }.uniq.size + def known_device_count + @authorizations.map(&:device_name).compact.uniq.size + end + + def unrecorded_device? + @authorizations.any? { |auth| auth.device_name.blank? } end def device_summary - "#{device_count} #{'device'.pluralize(device_count)}" + count = known_device_count + return "#{count} #{'device'.pluralize(count)}" unless unrecorded_device? + + # An authorization without a recorded user agent may cover several devices, + # so only a lower bound can be given. + lower_bound = count + 1 + "#{lower_bound}+ #{'device'.pluralize(lower_bound)}" 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 6fd0dfe..36da0f5 100644 --- a/app/components/rs_auth_details_modal_component.html.erb +++ b/app/components/rs_auth_details_modal_component.html.erb @@ -24,7 +24,7 @@ <% @authorizations.each do |auth| %>
  • -

    <%= device_name(auth) %>

    +

    <%= auth.device_label %>

    <%= tag.span "Connected #{auth.created_at.strftime('%-d %b %Y')}", title: connected_at_title(auth) %> diff --git a/app/components/rs_auth_details_modal_component.rb b/app/components/rs_auth_details_modal_component.rb index 20c55d5..0344e31 100644 --- a/app/components/rs_auth_details_modal_component.rb +++ b/app/components/rs_auth_details_modal_component.rb @@ -16,10 +16,6 @@ class RsAuthDetailsModalComponent < ViewComponent::Base (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? diff --git a/app/models/remote_storage_authorization.rb b/app/models/remote_storage_authorization.rb index cf5167f..6c7ae6e 100644 --- a/app/models/remote_storage_authorization.rb +++ b/app/models/remote_storage_authorization.rb @@ -2,6 +2,10 @@ class RemoteStorageAuthorization < ApplicationRecord belongs_to :user belongs_to :web_app, class_name: "AppCatalog::WebApp", optional: true + # Tokens issued before user agents were recorded may have been used from any + # number of devices, so they cannot be attributed to a single one. + UNKNOWN_DEVICE = "Unknown device(s)" + serialize :permissions, coder: YAML unless Rails.env.production? validates_presence_of :permissions @@ -60,6 +64,10 @@ class RemoteStorageAuthorization < ApplicationRecord [name, os].compact.join(" on ").presence end + def device_label + device_name || UNKNOWN_DEVICE + end + def display_name web_app&.name.presence || app_name.presence || client_id end diff --git a/spec/features/services/rs_auths_spec.rb b/spec/features/services/rs_auths_spec.rb index 7496ba0..44435c5 100644 --- a/spec/features/services/rs_auths_spec.rb +++ b/spec/features/services/rs_auths_spec.rb @@ -140,6 +140,40 @@ RSpec.describe "Storage connected apps", type: :feature do end end + context "with an authorization from before user agents were recorded" do + let!(:legacy) { create_auth(client: "notes.example.com", device: nil) } + + after { legacy.destroy } + + it "only claims a lower-bound device count and labels the device as unknown" do + visit apps_services_storage_path + + within ".rs-auth-card" do + expect(page).to have_content("1+ device") + end + + expect(page).to have_selector("p", text: "Unknown device(s)", visible: false) + end + end + + context "with an unrecorded authorization alongside a recorded device" do + let!(:legacy) { create_auth(client: "notes.example.com", device: nil) } + let!(:laptop) { create_auth(client: "notes.example.com", device: firefox_ua) } + + after do + legacy.destroy + laptop.destroy + end + + it "counts the unrecorded authorization as a possible extra device" do + visit apps_services_storage_path + + within ".rs-auth-card" do + expect(page).to have_content("2+ devices") + end + end + end + context "without connected apps" do it "shows the empty state" do visit apps_services_storage_path diff --git a/spec/models/remote_storage_authorization_spec.rb b/spec/models/remote_storage_authorization_spec.rb index 51cd7a0..7616603 100644 --- a/spec/models/remote_storage_authorization_spec.rb +++ b/spec/models/remote_storage_authorization_spec.rb @@ -372,4 +372,19 @@ RSpec.describe RemoteStorageAuthorization, type: :model do expect(auth_with_user_agent("Firefox/120.0").device_name).to eq("Firefox") end end + + describe "#device_label" do + def auth_with_user_agent(user_agent) + RemoteStorageAuthorization.new(user: user, user_agent: user_agent) + end + + it "falls back to the unrecorded placeholder without a user agent" do + expect(auth_with_user_agent(nil).device_label).to eq("Unknown device(s)") + end + + it "returns the device name when one can be determined" do + ua = "Mozilla/5.0 (X11; Linux x86_64; rv:120.0) Gecko/20100101 Firefox/120.0" + expect(auth_with_user_agent(ua).device_label).to eq("Firefox on Linux") + end + end end From faa7bf61468178c6480184e084ec4ff45135b455 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A2u=20Cao?= Date: Tue, 3 Nov 2026 17:45:37 +0400 Subject: [PATCH 15/16] Revoke device authorizations in place with Turbo Streams Revoking a device redirected away from the storage dashboard, which closed the details modal and reloaded the page. Refreshing the affected parts instead keeps the modal open and shows the change where it happened. - Extract the card and the modal's access list into their own components, so either can be re-rendered without touching the modal's open state, and share the group summaries between the components - Derive the app's DOM ids from its client_id (the grouping key) and the device row ids from the authorization, through one helper used by both the templates and the controller - Revoke one of several devices as a Turbo Stream: remove the row and refresh the card behind the modal and the access list - Revoke an app's last device with a redirect, so the dashboard re-renders itself and grouping, sorting and the empty state stay in one place - Fade the removed row out, deferring Turbo's own removal until the animation has ended Part of #254 --- .../stylesheets/components/animations.css | 24 ++++++++ .../rs_auth_access_component.html.erb | 7 +++ app/components/rs_auth_access_component.rb | 15 +++++ .../rs_auth_card_component.html.erb | 35 ++++++++++++ app/components/rs_auth_card_component.rb | 16 ++++++ app/components/rs_auth_component.html.erb | 38 +------------ app/components/rs_auth_component.rb | 42 ++------------ .../rs_auth_details_modal_component.html.erb | 10 +--- .../rs_auth_details_modal_component.rb | 13 +---- app/components/rs_auth_summary.rb | 40 +++++++++++++ .../services/rs_auths_controller.rb | 39 +++++++++++-- app/helpers/rs_auths_helper.rb | 14 +++++ app/javascript/application.js | 1 + app/javascript/turbo_stream_animations.js | 30 ++++++++++ config/importmap.rb | 1 + .../services/rs_auths_controller_spec.rb | 56 +++++++++++++++++++ spec/features/services/rs_auths_spec.rb | 10 ++++ 17 files changed, 295 insertions(+), 96 deletions(-) create mode 100644 app/components/rs_auth_access_component.html.erb create mode 100644 app/components/rs_auth_access_component.rb create mode 100644 app/components/rs_auth_card_component.html.erb create mode 100644 app/components/rs_auth_card_component.rb create mode 100644 app/components/rs_auth_summary.rb create mode 100644 app/helpers/rs_auths_helper.rb create mode 100644 app/javascript/turbo_stream_animations.js diff --git a/app/assets/stylesheets/components/animations.css b/app/assets/stylesheets/components/animations.css index 23b7664..315ab7f 100644 --- a/app/assets/stylesheets/components/animations.css +++ b/app/assets/stylesheets/components/animations.css @@ -14,3 +14,27 @@ animation-duration: 0.15s; animation-timing-function: cubic-bezier(0.2, 0, 0.13, 1); } + +@keyframes removeRow { + from { + opacity: 1; + transform: translateX(0); + } + to { + opacity: 0; + transform: translateX(0.75rem); + } +} + +.animate-remove { + animation: removeRow 0.2s ease-out forwards; + pointer-events: none; +} + +@media (prefers-reduced-motion: reduce) { + /* Keep the animation, so the deferred removal still completes, but make it + effectively instant. */ + .animate-remove { + animation-duration: 0.01s; + } +} diff --git a/app/components/rs_auth_access_component.html.erb b/app/components/rs_auth_access_component.html.erb new file mode 100644 index 0000000..cc50b66 --- /dev/null +++ b/app/components/rs_auth_access_component.html.erb @@ -0,0 +1,7 @@ +

    +
    + Access +
    + <%= render RsScopeListComponent.new(scopes: permissions, root: root_access?, + icon_class: "h-5 w-5") %> +
    \ No newline at end of file diff --git a/app/components/rs_auth_access_component.rb b/app/components/rs_auth_access_component.rb new file mode 100644 index 0000000..605d742 --- /dev/null +++ b/app/components/rs_auth_access_component.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class RsAuthAccessComponent < ViewComponent::Base + include RsAuthSummary + include RsAuthsHelper + + def initialize(authorizations:) + @authorizations = authorizations + @auth = authorizations.first + end + + def access_id + rs_app_dom_id(@auth.client_id, :access) + end +end \ No newline at end of file diff --git a/app/components/rs_auth_card_component.html.erb b/app/components/rs_auth_card_component.html.erb new file mode 100644 index 0000000..3ff311e --- /dev/null +++ b/app/components/rs_auth_card_component.html.erb @@ -0,0 +1,35 @@ + \ No newline at end of file diff --git a/app/components/rs_auth_card_component.rb b/app/components/rs_auth_card_component.rb new file mode 100644 index 0000000..ea8e7f1 --- /dev/null +++ b/app/components/rs_auth_card_component.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +class RsAuthCardComponent < ViewComponent::Base + include RsAuthSummary + include RsAuthsHelper + + def initialize(authorizations:) + @authorizations = authorizations + @auth = authorizations.first + @web_app = @auth.web_app + end + + def card_id + rs_app_dom_id(@auth.client_id, :card) + end +end \ No newline at end of file diff --git a/app/components/rs_auth_component.html.erb b/app/components/rs_auth_component.html.erb index 3bc335e..2244194 100644 --- a/app/components/rs_auth_component.html.erb +++ b/app/components/rs_auth_component.html.erb @@ -1,38 +1,4 @@
    - - + <%= render RsAuthCardComponent.new(authorizations: @authorizations) %> <%= render RsAuthDetailsModalComponent.new(authorizations: @authorizations) %> -
    +
    \ No newline at end of file diff --git a/app/components/rs_auth_component.rb b/app/components/rs_auth_component.rb index 5984941..cb29f0c 100644 --- a/app/components/rs_auth_component.rb +++ b/app/components/rs_auth_component.rb @@ -1,44 +1,10 @@ # frozen_string_literal: true +# Wrapper around one app's card and its details modal. The modal's open state +# lives on this element (via the `modal` controller), so the card and the modal +# can be re-rendered independently without closing it. class RsAuthComponent < ViewComponent::Base 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 known_device_count - @authorizations.map(&:device_name).compact.uniq.size - end - - def unrecorded_device? - @authorizations.any? { |auth| auth.device_name.blank? } - end - - def device_summary - count = known_device_count - return "#{count} #{'device'.pluralize(count)}" unless unrecorded_device? - - # An authorization without a recorded user agent may cover several devices, - # so only a lower bound can be given. - lower_bound = count + 1 - "#{lower_bound}+ #{'device'.pluralize(lower_bound)}" - end -end +end \ No newline at end of file diff --git a/app/components/rs_auth_details_modal_component.html.erb b/app/components/rs_auth_details_modal_component.html.erb index 36da0f5..2462dc6 100644 --- a/app/components/rs_auth_details_modal_component.html.erb +++ b/app/components/rs_auth_details_modal_component.html.erb @@ -11,18 +11,14 @@
    • -
    - Access -
    - <%= render RsScopeListComponent.new(scopes: permissions, root: root_access?, - icon_class: "h-5 w-5") %> + <%= render RsAuthAccessComponent.new(authorizations: @authorizations) %>
    Connected devices
      <% @authorizations.each do |auth| %> -
    • +

    • <%= auth.device_label %>

      @@ -50,4 +46,4 @@ Close

      -<% end %> +<% end %> \ No newline at end of file diff --git a/app/components/rs_auth_details_modal_component.rb b/app/components/rs_auth_details_modal_component.rb index 0344e31..be140d1 100644 --- a/app/components/rs_auth_details_modal_component.rb +++ b/app/components/rs_auth_details_modal_component.rb @@ -1,21 +1,14 @@ # frozen_string_literal: true class RsAuthDetailsModalComponent < ViewComponent::Base + include RsAuthsHelper + 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 status_label(auth) return "Never expires" if auth.expire_at.blank? return "Expired on #{auth.expire_at.strftime('%-d %b %Y')}" if auth.expired? @@ -30,4 +23,4 @@ class RsAuthDetailsModalComponent < ViewComponent::Base def expires_at_title(auth) auth.expire_at.strftime("%-d %b %Y, %H:%M") end -end +end \ No newline at end of file diff --git a/app/components/rs_auth_summary.rb b/app/components/rs_auth_summary.rb new file mode 100644 index 0000000..550a779 --- /dev/null +++ b/app/components/rs_auth_summary.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +# Summaries of a group of authorizations for the same app, shared by the card, +# the details modal and the Turbo Stream responses that refresh them. +module RsAuthSummary + # The union of the group's scopes: an older token may grant more 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 known_device_count + @authorizations.map(&:device_name).compact.uniq.size + end + + def unrecorded_device? + @authorizations.any? { |auth| auth.device_name.blank? } + end + + def device_summary + count = known_device_count + return "#{count} #{'device'.pluralize(count)}" unless unrecorded_device? + + # An authorization without a recorded user agent may cover several devices, + # so only a lower bound can be given. + lower_bound = count + 1 + "#{lower_bound}+ #{'device'.pluralize(lower_bound)}" + end +end \ No newline at end of file diff --git a/app/controllers/services/rs_auths_controller.rb b/app/controllers/services/rs_auths_controller.rb index f4df2c1..6245eab 100644 --- a/app/controllers/services/rs_auths_controller.rb +++ b/app/controllers/services/rs_auths_controller.rb @@ -14,14 +14,23 @@ class Services::RsAuthsController < Services::BaseController end def destroy + client_id = @auth.client_id @auth.destroy! + remaining = current_user.remote_storage_authorizations + .where(client_id: client_id) + .order(created_at: :desc) + .includes(:web_app) + .to_a + respond_to do |format| - format.html do redirect_to apps_services_storage_url, flash: { - success: 'App authorization revoked' - } - end format.json { head :no_content } + format.html do + redirect_to apps_services_storage_url, flash: { + success: 'App authorization revoked' + } + end + format.turbo_stream { revoke_stream(client_id, remaining) } end end @@ -48,6 +57,26 @@ class Services::RsAuthsController < Services::BaseController private + # Revoking one of several devices updates the open details modal in place. + # Revoking the app's last device leaves nothing to show, so the dashboard + # re-renders itself instead: that keeps grouping, sorting and the empty + # state in one place. + def revoke_stream(client_id, remaining) + if remaining.empty? + redirect_to apps_services_storage_url, flash: { + success: 'App authorization revoked' + } + else + render turbo_stream: [ + turbo_stream.remove(helpers.rs_device_dom_id(@auth)), + turbo_stream.replace(helpers.rs_app_dom_id(client_id, :card), + RsAuthCardComponent.new(authorizations: remaining)), + turbo_stream.replace(helpers.rs_app_dom_id(client_id, :access), + RsAuthAccessComponent.new(authorizations: remaining)) + ] + end + end + def app_name_for(auth) auth.display_name end @@ -66,4 +95,4 @@ class Services::RsAuthsController < Services::BaseController @auth = current_user.remote_storage_authorizations.find_by(id: params[:id]) http_status :not_found unless @auth end -end +end \ No newline at end of file diff --git a/app/helpers/rs_auths_helper.rb b/app/helpers/rs_auths_helper.rb new file mode 100644 index 0000000..125ae92 --- /dev/null +++ b/app/helpers/rs_auths_helper.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +module RsAuthsHelper + # An app is summarised by the authorizations grouped under its client_id, so + # its DOM ids derive from the client_id: revoking any single authorization must + # not invalidate them. + def rs_app_dom_id(client_id, part) + "rs_app_#{part}_#{Digest::SHA256.hexdigest(client_id)[0, 12]}" + end + + def rs_device_dom_id(auth) + ActionView::RecordIdentifier.dom_id(auth, :device) + end +end \ No newline at end of file diff --git a/app/javascript/application.js b/app/javascript/application.js index 0d7b494..1e2d3dc 100644 --- a/app/javascript/application.js +++ b/app/javascript/application.js @@ -1,3 +1,4 @@ // Configure your import map in config/importmap.rb. Read more: https://github.com/rails/importmap-rails import "@hotwired/turbo-rails" import "controllers" +import "turbo_stream_animations" \ No newline at end of file diff --git a/app/javascript/turbo_stream_animations.js b/app/javascript/turbo_stream_animations.js new file mode 100644 index 0000000..1cf8356 --- /dev/null +++ b/app/javascript/turbo_stream_animations.js @@ -0,0 +1,30 @@ +// A Turbo Stream `remove` action deletes its target right away. Pause that until +// the element's CSS animation has finished, so a revoked device row can fade out +// instead of vanishing. + +const REMOVE_ANIMATION_CLASS = "animate-remove" +const REMOVE_ANIMATION_TIMEOUT = 1000 + +document.addEventListener("turbo:before-stream-render", (event) => { + const renderDefault = event.detail.render + + event.detail.render = (streamElement) => { + const target = + streamElement.action === "remove" ? document.getElementById(streamElement.target) : null + + if (!target) return renderDefault(streamElement) + + let rendered = false + const render = () => { + if (rendered) return + rendered = true + renderDefault(streamElement) + } + + target.addEventListener("animationend", render, { once: true }) + target.classList.add(REMOVE_ANIMATION_CLASS) + + // Don't leave the element behind if the animation cannot run at all. + setTimeout(render, REMOVE_ANIMATION_TIMEOUT) + } +}) \ No newline at end of file diff --git a/config/importmap.rb b/config/importmap.rb index cb396cc..692f89b 100644 --- a/config/importmap.rb +++ b/config/importmap.rb @@ -4,5 +4,6 @@ pin "application", preload: true pin "@hotwired/turbo-rails", to: "turbo.min.js", preload: true pin "@hotwired/stimulus", to: "stimulus.min.js", preload: true pin "@hotwired/stimulus-loading", to: "stimulus-loading.js", preload: true +pin "turbo_stream_animations" pin_all_from "app/javascript/controllers", under: "controllers" pin "tailwindcss-stimulus-components" # @4.0.3 diff --git a/spec/controllers/services/rs_auths_controller_spec.rb b/spec/controllers/services/rs_auths_controller_spec.rb index 0970c3c..cd4813a 100644 --- a/spec/controllers/services/rs_auths_controller_spec.rb +++ b/spec/controllers/services/rs_auths_controller_spec.rb @@ -103,6 +103,62 @@ RSpec.describe Services::RsAuthsController, type: :controller do expect(response).to have_http_status(:not_found) expect(RemoteStorageAuthorization.exists?(auth.id)).to be(true) end + + context "when another device remains connected to the app" do + let!(:revoked) do + 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 + ) + end + + let!(:remaining) do + user.remote_storage_authorizations.create!( + permissions: %w(documents photos), client_id: "app.example.com", + redirect_uri: "https://app.example.com", expire_at: 2.days.from_now + ) + end + + after do + revoked.destroy + remaining.destroy + end + + it "streams the removed device row and the refreshed card and access list" do + delete :destroy, params: { id: revoked.id }, format: :turbo_stream + + expect(response.media_type).to eq("text/vnd.turbo-stream.html") + expect(response.body).to include( + %(action="remove" target="#{ActionView::RecordIdentifier.dom_id(revoked, :device)}") + ) + expect(response.body).to include( + %(action="replace" target="#{controller.helpers.rs_app_dom_id('app.example.com', :card)}") + ) + expect(response.body).to include( + %(action="replace" target="#{controller.helpers.rs_app_dom_id('app.example.com', :access)}") + ) + end + + it "leaves the other device connected" do + delete :destroy, params: { id: revoked.id }, format: :turbo_stream + + expect(RemoteStorageAuthorization.exists?(remaining.id)).to be(true) + end + end + + context "when the app's last device is revoked" do + it "redirects so that the dashboard re-renders itself" 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 + ) + + delete :destroy, params: { id: auth.id }, format: :turbo_stream + + expect(response).to redirect_to(apps_services_storage_url) + expect(flash[:success]).to eq("App authorization revoked") + end + end end end end diff --git a/spec/features/services/rs_auths_spec.rb b/spec/features/services/rs_auths_spec.rb index 44435c5..a8b8bc7 100644 --- a/spec/features/services/rs_auths_spec.rb +++ b/spec/features/services/rs_auths_spec.rb @@ -74,6 +74,16 @@ RSpec.describe "Storage connected apps", type: :feature do expect(page).to have_button("Revoke access", visible: false) expect(page).to have_link("Launch app", visible: false) end + + it "renders the ids that the revoke stream targets" do + visit apps_services_storage_path + + expect(page).to have_selector(".rs-auth-card[id^='rs_app_card_']") + expect(page).to have_selector("div[id^='rs_app_access_']", visible: false) + expect(page).to have_selector( + "li##{ActionView::RecordIdentifier.dom_id(auth, :device)}", visible: false + ) + end end context "with multiple apps" do From 17bad40bf4dc522f4ab03f60b82762dc044ce365 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A2u=20Cao?= Date: Tue, 3 Nov 2026 18:28:14 +0400 Subject: [PATCH 16/16] Show device timestamps in the reader's local time The exact issuance and expiry times in the device rows' hover titles were rendered in UTC, which reads as wrong to anyone not living there. - Mark the dates with a
    <% @authorizations.each do |auth| %> -
  • +

  • <%= auth.device_label %>

    - <%= tag.span "Connected #{auth.created_at.strftime('%-d %b %Y')}", - title: connected_at_title(auth) %> + Connected <%= timestamp_tag(auth.created_at, title: connected_at_title(auth)) %> · - <%= tag.span status_label(auth), - title: (expires_at_title(auth) if auth.expire_at.present?), - class: ("text-red-700" if auth.expired?) %> + <%= tag.span class: ("text-red-700" if auth.expired?) do %> + <% if auth.expire_at.present? %> + <%= expiry_prefix(auth) %> <%= timestamp_tag(auth.expire_at, title: expires_at_title(auth)) %> + <% else %> + Never expires + <% end %> + <% end %>

    <%= button_to "Revoke access", services_storage_rs_auth_path(auth), method: :delete, diff --git a/app/components/rs_auth_details_modal_component.rb b/app/components/rs_auth_details_modal_component.rb index be140d1..90aef7f 100644 --- a/app/components/rs_auth_details_modal_component.rb +++ b/app/components/rs_auth_details_modal_component.rb @@ -9,13 +9,15 @@ class RsAuthDetailsModalComponent < ViewComponent::Base @web_app = @auth.web_app 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" / "Expired on", or nothing when there is no expiry to describe. + def expiry_prefix(auth) + return nil if auth.expire_at.blank? - "Expires #{auth.expire_at.strftime('%-d %b %Y')}" + auth.expired? ? "Expired on" : "Expires" end + # Fallbacks for the hover titles, used when JavaScript is unavailable. + # app/javascript/local_time.js replaces them with the reader's own zone. def connected_at_title(auth) auth.created_at.strftime("%-d %b %Y, %H:%M") end @@ -23,4 +25,10 @@ class RsAuthDetailsModalComponent < ViewComponent::Base def expires_at_title(auth) auth.expire_at.strftime("%-d %b %Y, %H:%M") end + + # A