DEV: Move UserApiKey scopes to dedicated table (#10704)

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

diff --git a/app/controllers/user_api_keys_controller.rb b/app/controllers/user_api_keys_controller.rb
index 6de32f0..b860919 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 c705541..951add6 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 0000000..bae6a92
--- /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 837bbff..9817abe 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 0000000..93a2ec9
--- /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 d254517..010a7f1 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 8733c6f..94b3429 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 5c4ce63..c6614b9 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

[... diff too long, it was truncated ...]

GitHub sha: 1ba9b34b

This commit appears in #10704 which was approved by romanrizzi. It was merged by davidtaylorhq.