From 1ba9b34b034c2a27c9e04f430eefbccae5942bcf Mon Sep 17 00:00:00 2001 From: David Taylor Date: Tue, 29 Sep 2020 10:57:48 +0100 Subject: [PATCH] DEV: Move UserApiKey scopes to dedicated table (#10704) This has no functional impact yet, but it is the first step in adding more granular scopes to UserApiKeys --- app/controllers/user_api_keys_controller.rb | 5 +- app/models/user_api_key.rb | 21 +++++++-- app/models/user_api_key_scope.rb | 19 ++++++++ app/services/post_alerter.rb | 3 +- .../20200918095554_add_user_api_key_scopes.rb | 46 +++++++++++++++++++ lib/auth/default_current_user_provider.rb | 2 +- .../default_current_user_provider_spec.rb | 4 +- spec/fabricators/user_api_key_fabricator.rb | 4 +- spec/models/user_api_key_spec.rb | 6 +-- .../requests/user_api_keys_controller_spec.rb | 4 +- spec/services/post_alerter_spec.rb | 4 +- 11 files changed, 98 insertions(+), 20 deletions(-) create mode 100644 app/models/user_api_key_scope.rb create mode 100644 db/migrate/20200918095554_add_user_api_key_scopes.rb diff --git a/app/controllers/user_api_keys_controller.rb b/app/controllers/user_api_keys_controller.rb index 6de32f04afc..b8609190e2c 100644 --- a/app/controllers/user_api_keys_controller.rb +++ b/app/controllers/user_api_keys_controller.rb @@ -71,7 +71,7 @@ class UserApiKeysController < ApplicationController client_id: params[:client_id], user_id: current_user.id, push_url: params[:push_url], - scopes: scopes + scopes: scopes.map { |name| UserApiKeyScope.new(name: name) } ) # we keep the payload short so it encrypts easily with public key @@ -147,9 +147,6 @@ class UserApiKeysController < ApplicationController if current_key = request.env['HTTP_USER_API_KEY'] request_key = UserApiKey.with_key(current_key).first revoke_key ||= request_key - if request_key && request_key.id != revoke_key.id && !request_key.scopes.include?("write") - raise Discourse::InvalidAccess - end end raise Discourse::NotFound unless revoke_key diff --git a/app/models/user_api_key.rb b/app/models/user_api_key.rb index c705541a73d..951add6d7ac 100644 --- a/app/models/user_api_key.rb +++ b/app/models/user_api_key.rb @@ -1,6 +1,9 @@ # frozen_string_literal: true class UserApiKey < ActiveRecord::Base + self.ignored_columns = [ + "scopes" # TODO(2020-12-18): remove + ] SCOPES = { read: [:get], @@ -19,6 +22,7 @@ class UserApiKey < ActiveRecord::Base } belongs_to :user + has_many :scopes, class_name: "UserApiKeyScope", dependent: :destroy scope :active, -> { where(revoked_at: nil) } scope :with_key, ->(key) { where(key_hash: ApiKey.hash_key(key)) } @@ -41,6 +45,7 @@ class UserApiKey < ActiveRecord::Base @key.present? end + # Scopes allowed to be requested by external services def self.allowed_scopes Set.new(SiteSetting.allow_user_api_key_scopes.split("|")) end @@ -78,13 +83,15 @@ class UserApiKey < ActiveRecord::Base end def has_push? - (scopes.include?("push") || scopes.include?("notifications")) && push_url.present? && SiteSetting.allowed_user_api_push_urls.include?(push_url) + scopes.any? { |s| s.name == "push" || s.name == "notifications" } && + push_url.present? && + SiteSetting.allowed_user_api_push_urls.include?(push_url) end def allow?(env) - scopes.any? do |name| - UserApiKey.allow_scope?(name, env) - end + scopes.any? do |s| + UserApiKey.allow_scope?(s.name, env) + end || is_revoke_self_request?(env) end def self.invalid_auth_redirect?(auth_redirect) @@ -92,6 +99,12 @@ class UserApiKey < ActiveRecord::Base .split('|') .none? { |u| WildcardUrlChecker.check_url(u, auth_redirect) } end + + private + + def is_revoke_self_request?(env) + UserApiKey.allow_permission?([:post, 'user_api_keys#revoke'], env) && (env[:id].nil? || env[:id].to_i == id) + end end # == Schema Information diff --git a/app/models/user_api_key_scope.rb b/app/models/user_api_key_scope.rb new file mode 100644 index 00000000000..bae6a928005 --- /dev/null +++ b/app/models/user_api_key_scope.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class UserApiKeyScope < ActiveRecord::Base +end + +# == Schema Information +# +# Table name: user_api_key_scopes +# +# id :bigint not null, primary key +# user_api_key_id :integer not null +# name :string not null +# created_at :datetime not null +# updated_at :datetime not null +# +# Indexes +# +# index_user_api_key_scopes_on_user_api_key_id (user_api_key_id) +# diff --git a/app/services/post_alerter.rb b/app/services/post_alerter.rb index 837bbff3769..9817abebd1c 100644 --- a/app/services/post_alerter.rb +++ b/app/services/post_alerter.rb @@ -470,7 +470,8 @@ class PostAlerter if SiteSetting.allow_user_api_key_scopes.split("|").include?("push") && SiteSetting.allowed_user_api_push_urls.present? clients = user.user_api_keys - .where("('push' = ANY(scopes) OR 'notifications' = ANY(scopes))") + .joins(:scopes) + .where("user_api_key_scopes.name IN ('push', 'notifications')") .where("push_url IS NOT NULL AND push_url <> ''") .where("position(push_url IN ?) > 0", SiteSetting.allowed_user_api_push_urls) .where("revoked_at IS NULL") diff --git a/db/migrate/20200918095554_add_user_api_key_scopes.rb b/db/migrate/20200918095554_add_user_api_key_scopes.rb new file mode 100644 index 00000000000..93a2ec9a7a9 --- /dev/null +++ b/db/migrate/20200918095554_add_user_api_key_scopes.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +class AddUserApiKeyScopes < ActiveRecord::Migration[6.0] + def change + create_table :user_api_key_scopes do |t| + t.integer :user_api_key_id, null: false + t.string :name, null: false + t.timestamps + end + + add_index :user_api_key_scopes, :user_api_key_id + + reversible do |dir| + dir.up do + execute <<~SQL + INSERT INTO user_api_key_scopes + ( + user_api_key_id, + name, + created_at, + updated_at + ) + SELECT + user_api_keys.id, + unnest(user_api_keys.scopes), + created_at, + updated_at + FROM user_api_keys + SQL + + Migration::SafeMigrate.disable! + change_column_null :user_api_keys, :scopes, true + change_column_default :user_api_keys, :scopes, nil + Migration::SafeMigrate.enable! + + Migration::ColumnDropper.mark_readonly(:user_api_keys, :scopes) + end + + dir.down do + change_column_null :user_api_keys, :scopes, false + change_column_default :user_api_keys, :scopes, [] + Migration::ColumnDropper.drop_readonly(:user_api_keys, :scopes) + end + end + end +end diff --git a/lib/auth/default_current_user_provider.rb b/lib/auth/default_current_user_provider.rb index d2545179ff1..010a7f14d9d 100644 --- a/lib/auth/default_current_user_provider.rb +++ b/lib/auth/default_current_user_provider.rb @@ -307,7 +307,7 @@ class Auth::DefaultCurrentUserProvider protected def lookup_user_api_user_and_update_key(user_api_key, client_id) - if api_key = UserApiKey.active.with_key(user_api_key).includes(:user).first + if api_key = UserApiKey.active.with_key(user_api_key).includes(:user, :scopes).first unless api_key.allow?(@env) raise Discourse::InvalidAccess end diff --git a/spec/components/auth/default_current_user_provider_spec.rb b/spec/components/auth/default_current_user_provider_spec.rb index 8733c6f3276..94b34292d61 100644 --- a/spec/components/auth/default_current_user_provider_spec.rb +++ b/spec/components/auth/default_current_user_provider_spec.rb @@ -547,7 +547,7 @@ describe Auth::DefaultCurrentUserProvider do UserApiKey.create!( application_name: 'my app', client_id: '1234', - scopes: ['read'], + scopes: ['read'].map { |name| UserApiKeyScope.new(name: name) }, user_id: user.id ) end @@ -556,7 +556,7 @@ describe Auth::DefaultCurrentUserProvider do dupe = UserApiKey.create!( application_name: 'my app', client_id: '12345', - scopes: ['read'], + scopes: ['read'].map { |name| UserApiKeyScope.new(name: name) }, user_id: user.id ) diff --git a/spec/fabricators/user_api_key_fabricator.rb b/spec/fabricators/user_api_key_fabricator.rb index 5c4ce63d4d7..c6614b95bf9 100644 --- a/spec/fabricators/user_api_key_fabricator.rb +++ b/spec/fabricators/user_api_key_fabricator.rb @@ -1,8 +1,10 @@ # frozen_string_literal: true +Fabricator(:user_api_key_scope) + Fabricator(:readonly_user_api_key, from: :user_api_key) do user - scopes ['read'] + scopes { [Fabricate.build(:user_api_key_scope, name: 'read')] } client_id { SecureRandom.hex } application_name 'some app' end diff --git a/spec/models/user_api_key_spec.rb b/spec/models/user_api_key_spec.rb index 0337d44ddd8..22bda2eb98b 100644 --- a/spec/models/user_api_key_spec.rb +++ b/spec/models/user_api_key_spec.rb @@ -5,7 +5,7 @@ require 'rails_helper' describe UserApiKey do context "#allow?" do it "can look up permissions correctly" do - key = UserApiKey.new(scopes: ['message_bus', 'notifications']) + key = UserApiKey.new(scopes: ['message_bus', 'notifications'].map { |name| UserApiKeyScope.new(name: name) }) expect(key.allow?("PATH_INFO" => "/random", "REQUEST_METHOD" => "GET")).to eq(false) expect(key.allow?("PATH_INFO" => "/message-bus/1234/poll", "REQUEST_METHOD" => "POST")).to eq(true) @@ -20,7 +20,7 @@ describe UserApiKey do it "can allow all correct scopes to write" do - key = UserApiKey.new(scopes: ["write"]) + key = UserApiKey.new(scopes: ["write"].map { |name| UserApiKeyScope.new(name: name) }) expect(key.allow?("PATH_INFO" => "/random", "REQUEST_METHOD" => "GET")).to eq(true) expect(key.allow?("PATH_INFO" => "/random", "REQUEST_METHOD" => "PUT")).to eq(true) @@ -31,7 +31,7 @@ describe UserApiKey do it "can allow blanket read" do - key = UserApiKey.new(scopes: ["read"]) + key = UserApiKey.new(scopes: ["read"].map { |name| UserApiKeyScope.new(name: name) }) expect(key.allow?("PATH_INFO" => "/random", "REQUEST_METHOD" => "GET")).to eq(true) expect(key.allow?("PATH_INFO" => "/random", "REQUEST_METHOD" => "PUT")).to eq(false) diff --git a/spec/requests/user_api_keys_controller_spec.rb b/spec/requests/user_api_keys_controller_spec.rb index 11ab1db45da..115acf0e991 100644 --- a/spec/requests/user_api_keys_controller_spec.rb +++ b/spec/requests/user_api_keys_controller_spec.rb @@ -174,7 +174,7 @@ describe UserApiKeysController do expect(parsed["api"]).to eq(4) key = user.user_api_keys.first - expect(key.scopes).to include("push") + expect(key.scopes.map(&:name)).to include("push") expect(key.push_url).to eq("https://push.it/here") end @@ -208,7 +208,7 @@ describe UserApiKeysController do api_key = UserApiKey.with_key(parsed["key"]).first expect(api_key.user_id).to eq(user.id) - expect(api_key.scopes.sort).to eq(["push", "message_bus", "notifications", "session_info", "one_time_password"].sort) + expect(api_key.scopes.map(&:name).sort).to eq(["push", "message_bus", "notifications", "session_info", "one_time_password"].sort) expect(api_key.push_url).to eq("https://push.it/here") uri.query = "" diff --git a/spec/services/post_alerter_spec.rb b/spec/services/post_alerter_spec.rb index 92d90e86d4e..15f631ca521 100644 --- a/spec/services/post_alerter_spec.rb +++ b/spec/services/post_alerter_spec.rb @@ -724,7 +724,7 @@ describe PostAlerter do UserApiKey.create!(user_id: evil_trout.id, client_id: "xxx#{i}", application_name: "iPhone#{i}", - scopes: ['notifications'], + scopes: ['notifications'].map { |name| UserApiKeyScope.new(name: name) }, push_url: "https://site2.com/push") end @@ -739,7 +739,7 @@ describe PostAlerter do UserApiKey.create!(user_id: evil_trout.id, client_id: "xxx#{i}", application_name: "iPhone#{i}", - scopes: ['notifications'], + scopes: ['notifications'].map { |name| UserApiKeyScope.new(name: name) }, push_url: "https://site2.com/push") end