From 8fc7ffdd1d5c49c0ce9222e45410d3cd1665be80 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] 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