diff --git a/app/models/remote_storage_authorization.rb b/app/models/remote_storage_authorization.rb index 6c7ae6e..cd2c36d 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 + # The plaintext token is only ever held in memory, long enough to hand it to + # the client at issuance. At rest we store only its SHA256 digest. + attr_accessor :token + # 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)" @@ -77,8 +81,12 @@ class RemoteStorageAuthorization < ApplicationRecord end def delete_token_from_redis - key = "authorizations:#{user.cn}:#{token}" - redis.srem? key, redis.smembers(key) + value = token_digest.presence + value ||= read_attribute(:token) if self.class.column_names.include?("token") + return if value.blank? + + key = "authorizations:#{user.cn}:#{value}" + redis.del key rescue => e Rails.logger.error e Sentry.capture_exception(e) if Setting.sentry_enabled? @@ -92,10 +100,11 @@ class RemoteStorageAuthorization < ApplicationRecord def generate_token(length=16) self.token = SecureRandom.hex(length) if self.token.blank? + self.token_digest = Digest::SHA256.hexdigest(token) end def store_token_in_redis - redis.sadd "authorizations:#{user.cn}:#{token}", permissions + redis.sadd "authorizations:#{user.cn}:#{token_digest}", permissions end def schedule_token_expiry diff --git a/app/services/remote_storage_manager/rekey_tokens.rb b/app/services/remote_storage_manager/rekey_tokens.rb new file mode 100644 index 0000000..3f91125 --- /dev/null +++ b/app/services/remote_storage_manager/rekey_tokens.rb @@ -0,0 +1,111 @@ +require "digest" + +module RemoteStorageManager + # One-off, idempotent migration helper for issue #259. It backfills missing + # digests for authorizations created while the old plaintext code was still + # running, re-keys the Redis authorization sets from plaintext to digest + # keys, and removes any leftover non-digest keys. + # + # It only works while the plaintext `token` column still exists; after the + # follow-up contract migration drops it, `call` is a no-op. + class RekeyTokens < RemoteStorageManagerService + Result = Struct.new(:rekeyed, :legacy_removed, :skipped, keyword_init: true) + + def call + return Result.new(rekeyed: 0, legacy_removed: 0, skipped: true) unless token_column? + + rekeyed = 0 + + RemoteStorageAuthorization.find_each do |auth| + plaintext = persisted_token(auth) + next if plaintext.blank? && auth.token_digest.blank? + + digest = auth.token_digest.presence || Digest::SHA256.hexdigest(plaintext) + auth.update_columns(token_digest: digest) if auth.token_digest.blank? + + permissions = permissions_for(auth, plaintext) + + redis.sadd(redis_key(auth.user.cn, digest), permissions) if permissions.present? + redis.del(redis_key(auth.user.cn, plaintext)) if plaintext.present? + + rekeyed += 1 + end + + Result.new(rekeyed: rekeyed, legacy_removed: remove_legacy_keys, skipped: false) + end + + private + + def redis + @redis ||= Redis.new(url: Setting.rs_redis_url) + end + + def token_column? + RemoteStorageAuthorization.column_names.include?("token") + end + + # Reads the persisted plaintext directly, because the model's `token` + # accessor shadows the column after the token is moved into memory. + def persisted_token(auth) + auth.read_attribute(:token) if token_column? + end + + def redis_key(cn, value) + "authorizations:#{cn}:#{value}" + end + + def permissions_for(auth, plaintext) + permissions = redis.smembers(redis_key(auth.user.cn, plaintext)) if plaintext.present? + permissions.presence || permissions_from_db(auth) + end + + def permissions_from_db(auth) + case (permissions = auth.permissions) + when Array then permissions.map(&:to_s) + when String then parse_permissions(permissions) + else Array(permissions).map(&:to_s) + end + end + + def parse_permissions(raw) + parsed = parse_serialized(raw) + return parsed.map(&:to_s) if parsed.is_a?(Array) + + raw.to_s.gsub(/[{}]/, "").split(/[,\s]+/).reject(&:empty?) + end + + def parse_serialized(raw) + YAML.safe_load(raw) + rescue StandardError + begin + JSON.parse(raw) + rescue StandardError + nil + end + end + + # Removes leftover plaintext authorization keys. Legacy plaintext tokens are + # 32 hex chars (SecureRandom.hex(16)); digests are 64. Matching on shape + # instead of a snapshot of known keys means a digest issued concurrently + # during the sweep is never deleted. + def remove_legacy_keys + removed = 0 + cursor = "0" + + loop do + cursor, keys = redis.scan(cursor, match: "authorizations:*", count: 1000) + + keys.each do |key| + next if key.split(":", 3).last.match?(/\A[0-9a-f]{64}\z/) + + redis.del(key) + removed += 1 + end + + break if cursor == "0" + end + + removed + end + end +end diff --git a/app/services/remote_storage_manager_service.rb b/app/services/remote_storage_manager_service.rb new file mode 100644 index 0000000..9de9a97 --- /dev/null +++ b/app/services/remote_storage_manager_service.rb @@ -0,0 +1,2 @@ +class RemoteStorageManagerService < ApplicationService +end diff --git a/db/migrate/20261104120000_add_token_digest_to_remote_storage_authorizations.rb b/db/migrate/20261104120000_add_token_digest_to_remote_storage_authorizations.rb new file mode 100644 index 0000000..f196b02 --- /dev/null +++ b/db/migrate/20261104120000_add_token_digest_to_remote_storage_authorizations.rb @@ -0,0 +1,26 @@ +require "digest" + +class AddTokenDigestToRemoteStorageAuthorizations < ActiveRecord::Migration[8.1] + class MigrationRemoteStorageAuthorization < ActiveRecord::Base + self.table_name = "remote_storage_authorizations" + end + + def up + add_column :remote_storage_authorizations, :token_digest, :string + + MigrationRemoteStorageAuthorization.reset_column_information + + MigrationRemoteStorageAuthorization.find_each do |auth| + next if auth.token.blank? + + auth.update_columns(token_digest: Digest::SHA256.hexdigest(auth.token)) + end + + add_index :remote_storage_authorizations, :token_digest, unique: true + end + + def down + remove_index :remote_storage_authorizations, :token_digest + remove_column :remote_storage_authorizations, :token_digest + end +end diff --git a/db/schema.rb b/db/schema.rb index 4518582..b587aef 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_130000) do +ActiveRecord::Schema[8.1].define(version: 2026_11_04_120000) do create_table "active_storage_attachments", force: :cascade do |t| t.bigint "blob_id", null: false t.datetime "created_at", null: false @@ -109,11 +109,13 @@ ActiveRecord::Schema[8.1].define(version: 2026_11_02_130000) do t.text "permissions", default: "--- []\n" t.string "redirect_uri" t.string "token" + t.string "token_digest" t.datetime "updated_at", null: false 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" + t.index ["token_digest"], name: "index_remote_storage_authorizations_on_token_digest", unique: true t.index ["user_id"], name: "index_remote_storage_authorizations_on_user_id" t.index ["web_app_id"], name: "index_remote_storage_authorizations_on_web_app_id" end diff --git a/lib/tasks/remote_storage.rake b/lib/tasks/remote_storage.rake new file mode 100644 index 0000000..3af410b --- /dev/null +++ b/lib/tasks/remote_storage.rake @@ -0,0 +1,12 @@ +namespace :rs do + desc "Backfill remoteStorage token digests and re-key Redis authorization keys (idempotent)" + task rekey_tokens: :environment do + result = RemoteStorageManager::RekeyTokens.call + + if result.skipped + puts "remote_storage_authorizations.token column is gone; nothing to re-key." + else + puts "Re-keyed #{result.rekeyed} authorization(s); removed #{result.legacy_removed} legacy Redis key(s)." + end + end +end diff --git a/spec/controllers/rs/oauth_controller_spec.rb b/spec/controllers/rs/oauth_controller_spec.rb index e06be1a..c7530a9 100644 --- a/spec/controllers/rs/oauth_controller_spec.rb +++ b/spec/controllers/rs/oauth_controller_spec.rb @@ -376,7 +376,9 @@ RSpec.describe Rs::OauthController, type: :controller do end it "redirects to the redirect_uri" do - expect(response).to redirect_to("https://example.com#access_token=#{@auth.token}&state=foobar123") + expect(response).to redirect_to( + %r{\Ahttps://example\.com#access_token=[a-f0-9]{32}&state=foobar123\z} + ) end end @@ -400,7 +402,7 @@ RSpec.describe Rs::OauthController, type: :controller do end it "updates the token" do - expect(@auth.reload.token).not_to be_nil + expect(@auth.reload.token_digest).not_to be_nil end end diff --git a/spec/jobs/remote_storage_expire_authorization_job_spec.rb b/spec/jobs/remote_storage_expire_authorization_job_spec.rb index afe0f3e..4bf458e 100644 --- a/spec/jobs/remote_storage_expire_authorization_job_spec.rb +++ b/spec/jobs/remote_storage_expire_authorization_job_spec.rb @@ -28,7 +28,7 @@ RSpec.describe RemoteStorageExpireAuthorizationJob, type: :job do } it "removes the RS authorization from redis" do - redis_key = "authorizations:#{@user.cn}:#{@rs_authorization.token}" + redis_key = "authorizations:#{@user.cn}:#{@rs_authorization.token_digest}" expect(redis.keys(redis_key)).to_not be_empty perform_enqueued_jobs { job } diff --git a/spec/models/remote_storage_authorization_spec.rb b/spec/models/remote_storage_authorization_spec.rb index 7616603..d38d25f 100644 --- a/spec/models/remote_storage_authorization_spec.rb +++ b/spec/models/remote_storage_authorization_spec.rb @@ -25,10 +25,22 @@ RSpec.describe RemoteStorageAuthorization, type: :model do expect(auth.token).to match(/[a-zA-Z0-9]+/) end + it "stores only the token digest" do + expect(auth.token_digest).to eq(Digest::SHA256.hexdigest(auth.token)) + end + + it "does not persist the plaintext token" do + plaintext = auth.token + auth.reload + + expect(auth.read_attribute(:token)).to be_nil + expect(auth.token_digest).to eq(Digest::SHA256.hexdigest(plaintext)) + end + it "stores a token in redis" do auth - authorizations = redis_rs.smembers("authorizations:#{user.cn}:#{auth.token}") + authorizations = redis_rs.smembers("authorizations:#{user.cn}:#{auth.token_digest}") expect(authorizations.sort).to eq(%w(documents photos contacts:rw videos:r tasks/work:r).sort) end @@ -56,7 +68,21 @@ RSpec.describe RemoteStorageAuthorization, type: :model do ) auth.destroy! - expect(redis_rs.exists?("authorizations:#{user.cn}:#{auth.token}")).to be(false) + expect(redis_rs.exists?("authorizations:#{user.cn}:#{auth.token_digest}")).to be(false) + end + + it "removes a legacy plaintext token from redis" do + auth = user.remote_storage_authorizations.create!( + permissions: %w(shares:rw), + client_id: "sharesome.5apps.com", + redirect_uri: "https://sharesome.5apps.com" + ) + auth.update_columns(token: "legacytoken", token_digest: nil) + redis_rs.sadd("authorizations:#{user.cn}:legacytoken", %w(shares:rw)) + + auth.destroy! + + expect(redis_rs.exists?("authorizations:#{user.cn}:legacytoken")).to be(false) end context "with expiry set" do diff --git a/spec/services/remote_storage_manager/rekey_tokens_spec.rb b/spec/services/remote_storage_manager/rekey_tokens_spec.rb new file mode 100644 index 0000000..3dbd3e9 --- /dev/null +++ b/spec/services/remote_storage_manager/rekey_tokens_spec.rb @@ -0,0 +1,114 @@ +require 'rails_helper' + +RSpec.describe RemoteStorageManager::RekeyTokens, type: :model do + let(:user) { create :user, cn: "ronald", ou: "kosmos.org" } + let(:redis) { redis_rs } + + 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) + + redis.keys("authorizations:*").each { |key| redis.del(key) } + end + + def authorization_keys + redis.keys("authorizations:#{user.cn}:*") + end + + # A row as it looked before #259: the plaintext token lives in the DB column + # and only the plaintext Redis key exists. + def legacy_authorization(token: "legacytoken", permissions: %w(documents:rw)) + auth = create :remote_storage_authorization, user: user, permissions: permissions + auth.update_columns(token: token, token_digest: nil) + + redis.keys("authorizations:#{user.cn}:*").each { |key| redis.del(key) } + redis.sadd("authorizations:#{user.cn}:#{token}", permissions) + + auth + end + + it "re-keys an authorization from the plaintext to the digest form" do + auth = legacy_authorization + digest = Digest::SHA256.hexdigest("legacytoken") + + result = described_class.call + + expect(result).to have_attributes(skipped: false, rekeyed: 1) + expect(redis.smembers("authorizations:#{user.cn}:#{digest}").sort).to eq(%w(documents:rw)) + expect(redis.exists?("authorizations:#{user.cn}:legacytoken")).to be(false) + expect(auth.reload.token_digest).to eq(digest) + end + + it "is idempotent" do + legacy_authorization + described_class.call + + expect { described_class.call }.not_to raise_error + expect(authorization_keys.length).to eq(1) + end + + it "backfills the digest for rows created while the old code was still running" do + auth = legacy_authorization + + described_class.call + + expect(auth.reload.token_digest).to eq(Digest::SHA256.hexdigest("legacytoken")) + end + + it "falls back to the stored permissions when the legacy Redis key is gone" do + auth = create :remote_storage_authorization, user: user, permissions: %w(photos:rw) + auth.update_columns(token: "legacytoken", token_digest: nil) + authorization_keys.each { |key| redis.del(key) } + + described_class.call + + digest = Digest::SHA256.hexdigest("legacytoken") + expect(redis.smembers("authorizations:#{user.cn}:#{digest}").sort).to eq(%w(photos:rw)) + end + + it "removes leftover non-digest authorization keys" do + create :remote_storage_authorization, user: user + redis.sadd("authorizations:#{user.cn}:leftovertoken", %w(photos)) + + result = described_class.call + + expect(result.legacy_removed).to eq(1) + expect(redis.exists?("authorizations:#{user.cn}:leftovertoken")).to be(false) + end + + it "keeps digest-shaped keys that are not in the database" do + fresh = Digest::SHA256.hexdigest("brand-new-token") + redis.sadd("authorizations:#{user.cn}:#{fresh}", %w(documents:rw)) + + result = described_class.call + + expect(redis.exists?("authorizations:#{user.cn}:#{fresh}")).to be(true) + expect(result.legacy_removed).to eq(0) + end + + it "is a no-op once the plaintext token column is gone" do + allow(RemoteStorageAuthorization).to receive(:column_names) + .and_return(RemoteStorageAuthorization.column_names - ["token"]) + + expect(described_class.call).to have_attributes(skipped: true, rekeyed: 0) + end + + describe "permissions parsing" do + subject(:service) { described_class.new } + + it "parses a JSON array string" do + expect(service.send(:parse_permissions, '["documents:rw", "photos"]')) + .to eq(%w(documents:rw photos)) + end + + it "parses a YAML array string" do + expect(service.send(:parse_permissions, "---\n- documents:rw\n- photos\n")) + .to eq(%w(documents:rw photos)) + end + + it "parses a Postgres brace array string" do + expect(service.send(:parse_permissions, "{documents:rw,photos}")) + .to eq(%w(documents:rw photos)) + end + end +end